Fix resource double-decode and nonce prune-scheduling bugs - #3
Merged
Conversation
m.pruned was set in the constructor but never written back to, so the "has pruneEvery elapsed" check in Verify was true for every call after the first window passed -- spawning a new goroutine to lock and rebuild the entire seen map on every single subsequent Verify, instead of once per pruneEvery window as documented. Fix by writing m.pruned = now synchronously inside Verify's existing critical section at the moment a prune is scheduled, so concurrent/ later callers see the update immediately rather than waiting for the async prune goroutine to (never) report back. Added TestPruneAdvancesPruned, which fails against the old code (confirmed via git stash) and passes against the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
r.URL.Path is already percent-decoded once by net/http before it ever reaches serverResource. Unescaping it again meant a path segment containing a literal '%' followed by two hex digits (e.g. "%41", a valid path character sequence) would decode further on the server than it did on the client -- clientResource only ever decodes once, via its own r.URL.Path -- producing a different resource string and rejecting an otherwise legitimately-signed request. More importantly, this broke the core invariant the library documents: the resource the client signs must equal the resource the server checks. A consumer that scopes tokens to specific resources (this library's primary use case) could have that scoping bypassed by a crafted path whose double-decoded form collides with a resource the caller actually holds a valid token for, even though the request as routed/acted on by the consuming app uses the singly-decoded path. Added TestPercentEncodedPath, an end-to-end round trip through a real server that fails against the old code with: invalid resource: got "GET host/10%41", want "GET host/10A" and passes against the fix (confirmed via git stash). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
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
1 participant
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.
Summary
serverResource(http.go) unescapedr.URL.Patha second time -- it's already percent-decoded once bynet/http. A path segment containing a literal%followed by two hex digits (e.g.%41) would decode further on the server than on the client, computing a different resource string than the one that was actually signed. Beyond rejecting legitimate requests, this breaks the library's core invariant that the resource the client signs must equal the resource the server checks -- a consumer relying on per-resource-scoped tokens could have that scoping bypassed.MapVerifier.pruned(nonce/nonce.go) was set once in the constructor and never written back to, so the "has a prune window elapsed" check inVerifywas true for every call after the firstpruneEverywindow passed, not just the first one per window -- spawning a new goroutine to lock and rebuild the entire nonce map on every single subsequentVerifycall.Both were verified with reproductions that fail against the pre-fix code (confirmed via
git stash) and pass against the fix.Test plan
go build ./...go vet ./...gofmt -l .(clean)go test ./...andgo test ./... -raceTestPercentEncodedPath(http_test.go) andTestPruneAdvancesPruned(nonce/nonce_test.go), each confirmed to fail against the old code and pass against the fix🤖 Generated with Claude Code