fix(validate): parse numeric string constraints with strconv.ParseFloat - #1728
Merged
Conversation
`String.validateNumeric` used `fmt.Sscanf(v, "%f", &val)`, which stops at the
first byte that cannot continue a float and discards the remainder without
reporting an error. It also accepts `NaN` and `Inf`, and `NaN` compares false
against both `<` and `>`, so it defeats the minimum and maximum checks it was
just parsed for.
For `amount: {type: string, minimum: 1, maximum: 10}` a generated server
returned 200 and passed the value to the handler:
{"amount":"5abc"} -> 200 (expected 400)
{"amount":"5,000"} -> 200 (expected 400)
{"amount":"NaN"} -> 200 (expected 400)
{"amount":"nan"} -> 200 (expected 400)
`Inf` escapes a one-sided bound the same way: with only `minimum` set, `Inf`
is accepted; with only `maximum` set, `-Inf` is accepted.
`Sscanf` also loses the value in the error it does produce: `"abc"` reported
`parsing ""` because `%f` consumed nothing before failing.
Replace it with `strconv.ParseFloat`, which requires the whole string to be
consumed, then reject non-finite results. `Float.Validate` already rejects
NaN and Inf the same way.
Introduced in ad9d3c7 (cross-type constraint interpretation, on by default),
released in v1.17.0 through v1.23.0. No test covered `validateNumeric`.
Behaviour narrowing to note: `Sscanf` skipped leading and trailing whitespace,
so `" 5"` and `"5 "` were accepted and are now rejected. `strings.TrimSpace`
before `ParseFloat` would keep them. A differential sweep over 3,000,000
generated inputs plus 55 hand-picked ones found every difference to be in the
reject-more direction: no input is newly accepted and no accepted input
changes value.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cross-type numeric constraints on strings (
type: stringwithminimum/maximum, interpreted by default since #1580) accept values that are not numbers.Spec:
Generated server (
go run ./cmd/ogen, stock options),POST /pay:main{"amount":"5abc"}"5abc"{"amount":"5,000"}"5,000"{"amount":"NaN"}"NaN"{"amount":"nan"}"nan"{"amount":"11"}{"amount":"abc"}parsing ""— the value is goneInfescapes a one-sided bound the same way. With onlyminimum: 1,"Inf"and"Infinity"validate; with onlymaximum: 10,"-Inf"validates.Cause
validate/string.go:171:fmt.Sscanfwith%fstops at the first byte that cannot continue a float and returns successfully with whatever it managed to read; the rest of the input is discarded and not reported."5abc"parses as5,"5,000"as5,"1.2.3"as1.2. It also acceptsNaNandInf, andNaNcompares false against both<and>, so it defeats the very bounds it was parsed for.The value loss in the error message has the same root: for
"abc"the verb consumes nothing, sostrconv.ParseFloatis handed the empty string.Introduced in
ad9d3c73(feat: enable cross-type constraint interpretation by default), whose own commit message states the intent as "Parses string as float64, validates numeric bounds" — so this is a bug, not a deliberate choice.git tag --contains ad9d3c73→ v1.17.0 … v1.23.0, and it is an ancestor ofv1.23.0, so every release since v1.17.0 is affected.Nothing covered
validateNumeric:git grep -nE "Numeric|Sscanf" main -- '*_test.go'returns no hits.jsonschema/cross_type_constraints_test.gois parser-side only and asserts thatMaximumsurvives parsing; it pins no runtime behaviour.Fix
strconv.ParseFloat, which requires the whole string to be consumed, then reject non-finite results:The two guards are copied from
Float.Validate(validate/float.go:71-79), which already rejects NaN and Inf with exactly this wording.One file, +10/−4, plus the test.
make generate examplesproduces a 0-file diff —validateis a runtime library, socheck-generateis unaffected.Two things worth disclosing
1. This narrows behaviour slightly beyond the bug.
Sscanfskipped leading and trailing whitespace, so" 5","5 "and" 12 "were accepted and are now rejected. If you would rather keep those working,strings.TrimSpace(v)beforeParseFloatis a one-line change and I am happy to switch — the two test rowsLeadingSpace/TrailingSpaceare the only ones that flip.I ran an old-vs-new differential sweep before claiming this is otherwise safe: 55 hand-picked inputs plus 3,000,000 random strings of length 1–8 over the alphabet
0-9 + - . _ , e E x X p P a A b B c C n N i I f F, space and tab. Result: 0 newly accepted, 0 changed values, 657,226 newly rejected — every difference is in the reject-more direction. Unchanged accepts include+5,.5,5.,007,1e5,0x1p-2,1_0. Newly rejected beyond the cases above:"5%","1.2.3","0b101","0o17","1p5"(Sscanfreads these as5,1.2,0,0and32).2. There is an in-repo inconsistency that cuts against the finite check.
Float.ValidateStringified(validate/float.go:82) deliberately permits NaN and Inf —d2d85ae4, "AllowNaNandinfinity". That is a different method for a different case (type: numbertransported as a string, where the JSON decoder produced a real float), andFloat.Validateis the in-package precedent for rejecting non-finite, which is why I followed it here. But if you prefer consistency withValidateStringified, say so and I will drop the two guards and keep only the trailing-garbage fix. Note that dropping them leavesNaNdefeating both bounds, which is the more severe half of the report.Tests
TestString_ValidateNumericinvalidate/string_test.go, a table test in the shape of the neighbouringTestFloat_Validate. 23 cases across three validator configurations (min+max, min-only, max-only), covering both directions: the six accepts guard against over-narrowing, and the min-only/max-only rows are what pin theInfbehaviour that a two-sided bound would mask.Verified the test fails without the fix. With
validate/string.goreverted tomainand the test kept:12 of 23 subtests fail; restoring the fix turns them all green. The same generated server used for the table above now returns 400 for every bad row and still returns 200 for
{"amount":"5"}.Local gates
Run on go1.25.10 darwin/arm64,
CGO_ENABLED=0, caches cleared. Baseline onmain@2e755d12was identical (36ok, 0 failures) — no pre-existing red.go build ./...go test --timeout 15m ./...ok, 0 FAIL, 50[no test files]./go.test.sh(race)cd examples && go test ./...ok, 0 FAILgolangci-lint v2.12.2 run -c .golangci.yml ./...make generate examples+git diff