fix(authz): fold subject into the memo key, fix symbol/-0 and redirect issues
Fix round 2 for Task 7 (plan amendment 9e3624e5):
- N1 (Important): the memo key carried scope and permission but not the
subject, so a request that reassigns ctx.user mid-flight (impersonation,
step-up auth, session revocation, or an authz-before-auth middleware
ordering mistake) could be served the previous principal's cached
verdict. subjectId (typeof + String, matching the existing scope/value
encoding style) is now folded into every memo key.
- N2 (Minor): the primitive-value memo key used String(resource), which
collapses distinct Symbol("row") values into one slot and maps -0 onto
0's slot. Added a dedicated bySymbol identity memo (WeakMap-style, but a
plain Map since symbols aren't valid WeakMap keys pre-registry symbols
and the memo is request-scoped anyway) and special-cased Object.is(x,-0)
to render as "-0".
- N3 (Minor): the rejected-redirect console.error interpolated
redirectTo directly, exactly the value most likely to carry CR/LF in
that branch. Switched to JSON.stringify(redirectTo) for the log line.
- N4 (Minor): a non-ASCII (but otherwise valid, local) redirectTo passed
isLocalPath and then threw inside `new Response` building the Location
header. Wrapped it in encodeURI().
Added 5 regression tests: subject swap re-evaluates, clearing ctx.user
denies, two same-description symbols get separate verdicts, 0 vs -0 get
separate verdicts, non-ASCII redirectTo 303s with an encoded location
instead of throwing. N1 revert-checked: temporarily restored the
two-element (no-subject) key and confirmed both subject-swap tests fail
against it before restoring the fix.
This commit is contained in:
@@ -13,6 +13,8 @@ interface RequestAuthz {
|
||||
resolver: AuthzResolver;
|
||||
/** Memo for object resources, keyed by identity so two rows never collide. */
|
||||
byRef: WeakMap<object, Map<string, Promise<AuthorizationDecision>>>;
|
||||
/** Memo for symbol resources, keyed by identity for the same reason. */
|
||||
bySymbol: Map<symbol, Map<string, Promise<AuthorizationDecision>>>;
|
||||
/** Memo for primitive and absent resources. */
|
||||
byValue: Map<string, Promise<AuthorizationDecision>>;
|
||||
}
|
||||
@@ -35,6 +37,7 @@ export function authzMiddleware(options: AuthzResolverOptions): Middleware {
|
||||
const request: RequestAuthz = {
|
||||
resolver,
|
||||
byRef: new WeakMap(),
|
||||
bySymbol: new Map(),
|
||||
byValue: new Map(),
|
||||
};
|
||||
ctx.locals[AUTHZ_LOCALS_KEY] = request;
|
||||
@@ -55,9 +58,15 @@ function currentScope(ctx: Context): AuthzScope | 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.
|
||||
* funnelled into one bucket). Symbols are memoised by identity too (`bySymbol`)
|
||||
* since `String(symbol)` collapses distinct symbols with the same description.
|
||||
* Primitive/absent resources are memoised by a
|
||||
* `[scope, permission, typeof, String(value)]` tuple, with `-0` rendered
|
||||
* distinctly from `0` since `String(-0) === "0"` would otherwise merge them.
|
||||
*
|
||||
* Subject and scope are both part of the key. A request that reassigns
|
||||
* ctx.user (impersonation, step-up auth, session revocation) or ctx.tenant
|
||||
* must not be served the previous principal's verdict from the memo.
|
||||
*/
|
||||
export function decideFor(
|
||||
ctx: Context,
|
||||
@@ -66,9 +75,15 @@ export function decideFor(
|
||||
): Promise<AuthorizationDecision> {
|
||||
const request = readAuthz(ctx);
|
||||
const scope = currentScope(ctx);
|
||||
const key = JSON.stringify([scope?.tenantId ?? "", permission]);
|
||||
const subjectId = (ctx.user as { id?: unknown } | null | undefined)?.id;
|
||||
const key = JSON.stringify([
|
||||
scope?.tenantId ?? "",
|
||||
permission,
|
||||
typeof subjectId,
|
||||
String(subjectId),
|
||||
]);
|
||||
|
||||
const decide = () =>
|
||||
const run = () =>
|
||||
request.resolver.decide({
|
||||
subject: ctx.user as { id?: string } | null | undefined,
|
||||
permission,
|
||||
@@ -76,6 +91,19 @@ export function decideFor(
|
||||
scope,
|
||||
});
|
||||
|
||||
// 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;
|
||||
}
|
||||
|
||||
const isObjectResource =
|
||||
resource !== null &&
|
||||
resource !== undefined &&
|
||||
@@ -90,15 +118,16 @@ export function decideFor(
|
||||
}
|
||||
const cached = inner.get(key);
|
||||
if (cached) return cached;
|
||||
const pending = decide();
|
||||
const pending = run();
|
||||
inner.set(key, pending);
|
||||
return pending;
|
||||
}
|
||||
|
||||
const valueKey = JSON.stringify([key, typeof resource, String(resource)]);
|
||||
const rendered = Object.is(resource, -0) ? "-0" : String(resource);
|
||||
const valueKey = JSON.stringify([key, typeof resource, rendered]);
|
||||
const cached = request.byValue.get(valueKey);
|
||||
if (cached) return cached;
|
||||
const pending = decide();
|
||||
const pending = run();
|
||||
request.byValue.set(valueKey, pending);
|
||||
return pending;
|
||||
}
|
||||
@@ -174,11 +203,15 @@ export function guardPermission(permission: string, options: GuardOptions = {}):
|
||||
if (isLocalPath(options.redirectTo)) {
|
||||
return new Response(null, {
|
||||
status: 303,
|
||||
headers: { location: options.redirectTo, ...NO_STORE_HEADERS },
|
||||
// encodeURI: a non-ASCII local path (e.g. a localized login route)
|
||||
// is valid config but not a valid raw header value.
|
||||
headers: { location: encodeURI(options.redirectTo), ...NO_STORE_HEADERS },
|
||||
});
|
||||
}
|
||||
// JSON.stringify, not string interpolation: this branch exists precisely
|
||||
// for targets containing CR/LF, which must not reach the log verbatim.
|
||||
console.error(
|
||||
`[wrnexus:authz] guardPermission redirectTo '${options.redirectTo}' is not a local path; falling back to 403`,
|
||||
`[wrnexus:authz] guardPermission redirectTo ${JSON.stringify(options.redirectTo)} is not a local path; falling back to 403`,
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user