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

Fix NumberInput.looksLikeValidNumber() implementation #1241

Merged
merged 6 commits into from
Mar 23, 2024

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Mar 16, 2024

See FasterXML/jackson-databind#4435

I'm not a regex expert and sort of hate them from a code readability and maintenance perspective.

This is my attempt at fixing the issue but the method is now so neutered, I really wonder if the method should be removed. It is also going to cause perf issues as the check is non-trivial.

@EAlf91
Copy link

EAlf91 commented Mar 16, 2024

Thanks for dealing with this topic :D
IMHO it can be removed completely. According to RFC-8259 the rules for strings should be applied here for parsing from string inputs in jackson-databind and not number in this case. (see JsonTokenId.ID_STRING in NumberDeserializers.java)

It seems that we're encountering this error because the prevalidation method looksLikeValidNumber is misplaced for the given input, which is JSON-Type string and not number.

@cowtowncoder cowtowncoder changed the title fix regression caused by looksLikeValidNumber Fix NumberInput.looksLikeValidNumber() implementation Mar 23, 2024
@cowtowncoder cowtowncoder added the 2.17 Issues planned (at earliest) for 2.17 label Mar 23, 2024
@cowtowncoder cowtowncoder added this to the 2.17.1 milestone Mar 23, 2024
@cowtowncoder
Copy link
Member

(I thought I had added a comment here).

No. this cannot and should not be removed: while the location of method may be misplaced (it is really only needed by jackson-databind, not core streaming itself), it serves necessary function as can be seen from NumberDeserializers.
It has nothing to do wrt general validation of JSON Strings but needed for heuristics on when to apply maximum number length checks to avoid possibly DoS for decoding "too long" floating-point numbers.

Thank you @pjfanning for PR, will merge to get in 2.17.1

@cowtowncoder cowtowncoder merged commit c73bde2 into FasterXML:2.17 Mar 23, 2024
5 checks passed
@pjfanning pjfanning deleted the num-starting-dot branch March 26, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 Issues planned (at earliest) for 2.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants