fix: Do not reject a maximal-length number followed by a delimiter - #15
Open
youdie006 wants to merge 1 commit into
Open
fix: Do not reject a maximal-length number followed by a delimiter#15youdie006 wants to merge 1 commit into
youdie006 wants to merge 1 commit into
Conversation
parseNumber evaluates the length bound at the top of the loop, before looking at the next character, so a 15-digit integer or 16-character decimal fails as soon as anything follows it. RFC 9651 section 4.2.4 appends the character first and fails only when input_number already holds more than 15 or 16 characters, so those inputs are valid. marshalInteger allows the full +/-999999999999999 range, so the library emits headers it cannot read back: Marshal of a list containing 123456789012345 and 1 gives '123456789012345, 1', and UnmarshalList of that string returns 'integer or decimal out of range: character 15'. Move the check after the digit is consumed and compare with > instead of >=, matching the form the '.' branch already uses.
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.
What this fixes
parseNumber(integer.go:64) checks the length bound at the top of the loop, before looking at the next character:RFC 9651 4.2.4 appends the character to
input_numberfirst and fails only ifinput_numberthen contains more than 15 (integer) or 16 (decimal) characters. So a maximal-length number is valid; it just cannot be followed by another digit. Here it fails as soon as anything follows it -- a comma, a semicolon, an equals sign.The library emits headers it cannot read back
marshalInteger(integer.go:28) allows the full+/-999999999999999range, andinteger_test.go:20pins that999999999999999serializes. So:The same happens for a dictionary (
a=999999999999999, b=1) and for an item with parameters (123456789012345;x).The official suite has cases for this
httpwg/structured-field-testsnumber.jsongained4df10df "Add more number tests", which includes:long integer followed by comma--raw: ["123456789012345, 1"],header_type: listdecimal, followed by comma--raw: ["123456789012.123, 1.1"]With the submodule moved to upstream HEAD, both fail on
main:They pass with this change. I ran the whole upstream corpus: 2135 cases, 1403 of them
must_fail-- 4 failures before, 2 after, and zero must-fail regressions.Two notes on the suite, offered as observations rather than changes:
7970aff(2025-02-11); upstream is1e280c3(2026-06-09), 25 commits ahead. I have not bumped it here, because the remaining 2 failures are in a different area (string.json / two lines stringanddisplay-string.json / two lines display string, bothcan_fail: true, about howUnmarshal*joins multi-line[]stringinput). Bumping would turn this into a red PR. Happy to do the bump plus that fix as a separate one if you want it..github/workflows/ci.yaml's checkout step has nosubmodules:key, andhttpwg_test.go:146doesf, _ := os.Open(dir)and ignores the error -- so when the submodule is absent the loop runs zero cases and the suite test passes silently. That is why CI stays green regardless of the pin.The fix
Move the bound inside the digit branch, after
s.off++, and compare with>instead of>=. The'.'branch atinteger.go:75already uses the strictly-greater form; the loop-top check was the odd one out.Verification
With the submodule at its current pin -- i.e. what CI runs today --
go test -count=1 ./...isok github.com/dunglas/httpsfv.go vet ./...clean.gofmt -l .lists onlydictionary.go, which is pre-existing (confirmed by stashing my diff and re-running); both files I touched are clean.Two rows added to the existing
TestParseIntegerOrDecimaltable, in the file's own style -- no corpus vendored. Onmainthey give:I mutation-checked the bound, and two of the four mutations survive. I would rather report that than leave you to find it:
> 16> 17>= 16/>= 17The last is expected -- it is the same predicate. The middle two survive because this length check is a redundant early exit:
parseIntegeratinteger.go:119already rejects anything outside+/-999999999999999, and the'.'andparseDecimalchecks cover over-long decimals. That is precisely why tightening it by one had no upside and only produced false rejections. If you would rather I add a row that pins the loop bound itself, say so and I will.Behaviour change
Inputs that previously returned
ErrNumberOutOfRangenow parse. I checked the boundary literals already in the tests --{"1234567890123456", 0, true},{"123456789012345.6", 0, true},{"1234567890123.", 0, true}and the serialization rows atinteger_test.go:19-22all still pass. No existing test row had to change.Disclosure: AI-assisted. I found and prepared this with an AI assistant, and I ran and verified everything above myself.