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) + } +} 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...