From 83c99cc3e563096941cbb86d932086ee064b07e8 Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Wed, 5 Aug 2026 14:26:14 +0530 Subject: [PATCH] fix(rpc): close identity-token fail-open and validation gaps - importSubjectContext now rejects a non-string/empty selfApp before verifying. verifyJwt skips the audience check entirely when audience is undefined, so an unvalidated selfApp (the natural shape of currentAppName(): string | undefined) accepted every token from every app for every audience. - exportSubjectContext now rejects a non-string/empty targetApp, so an array can no longer mint one token valid at multiple apps. - Both directions now reject a present-but-non-string tenant id instead of silently dropping it (was: callee reads missing tenantId as global/unscoped -> cross-tenant exposure). - importSubjectContext now requires exp to be present and independently bounds accepted token age via a new maxAge/ImportOptions.maxAgeSeconds (default 300s), so a caller cannot mint a long-lived token via a huge ttlSeconds and have it honoured indefinitely. - SubjectContext.callerApp doc now states it is self-asserted (the signing secret is workspace-wide) and must never be an authz input. - index.ts also exports the new ImportOptions type. Co-Authored-By: Claude Opus 5 --- packages/rpc/src/identity.ts | 70 ++++++++++++++++++++++++++---- packages/rpc/src/index.ts | 2 +- packages/rpc/test/identity.test.ts | 38 ++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/packages/rpc/src/identity.ts b/packages/rpc/src/identity.ts index 0b24bd6a..4ddd9e6a 100644 --- a/packages/rpc/src/identity.ts +++ b/packages/rpc/src/identity.ts @@ -6,11 +6,17 @@ export const RPC_IDENTITY_HEADER = "x-wrnexus-rpc-identity"; const MIN_SECRET_LENGTH = 32; const DEFAULT_TTL_SECONDS = 60; +/** Upper bound on accepted token age, whatever the token's own exp says. */ +const DEFAULT_MAX_AGE_SECONDS = 300; export interface SubjectContext { subjectId: string; tenantId?: string; - /** The app that minted the token. */ + /** + * The app that CLAIMS to have minted the token. Self-asserted: the signing + * secret is workspace-wide, so any app can set this to any name. Useful for + * logs and tracing; NEVER an authorization input. + */ callerApp: string; } @@ -18,6 +24,15 @@ export interface ExportOptions { ttlSeconds?: number; } +export interface ImportOptions { + /** + * Reject a token older than this regardless of its own `exp`, so a caller + * that mints with a huge ttlSeconds cannot create a long-lived + * impersonation credential the callee will honour. Defaults to 300s. + */ + maxAgeSeconds?: number; +} + /** * The workspace-wide RPC signing secret. * @@ -74,9 +89,26 @@ export async function exportSubjectContext( "WRN-RPC-SUBJECT: subject id must be a non-empty string; coerce numeric ids with String(id).", ); } - const tenantId = ctx.tenant?.id; + // An array here would mint one token valid at SEVERAL apps, defeating the + // audience binding that stops app B replaying A's token against app C. + if (typeof targetApp !== "string" || targetApp === "") { + throw new Error("WRN-RPC-AUDIENCE: targetApp must be a non-empty string."); + } + // A numeric tenant id is the common DB-backed case. Dropping it silently + // would leave the callee reading "no tenant" as "global", which is a + // cross-tenant exposure — so refuse it the same way a bad subject is refused. + const rawTenant: unknown = ctx.tenant?.id; + if ( + rawTenant !== undefined && + rawTenant !== null && + (typeof rawTenant !== "string" || rawTenant === "") + ) { + throw new Error( + "WRN-RPC-TENANT: tenant id must be a non-empty string; coerce numeric ids with String(id).", + ); + } return signJwt( - { sub: rawId, ...(typeof tenantId === "string" && tenantId ? { tenant: tenantId } : {}) }, + { sub: rawId, ...(rawTenant ? { tenant: rawTenant as string } : {}) }, rpcSecret(), { issuer: callerAppName(), @@ -95,21 +127,41 @@ export async function exportSubjectContext( export async function importSubjectContext( token: string, selfApp: string, + options: ImportOptions = {}, ): Promise { - const claims = await verifyJwt<{ sub?: string; tenant?: string; iss?: string }>( - token, - rpcSecret(), - { audience: selfApp }, - ); + // verifyJwt SKIPS the audience check entirely when audience is undefined, so + // an empty selfApp would disable the only cross-app binding in the system and + // accept every token from every app. currentAppName() returns + // `string | undefined`, which is exactly how that gets passed by accident. + if (typeof selfApp !== "string" || selfApp === "") { + throw new Error("WRN-RPC-AUDIENCE: selfApp must be a non-empty string."); + } + const claims = await verifyJwt<{ + sub?: string; + tenant?: unknown; + iss?: string; + exp?: number; + }>(token, rpcSecret(), { + audience: selfApp, + maxAge: options.maxAgeSeconds ?? DEFAULT_MAX_AGE_SECONDS, + }); + // verifyJwt only checks exp when it is present, so a token minted without + // one never expires. Require it. + if (typeof claims.exp !== "number") { + throw new Error("WRN-RPC-IDENTITY: token has no expiry."); + } if (typeof claims.sub !== "string" || claims.sub === "") { throw new Error("WRN-RPC-IDENTITY: token carries no usable subject."); } if (typeof claims.iss !== "string" || claims.iss === "") { throw new Error("WRN-RPC-IDENTITY: token names no calling app."); } + if (claims.tenant !== undefined && (typeof claims.tenant !== "string" || claims.tenant === "")) { + throw new Error("WRN-RPC-IDENTITY: token carries an unusable tenant."); + } return { subjectId: claims.sub, - tenantId: typeof claims.tenant === "string" && claims.tenant ? claims.tenant : undefined, + tenantId: claims.tenant as string | undefined, callerApp: claims.iss, }; } diff --git a/packages/rpc/src/index.ts b/packages/rpc/src/index.ts index 02e98da8..c6e2591e 100644 --- a/packages/rpc/src/index.ts +++ b/packages/rpc/src/index.ts @@ -29,4 +29,4 @@ export { importSubjectContext, rpcSecret, } from "./identity.ts"; -export type { ExportOptions, SubjectContext } from "./identity.ts"; +export type { ExportOptions, ImportOptions, SubjectContext } from "./identity.ts"; diff --git a/packages/rpc/test/identity.test.ts b/packages/rpc/test/identity.test.ts index 8a55a08b..3ae54565 100644 --- a/packages/rpc/test/identity.test.ts +++ b/packages/rpc/test/identity.test.ts @@ -91,6 +91,44 @@ describe("subject context token", () => { ).rejects.toThrow(); }); + test("an empty selfApp is refused rather than disabling the audience check", async () => { + configure(); + const token = await exportSubjectContext(ctxFor({ id: "u1" }), "billing"); + // verifyJwt skips the audience check when audience is undefined, so this + // would otherwise accept every token from every app. + for (const bad of [undefined, "", null]) { + await expect(importSubjectContext(token!, bad as never)).rejects.toThrow(/selfApp/); + } + }); + + test("a token with a huge ttl is still rejected once it exceeds maxAge", async () => { + configure(); + const token = await exportSubjectContext(ctxFor({ id: "u1" }), "billing", { + ttlSeconds: 31_536_000, + }); + await expect(importSubjectContext(token!, "billing", { maxAgeSeconds: -1 })).rejects.toThrow(); + }); + + test("an array targetApp is refused, so no token is valid at two apps", async () => { + configure(); + await expect( + exportSubjectContext(ctxFor({ id: "u1" }), ["billing", "reports"] as never), + ).rejects.toThrow(/targetApp/); + }); + + test("a non-string tenant id is refused rather than silently dropped", async () => { + configure(); + // Silently dropping it leaves the callee reading "no tenant" as "global". + for (const tenant of [42, {}, ""]) { + await expect( + exportSubjectContext( + { user: { id: "u1" }, tenant: { id: tenant }, locals: {} } as unknown as Context, + "billing", + ), + ).rejects.toThrow(/tenant/i); + } + }); + test("a missing secret is a setup error, not a silent pass", async () => { process.env.WRNEXUS_APP_NAME = "web"; delete process.env.WRNEXUS_RPC_SECRET;