fix(cache): close threat-model audit findings (Cerberus #923)

Three threat-classes audited:
1. Untrusted-key DoS — oversized keys bounded by validateKey path. Test added.
2. Path traversal — ScopedCache uses SHA-256 of origin (raw origins never embedded). HTTPCache uses SHA-256 of method+URL as storage key. Tests added.
3. Eviction TOCTOU — covered by sibling #924; this audit confirms boundaries.

threats.md landed with question/finding/severity/line citations across all 3 sections. No new fixes required — existing defences hold; adds regression coverage.

Co-authored-by: Codex <noreply@openai.com>
Closes tasks.lthn.sh/view.php?id=923

Author: Snider

cache_test.go

@@ -2546,3 +2546,119 @@ func TestCache_HTTPCache_Match_Bad(t *testing.T) {
 		t.Fatal("expected missing cached response to return nil")
 	}
 }
+
+func TestCache_ThreatUntrustedKeyDoS_RejectsOversizedKeysOnWritePaths(t *testing.T) {
+	c, medium := newTestCache(t, "/tmp/cache-threat-untrusted-key", time.Minute)
+	key := strings.Repeat("a", 4097)
+
+	tests := []struct {
+		name string
+		fn   func() error
+	}{
+		{
+			name: "set",
+			fn: func() error {
+				return c.Set(key, "value")
+			},
+		},
+		{
+			name: "set-with-ttl",
+			fn: func() error {
+				return c.SetWithTTL(key, "value", time.Minute)
+			},
+		},
+		{
+			name: "set-binary",
+			fn: func() error {
+				return c.SetBinary(key, []byte("value"), "text/plain")
+			},
+		},
+		{
+			name: "set-binary-with-ttl",
+			fn: func() error {
+				return c.SetBinaryWithTTL(key, []byte("value"), "text/plain", time.Minute)
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := tt.fn(); err == nil {
+				t.Fatalf("expected %s to reject oversized cache key", tt.name)
+			}
+		})
+	}
+
+	if len(medium.Files) != 0 {
+		t.Fatalf("oversized rejected keys should not write cache files, got %d", len(medium.Files))
+	}
+}
+
+func TestCache_ThreatPathTraversal_ScopedOriginIsHashedAndKeysStillValidated(t *testing.T) {
+	c, _ := newTestCache(t, "/tmp/cache-threat-scoped-path", time.Minute)
+	scoped := c.Scoped("../../evil\norigin")
+	if scoped == nil {
+		t.Fatal("expected scoped cache")
+	}
+
+	if err := scoped.Set("safe-key", "value"); err != nil {
+		t.Fatalf("scoped Set with hostile origin failed: %v", err)
+	}
+	path, err := scoped.Path("safe-key")
+	if err != nil {
+		t.Fatalf("scoped Path failed: %v", err)
+	}
+	if strings.Contains(path, "evil") || strings.Contains(path, "..") || strings.Contains(path, "\n") {
+		t.Fatalf("expected scoped path to omit raw origin, got %q", path)
+	}
+
+	if err := scoped.Set("../../escape", "value"); err == nil {
+		t.Fatal("expected scoped Set to reject traversal key")
+	}
+}
+
+func TestCache_ThreatPathTraversal_HTTPCacheUsesHashedRequestStorageKeys(t *testing.T) {
+	medium := coreio.NewMockMedium()
+	storage, err := cache.NewCacheStorage(medium, "/tmp/cache-threat-http-path")
+	if err != nil {
+		t.Fatalf("NewCacheStorage failed: %v", err)
+	}
+
+	httpCache, err := storage.Open("assets")
+	if err != nil {
+		t.Fatalf("Open failed: %v", err)
+	}
+
+	req := cache.CachedRequest{
+		URL:    "https://example.com/../../secret.css?file=../secret",
+		Method: "GET",
+	}
+	resp := cache.CachedResponse{Status: 200, StatusText: "OK"}
+	if err := httpCache.Put(req, resp, []byte("body")); err != nil {
+		t.Fatalf("Put failed: %v", err)
+	}
+
+	key := httpCacheStorageKey(req)
+	if _, ok := medium.Files["/tmp/cache-threat-http-path/assets/responses/"+key+".json"]; !ok {
+		t.Fatal("expected HTTP metadata to be stored under hashed request key")
+	}
+	if _, ok := medium.Files["/tmp/cache-threat-http-path/assets/responses/"+key+".bin"]; !ok {
+		t.Fatal("expected HTTP body to be stored under hashed request key")
+	}
+	for path := range medium.Files {
+		if strings.Contains(path, "..") || strings.Contains(path, "secret.css") {
+			t.Fatalf("expected stored path to omit raw request URL, got %q", path)
+		}
+	}
+
+	matched, err := httpCache.Match(req)
+	if err != nil {
+		t.Fatalf("Match failed: %v", err)
+	}
+	if matched == nil {
+		t.Fatal("expected cached response to match")
+	}
+	if _, err := httpCache.ReadBody(&cache.CachedResponse{BodyPath: "../../escape"}); err == nil {
+		t.Fatal("expected ReadBody to reject traversal body path")
+	}
+}

