fix(ai-chat): validate the open page server-side so the agent edits the right one (#159)

The client sends the "current page" as { id, title } in the request body and the
server echoed BOTH verbatim into the system prompt context and the
getCurrentPage tool. id and title are independently attacker/desync-controllable
(two tabs, stale navigation), so openPage.id could point at page B while
openPage.title said "Page A" — the model then reported "updated Page A" while it
actually edited page B (CASL still allowed it; the user has access). Red-team
finding #4.

Resolve the open page ONCE against the DB via a new `resolveOpenPageContext`:
workspace-scoped lookup + access check, returning the AUTHORITATIVE { id, title }
(title from the DB row, never the client) or null (fail-closed) for a missing /
foreign / inaccessible page. That validated value now feeds the system prompt,
the getCurrentPage tool, AND the new-chat history origin (which previously did
this validation inline, for the id only — now shared, and the title is fixed
too).

Tests: resolveOpenPageContext covers no-id, not-found, foreign-workspace,
Forbidden, non-Forbidden-fault (fail-closed), the DB-title-wins-over-client case,
and null-title coercion.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/server/src/core/ai-chat/ai-chat.service.spec.ts

@@ -1,4 +1,6 @@
+import { ForbiddenException } from '@nestjs/common';
 import {
+  AiChatService,
   compactToolOutput,
   assistantParts,
   serializeSteps,
@@ -532,3 +534,93 @@ describe('AiChatService system prompt wiring (#180)', () => {
     expect(system).not.toContain('<mcp_tooling');
   });
 });
+
+/**
+ * resolveOpenPageContext: the open page the client sends is attacker-controllable
+ * (id AND title), so the service must validate the id against the DB and take the
+ * title from the DB row — never echo the client title (#159, AI edits the wrong
+ * page). Built with Object.create so the test exercises the real method without
+ * the service's full dependency graph (the constructor only assigns fields).
+ */
+describe('AiChatService.resolveOpenPageContext (#159 current-page validation)', () => {
+  const ws = { id: 'ws-1' } as Workspace;
+  const user = { id: 'u-1' } as any;
+
+  function makeService(opts: {
+    page?: { id: string; workspaceId: string; title: string | null } | null;
+    canView?: boolean | 'throw-other';
+  }) {
+    const svc = Object.create(AiChatService.prototype) as AiChatService;
+    (svc as any).logger = { warn: () => {} };
+    (svc as any).pageRepo = {
+      findById: async () => opts.page ?? undefined,
+    };
+    (svc as any).pageAccess = {
+      validateCanView: async () => {
+        if (opts.canView === 'throw-other') throw new Error('db down');
+        if (opts.canView === false) throw new ForbiddenException();
+        return true;
+      },
+    };
+    return svc;
+  }
+
+  const call = (svc: AiChatService, openPage: any) =>
+    (svc as any).resolveOpenPageContext(openPage, ws, user) as Promise<{
+      id: string;
+      title: string;
+    } | null>;
+
+  it('returns null when no page is open (no id)', async () => {
+    const svc = makeService({});
+    expect(await call(svc, null)).toBeNull();
+    expect(await call(svc, {})).toBeNull();
+    expect(await call(svc, { title: 'spoofed' })).toBeNull();
+  });
+
+  it('returns null when the page does not exist', async () => {
+    const svc = makeService({ page: null });
+    expect(await call(svc, { id: 'p-x' })).toBeNull();
+  });
+
+  it('returns null for a page in a DIFFERENT workspace (tenant isolation)', async () => {
+    const svc = makeService({
+      page: { id: 'p-1', workspaceId: 'ws-OTHER', title: 'Secret' },
+    });
+    expect(await call(svc, { id: 'p-1' })).toBeNull();
+  });
+
+  it('returns null when the user may not view the page (Forbidden)', async () => {
+    const svc = makeService({
+      page: { id: 'p-1', workspaceId: 'ws-1', title: 'Restricted' },
+      canView: false,
+    });
+    expect(await call(svc, { id: 'p-1' })).toBeNull();
+  });
+
+  it('returns null (fail-closed) on a non-Forbidden access-check fault', async () => {
+    const svc = makeService({
+      page: { id: 'p-1', workspaceId: 'ws-1', title: 'X' },
+      canView: 'throw-other',
+    });
+    expect(await call(svc, { id: 'p-1' })).toBeNull();
+  });
+
+  it('uses the AUTHORITATIVE DB title, IGNORING the client-supplied title', async () => {
+    const svc = makeService({
+      page: { id: 'p-1', workspaceId: 'ws-1', title: 'Real Title B' },
+      canView: true,
+    });
+    // The client claims it is on "Page A" but the id points at page B.
+    const result = await call(svc, { id: 'p-1', title: 'Page A' });
+    expect(result).toEqual({ id: 'p-1', title: 'Real Title B' });
+  });
+
+  it('coerces a null DB title to an empty string', async () => {
+    const svc = makeService({
+      page: { id: 'p-1', workspaceId: 'ws-1', title: null },
+      canView: true,
+    });
+    expect(await call(svc, { id: 'p-1' })).toEqual({ id: 'p-1', title: '' });
+  });
+});

apps/server/src/core/ai-chat/ai-chat.service.ts

