From 8780ea1b24bbf447aca17ee2a6a1480f002d4cc8 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Mon, 8 Jun 2026 20:49:09 +0200 Subject: [PATCH 1/3] fix(appkit): trim x-forwarded-user in core OBO path resolveUserId and asUser now trim the x-forwarded-user header at read time, so surrounding whitespace can't fork user identity or the per-user analytics cache key. Mirrors the existing files-plugin precedent; whitespace-only values resolve to the missing-header path (prod throws, dev falls back). Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugin/plugin.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index ab4210ec..ca6f01fd 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -410,7 +410,7 @@ export abstract class Plugin< * @throws AuthenticationError in production when no user header is present. */ protected resolveUserId(req: express.Request): string { - const userId = req.header("x-forwarded-user"); + const userId = req.header("x-forwarded-user")?.trim(); if (userId) return userId; if (process.env.NODE_ENV === "development") return getCurrentUserId(); throw AuthenticationError.missingToken( @@ -430,7 +430,7 @@ export abstract class Plugin< */ asUser(req: express.Request): this { const token = req.header("x-forwarded-access-token"); - const userId = req.header("x-forwarded-user"); + const userId = req.header("x-forwarded-user")?.trim(); const userEmail = req.header("x-forwarded-email"); const isDev = process.env.NODE_ENV === "development"; From a07e4a6fb7855a08688edf3ac2159581c9f8ffb4 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Mon, 8 Jun 2026 20:52:42 +0200 Subject: [PATCH 2/3] test(appkit): cover x-forwarded-user whitespace normalization Lock the Phase 1 trim: resolveUserId/asUser normalize a padded x-forwarded-user to the bare id; whitespace-only takes the missing-header path (prod throws, dev falls back); and two OBO analytics requests differing only by header whitespace share one cache key. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .../src/plugin/tests/asUser-proxy.test.ts | 119 ++++++++++++++++++ .../plugins/analytics/tests/analytics.test.ts | 57 +++++++++ 2 files changed, 176 insertions(+) diff --git a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts index 9fe3f89a..69c19818 100644 --- a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts +++ b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts @@ -35,6 +35,7 @@ import { AppManager } from "../../app"; import { CacheManager } from "../../cache"; import { getUserContext } from "../../context/execution-context"; import { ServiceContext } from "../../context/service-context"; +import { AuthenticationError } from "../../errors/authentication"; import { StreamManager } from "../../stream"; import type { ITelemetry, TelemetryProvider } from "../../telemetry"; import { TelemetryManager } from "../../telemetry"; @@ -124,6 +125,17 @@ class CallablePlugin extends Plugin { } } +/** + * Exposes the `protected resolveUserId(req)` so the whitespace-normalization + * tests can assert on the resolved identity directly (the value that feeds + * the analytics per-user cache key / executorKey). + */ +class ResolveProbePlugin extends ProbePlugin { + resolve(req: express.Request): string { + return this.resolveUserId(req); + } +} + /** Exports returns `undefined` — must be treated as `{}`. */ class NullExportsPlugin extends Plugin { exports(): undefined { @@ -152,6 +164,24 @@ function createReqWithoutToken(): express.Request { } as unknown as express.Request; } +/** + * OBO request with a caller-supplied `x-forwarded-user` value. Used by the + * whitespace-normalization tests to feed padded / whitespace-only identities + * through the same code path as `createReqWithObo`. + */ +function createReqWithUser(forwardedUser: string): express.Request { + return { + header: (name: string) => { + const map: Record = { + "x-forwarded-access-token": "user-token-abc", + "x-forwarded-user": forwardedUser, + "x-forwarded-email": "alice@example.com", + }; + return map[name.toLowerCase()]; + }, + } as unknown as express.Request; +} + describe("Plugin.asUser proxy", () => { let mockTelemetry: ITelemetry; let mockCache: CacheManager; @@ -584,4 +614,93 @@ describe("Plugin.asUser proxy", () => { expect(exports.label).toBe("hello"); }); }); + + // ── Whitespace normalization of x-forwarded-user (PR0 / OBO hardening) ── + // + // The core OBO path trims `x-forwarded-user` at both read sites + // (`resolveUserId` and `asUser`). These tests lock that behavior so a + // whitespace-variant header can never (a) mint a distinct identity or + // (b) fork the per-user analytics cache key (which derives from the + // value `resolveUserId` returns — see analytics `executorKey`). + describe("x-forwarded-user whitespace normalization", () => { + test("resolveUserId trims a padded x-forwarded-user to the bare id", () => { + const plugin = new ResolveProbePlugin(config); + + expect(plugin.resolve(createReqWithUser(" alice "))).toBe("alice"); + }); + + test("resolveUserId returns the same id for padded and unpadded headers", () => { + const plugin = new ResolveProbePlugin(config); + + const padded = plugin.resolve(createReqWithUser(" alice ")); + const unpadded = plugin.resolve(createReqWithUser("alice")); + + // Same resolved id => same analytics per-user cache key (the cache key + // derives from this resolved id via `executorKey`), so whitespace can + // neither fork the cache nor bypass per-user isolation. + expect(padded).toBe(unpadded); + expect(padded).toBe("alice"); + }); + + test("asUser with a padded header builds the same identity as the unpadded case", () => { + const plugin = new ProbePlugin(config); + + const padded = plugin.asUser(createReqWithUser(" alice ")).observeSync(); + const unpadded = plugin.asUser(createReqWithUser("alice")).observeSync(); + + // The identity flowing into the user context is the trimmed value. + expect(padded).toBe("alice"); + expect(unpadded).toBe("alice"); + expect(padded).toBe(unpadded); + }); + + test("asUser passes the trimmed id into ServiceContext.createUserContext", () => { + const plugin = new ProbePlugin(config); + + plugin.asUser(createReqWithUser(" alice ")); + + // The first positional arg is the token, the second is the user id — + // it must be the trimmed value, never the padded " alice ". + expect(serviceContextMock.createUserContextSpy).toHaveBeenCalledWith( + "user-token-abc", + "alice", + undefined, + "alice@example.com", + ); + }); + + describe("whitespace-only header takes the missing-header path", () => { + let originalNodeEnv: string | undefined; + + beforeEach(() => { + originalNodeEnv = process.env.NODE_ENV; + }); + + afterEach(() => { + process.env.NODE_ENV = originalNodeEnv; + }); + + test("resolveUserId throws AuthenticationError in production", () => { + process.env.NODE_ENV = "production"; + const plugin = new ResolveProbePlugin(config); + + // A whitespace-only header trims to "" (falsy) and is treated as + // missing — it must NOT become a " " identity. + expect(() => plugin.resolve(createReqWithUser(" "))).toThrow( + AuthenticationError, + ); + }); + + test("resolveUserId falls back to the context user id in development", () => { + process.env.NODE_ENV = "development"; + const plugin = new ResolveProbePlugin(config); + + // Dev fallback resolves to the current context user id (here: the + // mocked service principal), never the raw " " header. + const resolved = plugin.resolve(createReqWithUser(" ")); + expect(resolved).not.toBe(" "); + expect(resolved).toBe(serviceContextMock.serviceContext.serviceUserId); + }); + }); + }); }); diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 19ab4700..08ede7bc 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -543,6 +543,63 @@ describe("Analytics Plugin", () => { ); }); + test("OBO requests differing only by whitespace in x-forwarded-user share one cache key", async () => { + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ + query: "SELECT * FROM my_data", + isAsUser: true, + }); + + const executeMock = vi.fn().mockResolvedValue({ + result: { data: [{ owner: "alice-data" }] }, + }); + (plugin as any).SQLClient.executeStatement = executeMock; + + plugin.injectRoutes(router); + const handler = getHandler("POST", "/query/:query_key"); + + // Same user, but the forwarded header is padded with surrounding + // whitespace. The OBO cache key derives from the trimmed user id + // (executorKey = resolveUserId(req)), so this must hit the SAME cache + // entry as the unpadded request below — no per-whitespace cache fork. + const paddedReq = createMockRequest({ + params: { query_key: "my_data" }, + body: { parameters: {} }, + headers: { + "x-forwarded-access-token": "alice-token", + "x-forwarded-user": " alice ", + }, + }); + const paddedRes = createMockResponse(); + await handler(paddedReq, paddedRes); + + // Same user, unpadded header — must reuse the cached result. + const bareReq = createMockRequest({ + params: { query_key: "my_data" }, + body: { parameters: {} }, + headers: { + "x-forwarded-access-token": "alice-token", + "x-forwarded-user": "alice", + }, + }); + const bareRes = createMockResponse(); + await handler(bareReq, bareRes); + + // Only one execution: the whitespace variant resolved to the same + // per-user cache key as the bare id, so the second request was a hit. + expect(executeMock).toHaveBeenCalledTimes(1); + + // Both responses serve the same (cached) data. + expect(paddedRes.write).toHaveBeenCalledWith( + expect.stringContaining('"owner":"alice-data"'), + ); + expect(bareRes.write).toHaveBeenCalledWith( + expect.stringContaining('"owner":"alice-data"'), + ); + }); + test("should handle AbortSignal cancellation", async () => { const plugin = new AnalyticsPlugin(config); const { router, getHandler } = createMockRouter(); From fd44528a246e08b59458b787fa22ded8be923d9d Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 9 Jun 2026 12:34:06 +0200 Subject: [PATCH 3/3] fix(appkit): trim x-forwarded-access-token + correct resolveUserId error Address Copilot review feedback on #427: - asUser now trims x-forwarded-access-token too, so a whitespace-only token is treated as missing rather than forwarded to ServiceContext as a bogus credential. - resolveUserId throws AuthenticationError.missingUserId() instead of passing a full sentence to missingToken() (which produced a doubled "Missing Missing ..." message), matching asUser. Adds tests for both. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugin/plugin.ts | 6 +- .../src/plugin/tests/asUser-proxy.test.ts | 67 +++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index ca6f01fd..c8138fd3 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -413,9 +413,7 @@ export abstract class Plugin< const userId = req.header("x-forwarded-user")?.trim(); if (userId) return userId; if (process.env.NODE_ENV === "development") return getCurrentUserId(); - throw AuthenticationError.missingToken( - "Missing x-forwarded-user header. Cannot resolve user ID.", - ); + throw AuthenticationError.missingUserId(); } /** @@ -429,7 +427,7 @@ export abstract class Plugin< * In development mode (`NODE_ENV=development`), skips user impersonation instead of throwing. */ asUser(req: express.Request): this { - const token = req.header("x-forwarded-access-token"); + const token = req.header("x-forwarded-access-token")?.trim(); const userId = req.header("x-forwarded-user")?.trim(); const userEmail = req.header("x-forwarded-email"); const isDev = process.env.NODE_ENV === "development"; diff --git a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts index 69c19818..d67c4748 100644 --- a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts +++ b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts @@ -182,6 +182,23 @@ function createReqWithUser(forwardedUser: string): express.Request { } as unknown as express.Request; } +/** + * OBO request with a caller-supplied `x-forwarded-access-token` value (and a + * valid user). Used by the token-trim tests. + */ +function createReqWithToken(forwardedToken: string): express.Request { + return { + header: (name: string) => { + const map: Record = { + "x-forwarded-access-token": forwardedToken, + "x-forwarded-user": "alice", + "x-forwarded-email": "alice@example.com", + }; + return map[name.toLowerCase()]; + }, + } as unknown as express.Request; +} + describe("Plugin.asUser proxy", () => { let mockTelemetry: ITelemetry; let mockCache: CacheManager; @@ -689,6 +706,12 @@ describe("Plugin.asUser proxy", () => { expect(() => plugin.resolve(createReqWithUser(" "))).toThrow( AuthenticationError, ); + // The message must be the purpose-built missingUserId() text, not the + // doubled "Missing Missing … in request headers" the old missingToken() + // call produced. + expect(() => plugin.resolve(createReqWithUser(" "))).toThrow( + /User ID not available in request headers/, + ); }); test("resolveUserId falls back to the context user id in development", () => { @@ -703,4 +726,48 @@ describe("Plugin.asUser proxy", () => { }); }); }); + + // `asUser` also trims `x-forwarded-access-token` at read time, so a + // whitespace-only token is treated as missing (not forwarded to the SDK as a + // bogus credential), and a padded token is normalized before it reaches + // ServiceContext. + describe("x-forwarded-access-token whitespace handling", () => { + let originalNodeEnv: string | undefined; + + beforeEach(() => { + originalNodeEnv = process.env.NODE_ENV; + }); + + afterEach(() => { + process.env.NODE_ENV = originalNodeEnv; + }); + + test("asUser throws in production when the token is whitespace-only", () => { + process.env.NODE_ENV = "production"; + const plugin = new ProbePlugin(config); + + // " " trims to "" (falsy) → treated as a missing token rather than + // being forwarded to ServiceContext.createUserContext as a bogus value. + expect(() => plugin.asUser(createReqWithToken(" "))).toThrow( + AuthenticationError, + ); + expect(() => plugin.asUser(createReqWithToken(" "))).toThrow( + /Missing user token in request headers/, + ); + }); + + test("asUser passes the trimmed token into ServiceContext.createUserContext", () => { + const plugin = new ProbePlugin(config); + + plugin.asUser(createReqWithToken(" user-token-abc ")); + + // First positional arg is the token — it must be the trimmed value. + expect(serviceContextMock.createUserContextSpy).toHaveBeenCalledWith( + "user-token-abc", + "alice", + undefined, + "alice@example.com", + ); + }); + }); });