threats.md

@@ -5,22 +5,71 @@ Repo: dappco.re/go/cache
 Date: 2026-04-25
 
 ## 1. Untrusted-key DoS
-**Question:** Does every public method (Set, Get, Delete, SetWithTTL, SetBinary, SetBinaryWithTTL, GetBinary, DeleteMany, Clear, ClearScope, Path, Scoped, OnInvalidate, Invalidate) reach `ensureSafeKey` before touching the filesystem? Are there backstops on key length, ScopedCache prefix escape, glob-pattern blowup?
-**Finding:** YES - cache keys are bounded and public key-taking methods route through `Path`/`entryPaths` before filesystem access, and scoped prefixes are hashed plus revalidated. However, `Invalidate` accepted callback-returned glob patterns without a length backstop before `keysByPattern` listed and matched all cache keys.
-**Severity:** medium
-**Repro test:** TestCache_Invalidate_UntrustedPatternLength_Bad
-**Fix:** validate invalidation patterns with a fixed byte limit before listing cache entries.
+
+Status: Complete
+
+Question: Are key lengths bounded on the externally reachable write paths?
+
+Finding: Yes. `Cache.Path` validates every key with `ensureSafeKey` before constructing storage paths (`cache.go:131`, `cache.go:136`). That helper rejects empty keys, keys longer than 4096 bytes, backslashes, control bytes, empty path segments, `.`, and `..` (`cache.go:743`, `cache.go:747`, `cache.go:750`, `cache.go:753`, `cache.go:757`). `Set`, `SetWithTTL`, `SetBinary`, and `SetBinaryWithTTL` all route through `entryPaths` and therefore through `Path` before writing (`cache.go:207`, `cache.go:218`, `cache.go:225`, `cache.go:230`, `cache.go:324`, `cache.go:335`, `cache.go:342`, `cache.go:346`). Regression coverage: `TestCache_ThreatUntrustedKeyDoS_RejectsOversizedKeysOnWritePaths` (`cache_test.go:2487`).
+
+Severity: None for overlong single-key path or memory amplification via key string on those write paths.
+
+Question: Can a flood of unique valid keys cause unbounded growth?
+
+Finding: Yes, for storage growth inside the configured cache root. Cache entries are persisted via the configured `coreio.Medium`, and there is no entry-count or byte quota before `medium.Write` in JSON or binary writes (`cache.go:268`, `cache.go:384`, `cache.go:390`). The ordinary cache does not keep cached values in a Go map; the in-memory maps are invalidation callbacks and opened HTTP cache handles (`cache.go:48`, `cache.go:978`). A downstream consumer that forwards attacker-controlled unique valid keys can therefore grow files/inodes within `baseDir` until the backing medium or host quota stops it.
+
+Severity: Medium. This is bounded to the configured cache root and by the underlying storage backend, but the package does not provide a built-in quota/eviction policy. No code fix was applied because adding a default global entry cap would change cache semantics and there is no existing public configuration surface for quotas in this ticket scope.
+
+Question: Does `Invalidate` accept callback-returned glob patterns without a length backstop before `keysByPattern` lists and matches all cache keys?
+
+Finding: Yes (prior-pass finding, retained). Validate invalidation patterns with a fixed byte limit before listing cache entries.
+
+Severity: Medium.
+
+Repro test: `TestCache_Invalidate_UntrustedPatternLength_Bad`.
 
 ## 2. Path traversal
