From 683b9d5de26c473dc750af72e9b66c3ce45c258d Mon Sep 17 00:00:00 2001 From: claude_code Date: Wed, 24 Jun 2026 02:04:23 +0300 Subject: [PATCH] =?UTF-8?q?fix(provenance):=20address=20#143=20review=20?= =?UTF-8?q?=E2=80=94=20page-stamp=20tests,=20confine=20is=5Fagent,=20doc?= =?UTF-8?q?=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the open items from the latest PR #143 code review: - test(page): cover the four agentSourceFields stamp sites (create, update, movePage, movePageToSpace) with agent + normal-user payload assertions; add findById({ includeIsAgent: true }) wiring guards to the JWT and collab auth-seam specs so a future drop of the option is caught. - fix(privacy): drop `isAgent` from UserRepo.baseFields and gate it behind a new opt-in `findById({ includeIsAgent })`, requested only by the two auth seams that derive provenance — stops the flag leaking via the workspace member list and generic user payloads. - docs: correct the agentSourceFields JSDoc and the two UPDATE-site comments to distinguish INSERT (omitted column → DB default 'user') from UPDATE (omitted column → existing value kept, Kysely writes only present keys). - style(page): collapse three stray double blank lines left by an earlier edit. Co-Authored-By: Claude Opus 4.8 --- .../authentication.extension.spec.ts | 8 + .../extensions/authentication.extension.ts | 4 +- .../decorators/auth-provenance.decorator.ts | 12 +- .../core/auth/strategies/jwt.strategy.spec.ts | 10 +- .../src/core/auth/strategies/jwt.strategy.ts | 4 +- .../core/page/services/page.service.spec.ts | 242 ++++++++++++++++++ .../src/core/page/services/page.service.ts | 8 +- .../src/database/repos/user/user.repo.ts | 10 +- 8 files changed, 282 insertions(+), 16 deletions(-) diff --git a/apps/server/src/collaboration/extensions/authentication.extension.spec.ts b/apps/server/src/collaboration/extensions/authentication.extension.spec.ts index 1871d09a..585393a4 100644 --- a/apps/server/src/collaboration/extensions/authentication.extension.spec.ts +++ b/apps/server/src/collaboration/extensions/authentication.extension.spec.ts @@ -207,6 +207,14 @@ describe('AuthenticationExtension.onAuthenticate', () => { expect(ctx.actor).toBe('user'); expect(ctx.aiChatId).toBeNull(); + // Wiring guard (#143): the collab seam MUST opt into the isAgent flag — + // it is not in baseFields, so without this option findById omits it and a + // flagged service account's collab edits would silently persist as 'user'. + expect(userRepo.findById).toHaveBeenCalledWith( + USER_ID, + WORKSPACE_ID, + expect.objectContaining({ includeIsAgent: true }), + ); }); it('is_agent user with NO claim → actor=agent (collab seam consults the signed identity)', async () => { diff --git a/apps/server/src/collaboration/extensions/authentication.extension.ts b/apps/server/src/collaboration/extensions/authentication.extension.ts index 41646c12..addce7af 100644 --- a/apps/server/src/collaboration/extensions/authentication.extension.ts +++ b/apps/server/src/collaboration/extensions/authentication.extension.ts @@ -44,7 +44,9 @@ export class AuthenticationExtension implements Extension { const userId = jwtPayload.sub; const workspaceId = jwtPayload.workspaceId; - const user = await this.userRepo.findById(userId, workspaceId); + const user = await this.userRepo.findById(userId, workspaceId, { + includeIsAgent: true, + }); if (!user) { throw new UnauthorizedException(); diff --git a/apps/server/src/common/decorators/auth-provenance.decorator.ts b/apps/server/src/common/decorators/auth-provenance.decorator.ts index e4be9d20..3bb1e61d 100644 --- a/apps/server/src/common/decorators/auth-provenance.decorator.ts +++ b/apps/server/src/common/decorators/auth-provenance.decorator.ts @@ -40,11 +40,13 @@ export function resolveProvenance( /** * Agent-edit write-stamp fields for a repository insert/update (#143 review). * Spread into the row being written: for an agent it stamps the `*Source` - * column 'agent' and the AI-chat id; for a normal user it returns `{}` so the - * column keeps its default ('user'). The only per-table variation is the column - * names, passed as `sourceKey`/`chatKey`, so the agent-stamp idiom lives in ONE - * place instead of being hand-reimplemented at every write site (where a wrong - * literal or a forgotten `aiChatId` could drift). + * column 'agent' and the AI-chat id; for a normal user it returns `{}` — on an + * INSERT the omitted column falls back to its DB default ('user'); on an UPDATE + * the column simply keeps its existing stored value (Kysely only writes the keys + * present). The only per-table variation is the column names, passed as + * `sourceKey`/`chatKey`, so the agent-stamp idiom lives in ONE place instead of + * being hand-reimplemented at every write site (where a wrong literal or a + * forgotten `aiChatId` could drift). * * insertComment({ ..., ...agentSourceFields(p, 'createdSource', 'aiChatId') }) * updatePage({ ..., ...agentSourceFields(p, 'lastUpdatedSource', 'lastUpdatedAiChatId') }) diff --git a/apps/server/src/core/auth/strategies/jwt.strategy.spec.ts b/apps/server/src/core/auth/strategies/jwt.strategy.spec.ts index ac0e2c08..11544df7 100644 --- a/apps/server/src/core/auth/strategies/jwt.strategy.spec.ts +++ b/apps/server/src/core/auth/strategies/jwt.strategy.spec.ts @@ -47,7 +47,7 @@ describe('JwtStrategy — provenance derivation', () => { }); it("stamps actor='agent' for an is_agent user (derived from the signed identity)", async () => { - const { strategy } = makeStrategy({ + const { strategy, userRepo } = makeStrategy({ id: 'user-1', isAgent: true, deactivatedAt: null, @@ -60,6 +60,14 @@ describe('JwtStrategy — provenance derivation', () => { expect(req.raw.actor).toBe('agent'); // External MCP agent: no internal ai_chats row → null. expect(req.raw.aiChatId).toBeNull(); + // Wiring guard (#143): the seam MUST opt into the isAgent flag, otherwise + // findById omits it (it is not in baseFields) and provenance silently + // degrades to 'user'. + expect(userRepo.findById).toHaveBeenCalledWith( + 'user-1', + 'ws-1', + expect.objectContaining({ includeIsAgent: true }), + ); }); it("stamps actor='user' for an ordinary user", async () => { diff --git a/apps/server/src/core/auth/strategies/jwt.strategy.ts b/apps/server/src/core/auth/strategies/jwt.strategy.ts index 791da95c..024b05de 100644 --- a/apps/server/src/core/auth/strategies/jwt.strategy.ts +++ b/apps/server/src/core/auth/strategies/jwt.strategy.ts @@ -56,7 +56,9 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') { if (!workspace) { throw new UnauthorizedException(); } - const user = await this.userRepo.findById(payload.sub, payload.workspaceId); + const user = await this.userRepo.findById(payload.sub, payload.workspaceId, { + includeIsAgent: true, + }); if (!user || isUserDisabled(user)) { throw new UnauthorizedException(); diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts index f923c432..bb387b31 100644 --- a/apps/server/src/core/page/services/page.service.spec.ts +++ b/apps/server/src/core/page/services/page.service.spec.ts @@ -147,4 +147,246 @@ describe('PageService', () => { expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); }); }); + + describe('agent provenance stamping (#143)', () => { + // Provenance handed to the four write sites. The agent case must surface the + // signed source marker + chat id on the persisted payload; the user case must + // leave both keys absent so the column keeps its INSERT default / existing + // UPDATE value (agentSourceFields returns {} for a non-agent). + const AGENT = { actor: 'agent', aiChatId: 'chat-7' } as any; + const USER = { actor: 'user', aiChatId: null } as any; + + // A general-queue stub whose `.add(...)` returns a `{ catch }` thenable — + // the service does `generalQueue.add(...).catch(...)` and never awaits it. + const makeGeneralQueue = () => + ({ add: jest.fn().mockReturnValue({ catch: jest.fn() }) }) as any; + + // Build a PageService where only the deps a given site touches are real + // stubs; everything else stays a bare object. db is supplied per-test. + const makeSvc = (overrides: { + pageRepo?: any; + generalQueue?: any; + db?: any; + }) => + new PageService( + (overrides.pageRepo ?? {}) as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // attachmentRepo + (overrides.db ?? {}) as any, // db + {} as any, // storageService + {} as any, // attachmentQueue + {} as any, // aiQueue + (overrides.generalQueue ?? makeGeneralQueue()) as any, // generalQueue + {} as any, // eventEmitter + {} as any, // collaborationGateway + {} as any, // watcherService + {} as any, // transclusionService + ); + + describe('create() → insertPage', () => { + const run = async (provenance: any) => { + const pageRepo = { + insertPage: jest.fn().mockResolvedValue({ id: 'p1' }), + }; + const svc = makeSvc({ pageRepo, generalQueue: makeGeneralQueue() }); + // nextPagePosition runs a real db query; stub it out. + jest.spyOn(svc, 'nextPagePosition').mockResolvedValue('a0' as any); + // No content/format → the prosemirror parse branch is skipped. No + // parentPageId → no parent lookup. + await svc.create( + 'u1', + 'w1', + { title: 't', spaceId: 's1' } as any, + provenance, + ); + return pageRepo.insertPage.mock.calls[0][0]; + }; + + it('stamps lastUpdatedSource/lastUpdatedAiChatId for an agent', async () => { + const payload = await run(AGENT); + expect(payload).toEqual( + expect.objectContaining({ + lastUpdatedSource: 'agent', + lastUpdatedAiChatId: 'chat-7', + }), + ); + }); + + it('omits the source columns for a normal user', async () => { + const payload = await run(USER); + expect(payload).not.toHaveProperty('lastUpdatedSource'); + expect(payload).not.toHaveProperty('lastUpdatedAiChatId'); + }); + }); + + describe('update() → updatePage', () => { + const run = async (provenance: any) => { + const pageRepo = { + updatePage: jest.fn().mockResolvedValue(undefined), + findById: jest.fn().mockResolvedValue({ id: 'p1' }), + }; + const svc = makeSvc({ pageRepo, generalQueue: makeGeneralQueue() }); + const page = { + id: 'p1', + contributorIds: [], + spaceId: 's1', + workspaceId: 'w1', + slugId: 'sl1', + title: 't', + parentPageId: null, + } as any; + // dto carries no content/operation/format → updatePageContent skipped. + await svc.update(page, {} as any, { id: 'u1' } as any, provenance); + return pageRepo.updatePage.mock.calls[0][0]; + }; + + it('stamps lastUpdatedSource/lastUpdatedAiChatId for an agent', async () => { + const payload = await run(AGENT); + expect(payload).toEqual( + expect.objectContaining({ + lastUpdatedSource: 'agent', + lastUpdatedAiChatId: 'chat-7', + }), + ); + }); + + it('omits the source columns for a normal user', async () => { + const payload = await run(USER); + expect(payload).not.toHaveProperty('lastUpdatedSource'); + expect(payload).not.toHaveProperty('lastUpdatedAiChatId'); + }); + }); + + describe('movePage() → updatePage', () => { + const VALID_POSITION = 'a0'; + const run = async (provenance: any) => { + const pageRepo = { + findById: jest.fn().mockResolvedValue({ + id: 'dest-parent', + deletedAt: null, + spaceId: 'space-1', + }), + updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }), + }; + const svc = makeSvc({ + pageRepo, + db: {} as any, + }); + // Legitimate move: destination ancestors do NOT include the moved page. + jest + .spyOn(svc, 'getPageBreadCrumbs') + .mockResolvedValue([{ id: 'dest-parent' }, { id: 'root' }] as any); + // eventEmitter is a bare {} stub; movePage emits PAGE_MOVED, so give it + // an emit. Re-wire via the private field to avoid threading it through. + (svc as any).eventEmitter = { emit: jest.fn() }; + const movedPage = { + id: 'page-1', + parentPageId: 'old-parent', + spaceId: 'space-1', + workspaceId: 'ws-1', + slugId: 'slug-1', + title: 'Page 1', + icon: null, + } as any; + const dto = { + pageId: 'page-1', + position: VALID_POSITION, + parentPageId: 'dest-parent', + } as any; + await svc.movePage(dto, movedPage, provenance); + return pageRepo.updatePage.mock.calls[0][0]; + }; + + it('stamps lastUpdatedSource/lastUpdatedAiChatId for an agent', async () => { + const payload = await run(AGENT); + expect(payload).toEqual( + expect.objectContaining({ + lastUpdatedSource: 'agent', + lastUpdatedAiChatId: 'chat-7', + }), + ); + }); + + it('omits the source columns for a normal user', async () => { + const payload = await run(USER); + expect(payload).not.toHaveProperty('lastUpdatedSource'); + expect(payload).not.toHaveProperty('lastUpdatedAiChatId'); + }); + }); + + describe('movePageToSpace() → root-page updatePage', () => { + // movePageToSpace runs its writes inside executeTx(this.db, cb), which + // calls this.db.transaction().execute(fn => fn(trx)). A permissive + // chainable Proxy stands in for the Kysely trx so arbitrary chains resolve. + const makeChain = () => { + const c: any = new Proxy(function () {}, { + get: (_t, p) => + p === 'then' + ? undefined + : p === 'execute' || p === 'executeTakeFirst' + ? () => Promise.resolve([]) + : () => c, + }); + return c; + }; + + const run = async (provenance: any) => { + const trxStub = makeChain(); + const db = { + transaction: () => ({ execute: (fn: any) => fn(trxStub) }), + } as any; + const rootPage = { + id: 'root', + spaceId: 'src-space', + parentPageId: null, + workspaceId: 'ws-1', + } as any; + const pageRepo = { + getPageAndDescendants: jest.fn().mockResolvedValue([rootPage]), + updatePage: jest.fn().mockResolvedValue(undefined), + updatePages: jest.fn().mockResolvedValue(undefined), + }; + const svc = makeSvc({ pageRepo, db }); + // The single-accessible-page path still runs the bulk side-effect writes + // (attachments/watchers/ai-queue) AFTER the root updatePage we assert on; + // stub them so the transaction completes without throwing. + (svc as any).attachmentRepo = { + updateAttachmentsByPageId: jest.fn().mockResolvedValue(undefined), + }; + (svc as any).watcherService = { + movePageWatchersToSpace: jest.fn().mockResolvedValue(undefined), + }; + (svc as any).aiQueue = { add: jest.fn().mockResolvedValue(undefined) }; + // Single accessible page (the root) → pagesToOrphan is empty, so the + // root updatePage is the first/only provenance-carrying updatePage call. + // filterAccessibleTreePages is private; spy via an `any` cast. + jest + .spyOn(svc as any, 'filterAccessibleTreePages') + .mockResolvedValue([rootPage] as any); + jest.spyOn(svc, 'nextPagePosition').mockResolvedValue('a0' as any); + await svc.movePageToSpace(rootPage, 'dst-space', 'u1', provenance); + return pageRepo.updatePage.mock.calls[0][0]; + }; + + it('stamps the moved root with the agent source + chat id', async () => { + const payload = await run(AGENT); + expect(payload).toEqual( + expect.objectContaining({ + spaceId: 'dst-space', + lastUpdatedSource: 'agent', + lastUpdatedAiChatId: 'chat-7', + }), + ); + }); + + it('omits the source columns on the moved root for a normal user', async () => { + const payload = await run(USER); + expect(payload).toEqual( + expect.objectContaining({ spaceId: 'dst-space' }), + ); + expect(payload).not.toHaveProperty('lastUpdatedSource'); + expect(payload).not.toHaveProperty('lastUpdatedAiChatId'); + }); + }); + }); }); diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 88ef01c8..ff205350 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -138,7 +138,6 @@ export class PageService { ydoc = createYdocFromJson(prosemirrorJson); } - const page = await this.pageRepo.insertPage({ slugId: generateSlugId(), title: createPageDto.title, @@ -228,7 +227,6 @@ export class PageService { contributors.add(user.id); const contributorIds = Array.from(contributors); - // Detect a real title/icon change so the WS tree listener can broadcast an // `updateOne` to the space (rename / icon swap) WITHOUT re-broadcasting on a // content-only save. Only treat a field as changed when the DTO actually @@ -246,7 +244,8 @@ export class PageService { icon: updatePageDto.icon, lastUpdatedById: user.id, // Agent-edit provenance: annotate the source without changing the - // responsible author. A normal user request leaves the column default. + // responsible author. A normal user request leaves the existing source + // value unchanged. ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'), updatedAt: new Date(), contributorIds: contributorIds, @@ -934,13 +933,12 @@ export class PageService { } } - const updateResult = await this.pageRepo.updatePage( { position: dto.position, parentPageId: parentPageId, // Agent-edit provenance: annotate the source on an agent move. A normal - // user request leaves the column default ('user'). + // user request leaves the existing source value unchanged. ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'), }, dto.pageId, diff --git a/apps/server/src/database/repos/user/user.repo.ts b/apps/server/src/database/repos/user/user.repo.ts index c9d47e94..ad2a6bef 100644 --- a/apps/server/src/database/repos/user/user.repo.ts +++ b/apps/server/src/database/repos/user/user.repo.ts @@ -36,9 +36,6 @@ export class UserRepo { 'updatedAt', 'deletedAt', 'hasGeneratedPassword', - // AI agent identity flag — needed by the JWT strategy to derive a - // non-spoofable 'agent' provenance from the signed server-side identity. - 'isAgent', ]; async findById( @@ -48,6 +45,12 @@ export class UserRepo { includePassword?: boolean; includeUserMfa?: boolean; includeScimExternalId?: boolean; + // Opt-in: `isAgent` is internal provenance state, not part of the generic + // user payload. Keeping it out of `baseFields` stops it from leaking into + // the workspace member list / `/users/me` (an enumeration leak). Only the + // JWT + collab auth seams opt in, because they derive a non-spoofable + // 'agent' provenance from the signed server-side identity. + includeIsAgent?: boolean; trx?: KyselyTransaction; }, ): Promise { @@ -58,6 +61,7 @@ export class UserRepo { .$if(opts?.includePassword, (qb) => qb.select('password')) .$if(opts?.includeUserMfa, (qb) => qb.select(this.withUserMfa)) .$if(opts?.includeScimExternalId, (qb) => qb.select('scimExternalId')) + .$if(opts?.includeIsAgent, (qb) => qb.select('isAgent')) .where('id', '=', userId) .where('workspaceId', '=', workspaceId) .executeTakeFirst();