fix(appkit): trim x-forwarded-user in core OBO path (#427)

* 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 <atila@fassina.eu>

* 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 <atila@fassina.eu>

* 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 <atila@fassina.eu>

---------

Signed-off-by: Atila Fassina <atila@fassina.eu>

Author: atilafassina

packages/appkit/src/plugin/plugin.ts

@@ -410,12 +410,10 @@ 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(
-      "Missing x-forwarded-user header. Cannot resolve user ID.",
-    );
+    throw AuthenticationError.missingUserId();
   }
 
   /**
@@ -429,8 +427,8 @@ 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 userId = req.header("x-forwarded-user");
+    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";
 

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<BasePluginConfig> {
   }
 }
 
+/**
+ * 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<BasePluginConfig> {
   exports(): undefined {
@@ -152,6 +164,41 @@ 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<string, string> = {
+        "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;
+}
+
+/**
+ * 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<string, string> = {
+        "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;
@@ -584,4 +631,143 @@ 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,
+        );
+        // 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", () => {
+        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);
+      });
+    });
+  });
+
+  // `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",
+      );
+    });
+  });
 });

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();