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

date-time validation is too lenient #508

Closed
aznan2 opened this issue Feb 3, 2022 · 13 comments
Closed

date-time validation is too lenient #508

aznan2 opened this issue Feb 3, 2022 · 13 comments

Comments

@aznan2
Copy link
Contributor

aznan2 commented Feb 3, 2022

Validation of date-time format accepts the value 2022-02-03T11:47:43.123 as valid, even though a time zone offset or Z is required.

From the JSON schema spec, section 7.3.1:

Date and time format names are derived from RFC 3339, section 5.6.
...
date-time:
A string instance is valid against this attribute if it is a valid representation according to the "date-time" production.

And looking at RFC 3339, section 5.6:

full-time = partial-time time-offset

date-time = full-date "T" full-time

The time-offset part is not optional.

To make it backwards compatible, perhaps the lenient validation could be available as an option, though in that case I think the stricter version should be the default behavior.

@ethlo
Copy link
Contributor

ethlo commented Feb 11, 2022

Also, the offset is not following the RFC-3339 specification. 2014-05-30T00:00:00+0000 is not valid as +0000 is not following the pattern defined. It should be 2014-05-30T00:00:00+00:00.

@stevehu
Copy link
Contributor

stevehu commented Feb 22, 2022

The current date time validator is based on regex and it is very hard to cover all the edge cases. I would be very happy to accept a PR if you guys come up with a new regex to fill in the gaps. Thanks.

@stevehu
Copy link
Contributor

stevehu commented Feb 25, 2022

@aznan2 I want to test the new regex with more test cases so I have synched the date-time.json from the official test suite. It is broken with one of the test cases.

            {
                "description": "a valid date-time with a leap second, UTC",
                "data": "1998-12-31T23:59:60Z",
                "valid": true
            },

The test case is disabled currently and I am wondering if you could take a look to get this test case resolved. The test case is located at https://github.com/networknt/json-schema-validator/blob/master/src/test/java/com/networknt/schema/V4JsonSchemaTest.java#L45

@aznan2
Copy link
Contributor Author

aznan2 commented Feb 26, 2022

Sure, I'll check it out.

@ethlo
Copy link
Contributor

ethlo commented Feb 26, 2022

Please feel free to ignore this, but if you want another solution: https://github.com/ethlo/itu - A dependency-free RFC-3339 parser.

Full disclosure: I'm the author

@stevehu
Copy link
Contributor

stevehu commented Feb 27, 2022

@ethlo Thanks a lot for the idea. I have checked the lib and it is a very good candidate. It is green without any dependency, very small and built with Java 8. Let's try the regex first and move to it if we cannot resolve it with regex.

@aznan2
Copy link
Contributor Author

aznan2 commented Feb 27, 2022

The issue is that SimpleDateFormat doesn't handle leap seconds. The new PR uses ethlo's ITU to accept leap seconds, though it's still not 100%. ITU's implementation only checks that the leap second occurs on July or December 31, so it accepts 1997-12-31T23:59:60Z as a valid date-time even though 1997 didn't have a leap second. If we want that kind of accuracy, we'll need to look at Time4J instead. However, I think this is good enough.

stevehu pushed a commit that referenced this issue Feb 28, 2022
* Make date-time validation align with RFC3339 (#508)

* Make date-time validation handle leap seconds (#508)

Co-authored-by: Matti Hansson <[email protected]>
Co-authored-by: mathan <[email protected]>
@ethlo
Copy link
Contributor

ethlo commented Feb 28, 2022

Unfortunately not 100%, as almost all sanity checking is delegated to the JDK java.time.* classes, or in the case of leap-seconds, a simplified rule. I assume Time4j is an option, but at 2.4 MB I would rather let people validate those themselves if such (rather extreme) accuracy is required. ITU is 18KB.

~~Also, I released a new version, with some cleanup and better formatUtc...() speed (not relevant here, I assume) https://github.com/ethlo/itu/releases/tag/v1.5.0. ~~

UPDATE: I have just released 1.5.1 with proper support for leap seconds! https://github.com/ethlo/itu/releases/tag/v1.5.1

IMPORTANT: Please take note that the parser no longer accepts null/empty since version 1.5.0, to be in line with java.time classes.

Also, I think that the performance of ITU is very much in line with the performance of this library: https://github.com/ethlo/itu#performance

Thank you.

@aznan2
Copy link
Contributor Author

aznan2 commented Feb 28, 2022

@ethlo Cool!

@stevehu The dependency to ITU should be updated to 1.5.1 when it's available in Maven central. Nevermind, made a new PR myself.

aznan2 pushed a commit to aznan2/json-schema-validator that referenced this issue Feb 28, 2022
stevehu pushed a commit that referenced this issue Feb 28, 2022
@dhpalan
Copy link

dhpalan commented Apr 16, 2022

Also, the offset is not following the RFC-3339 specification. 2014-05-30T00:00:00+0000 is not valid as +0000 is not following the pattern defined. It should be 2014-05-30T00:00:00+00:00.

We upgraded to 1.0.68 and the above strict validation for offset is breaking all our server API's!

  • We do not have control over which language or date library our clients use.
  • Any client application that uses +0000 format is affected due to this change.

Is there a way to configure the DateTimeValidator to be backwards compatible?

@ethlo
Copy link
Contributor

ethlo commented Apr 19, 2022

@dhpalan This is not "strict" validation, it is validation according to the RFC3339 specification. Any client that uses +0000 is in violation of the said specification. That you do not control the client is fair, but your API specification states that it should be a date-time, and as such, according to the OpenAPI standard, it should follow the RFC3339 specification.

There are ways that you can inject your own validator(s), but there are people that know more about that than I do, and this ticket is not the right place to address such a request.

@dhpalan
Copy link

dhpalan commented Apr 19, 2022

Since upgrading to the latest version will break our production, We have decided to upgrade our java project in two phases.

If the date-time format does not follow RFC3339 Spec,
Phase 1) Do not reject the client request, instead send them a warning message.
Phase 2) Enforce the validation. We will reject the client request with an error message.

@stevehu
Copy link
Contributor

stevehu commented Apr 19, 2022

@dhpalan I think that is a very good plan to encourage users to use the RFC3339 format as it is the standard.

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

No branches or pull requests

4 participants