fix(authz): close memo cross-authorization and guard hardening gaps

Fix round 1 for Task 7, addressing review findings against the brief's
own memoKey design (now superseded per plan amendment cc8085bc):

- C1: memoKey's String(id) + JSON.stringify-with-catch cross-authorized
  distinct resources whenever their ids stringified the same (numeric
  vs string ids, object-shaped ids) or whenever JSON.stringify threw
  (circular references, BigInt fields, throwing getters all shared one
  "<unserialisable>" bucket, so the first verdict computed for any of
  them became the cached verdict for all of them in that request).
- C2: filterCan inherited the same bypass, returning rows the subject
  could not act on.
- Replaced serialisation-based memoization with identity-based
  memoization: object resources are memoised in a WeakMap keyed by the
  resource reference itself (never serialised), primitives/absent
  resources in a Map keyed by [scope, permission, typeof, String(value)]
  so 7 and "7" can never collide.
- I1: scope is now read from ctx.tenant at decision time (currentScope),
  not captured once at middleware-install time, so a tenant switch
  mid-request is honoured on the next check.
- I2/M1: guardPermission's redirectTo now only fires for non-JSON/API
  requests (replicated wantsJson check, since authz may only import
  core as types) and only for a validated local path (isLocalPath),
  closing an open-redirect and a JSON-caller-follows-303 gap.
- I3: getResource is now wrapped in try/catch; a throw denies with the
  standard opaque 403 body instead of propagating the loader's error
  (e.g. a SQL string) to the client.
- Added cache-control: private, no-store to both the 303 and 403
  responses.

Added 11 regression tests. C1/C2 revert-checked: temporarily restored
the old memoKey design and confirmed the four collision tests fail
against it before restoring the fix.

packages/authz/src/middleware.ts

