fix(authz): eliminate cache-key collision in cachedPermissionStore

The scope-prefix concatenation cacheKey used a bare U+FFFD separator with
no escaping, so an adversarial subject/tenant id containing that character
could collide with a different subject/tenant pair and leak cached roles
across tenants. Switch to JSON.stringify([scopeKey, subjectId]) for an
unambiguous key.

Also replace the untested key.endsWith() substring sweep used to
invalidate a subject across all tenants on a global write with an
explicit bySubject index, and add test coverage for both the collision
and the cross-tenant invalidation sweep.
This commit is contained in:
2026-08-04 17:04:19 +05:30
parent 83f2951035
commit dc0771308a
2 changed files with 44 additions and 9 deletions
+28 -9
View File
@@ -113,17 +113,22 @@ export function cachedPermissionStore(
): CachedPermissionStore {
const ttlMs = options.ttlMs ?? 5_000;
const max = options.max ?? 1_000;
const entries = new Map<string, { at: number; value: SubjectAssignments }>();
const entries = new Map<string, { at: number; value: SubjectAssignments; subjectId: string }>();
const bySubject = new Map<string, Set<string>>();
const cacheKey = (subjectId: string, scope?: AuthzScope) => `${scopeKey(scope)}${subjectId}`;
const cacheKey = (subjectId: string, scope?: AuthzScope) =>
JSON.stringify([scopeKey(scope), subjectId]);
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);
}
const keys = bySubject.get(subjectId);
if (keys) for (const key of keys) entries.delete(key);
bySubject.delete(subjectId);
return;
}
const key = cacheKey(subjectId, scope);
entries.delete(key);
bySubject.get(subjectId)?.delete(key);
};
return {
@@ -132,8 +137,19 @@ 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!);
entries.set(key, { at: Date.now(), value });
if (entries.size >= max) {
const oldestKey = entries.keys().next().value!;
const oldest = entries.get(oldestKey);
entries.delete(oldestKey);
if (oldest) bySubject.get(oldest.subjectId)?.delete(oldestKey);
}
entries.set(key, { at: Date.now(), value, subjectId });
let keys = bySubject.get(subjectId);
if (!keys) {
keys = new Set();
bySubject.set(subjectId, keys);
}
keys.add(key);
return value;
},
async assignRole(subjectId, role, scope) {
@@ -154,7 +170,10 @@ export function cachedPermissionStore(
},
listSubjects: (scope) => inner.listSubjects(scope),
invalidate: drop,
invalidateAll: () => entries.clear(),
invalidateAll: () => {
entries.clear();
bySubject.clear();
},
size: () => entries.size,
};
}
+16
View File
@@ -63,4 +63,20 @@ describe("cachedPermissionStore", () => {
expect((await store.assignmentsFor("u1")).roles).toEqual([]);
expect((await store.assignmentsFor("u1", { tenantId: "t1" })).roles).toEqual(["editor"]);
});
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
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 });
await inner.assignRole("b\uFFFDc", "editor", { tenantId: "a" });
expect((await store.assignmentsFor("b\uFFFDc", { tenantId: "a" })).roles).toEqual(["editor"]);
expect((await store.assignmentsFor("c", { tenantId: "a\uFFFDb" })).roles).toEqual([]);
});
});