From 83f2951035bb01352ff35605680fad9e3a21fb54 Mon Sep 17 00:00:00 2001
From: Ajay Ghanwat <ghanwat.ajay@workroot.in>
Date: Tue, 4 Aug 2026 17:00:35 +0530
Subject: [PATCH] docs: fix cache-key collision in the Task 4 plan snippet

The plan's cachedPermissionStore used scopeKey + U+FFFD + subjectId as a
cache key with no escaping, so ('a', 'b<sep>c') and ('a<sep>b', 'c') collide
and one subject is served another's permissions. Subject and tenant ids are
unconstrained strings, so nothing prevented it.

Key is now JSON-encoded, and the global-write sweep tracks keys per subject
instead of substring-matching. Adds the two regression tests that were
missing: cross-tenant invalidation on a global write, and key collision.

Ruled by the human as plan-mandated; source of truth amended so a re-run of
the plan does not reintroduce the defect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 ...-08-04-authz-permissions-implementation.md | 54 ++++++++++++++++---
 1 file changed, 47 insertions(+), 7 deletions(-)

diff --git a/docs/plans/2026-08-04-authz-permissions-implementation.md b/docs/plans/2026-08-04-authz-permissions-implementation.md
index 8294afaf..e50606d1 100644
--- a/docs/plans/2026-08-04-authz-permissions-implementation.md
+++ b/docs/plans/2026-08-04-authz-permissions-implementation.md
@@ -809,6 +809,26 @@ describe("cachedPermissionStore", () => {
     expect(store.size()).toBeLessThanOrEqual(2);
   });
 
+  test("a global write invalidates the subject in every tenant", async () => {
+    const inner = memoryPermissionStore();
+    const store = cachedPermissionStore(inner, { ttlMs: 60_000 });
+    await store.assignmentsFor("u1", { tenantId: "t1" }); // warm the tenant entry
+    await store.assignRole("u1", "editor"); // global write
+    // Global roles are visible inside every tenant, so the cached t1 entry
+    // must not survive this write.
+    expect((await store.assignmentsFor("u1", { tenantId: "t1" })).roles).toEqual(["editor"]);
+  });
+
+  test("cache keys cannot collide across subject/tenant boundaries", async () => {
+    const inner = memoryPermissionStore();
+    const store = cachedPermissionStore(inner, { ttlMs: 60_000 });
+    // Naive "scope + separator + subject" concatenation makes these two pairs
+    // produce the same key, serving one subject the other's permissions.
+    await inner.assignRole("b�c", "editor", { tenantId: "a" });
+    expect((await store.assignmentsFor("b�c", { tenantId: "a" })).roles).toEqual(["editor"]);
+    expect((await store.assignmentsFor("c", { tenantId: "a�b" })).roles).toEqual([]);
+  });
+
   test("scoped and global reads cache separately", async () => {
     const inner = memoryPermissionStore();
     const store = cachedPermissionStore(inner, { ttlMs: 60_000 });
@@ -853,15 +873,25 @@ export function cachedPermissionStore(
   const max = options.max ?? 1_000;
   const entries = new Map<string, { at: number; value: SubjectAssignments }>();
 
-  const cacheKey = (subjectId: string, scope?: AuthzScope) => `${scopeKey(scope)}�${subjectId}`;
+  // Subject and tenant ids are unconstrained strings, so the key must be
+  // unambiguous: concatenating around a separator lets ("a", "b<sep>c") and
+  // ("a<sep>b", "c") collide, which would serve one subject another's
+  // permissions. JSON encoding escapes the components.
+  const cacheKey = (subjectId: string, scope?: AuthzScope) =>
+    JSON.stringify([scopeKey(scope), subjectId]);
+  // Track subjects separately rather than pattern-matching key strings, so a
+  // global write can find every tenant entry without substring guesswork.
+  const bySubject = new Map<string, Set<string>>();
   const drop = (subjectId: string, scope?: AuthzScope) => {
-    entries.delete(cacheKey(subjectId, scope));
     // A global write changes what every tenant sees for that subject.
     if (scopeKey(scope) === "") {
-      for (const key of [...entries.keys()]) {
-        if (key.endsWith(`�${subjectId}`)) entries.delete(key);
-      }
+      for (const key of bySubject.get(subjectId) ?? []) entries.delete(key);
+      bySubject.delete(subjectId);
+      return;
     }
+    const key = cacheKey(subjectId, scope);
+    entries.delete(key);
+    bySubject.get(subjectId)?.delete(key);
   };
 
   return {
@@ -870,8 +900,15 @@ export function cachedPermissionStore(
       const hit = entries.get(key);
       if (hit && Date.now() - hit.at < ttlMs) return hit.value;
       const value = await inner.assignmentsFor(subjectId, scope);
-      if (entries.size >= max) entries.delete(entries.keys().next().value!);
+      if (entries.size >= max) {
+        const oldest = entries.keys().next().value!;
+        entries.delete(oldest);
+        for (const keys of bySubject.values()) keys.delete(oldest);
+      }
       entries.set(key, { at: Date.now(), value });
+      let keys = bySubject.get(subjectId);
+      if (!keys) bySubject.set(subjectId, (keys = new Set()));
+      keys.add(key);
       return value;
     },
     async assignRole(subjectId, role, scope) {
@@ -892,7 +929,10 @@ export function cachedPermissionStore(
     },
     listSubjects: (scope) => inner.listSubjects(scope),
     invalidate: drop,
-    invalidateAll: () => entries.clear(),
+    invalidateAll: () => {
+      entries.clear();
+      bySubject.clear();
+    },
     size: () => entries.size,
   };
 }