@@ -11,8 +11,10 @@ export const AUTHZ_LOCALS_KEY = "_authz";
 
 interface RequestAuthz {
   resolver: AuthzResolver;
-  scope?: AuthzScope;
-  memo: Map<string, Promise<AuthorizationDecision>>;
+  /** Memo for object resources, keyed by identity so two rows never collide. */
+  byRef: WeakMap<object, Map<string, Promise<AuthorizationDecision>>>;
+  /** Memo for primitive and absent resources. */
+  byValue: Map<string, Promise<AuthorizationDecision>>;
 }
 
 function readAuthz(ctx: Context): RequestAuthz {
@@ -32,42 +34,72 @@ export function authzMiddleware(options: AuthzResolverOptions): Middleware {
   return (ctx, next) => {
     const request: RequestAuthz = {
       resolver,
-      scope: ctx.tenant?.id ? { tenantId: ctx.tenant.id } : undefined,
-      memo: new Map(),
+      byRef: new WeakMap(),
+      byValue: new Map(),
     };
     ctx.locals[AUTHZ_LOCALS_KEY] = request;
     return next();
   };
 }
 
-/** Stable memo key. Resources without an id fall back to their JSON shape. */
-function memoKey(permission: string, resource: unknown): string {
-  if (resource === undefined) return permission;
-  const id = (resource as { id?: unknown })?.id;
-  if (id !== undefined && id !== null) return `${permission}::${String(id)}`;
-  try {
-    return `${permission}::${JSON.stringify(resource)}`;
-  } catch {
-    return `${permission}::<unserialisable>`;
-  }
+/**
+ * Read the tenant from the context at decision time, not at middleware time:
+ * a request that switches tenant mid-flight must not keep the old scope.
+ */
+function currentScope(ctx: Context): AuthzScope | undefined {
+  const tenantId = ctx.tenant?.id;
+  return typeof tenantId === "string" && tenantId !== "" ? { tenantId } : undefined;
 }
 
+/**
+ * Object resources are memoised by identity (`byRef`), never by serialising
+ * their contents — serialisation is what let unrelated resources collide
+ * (same `id` shape, circular references, BigInt fields, throwing getters all
+ * funnelled into one bucket). Primitive/absent resources are memoised by a
+ * `[scope, permission, typeof, String(value)]` tuple so that e.g. `7` and
+ * `"7"` never share a cache slot.
+ */
 export function decideFor(
   ctx: Context,
   permission: string,
   resource?: unknown,
 ): Promise<AuthorizationDecision> {
   const request = readAuthz(ctx);
-  const key = memoKey(permission, resource);
-  const cached = request.memo.get(key);
+  const scope = currentScope(ctx);
+  const key = JSON.stringify([scope?.tenantId ?? "", permission]);
+
+  const decide = () =>
+    request.resolver.decide({
+      subject: ctx.user as { id?: string } | null | undefined,
+      permission,
+      resource,
+      scope,
+    });
+
+  const isObjectResource =
+    resource !== null &&
+    resource !== undefined &&
+    (typeof resource === "object" || typeof resource === "function");
+
+  if (isObjectResource) {
+    const resourceObject = resource as object;
+    let inner = request.byRef.get(resourceObject);
+    if (!inner) {
+      inner = new Map();
+      request.byRef.set(resourceObject, inner);
+    }
+    const cached = inner.get(key);
+    if (cached) return cached;
+    const pending = decide();
+    inner.set(key, pending);
+    return pending;
+  }
+
+  const valueKey = JSON.stringify([key, typeof resource, String(resource)]);
+  const cached = request.byValue.get(valueKey);
   if (cached) return cached;
-  const pending = request.resolver.decide({
-    subject: ctx.user as { id?: string } | null | undefined,
-    permission,
-    resource,
-    scope: request.scope,
-  });
-  request.memo.set(key, pending);
+  const pending = decide();
+  request.byValue.set(valueKey, pending);
   return pending;
 }
 
@@ -75,12 +107,44 @@ export async function can(ctx: Context, permission: string, resource?: unknown):
   return (await decideFor(ctx, permission, resource)).allowed;
 }
 
+/**
+ * Replicates `packages/core/src/auth.ts`'s `wantsJson` (not imported: authz
+ * may only pull TYPES from @wrnexus/core, never runtime code).
+ */
+function wantsJson(ctx: Context): boolean {
+  if (ctx.url.pathname.startsWith("/api/")) return true;
+  const accept = ctx.req.headers.get("accept") ?? "";
+  return accept.includes("application/json") && !accept.includes("text/html");
+}
+
+/**
+ * Refuse anything but a same-origin, same-app path: no scheme/host
+ * (`https://evil.example.com/...`), no protocol-relative target (`//evil...`
+ * is host-relative in a browser, not path-relative), no backslashes (some
+ * user agents treat `\` as `/`, which can smuggle a host past a naive
+ * `startsWith("/")` check), and no control characters (CR/LF header/response
+ * splitting, etc). Written as a codepoint loop rather than a control-char
+ * regex literal, which tooling in this repo mangles.
+ */
+function isLocalPath(target: string): boolean {
+  if (!target.startsWith("/")) return false;
+  if (target.startsWith("//")) return false;
+  if (target.includes("\\")) return false;
+  for (const ch of target) {
+    const code = ch.codePointAt(0) ?? 0;
+    if (code < 0x20 || code === 0x7f) return false;
+  }
+  return true;
+}
+
+const NO_STORE_HEADERS = { "cache-control": "private, no-store" } as const;
+
 export interface GuardOptions {
   /** Load the resource a bound policy needs. */
-  getResource?: (ctx: Context) => unknown | Promise<unknown>;
+  getResource?: (ctx: Context) => unknown;
   /** Include reason and policy name in the 403 body. Off by default. */
   exposeReason?: boolean;
-  /** Redirect page requests here instead of returning 403. */
+  /** Redirect page requests here instead of returning 403. Ignored for JSON/API requests and for any non-local target. */
   redirectTo?: string;
 }
 
@@ -90,17 +154,39 @@ export interface GuardOptions {
  */
 export function guardPermission(permission: string, options: GuardOptions = {}): Middleware {
   return async (ctx, next) => {
-    const resource = options.getResource ? await options.getResource(ctx) : undefined;
+    let resource: unknown;
+    if (options.getResource) {
+      try {
+        resource = await options.getResource(ctx);
+      } catch (error) {
+        console.error(`[wrnexus:authz] getResource threw for '${permission}'; denying`, error);
+        return Response.json(
+          { ok: false, error: "Forbidden" },
+          { status: 403, headers: NO_STORE_HEADERS },
+        );
+      }
+    }
+
     const result = await decideFor(ctx, permission, resource);
     if (result.allowed) return next();
-    if (options.redirectTo) {
-      return new Response(null, { status: 303, headers: { location: options.redirectTo } });
+
+    if (options.redirectTo && !wantsJson(ctx)) {
+      if (isLocalPath(options.redirectTo)) {
+        return new Response(null, {
+          status: 303,
+          headers: { location: options.redirectTo, ...NO_STORE_HEADERS },
+        });
+      }
+      console.error(
+        `[wrnexus:authz] guardPermission redirectTo '${options.redirectTo}' is not a local path; falling back to 403`,
+      );
     }
+
     return Response.json(
       options.exposeReason
         ? { ok: false, error: "Forbidden", reason: result.reason, policy: result.policy }
         : { ok: false, error: "Forbidden" },
-      { status: 403 },
+      { status: 403, headers: NO_STORE_HEADERS },
     );
   };
 }

packages/authz/test/middleware.test.ts

@@ -139,4 +139,131 @@ describe("filterCan", () => {
     const posts = [{ authorId: "u1" }, { authorId: "other" }, { authorId: "u1" }];
     expect(await filterCan(ctx, "post:delete", posts)).toHaveLength(2);
   });
+
+  test("handles BigInt fields and circular references without leaking", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    const store = memoryPermissionStore();
+    await store.grant("u1", "post:delete", "allow");
+    await withMiddleware(ctx, store);
+
+    const mine = { authorId: "u1", views: 10n } as Record<string, unknown>;
+    const other = { authorId: "other", views: 11n } as Record<string, unknown>;
+    const circularMine = { authorId: "u1" } as Record<string, unknown>;
+    circularMine.self = circularMine;
+    const circularOther = { authorId: "other" } as Record<string, unknown>;
+    circularOther.self = circularOther;
+
+    const result = await filterCan(ctx, "post:delete", [mine, other, circularMine, circularOther]);
+    expect(result).toEqual([mine, circularMine]);
+  });
+
+  test("returns an empty array for an empty input", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    await withMiddleware(ctx);
+    expect(await filterCan(ctx, "post:delete", [])).toEqual([]);
+  });
+});
+
+describe("memoisation does not cross-authorize distinct resources", () => {
+  test("a numeric id and a string id on different resources do not collide", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    const store = memoryPermissionStore();
+    await store.grant("u1", "post:delete", "allow");
+    await withMiddleware(ctx, store);
+    expect(await can(ctx, "post:delete", { id: 7, authorId: "u1" })).toBe(true);
+    expect(await can(ctx, "post:delete", { id: "7", authorId: "other" })).toBe(false);
+  });
+
+  test("resources with object-shaped ids do not collide", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    const store = memoryPermissionStore();
+    await store.grant("u1", "post:delete", "allow");
+    await withMiddleware(ctx, store);
+    expect(await can(ctx, "post:delete", { id: { tenant: "A" }, authorId: "u1" })).toBe(true);
+    expect(await can(ctx, "post:delete", { id: { tenant: "B" }, authorId: "other" })).toBe(false);
+  });
+
+  test("two distinct resource objects sharing the same id value do not share a verdict", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    const store = memoryPermissionStore();
+    await store.grant("u1", "post:delete", "allow");
+    await withMiddleware(ctx, store);
+    expect(await can(ctx, "post:delete", { id: 1, authorId: "u1" })).toBe(true);
+    expect(await can(ctx, "post:delete", { id: 1, authorId: "other" })).toBe(false);
+  });
+
+  test("switching ctx.tenant mid-request changes the scope for subsequent checks", async () => {
+    const ctx = makeCtx({ id: "u1" }, "t1");
+    const store = memoryPermissionStore();
+    await store.assignRole("u1", "editor", { tenantId: "t1" });
+    await withMiddleware(ctx, store);
+    expect(await can(ctx, "post:write")).toBe(true);
+    (ctx as unknown as { tenant?: { id: string } }).tenant = { id: "t2" };
+    expect(await can(ctx, "post:write")).toBe(false);
+  });
+});
+
+describe("guardPermission hardening", () => {
+  test("throws the setup error and never calls next without the middleware", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    let called = false;
+    await expect(
+      guardPermission("post:write")(ctx, async () => {
+        called = true;
+        return new Response("passed");
+      }),
+    ).rejects.toThrow(/authzMiddleware/);
+    expect(called).toBe(false);
+  });
+
+  test("a throwing getResource denies with the standard body, not the loader's message", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    await withMiddleware(ctx);
+    const guard = guardPermission("post:delete", {
+      getResource: () => {
+        throw new Error("SELECT * FROM posts WHERE id = 1 -- boom");
+      },
+    });
+    const res = await guard(ctx, async () => new Response("passed"));
+    expect(res.status).toBe(403);
+    const body = (await res.json()) as Record<string, unknown>;
+    expect(body).toEqual({ ok: false, error: "Forbidden" });
+  });
+
+  test("redirectTo issues a 303 for a page request", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    await withMiddleware(ctx);
+    const res = await guardPermission("post:write", { redirectTo: "/login" })(
+      ctx,
+      async () => new Response("passed"),
+    );
+    expect(res.status).toBe(303);
+    expect(res.headers.get("location")).toBe("/login");
+    expect(res.headers.get("cache-control")).toBe("private, no-store");
+  });
+
+  test("redirectTo is ignored for an /api/ request, which gets 403 instead", async () => {
+    const ctx = {
+      user: { id: "u1" },
+      tenant: undefined,
+      locals: {},
+      url: new URL("http://localhost/api/x"),
+      req: new Request("http://localhost/api/x"),
+    } as unknown as Context;
+    await withMiddleware(ctx);
+    const res = await guardPermission("post:write", { redirectTo: "/login" })(
+      ctx,
+      async () => new Response("passed"),
+    );
+    expect(res.status).toBe(403);
+  });
+
+  test("an off-site redirectTo is refused and falls back to 403", async () => {
+    const ctx = makeCtx({ id: "u1" });
+    await withMiddleware(ctx);
+    const res = await guardPermission("post:write", {
+      redirectTo: "https://evil.example.com/harvest",
+    })(ctx, async () => new Response("passed"));
+    expect(res.status).toBe(403);
+  });
 });