docs: put the subject in the memo key in the Task 7 plan snippet
The re-review closed all six earlier findings but surfaced the same bug class one level over: the memo key carried the scope but not the subject, so reassigning ctx.user mid-request served the previous principal's verdict. Demonstrated - u1 allowed, then ctx.user = u2 still returned true, and clearing ctx.user entirely revoked nothing. Triggered by impersonation or "view as user" middleware, step-up auth, session revocation mid-request, or simply registering an auth middleware after authzMiddleware. Also: symbols now memo by identity (String() collapsed two distinct symbols sharing a description into one slot), -0 stays distinct from 0, the rejected-redirect log no longer echoes CR/LF verbatim into the log stream, and a non-ASCII redirect target is encodeURI'd rather than throwing out of the Response constructor and 500ing on a denial path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -2031,6 +2031,8 @@ interface RequestAuthz {
|
|||||||
resolver: AuthzResolver;
|
resolver: AuthzResolver;
|
||||||
/** Memo for object resources, keyed by identity so two rows never collide. */
|
/** Memo for object resources, keyed by identity so two rows never collide. */
|
||||||
byRef: WeakMap<object, Map<string, Promise<AuthorizationDecision>>>;
|
byRef: WeakMap<object, Map<string, Promise<AuthorizationDecision>>>;
|
||||||
|
/** Memo for symbol resources, which also carry identity. */
|
||||||
|
bySymbol: Map<symbol, Map<string, Promise<AuthorizationDecision>>>;
|
||||||
/** Memo for primitive and absent resources. */
|
/** Memo for primitive and absent resources. */
|
||||||
byValue: Map<string, Promise<AuthorizationDecision>>;
|
byValue: Map<string, Promise<AuthorizationDecision>>;
|
||||||
}
|
}
|
||||||
@@ -2062,6 +2064,7 @@ export function authzMiddleware(options: AuthzResolverOptions): Middleware {
|
|||||||
ctx.locals[AUTHZ_LOCALS_KEY] = {
|
ctx.locals[AUTHZ_LOCALS_KEY] = {
|
||||||
resolver,
|
resolver,
|
||||||
byRef: new WeakMap(),
|
byRef: new WeakMap(),
|
||||||
|
bySymbol: new Map(),
|
||||||
byValue: new Map(),
|
byValue: new Map(),
|
||||||
} satisfies RequestAuthz;
|
} satisfies RequestAuthz;
|
||||||
return next();
|
return next();
|
||||||
@@ -2076,8 +2079,16 @@ export function decideFor(
|
|||||||
const request = readAuthz(ctx);
|
const request = readAuthz(ctx);
|
||||||
const scope = currentScope(ctx);
|
const scope = currentScope(ctx);
|
||||||
// Scope is part of the key: the same permission decides differently per tenant.
|
// Scope is part of the key: the same permission decides differently per tenant.
|
||||||
// JSON-encoded so a tenant id containing the separator cannot collide.
|
// Subject and scope are both part of the key. A request that reassigns
|
||||||
const key = JSON.stringify([scope?.tenantId ?? "", permission]);
|
// ctx.user (impersonation, step-up auth, session revocation) or ctx.tenant
|
||||||
|
// must not be served the previous principal's verdict from the memo.
|
||||||
|
const subjectId = (ctx.user as { id?: unknown } | null | undefined)?.id;
|
||||||
|
const key = JSON.stringify([
|
||||||
|
scope?.tenantId ?? "",
|
||||||
|
permission,
|
||||||
|
typeof subjectId,
|
||||||
|
String(subjectId),
|
||||||
|
]);
|
||||||
|
|
||||||
const run = () =>
|
const run = () =>
|
||||||
request.resolver.decide({
|
request.resolver.decide({
|
||||||
@@ -2090,6 +2101,19 @@ export function decideFor(
|
|||||||
// Object resources memo by IDENTITY. Serialising them would let two distinct
|
// Object resources memo by IDENTITY. Serialising them would let two distinct
|
||||||
// rows share a key and cross-authorize, and unserialisable ones (circular
|
// rows share a key and cross-authorize, and unserialisable ones (circular
|
||||||
// refs, BigInt fields, throwing getters) would all collapse into one bucket.
|
// refs, BigInt fields, throwing getters) would all collapse into one bucket.
|
||||||
|
// Symbols carry identity that String() erases, so they memo by identity too.
|
||||||
|
// They are held in a plain Map rather than the WeakMap: the memo is discarded
|
||||||
|
// with the request, so there is nothing to leak.
|
||||||
|
if (typeof resource === "symbol") {
|
||||||
|
let perSymbol = request.bySymbol.get(resource);
|
||||||
|
if (!perSymbol) request.bySymbol.set(resource, (perSymbol = new Map()));
|
||||||
|
const cached = perSymbol.get(key);
|
||||||
|
if (cached) return cached;
|
||||||
|
const pending = run();
|
||||||
|
perSymbol.set(key, pending);
|
||||||
|
return pending;
|
||||||
|
}
|
||||||
|
|
||||||
if (resource !== null && (typeof resource === "object" || typeof resource === "function")) {
|
if (resource !== null && (typeof resource === "object" || typeof resource === "function")) {
|
||||||
let perResource = request.byRef.get(resource as object);
|
let perResource = request.byRef.get(resource as object);
|
||||||
if (!perResource) request.byRef.set(resource as object, (perResource = new Map()));
|
if (!perResource) request.byRef.set(resource as object, (perResource = new Map()));
|
||||||
@@ -2100,8 +2124,10 @@ export function decideFor(
|
|||||||
return pending;
|
return pending;
|
||||||
}
|
}
|
||||||
|
|
||||||
// typeof is part of the key so 7 and "7" are not the same resource.
|
// typeof is part of the key so 7 and "7" are not the same resource, and
|
||||||
const valueKey = JSON.stringify([key, typeof resource, String(resource)]);
|
// -0 keeps its sign because String(-0) is "0".
|
||||||
|
const rendered = Object.is(resource, -0) ? "-0" : String(resource);
|
||||||
|
const valueKey = JSON.stringify([key, typeof resource, rendered]);
|
||||||
const cached = request.byValue.get(valueKey);
|
const cached = request.byValue.get(valueKey);
|
||||||
if (cached) return cached;
|
if (cached) return cached;
|
||||||
const pending = run();
|
const pending = run();
|
||||||
@@ -2161,13 +2187,20 @@ export function guardPermission(permission: string, options: GuardOptions = {}):
|
|||||||
|
|
||||||
if (options.redirectTo && !wantsJson(ctx)) {
|
if (options.redirectTo && !wantsJson(ctx)) {
|
||||||
if (!isLocalPath(options.redirectTo)) {
|
if (!isLocalPath(options.redirectTo)) {
|
||||||
|
// JSON-encode: this branch exists precisely for values containing
|
||||||
|
// CR/LF, which would otherwise forge a second log line.
|
||||||
console.error(
|
console.error(
|
||||||
`[wrnexus:authz] redirectTo must be a local path, got '${options.redirectTo}'; denying`,
|
`[wrnexus:authz] redirectTo must be a local path, got ${JSON.stringify(options.redirectTo)}; denying`,
|
||||||
);
|
);
|
||||||
} else {
|
} else {
|
||||||
return new Response(null, {
|
return new Response(null, {
|
||||||
status: 303,
|
status: 303,
|
||||||
headers: { location: options.redirectTo, "cache-control": "private, no-store" },
|
headers: {
|
||||||
|
// Header values must be Latin-1; a localized path like /accounts
|
||||||
|
// in non-ASCII would otherwise throw and 500 on a denial path.
|
||||||
|
location: encodeURI(options.redirectTo),
|
||||||
|
"cache-control": "private, no-store",
|
||||||
|
},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user