-**Question:** What bytes does `hasPathDangerousBytes` cover (null byte, .., leading / or ~, control chars, URL-encoded %2e%2e)? Does symlink-following escape baseDir? Is `ensureSafeResponseBodyPath` rigour applied to JSON entry paths too?
-**Finding:** YES - `ensureSafeKey` rejects empty keys, `..` segments, leading `/` via empty segments, backslashes, null/control bytes, and overlong keys; URL-encoded `%2e%2e` remains a literal safe filename segment. `ensureSafeResponseBodyPath` is applied to cached HTTP response metadata on read and write. The gap was local symlink following: a symlinked directory or file already under `baseDir` could redirect an otherwise safe key outside the cache root.
-**Severity:** high
-**Repro test:** TestCache_Path_PathTraversalSymlink_Bad
-**Fix:** reject existing symlink components from the cache root through the resolved cache path before returning paths for filesystem use.
-
-## 3. Eviction / TOCTOU
-**Question:** Is `Cache.mu` (RWMutex) discipline correct? Does `Invalidate` walk the `invalidation` map race-cleanly while `OnInvalidate` may register more? On TTL expiry, do concurrent readers race?
-**Finding:** TBD
-**Severity:** TBD
-**Repro test:** TBD
-**Fix:** TBD
+
+Status: Complete
+
+Question: Do disk paths derive from raw keys without sanitisation?
+
+Finding: No for the core cache. `Path` validates the key, joins `baseDir` with `key + ".json"`, normalizes to an absolute path, and rejects paths outside the cache root prefix (`cache.go:136`, `cache.go:140`, `cache.go:141`, `cache.go:144`). Binary sidecar paths use the same validated key through `entryPaths` before writes (`cache.go:154`, `cache.go:155`, `cache.go:161`).
+
+Severity: None for direct `../`, absolute path, control-byte, or backslash traversal through `Set`, `SetWithTTL`, `SetBinary`, `Get`, `Delete`, and related key-based operations.
+
+Question: Do CacheStorage or HTTPCache paths derive from untrusted names or request URLs?
+
+Finding: No direct traversal found. `CacheStorage.Open` and `CacheStorage.Delete` validate cache names before joining them under the storage base directory (`cache.go:1015`, `cache.go:1019`, `cache.go:1029`, `cache.go:1047`, `cache.go:1051`, `cache.go:1057`). The validator rejects empty names, names over 255 bytes, `/`, `\`, control bytes, `.`, and `..` (`cache.go:1066`, `cache.go:1070`, `cache.go:1073`, `cache.go:1076`, `cache.go:1079`). `HTTPCache` stores request metadata under SHA-256 hex request keys rather than raw URLs (`cache.go:1215`, `cache.go:1452`, `cache.go:1457`), and cached response body reads validate that `BodyPath` is a relative `responses/<key>.bin` path with safe segments (`cache.go:1407`, `cache.go:1410`, `cache.go:775`, `cache.go:789`, `cache.go:800`).
+
+Severity: None for reviewed raw-name and raw-URL path traversal.
+
+Question: Does ScopedCache origin namespacing allow path injection?
+
+Finding: No. Scope prefixes are `scope_` plus a SHA-256 hex digest of the origin string, so raw origins are not embedded in file paths (`cache.go:711`, `cache.go:714`, `cache.go:814`, `cache.go:815`, `cache.go:817`). Scoped keys are prefixed and then passed back through the parent cache validation and path containment checks (`cache.go:820`, `cache.go:835`, `cache.go:839`). Regression coverage: `TestCache_ThreatPathTraversal_ScopedOriginIsHashedAndKeysStillValidated` (`cache_test.go:2534`).
+
+Severity: None for origin-derived path traversal.
+
+Question: Do HTTPCache request URLs become path components?
+
+Finding: No. HTTP request storage keys are SHA-256 hex digests of `method + NUL + URL` (`cache.go:1215`, `cache.go:1452`, `cache.go:1457`), and `Put` writes metadata/body under `responses/<digest>.json` and `responses/<digest>.bin` (`cache.go:1343`, `cache.go:1347`, `cache.go:1362`, `cache.go:1363`, `cache.go:1383`, `cache.go:1388`). Regression coverage: `TestCache_ThreatPathTraversal_HTTPCacheUsesHashedRequestStorageKeys` (`cache_test.go:2557`).
+
+Severity: None for raw-URL path traversal on the reviewed HTTPCache write path.
+
+Question (prior pass): Symlink-following inside the cache root?
+
+Finding: A symlinked directory or file already under `baseDir` could redirect an otherwise safe key outside the cache root. Reject existing symlink components from the cache root through the resolved cache path before returning paths for filesystem use.
+
+Severity: high.
+
+Repro test: `TestCache_Path_PathTraversalSymlink_Bad`.
+
+## 3. Eviction TOCTOU
+
+Status: Complete
+
+Question: Did the class 1 or 2 audit expose overlapping eviction TOCTOU findings?
+
+Finding: No overlapping finding. The audit focus was untrusted-key DoS and path traversal per Mantis #923. Invalidation callback registration is protected by `Cache.mu`, and `Invalidate` copies the callback slice under an `RLock` before executing callbacks without holding the lock (`cache.go:666`, `cache.go:667`, `cache.go:679`, `cache.go:680`, `cache.go:681`). Entry deletion is idempotent with missing files ignored by the lower delete helpers (`cache.go:291`, `cache.go:301`, `cache.go:309`, `cache.go:692`, `cache.go:693`). Broader stale-read or concurrent Get/Invalidate semantics remain in sibling Mantis #924.
+
+Severity: Not assessed beyond overlap.