Skip to content

fix: Do not reject a maximal-length number followed by a delimiter - #15

Open
youdie006 wants to merge 1 commit into
dunglas:mainfrom
youdie006:fix-maximal-length-number
Open

fix: Do not reject a maximal-length number followed by a delimiter#15
youdie006 wants to merge 1 commit into
dunglas:mainfrom
youdie006:fix-maximal-length-number

Conversation

@youdie006

Copy link
Copy Markdown

What this fixes

parseNumber (integer.go:64) checks the length bound at the top of the loop, before looking at the next character:

for s.off < len(s.data) {
    size := s.off - start
    if (t == typeInteger && (size >= 15)) || size >= 16 {
        return 0, &UnmarshalError{s.off, ErrNumberOutOfRange}
    }

    c := s.data[s.off]

RFC 9651 4.2.4 appends the character to input_number first and fails only if input_number then 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 +/-999999999999999 range, and integer_test.go:20 pins that 999999999999999 serializes. So:

l := httpsfv.List{httpsfv.NewItem(int64(123456789012345)), httpsfv.NewItem(int64(1))}
s, _ := httpsfv.Marshal(l)              // "123456789012345, 1"
_, err := httpsfv.UnmarshalList([]string{s})
Marshal   -> "123456789012345, 1" err=<nil>
Unmarshal -> err=integer or decimal out of range: character 15

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-tests number.json gained 4df10df "Add more number tests", which includes:

  • long integer followed by comma -- raw: ["123456789012345, 1"], header_type: list
  • decimal, followed by comma -- raw: ["123456789012.123, 1.1"]

With the submodule moved to upstream HEAD, both fail on main:

httpwg_test.go:190: number.json: long integer followed by comma: must not fail, got error integer or decimal out of range: character 15
httpwg_test.go:190: number.json: decimal, followed by comma: must not fail, got error integer or decimal out of range: character 16

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:

  • The submodule is pinned at 7970aff (2025-02-11); upstream is 1e280c3 (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 string and display-string.json / two lines display string, both can_fail: true, about how Unmarshal* joins multi-line []string input). 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 no submodules: key, and httpwg_test.go:146 does f, _ := 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 at integer.go:75 already 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 ./... is ok github.com/dunglas/httpsfv. go vet ./... clean. gofmt -l . lists only dictionary.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 TestParseIntegerOrDecimal table, in the file's own style -- no corpus vendored. On main they give:

integer_test.go:80: parseIntegerOrDecimal(123456789012345, 1) = 0, integer or decimal out of range: character 15; 123456789012345, <nil> expected
integer_test.go:80: parseIntegerOrDecimal(123456789012.123, 1.1) = 0, integer or decimal out of range: character 16; 1.23456789012123e+11, <nil> expected

I mutation-checked the bound, and two of the four mutations survive. I would rather report that than leave you to find it:

mutation result
put the check back at the loop top fails, both new rows
relax the integer bound to > 16 passes
relax the decimal bound to > 17 passes
write it as >= 16 / >= 17 passes

The last is expected -- it is the same predicate. The middle two survive because this length check is a redundant early exit: parseInteger at integer.go:119 already rejects anything outside +/-999999999999999, and the '.' and parseDecimal checks 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 ErrNumberOutOfRange now parse. I checked the boundary literals already in the tests -- {"1234567890123456", 0, true}, {"123456789012345.6", 0, true}, {"1234567890123.", 0, true} and the serialization rows at integer_test.go:19-22 all 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.

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.
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

Successfully merging this pull request may close these issues.

1 participant