From 674ce330761334b01cafc3baeb64413086569c3a Mon Sep 17 00:00:00 2001 From: jdtw Date: Fri, 31 Jul 2026 12:49:16 -0700 Subject: [PATCH 1/2] Fix MapVerifier so pruning is scheduled once per window, not every call 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 --- nonce/nonce.go | 1 + nonce/nonce_test.go | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/nonce/nonce.go b/nonce/nonce.go index 23e846a..975d6c0 100644 --- a/nonce/nonce.go +++ b/nonce/nonce.go @@ -55,6 +55,7 @@ func (m *MapVerifier) Verify(nonce []byte, expires time.Time) error { defer m.Unlock() // Schedule a prune if needed... if now := time.Now(); now.Sub(m.pruned) >= m.pruneEvery { + m.pruned = now m.wg.Add(1) go func() { defer m.wg.Done() diff --git a/nonce/nonce_test.go b/nonce/nonce_test.go index 752032f..c01f91e 100644 --- a/nonce/nonce_test.go +++ b/nonce/nonce_test.go @@ -56,6 +56,27 @@ func TestPrune(t *testing.T) { } } +// TestPruneAdvancesPruned guards against a bug where m.pruned was never +// written back to after being set in the constructor, so the "has a prune +// window elapsed" check in Verify was always true once pruneEvery had +// passed a single time -- spawning a new prune goroutine on every +// subsequent call forever, instead of once per pruneEvery window. +func TestPruneAdvancesPruned(t *testing.T) { + nv := NewMapVerifier(time.Hour) + // Force the prune condition to trigger on the next Verify, as in TestPruneEvery. + nv.pruned = nv.pruned.Add(-nv.pruneEvery) + forced := nv.pruned + + if err := nv.Verify([]byte("YELLOW SUBMARINE"), time.Now().Add(time.Hour)); err != nil { + t.Fatal(err) + } + nv.wg.Wait() + + if !nv.pruned.After(forced) { + t.Fatalf("nv.pruned = %v, want a time after %v -- it should advance when a prune is scheduled, otherwise every subsequent Verify call re-triggers a prune", nv.pruned, forced) + } +} + func TestPruneEvery(t *testing.T) { nv := NewMapVerifier(time.Hour) // Add an expired nonce and ensure that it is not pruned on Verify since the delta hasn't elapsed... From b52f75b9d645bcba06b31beb965b34b40109ca82 Mon Sep 17 00:00:00 2001 From: jdtw Date: Fri, 31 Jul 2026 12:49:24 -0700 Subject: [PATCH 2/2] Stop double-unescaping the path in serverResource 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 --- http.go | 12 ++++++------ http_test.go | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/http.go b/http.go index 3e8d339..80cecd0 100644 --- a/http.go +++ b/http.go @@ -4,7 +4,6 @@ import ( "encoding/base64" "fmt" "net/http" - "net/url" "strings" "time" @@ -64,9 +63,10 @@ func clientResource(r *http.Request) string { } func serverResource(r *http.Request) string { - path, err := url.PathUnescape(r.URL.Path) - if err != nil { - path = r.URL.Path - } - return fmt.Sprintf("%s %s%s", r.Method, r.Host, path) + // r.URL.Path is already percent-decoded once by net/http; unescaping + // it again here would compute a different resource than the client + // signed (which uses its own, singly-decoded r.URL.Path), rejecting + // legitimate requests whose path contains a literal '%' followed by + // hex digits. + return fmt.Sprintf("%s %s%s", r.Method, r.Host, r.URL.Path) } diff --git a/http_test.go b/http_test.go index 85aaf2f..39fe7f5 100644 --- a/http_test.go +++ b/http_test.go @@ -214,3 +214,21 @@ func TestUnicode(t *testing.T) { t.Fatalf("want subject bob, got %q", r.Subject) } } + +// TestPercentEncodedPath guards against a bug where serverResource +// unescaped r.URL.Path a second time (it's already decoded once by +// net/http), so a path segment containing a literal '%' followed by valid +// hex digits (like "%41") would decode differently -- and thus mismatch -- +// on the server than on the client, rejecting an otherwise legitimate, +// correctly-signed request. +func TestPercentEncodedPath(t *testing.T) { + ks, priv := generateKey(t, "carol") + url := startServer(t, ks) + r := get(t, url+"/10%2541", authorize(priv)) + if r.Err != "" { + t.Fatal(r.Err) + } + if r.Subject != "carol" { + t.Fatalf("want subject carol, got %q", r.Subject) + } +}