@@ -185,6 +185,41 @@ export class AiChatService {
     return this.ai.getChatModel(workspaceId, roleModelOverride(role));
   }
 
+  /**
+   * Validate the client-supplied open page and return its AUTHORITATIVE identity
+   * ({ id, title }) or null. The client controls BOTH the id and the title in the
+   * request body, so neither is trusted: the id must resolve to a real page in
+   * THIS workspace that the user may read, and the title is taken from the DB row
+   * (never the client) so the model can't be told it is "on Page A" while the id
+   * points at page B (#159). Fail-closed — any missing / foreign / inaccessible
+   * page, or any non-Forbidden access-check fault, returns null.
+   */
+  private async resolveOpenPageContext(
+    openPage: { id?: string; title?: string } | null | undefined,
+    workspace: Workspace,
+    user: User,
+  ): Promise<{ id: string; title: string } | null> {
+    const candidatePageId = openPage?.id;
+    if (!candidatePageId) return null;
+    const page = await this.pageRepo.findById(candidatePageId);
+    if (!page || page.workspaceId !== workspace.id) return null;
+    try {
+      await this.pageAccess.validateCanView(page, user);
+    } catch (e) {
+      // A ForbiddenException is the expected "user cannot read this page" case;
+      // log anything else (e.g. a DB error) so a real fault is not masked.
+      if (!(e instanceof ForbiddenException)) {
+        this.logger.warn(
+          `open page access check failed: ${
+            e instanceof Error ? e.message : 'unknown error'
+          }`,
+        );
+      }
+      return null;
+    }
+    return { id: page.id, title: page.title ?? '' };
+  }
+
   async stream({
     user,
     workspace,
@@ -205,37 +240,26 @@ export class AiChatService {
         chatId = undefined;
       }
     }
-    if (!chatId) {
+    // The open page the client sent is attacker-controllable — BOTH its id and
-      // Resolve the origin document for the history list. body.openPage.id is
+    // its title. Resolve it ONCE against the DB (workspace-scoped + access-
-      // attacker-controllable, so validate it before persisting: it must be a
+    // checked) and use the AUTHORITATIVE identity everywhere below: the system
-      // real page in THIS workspace that the user is allowed to read. Anything
+    // prompt context, the getCurrentPage tool, and the new-chat history origin.
-      // else (foreign workspace, inaccessible/restricted, or non-existent) is
+    // Previously the client title was echoed verbatim, so a navigation / two-tab
-      // dropped to null — persisting it would leak the page's title via the
+    // desync (openPage.id -> page B, title -> "Page A") made the model report
-      // chat-list join, or violate the page_id FK on insert (this runs after
+    // "updated Page A" while it edited page B (#159). Null when no page is open
-      // res.hijack(), so a DB error would break the stream).
+    // or the page is foreign / inaccessible / missing.
-      let originPageId: string | null = null;
+    const openPageContext = await this.resolveOpenPageContext(
-      const candidatePageId = body.openPage?.id;
+      body.openPage,
-      if (candidatePageId) {
+      workspace,
-        const page = await this.pageRepo.findById(candidatePageId);
+      user,
-        if (page && page.workspaceId === workspace.id) {
-          try {
-            await this.pageAccess.validateCanView(page, user);
-            originPageId = page.id;
-          } catch (e) {
-            // Fail-closed: no provenance on any failure. A ForbiddenException is
-            // the expected "user cannot read this page" case; log anything else
-            // (e.g. a DB error) so a real fault is not masked as "no access".
-            if (!(e instanceof ForbiddenException)) {
-              this.logger.warn(
-                `origin page access check failed: ${
-                  e instanceof Error ? e.message : 'unknown error'
-                }`,
     );
-            }
+
-            originPageId = null;
+    if (!chatId) {
-          }
+      // The history-list origin is the validated open page (see above):
-        }
+      // persisting an unvalidated id would leak a title via the chat-list join,
-      }
+      // or violate the page_id FK on insert (this runs after res.hijack(), so a
+      // DB error would break the stream).
+      const originPageId: string | null = openPageContext?.id ?? null;
       const chat = await this.aiChatRepo.insert({
         creatorId: user.id,
         workspaceId: workspace.id,
@@ -314,7 +338,8 @@ export class AiChatService {
       // The role (pre-resolved by the controller) REPLACES the persona layer;
       // the safety framework is still appended by buildSystemPrompt.
       roleInstructions: role?.instructions,
-      openedPage: body.openPage,
+      // Server-validated open page (authoritative title), not the client value.
+      openedPage: openPageContext,
       // Guidance only for servers that connected and yielded ≥1 callable tool.
       mcpInstructions: external.instructions,
     });
@@ -327,9 +352,10 @@ export class AiChatService {
       sessionId,
       workspace.id,
       chatId,
-      // Same open-page value used by the system prompt above; exposed to the
+      // Same server-validated open page used by the system prompt above; exposed
-      // model via getCurrentPage so page identity survives prompt mangling.
+      // to the model via getCurrentPage so page identity (and the AUTHORITATIVE
-      body.openPage,
+      // title) survives prompt mangling and client title spoofing (#159).
+      openPageContext,
     );
 
     const tools = { ...external.tools, ...docmostTools };