Fix resource double-decode and nonce prune-scheduling bugs

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

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

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()

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