fix(uri): check escape sequence bounds on NormalizeEscapedPath slow path - #1732
Open
shuvamk wants to merge 1 commit into
Open
fix(uri): check escape sequence bounds on NormalizeEscapedPath slow path#1732shuvamk wants to merge 1 commit into
shuvamk wants to merge 1 commit into
Conversation
NormalizeEscapedPath documents that it returns an empty string and false when s
contains an invalid escape sequence, but only the fast path checks for one. Once
a lowercase hex digit or a redundant escape sends it to the slow path, the
remaining bytes are read without a bounds or hex check.
openapi/parser/parser.go:243 passes raw spec path keys straight to it, so a
document whose path key is "/caf%e9/100%" kills the generator at
uri/normalize.go:98 with a raw stack trace:
panic: runtime error: index out of range [12] with length 12
while the same typo without the earlier escape, "/report%", is reported the way
a user expects:
- spec2.json:5:5 -> parse path "/report%": parse "/report%": invalid URL escape "%"
Trailing "%zz" on the slow path did not panic, but returned ("/foo%3Fbar%zz",
true) — a silent accept of an invalid escape. Checking bounds and hex digits
before indexing covers both.
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.
A spec whose path key has an escape typo crashes
ogenwith a raw Go stack trace — but only when an earlier escape in the same path is lowercase or redundant."/report%"(no earlier escape) is reported the way a user expects:"/caf%e9/100%"— the same trailing%, with a lowercase escape in front of it — instead prints:with
uri.NormalizeEscapedPathaturi/normalize.go:98on top, called fromopenapi/parser/parser.go:243.NormalizeEscapedPathdocuments that it "returns empty string and false" for an invalid escape sequence, and the fast loop does check for one. But once a lowercase hex digit or a redundant escape triggersgoto slow, the slow loop readss[i+1], s[i+2]with no bounds or hex check.openapi/parser/parser.go:243passes raw spec path keys to it with nourl.Parsein front, so the panic reaches the CLI.The same gap also makes the function return
truefor input it should reject: onmain,NormalizeEscapedPath("/foo%3fbar%zz")returns("/foo%3Fbar%zz", true).The fix checks bounds and hex digits before indexing, mirroring the fast loop. With it,
"/caf%e9/100%"produces the same located parse error"/report%"already did.Four rows added to the
TestNormalizeEscapedPathtable reach the slow path, which the three existing invalid rows do not. I checked they fail with onlyuri/normalize.goreverted — the truncated ones panic, the%zzone fails onShould be false— and pass with the fix.go test ./...,./go.test.sh,cd examples && go test ./...andgolangci-lint runare clean locally, andmake generate examplesleaves a 0-file diff.