Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GeneralizedTime incorrectly handles years greater than 9999 #83

Open
karelmaxa opened this issue Jul 21, 2023 · 2 comments
Open

GeneralizedTime incorrectly handles years greater than 9999 #83

karelmaxa opened this issue Jul 21, 2023 · 2 comments

Comments

@karelmaxa
Copy link
Member

Summary

The GeneralizedTime component inconsistently formats / parses the date when the year of the date is greater than 9999. During serialization, the year is serialized as is (e.g. 292278994), but the parsing process expects the year to be in YYYY format.

Steps To Reproduce ("Repro Steps")

@Test
public void testGeneralizedTimeMaxYear() {
    String serialized = GeneralizedTime.valueOf(Long.MAX_VALUE).toString();
    GeneralizedTime parsed = GeneralizedTime.valueOf(serialized);
}

The preceding test ends with the following exception:

org.forgerock.i18n.LocalizedIllegalArgumentException: The provided value "2922789940817071255.807Z" is not a valid generalized time value because "78" is not a valid month specification
	at org.forgerock.opendj.ldap.GeneralizedTime.valueOf(GeneralizedTime.java:221)

Expected Result (Behavior You Expected to See)

The behaviour of the GeneralizedTime component will be consistent if the year is greater than 9999.

@pavelhoral
Copy link
Member

RFC 4517 is very clear on this -- a year is represented exactly by four digits. Anything else is invalid. So there are only a few approaches to this issue:

  1. either the incorrect timestamp is rejected with exception when GeneralizedTime is first created;
  2. or the exception is thrown when creating invalid string representation in #toString();
  3. or the date is silently capped at max allowed date in #toString()

The right thing seems to be to throw exception, however that would be a major breaking change and that should probably at least increase minor release version.

@pavelhoral
Copy link
Member

pavelhoral commented Jul 24, 2023

I have few additional remarks after considering alternatives from my previous comment:

  • Checking for the max allowed value is not that simple if we want to take TZ into account (e.g. 99991231Z vs 99991231-10). This needs to be investigated a bit more.
  • As GeneralizedTime is basically immutable we might want to check the value during construction. However we should be aware of potential performance cost.
  • If the value is truly invalid from the LDAP point of view, I think we can add that check and don't worry about backwards compatibility.
Sample code to illustrate the first point:
jshell> import java.time.*;

jshell> var date = ZonedDateTime.of(9999, 12, 31, 23, 59, 59, 0, ZoneId.of("UTC-10"));
date ==> 9999-12-31T23:59:59-10:00[UTC-10:00]

jshell> date.withZoneSameInstant(ZoneOffset.UTC);
$3 ==> +10000-01-01T09:59:59Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants