diff --git a/.env.example b/.env.example index 51fc4d4d..bb189adc 100644 --- a/.env.example +++ b/.env.example @@ -251,6 +251,16 @@ MCP_DOCMOST_PASSWORD= # Default 120000 (2 min). # AI_MCP_CALL_TIMEOUT_MS=120000 +# Kill-switch for the agent API-key feature (#501). Default ON when unset — a +# deploy that never sets it must NOT silently kill every agent. STRICT parse: +# only the literals `true` / `false` are accepted; a typo like `=0`/`=off`/`=False` +# FAILS AT BOOT by design (never silently read as "enabled"), so the switch is +# guaranteed to actually flip when an operator flips it during an incident. When +# set to `false`: all api-key auth is DENIED (every api-key token is rejected) and +# the api-key management endpoints return 404. The resolved state is logged at boot +# (`API keys: ENABLED/DISABLED (API_KEYS_ENABLED=...)`) so it is verifiable per deploy. +# API_KEYS_ENABLED=true + # Max JSON/urlencoded request body size (bytes). Fastify's 1 MiB default is too # small for a long AI-chat research turn: the client resends the FULL message # history (every tool call + search result) on each turn, so a deep conversation's diff --git a/CHANGELOG.md b/CHANGELOG.md index e33e1149..53dfdcd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -392,6 +392,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `@docmost/token-estimate` package) so they can never diverge. Deferred-tool activation is also cached in the chat metadata to avoid re-resolving it each turn. (#490) +- **Cyrillic (and any non-ASCII) draw.io labels no longer turn into mojibake + when a diagram is opened in the draw.io editor.** Agent-created diagrams + (`drawioCreate`) and Confluence-imported diagrams stored their model in the + SVG's `content=` attribute as base64; the draw.io editor decodes that via + Latin-1 `atob` (no UTF-8 step), so every non-ASCII char (e.g. `Старт-бит`, + `ё`, `—`) split into garbage and the editor's autosave then persisted the + corrupted model, breaking the page preview too. Both write paths + (`buildDrawioSvg`, the import service's `createDrawioSvg`) now write `content=` + as XML-entity-escaped mxfile XML — draw.io's own native form, decoded by the + DOM as UTF-8 — so labels open intact. The decoder reads both the new + entity-encoded form and the old base64 form, so existing diagrams still open. + *Healing pre-fix diagrams:* only a diagram that still holds its original + (correct-UTF-8) base64 — i.e. one not yet opened/autosaved in the draw.io + editor — can be repaired in place by `drawioGet` → `drawioUpdate` with the + same XML (rewrites the attachment in the new form); no migration script is + needed. A diagram that was already opened in the editor persisted the + mojibake at rest, so `drawioGet` reads the already-corrupted text and + `drawioUpdate` faithfully rewrites it — that text is lost and is not + recoverable by a rewrite. (#507) - **A chat with one malformed message part no longer 500s on every turn, and a failed send no longer duplicates the user's message.** Incoming client parts are now whitelisted to `text` (a forged tool-result part can no longer reach diff --git a/apps/client/src/features/page/tree/components/space-tree.tsx b/apps/client/src/features/page/tree/components/space-tree.tsx index 98317797..64586f9c 100644 --- a/apps/client/src/features/page/tree/components/space-tree.tsx +++ b/apps/client/src/features/page/tree/components/space-tree.tsx @@ -281,10 +281,12 @@ const SpaceTree = forwardRef(function SpaceTree( setOpenTreeNodes((prev) => ({ ...prev, [id]: isOpen })); if (isOpen) { const node = treeModel.find(data, id) as SpaceTreeNode | null; - if ( - node?.hasChildren && - (!node.children || node.children.length === 0) - ) { + // Same "unloaded branch" predicate the realtime insert paths use + // (`isUnloadedBranch`) so the lazy-load gate and the realtime inserts + // (`insertByPosition` / `placeByPosition`) can never disagree about what + // counts as unloaded (#525). Note: local raw `insert` (DnD/create-page) + // does not yet route through it — see #525 follow-up. + if (treeModel.isUnloadedBranch(node)) { const fetched = await fetchAllAncestorChildren({ pageId: id, spaceId: node.spaceId, diff --git a/apps/client/src/features/page/tree/model/tree-model.test.ts b/apps/client/src/features/page/tree/model/tree-model.test.ts index 3b365bf4..4ea060e9 100644 --- a/apps/client/src/features/page/tree/model/tree-model.test.ts +++ b/apps/client/src/features/page/tree/model/tree-model.test.ts @@ -74,6 +74,48 @@ describe("treeModel.isDescendant", () => { }); }); +// #525: the single "is this branch unloaded?" predicate shared by the lazy-load +// gate and the insert paths. Unloaded == server says hasChildren but none are +// present locally (canonical form `children: []`, also `undefined`). A parent +// without hasChildren is genuinely empty, not unloaded. +describe("treeModel.isUnloadedBranch", () => { + type PH = TreeNode<{ name: string; hasChildren?: boolean }>; + it("true for hasChildren + empty array (canonical unloaded form)", () => { + const n: PH = { id: "p", name: "P", hasChildren: true, children: [] }; + expect(treeModel.isUnloadedBranch(n)).toBe(true); + }); + it("true for hasChildren + undefined children", () => { + const n: PH = { id: "p", name: "P", hasChildren: true }; + expect(treeModel.isUnloadedBranch(n)).toBe(true); + }); + it("false for hasChildren + already-loaded children", () => { + const n: PH = { + id: "p", + name: "P", + hasChildren: true, + children: [{ id: "c", name: "C" }], + }; + expect(treeModel.isUnloadedBranch(n)).toBe(false); + }); + it("false for a genuinely-empty parent (no hasChildren)", () => { + expect( + treeModel.isUnloadedBranch({ + id: "p", + name: "P", + hasChildren: false, + children: [], + } as PH), + ).toBe(false); + expect( + treeModel.isUnloadedBranch({ id: "p", name: "P" } as PH), + ).toBe(false); + }); + it("false for null/undefined", () => { + expect(treeModel.isUnloadedBranch(null)).toBe(false); + expect(treeModel.isUnloadedBranch(undefined)).toBe(false); + }); +}); + describe("treeModel.visible", () => { it("returns only root nodes when no openIds", () => { const v = treeModel.visible(fixture, new Set()); @@ -197,43 +239,64 @@ describe("treeModel.insertByPosition", () => { ]); }); - // #159 #1: inserting/moving a node under a parent whose children are NOT - // loaded (`children === undefined`, e.g. a collapsed page) must NOT materialize - // a partial `[node]` list — that would defeat the lazy-load gate and hide the - // parent's other real children. The node is left to be lazy-loaded; only - // `hasChildren` is flagged so the chevron appears. - it("does NOT materialize a child under an UNLOADED parent (children undefined)", () => { - type PH = TreeNode<{ - name: string; - position?: string; - hasChildren?: boolean; - }>; + type PH = TreeNode<{ + name: string; + position?: string; + hasChildren?: boolean; + }>; + + // #159 #1 / #525: inserting/moving a node under an UNLOADED parent must NOT + // materialize a partial `[node]` list — that would defeat the lazy-load gate and + // hide the parent's other real children. The canonical unloaded form here is + // `children: []` + `hasChildren: true` (from `pageToTreeNode` / + // `pruneCollapsedChildren`), which the pre-#525 `=== undefined` guard MISSED. + // The node is left to be lazy-loaded; the chevron stays enabled. + it("does NOT materialize a child under an UNLOADED parent (children: [], hasChildren: true)", () => { const tree: PH[] = [ - { id: "p", name: "P", position: "a0", hasChildren: false }, // children: undefined + { id: "p", name: "P", position: "a0", hasChildren: true, children: [] }, ]; const node: PH = { id: "x", name: "X", position: "a1" }; const t = treeModel.insertByPosition(tree, "p", node); const parent = treeModel.find(t, "p"); // The node was NOT inserted (children stay unloaded -> lazy-load fetches the - // full set, including this node, on expand). - expect(parent?.children).toBeUndefined(); + // full set, including this node, on expand). MUTATION: the pre-#525 predicate + // `children === undefined` does not fire for `[]`, so it would insert `[x]` + // here and reredden this expectation. + expect(parent?.children).toEqual([]); expect(treeModel.find(t, "x")).toBeNull(); - // ...but the chevron is enabled so the user can expand to load it. + // ...and the chevron stays enabled so the user can expand to load it. expect((parent as PH).hasChildren).toBe(true); }); - it("DOES insert under a LOADED-but-empty parent (children: [])", () => { - type PH = TreeNode<{ - name: string; - position?: string; - hasChildren?: boolean; - }>; + it("does NOT materialize a child under an UNLOADED parent (children undefined, hasChildren: true)", () => { + const tree: PH[] = [ + { id: "p", name: "P", position: "a0", hasChildren: true }, // children: undefined + ]; + const node: PH = { id: "x", name: "X", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + const parent = treeModel.find(t, "p"); + expect(parent?.children).toBeUndefined(); + expect(treeModel.find(t, "x")).toBeNull(); + expect((parent as PH).hasChildren).toBe(true); + }); + + it("DOES insert under a genuinely-empty parent (children: [], hasChildren: false)", () => { const tree: PH[] = [ { id: "p", name: "P", position: "a0", hasChildren: false, children: [] }, ]; const node: PH = { id: "x", name: "X", position: "a1" }; const t = treeModel.insertByPosition(tree, "p", node); - // A loaded (empty) child list is complete, so the node IS inserted. + // No server children (`hasChildren: false`), so materializing the first child + // is correct — nothing is hidden. + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]); + }); + + it("DOES insert under a genuinely-empty parent (children undefined, hasChildren: false)", () => { + const tree: PH[] = [ + { id: "p", name: "P", position: "a0", hasChildren: false }, // children: undefined + ]; + const node: PH = { id: "x", name: "X", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]); }); diff --git a/apps/client/src/features/page/tree/model/tree-model.ts b/apps/client/src/features/page/tree/model/tree-model.ts index bda4a74b..7cdfc797 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -43,6 +43,26 @@ export const treeModel = { }; }, + // A branch is "unloaded" when the server says it HAS children (`hasChildren`) + // but none are present locally. The canonical unloaded form in this codebase + // is `children: []` (produced by `pageToTreeNode` and by `pruneCollapsedChildren` + // resetting collapsed branches), NOT `children: undefined` — so a predicate that + // only checks `=== undefined` misses the real case and materializes a misleading + // partial list (#525). This is the SINGLE source of truth for "should a + // fetch/materialize be deferred?", shared by the lazy-load gate (`handleToggle`) + // and the realtime insert paths (`insertByPosition` / `placeByPosition`), so they + // can never drift apart again. (The local raw `insert` primitive and its DnD/ + // create-page callers do NOT yet route through this predicate — see #525 + // follow-up.) A parent WITHOUT `hasChildren` is genuinely empty + // (no server children) — inserting its first child is correct, not deferred. + isUnloadedBranch( + node: TreeNode | null | undefined, + ): boolean { + if (!node) return false; + const hasChildren = (node as { hasChildren?: boolean }).hasChildren === true; + return hasChildren && (node.children == null || node.children.length === 0); + }, + isDescendant( tree: TreeNode[], ancestorId: string, @@ -127,14 +147,15 @@ export const treeModel = { } const parent = treeModel.find(tree, parentId); // The parent is in the tree but its children have NOT been lazy-loaded yet - // (`children === undefined`, distinct from a loaded-but-empty `[]`). Inserting + // (`hasChildren` set + children absent/empty — see `isUnloadedBranch`; the + // canonical unloaded form is `children: []`, NOT just `undefined`). Inserting // here would MATERIALIZE a misleading partial child list (`[node]`) that // defeats the lazy-load gate — which fetches only when children are // absent/empty — so the parent's OTHER real children would never load and the // moved/added node would be the only one shown (a silent data loss, #159 #1). // Instead, leave the children unloaded and just flag `hasChildren` so the // chevron appears; expanding fetches the FULL set (including this node). - if (parent && parent.children === undefined) { + if (parent && treeModel.isUnloadedBranch(parent)) { return treeModel.update( tree, parentId, diff --git a/apps/client/src/features/websocket/tree-socket-reducers.test.ts b/apps/client/src/features/websocket/tree-socket-reducers.test.ts index ae93a714..43aa642f 100644 --- a/apps/client/src/features/websocket/tree-socket-reducers.test.ts +++ b/apps/client/src/features/websocket/tree-socket-reducers.test.ts @@ -82,17 +82,19 @@ describe("applyMoveTreeNode", () => { ]); }); - it("does NOT create a partial child list when the destination is loaded-but-collapsed (children unloaded) — keeps it lazy-loadable (#159)", () => { - // `dstCollapsed` is in the tree but its children were never lazy-loaded - // (children === undefined). The OLD behavior inserted `src` as the ONLY - // child ([src]), which defeated the lazy-load gate and HID the parent's - // other real children. Now the move leaves children unloaded (so expanding - // fetches the FULL set, including src) and just flags hasChildren. + it("does NOT create a partial child list when the destination is loaded-but-collapsed (children unloaded) — keeps it lazy-loadable (#159 #525)", () => { + // `dstCollapsed` is in the tree but its children were never lazy-loaded. The + // CANONICAL unloaded form here is `hasChildren: true` + `children: []` (from + // `pageToTreeNode` / `pruneCollapsedChildren`), NOT `children: undefined`. + // The pre-#525 predicate (`children === undefined`) missed this form and + // inserted `src` as the ONLY child ([src]), defeating the lazy-load gate and + // HIDING the parent's other real children. Now the move leaves children + // unloaded (so expanding fetches the FULL set, including src). const tree: SpaceTreeNode[] = [ node("dstCollapsed", { position: "a0", - hasChildren: false, - children: undefined as unknown as SpaceTreeNode[], + hasChildren: true, + children: [], }), node("src", { position: "a9" }), ]; @@ -105,9 +107,10 @@ describe("applyMoveTreeNode", () => { pageData: {}, }); const dst = treeModel.find(next, "dstCollapsed"); - // Children stay unloaded -> the lazy-load gate fetches the FULL set (incl. - // src) on expand, rather than showing a misleading partial [src] list. - expect(dst?.children).toBeUndefined(); + // Children stay unloaded ([] not materialized to [src]) -> the lazy-load gate + // fetches the FULL set (incl. src) on expand. MUTATION: the pre-#525 + // `=== undefined` predicate would insert [src] here and redden this. + expect(dst?.children).toEqual([]); expect(dst?.hasChildren).toBe(true); // src moved away from its old root slot (it lives under dstCollapsed // server-side and reappears when the parent is expanded/loaded). diff --git a/apps/server/src/collaboration/collaboration.module.ts b/apps/server/src/collaboration/collaboration.module.ts index 66d8001f..94ebba62 100644 --- a/apps/server/src/collaboration/collaboration.module.ts +++ b/apps/server/src/collaboration/collaboration.module.ts @@ -16,6 +16,7 @@ import { TransclusionService } from '../core/page/transclusion/transclusion.serv import { TransclusionModule } from '../core/page/transclusion/transclusion.module'; import { StorageModule } from '../integrations/storage/storage.module'; import { EnvironmentModule } from '../integrations/environment/environment.module'; +import { ApiKeyModule } from '../core/api-key/api-key.module'; @Module({ providers: [ @@ -31,6 +32,7 @@ import { EnvironmentModule } from '../integrations/environment/environment.modul exports: [CollaborationGateway], imports: [ TokenModule, + ApiKeyModule, WatcherModule, StorageModule.forRootAsync({ imports: [EnvironmentModule], diff --git a/apps/server/src/collaboration/extensions/authentication.extension.spec.ts b/apps/server/src/collaboration/extensions/authentication.extension.spec.ts index 585393a4..c22118d5 100644 --- a/apps/server/src/collaboration/extensions/authentication.extension.spec.ts +++ b/apps/server/src/collaboration/extensions/authentication.extension.spec.ts @@ -52,6 +52,7 @@ describe('AuthenticationExtension.onAuthenticate', () => { let pageRepo: { findById: jest.Mock }; let spaceMemberRepo: { getUserSpaceRoles: jest.Mock }; let pagePermissionRepo: { canUserEditPage: jest.Mock }; + let apiKeyService: { validate: jest.Mock }; // Build the hocuspocus onAuthenticate payload. connectionConfig.readOnly // starts false; the extension flips it to true on a read-only downgrade. @@ -79,12 +80,15 @@ describe('AuthenticationExtension.onAuthenticate', () => { }), }; + apiKeyService = { validate: jest.fn().mockResolvedValue({ user: {}, workspace: {} }) }; + ext = new AuthenticationExtension( tokenService as any, userRepo as any, pageRepo as any, spaceMemberRepo as any, pagePermissionRepo as any, + apiKeyService as any, ); // Silence the extension's logger (it warns/debugs on denial branches). jest.spyOn(ext['logger'], 'warn').mockImplementation(() => undefined); @@ -231,4 +235,73 @@ describe('AuthenticationExtension.onAuthenticate', () => { // No internal ai_chats row for an MCP/service-account collab edit → null. expect(ctx.aiChatId).toBeNull(); }); + + // --- #501: api-key laundering guard (fail-closed discriminator) ---------- + describe('api-key laundering guard', () => { + it('api_key principal → row-checks the key on connect (valid key proceeds)', async () => { + tokenService.verifyJwt.mockResolvedValue( + buildJwt({ principal: 'api_key', apiKeyId: 'key-1' }), + ); + const data = buildData(); + await ext.onAuthenticate(data as any); + + expect(apiKeyService.validate).toHaveBeenCalledTimes(1); + expect(apiKeyService.validate).toHaveBeenCalledWith( + expect.objectContaining({ apiKeyId: 'key-1', type: JwtType.API_KEY }), + ); + }); + + it('REVOKED api_key → Unauthorized on connect, BEFORE any page/user lookup', async () => { + tokenService.verifyJwt.mockResolvedValue( + buildJwt({ principal: 'api_key', apiKeyId: 'key-1' }), + ); + // The shared validator denies a revoked key. + apiKeyService.validate.mockRejectedValue(new UnauthorizedException()); + + await expect(ext.onAuthenticate(buildData() as any)).rejects.toThrow( + UnauthorizedException, + ); + // No new collab connection: the key check gates before page access. + expect(pageRepo.findById).not.toHaveBeenCalled(); + }); + + it('api_key principal missing apiKeyId → Unauthorized (malformed)', async () => { + tokenService.verifyJwt.mockResolvedValue(buildJwt({ principal: 'api_key' })); + await expect(ext.onAuthenticate(buildData() as any)).rejects.toThrow( + UnauthorizedException, + ); + expect(apiKeyService.validate).not.toHaveBeenCalled(); + }); + + it('session principal → NO api-key check (session-backed, incl. internal agent)', async () => { + tokenService.verifyJwt.mockResolvedValue(buildJwt({ principal: 'session' })); + await ext.onAuthenticate(buildData() as any); + expect(apiKeyService.validate).not.toHaveBeenCalled(); + }); + + it('claimless token WITHIN the grace window → trusted (legacy pre-rollout)', async () => { + // Default rolloutAt = now, so we are inside the grace window. + tokenService.verifyJwt.mockResolvedValue(buildJwt()); // no principal + await expect(ext.onAuthenticate(buildData() as any)).resolves.toBeDefined(); + expect(apiKeyService.validate).not.toHaveBeenCalled(); + }); + + it('claimless token AFTER the grace window → Unauthorized (fail-closed)', async () => { + // Move the rollout reference far into the past so the grace has elapsed. + (ext as any).rolloutAt = Date.now() - 25 * 60 * 60 * 1000; + tokenService.verifyJwt.mockResolvedValue(buildJwt()); // no principal + await expect(ext.onAuthenticate(buildData() as any)).rejects.toThrow( + UnauthorizedException, + ); + }); + + it('infra error from the api-key row-check propagates (not masked)', async () => { + tokenService.verifyJwt.mockResolvedValue( + buildJwt({ principal: 'api_key', apiKeyId: 'key-1' }), + ); + const boom = new Error('db down'); + apiKeyService.validate.mockRejectedValue(boom); + await expect(ext.onAuthenticate(buildData() as any)).rejects.toBe(boom); + }); + }); }); diff --git a/apps/server/src/collaboration/extensions/authentication.extension.ts b/apps/server/src/collaboration/extensions/authentication.extension.ts index 79b4246c..5f1c3f73 100644 --- a/apps/server/src/collaboration/extensions/authentication.extension.ts +++ b/apps/server/src/collaboration/extensions/authentication.extension.ts @@ -14,20 +14,37 @@ import { findHighestUserSpaceRole } from '@docmost/db/repos/space/utils'; import { SpaceRole } from '../../common/helpers/types/permission'; import { isUserDisabled } from '../../common/helpers'; import { getPageId } from '../collaboration.util'; -import { JwtCollabPayload, JwtType } from '../../core/auth/dto/jwt-payload'; +import { + JwtApiKeyPayload, + JwtCollabPayload, + JwtType, +} from '../../core/auth/dto/jwt-payload'; import { resolveProvenance } from '../../common/decorators/auth-provenance.decorator'; import { observeCollabAuth } from '../../integrations/metrics/metrics.registry'; +import { ApiKeyService } from '../../core/api-key/api-key.service'; + +// Max lifetime of a collab token (generateCollabToken uses expiresIn '24h'). Used +// as the rollout grace window below: once this long has elapsed since this +// process started serving the #501 code, every STILL-VALID collab token was +// necessarily minted post-rollout and MUST carry the `principal` discriminator, +// so a claimless one is a bug and is rejected (fail-closed) rather than trusted. +const COLLAB_TOKEN_GRACE_MS = 24 * 60 * 60 * 1000; @Injectable() export class AuthenticationExtension implements Extension { private readonly logger = new Logger(AuthenticationExtension.name); + // Reference instant for the claimless-rejection grace window. Overridable so a + // unit test can drive the pre-/post-grace boundary without wall-clock waits. + protected rolloutAt = Date.now(); + constructor( private tokenService: TokenService, private userRepo: UserRepo, private pageRepo: PageRepo, private readonly spaceMemberRepo: SpaceMemberRepo, private readonly pagePermissionRepo: PagePermissionRepo, + private readonly apiKeyService: ApiKeyService, ) {} async onAuthenticate(data: onAuthenticatePayload) { @@ -54,6 +71,36 @@ export class AuthenticationExtension implements Extension { throw new UnauthorizedException('Invalid collab token'); } + // #501 — fail-closed api-key laundering guard. A collab token minted by an + // api-key principal carries principal='api_key' + apiKeyId; re-check the key + // on connect so a REVOKED key gets NO new collab connections (a collab token + // outlives its 24h, but a revoked key can no longer open fresh ones). An + // api-key token missing its apiKeyId is malformed → reject. A claimless token + // (no recognized principal) is trusted only DURING the rollout grace window + // (a legacy pre-rollout session token, which api keys could never mint); + // once the grace has elapsed every valid token must carry the discriminator, + // so a claimless one is a bug and is rejected (not silently trusted for 24h). + const principal = jwtPayload.principal; + if (principal === 'api_key') { + if (!jwtPayload.apiKeyId) { + throw new UnauthorizedException(); + } + // Row-check via the SHARED validator: throws Unauthorized on a revoked/ + // expired/disabled key; an infra error propagates (not masked). No new + // connection for a dead key. + await this.apiKeyService.validate({ + sub: jwtPayload.sub, + workspaceId: jwtPayload.workspaceId, + apiKeyId: jwtPayload.apiKeyId, + type: JwtType.API_KEY, + } as JwtApiKeyPayload); + } else if (principal !== 'session') { + // Unrecognized/absent discriminator: reject once past the grace window. + if (Date.now() - this.rolloutAt >= COLLAB_TOKEN_GRACE_MS) { + throw new UnauthorizedException(); + } + } + const userId = jwtPayload.sub; const workspaceId = jwtPayload.workspaceId; diff --git a/apps/server/src/core/api-key/api-key.controller.spec.ts b/apps/server/src/core/api-key/api-key.controller.spec.ts new file mode 100644 index 00000000..3cf3d94c --- /dev/null +++ b/apps/server/src/core/api-key/api-key.controller.spec.ts @@ -0,0 +1,193 @@ +import { ForbiddenException, NotFoundException } from '@nestjs/common'; +import { ApiKeyController } from './api-key.controller'; +import { + WorkspaceCaslAction, + WorkspaceCaslSubject, +} from '../casl/interfaces/workspace-ability.type'; + +/** + * Authorization contract for the /api-keys management surface. + * + * - A token cannot manage tokens (an api_key PRINCIPAL is 403 on every method) + * — GitHub-PAT semantics closing post-revocation laundering. + * - admin (CASL Manage on API) sees/revokes all workspace keys; a member only + * their own. + * - kill-switch OFF -> the surface 404s (looks like the feature does not exist). + */ + +function makeController(over: any = {}) { + const apiKeyService = { + create: jest.fn().mockResolvedValue({ + token: 'tok', + key: { id: 'k1', name: 'n', expiresAt: null, createdAt: new Date() }, + }), + revoke: jest.fn().mockResolvedValue(undefined), + ...(over.apiKeyService ?? {}), + }; + const apiKeyRepo = { + findAllInWorkspace: jest.fn().mockResolvedValue([]), + findByCreator: jest.fn().mockResolvedValue([]), + findById: jest.fn(), + ...(over.apiKeyRepo ?? {}), + }; + const workspaceAbility = { + createForUser: jest.fn().mockReturnValue({ + can: (_a: any, _s: any) => over.canManage ?? false, + }), + ...(over.workspaceAbility ?? {}), + }; + const environmentService = { + isApiKeysEnabled: jest.fn().mockReturnValue(over.enabled ?? true), + ...(over.environmentService ?? {}), + }; + const auditService = { log: jest.fn() }; + const controller = new ApiKeyController( + apiKeyService as any, + apiKeyRepo as any, + workspaceAbility as any, + environmentService as any, + auditService as any, + ); + return { + controller, + apiKeyService, + apiKeyRepo, + workspaceAbility, + environmentService, + auditService, + }; +} + +const user = { id: 'u-1', workspaceId: 'ws-1' } as any; +const workspace = { id: 'ws-1' } as any; +const reqAccess = () => ({ raw: {}, ip: '1.2.3.4', socket: {} }) as any; +const reqApiKey = () => + ({ raw: { authType: 'api_key', apiKeyId: 'k-self' }, ip: '1.2.3.4', socket: {} }) as any; + +describe('ApiKeyController — a token cannot manage tokens', () => { + it('403 on create for an api_key principal', async () => { + const { controller, apiKeyService } = makeController(); + await expect( + controller.create({ name: 'x' } as any, user, reqApiKey()), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(apiKeyService.create).not.toHaveBeenCalled(); + }); + + it('403 on list for an api_key principal', async () => { + const { controller } = makeController(); + await expect( + controller.list(user, workspace, reqApiKey()), + ).rejects.toBeInstanceOf(ForbiddenException); + }); + + it('403 on revoke for an api_key principal', async () => { + const { controller } = makeController(); + await expect( + controller.revoke({ id: 'k1' } as any, user, workspace, reqApiKey()), + ).rejects.toBeInstanceOf(ForbiddenException); + }); +}); + +describe('ApiKeyController — kill-switch OFF → 404', () => { + it('create/list/revoke all 404 when disabled', async () => { + const { controller } = makeController({ enabled: false }); + await expect( + controller.create({ name: 'x' } as any, user, reqAccess()), + ).rejects.toBeInstanceOf(NotFoundException); + await expect( + controller.list(user, workspace, reqAccess()), + ).rejects.toBeInstanceOf(NotFoundException); + await expect( + controller.revoke({ id: 'k1' } as any, user, workspace, reqAccess()), + ).rejects.toBeInstanceOf(NotFoundException); + }); +}); + +describe('ApiKeyController — list scoping', () => { + it('admin (Manage on API) lists ALL workspace keys', async () => { + const { controller, apiKeyRepo } = makeController({ canManage: true }); + await controller.list(user, workspace, reqAccess()); + expect(apiKeyRepo.findAllInWorkspace).toHaveBeenCalledWith('ws-1'); + expect(apiKeyRepo.findByCreator).not.toHaveBeenCalled(); + }); + + it('member lists ONLY their own keys', async () => { + const { controller, apiKeyRepo } = makeController({ canManage: false }); + await controller.list(user, workspace, reqAccess()); + expect(apiKeyRepo.findByCreator).toHaveBeenCalledWith('u-1', 'ws-1'); + expect(apiKeyRepo.findAllInWorkspace).not.toHaveBeenCalled(); + }); +}); + +describe('ApiKeyController — revoke scoping', () => { + it("member cannot revoke another user's key (403)", async () => { + const { controller, apiKeyRepo, apiKeyService } = makeController({ + canManage: false, + }); + apiKeyRepo.findById.mockResolvedValue({ + id: 'k1', + creatorId: 'someone-else', + workspaceId: 'ws-1', + }); + await expect( + controller.revoke({ id: 'k1' } as any, user, workspace, reqAccess()), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(apiKeyService.revoke).not.toHaveBeenCalled(); + }); + + it('member CAN revoke their own key', async () => { + const { controller, apiKeyRepo, apiKeyService } = makeController({ + canManage: false, + }); + apiKeyRepo.findById.mockResolvedValue({ + id: 'k1', + creatorId: 'u-1', + workspaceId: 'ws-1', + }); + await controller.revoke({ id: 'k1' } as any, user, workspace, reqAccess()); + expect(apiKeyService.revoke).toHaveBeenCalledWith('k1', 'ws-1'); + }); + + it("admin CAN revoke another user's key", async () => { + const { controller, apiKeyRepo, apiKeyService } = makeController({ + canManage: true, + }); + apiKeyRepo.findById.mockResolvedValue({ + id: 'k1', + creatorId: 'someone-else', + workspaceId: 'ws-1', + }); + await controller.revoke({ id: 'k1' } as any, user, workspace, reqAccess()); + expect(apiKeyService.revoke).toHaveBeenCalledWith('k1', 'ws-1'); + }); + + it('404 when the key does not exist', async () => { + const { controller, apiKeyRepo } = makeController({ canManage: true }); + apiKeyRepo.findById.mockResolvedValue(undefined); + await expect( + controller.revoke({ id: 'k1' } as any, user, workspace, reqAccess()), + ).rejects.toBeInstanceOf(NotFoundException); + }); +}); + +describe('ApiKeyController — create emits audit + returns token once', () => { + it('logs API_KEY_CREATED and returns the token + computed expiry', async () => { + const { controller, auditService } = makeController(); + const res = await controller.create( + { name: 'ci' } as any, + user, + reqAccess(), + ); + expect(res.token).toBe('tok'); + expect(res.apiKey).toMatchObject({ id: 'k1', name: 'n' }); + expect(auditService.log).toHaveBeenCalledWith( + expect.objectContaining({ event: 'api_key.created' }), + ); + }); +}); + +// Sanity: the CASL subject used is the workspace API subject. +it('uses WorkspaceCaslSubject.API with the Manage action', () => { + expect(WorkspaceCaslSubject.API).toBe('api_key'); + expect(WorkspaceCaslAction.Manage).toBeDefined(); +}); diff --git a/apps/server/src/core/api-key/api-key.controller.ts b/apps/server/src/core/api-key/api-key.controller.ts new file mode 100644 index 00000000..455d64d0 --- /dev/null +++ b/apps/server/src/core/api-key/api-key.controller.ts @@ -0,0 +1,201 @@ +import { + Body, + Controller, + ForbiddenException, + HttpCode, + HttpStatus, + Inject, + Logger, + NotFoundException, + Post, + Req, + UseGuards, +} from '@nestjs/common'; +import { Throttle } from '@nestjs/throttler'; +import { FastifyRequest } from 'fastify'; +import { ApiKeyService } from './api-key.service'; +import { ApiKeyRepo } from '@docmost/db/repos/api-key/api-key.repo'; +import { CreateApiKeyDto } from './dto/create-api-key.dto'; +import { RevokeApiKeyDto } from './dto/revoke-api-key.dto'; +import { AuthUser } from '../../common/decorators/auth-user.decorator'; +import { AuthWorkspace } from '../../common/decorators/auth-workspace.decorator'; +import { JwtAuthGuard } from '../../common/guards/jwt-auth.guard'; +import { User, Workspace } from '@docmost/db/types/entity.types'; +import WorkspaceAbilityFactory from '../casl/abilities/workspace-ability.factory'; +import { + WorkspaceCaslAction, + WorkspaceCaslSubject, +} from '../casl/interfaces/workspace-ability.type'; +import { AuditEvent, AuditResource } from '../../common/events/audit-events'; +import { + AUDIT_SERVICE, + IAuditService, +} from '../../integrations/audit/audit.service'; +import { EnvironmentService } from '../../integrations/environment/environment.service'; +import { UserThrottlerGuard } from '../../integrations/throttle/user-throttler.guard'; +import { AUTH_THROTTLER } from '../../integrations/throttle/throttler-names'; + +@UseGuards(JwtAuthGuard) +@Controller('api-keys') +export class ApiKeyController { + private readonly logger = new Logger(ApiKeyController.name); + + constructor( + private readonly apiKeyService: ApiKeyService, + private readonly apiKeyRepo: ApiKeyRepo, + private readonly workspaceAbility: WorkspaceAbilityFactory, + private readonly environmentService: EnvironmentService, + @Inject(AUDIT_SERVICE) private readonly auditService: IAuditService, + ) {} + + // Kill-switch OFF: the issuance surface disappears entirely (404), not a 403 — + // it must look like the feature does not exist. + private assertEnabled(): void { + if (!this.environmentService.isApiKeysEnabled()) { + throw new NotFoundException(); + } + } + + // A token cannot manage tokens (GitHub-PAT semantics): an api-key principal is + // refused on the management surface, so a leaked key cannot mint a replacement + // or revoke the keys that would lock it out (closes post-revocation laundering). + private rejectApiKeyPrincipal(req: FastifyRequest): void { + if ((req.raw as { authType?: string }).authType === 'api_key') { + throw new ForbiddenException('API keys cannot manage API keys'); + } + } + + private clientIp(req: FastifyRequest): string { + return req.ip ?? req.socket?.remoteAddress ?? 'unknown'; + } + + @HttpCode(HttpStatus.OK) + @UseGuards(JwtAuthGuard, UserThrottlerGuard) + @Throttle({ [AUTH_THROTTLER]: { limit: 10, ttl: 60_000 } }) + @Post('create') + async create( + @Body() dto: CreateApiKeyDto, + @AuthUser() user: User, + @Req() req: FastifyRequest, + ) { + this.assertEnabled(); + this.rejectApiKeyPrincipal(req); + + // undefined -> service default (1 year); null -> unlimited; string -> Date. + const expiresAt = + dto.expiresAt === undefined + ? undefined + : dto.expiresAt === null + ? null + : new Date(dto.expiresAt); + + const { token, key } = await this.apiKeyService.create( + user, + dto.name, + expiresAt, + ); + + this.auditService.log({ + event: AuditEvent.API_KEY_CREATED, + resourceType: AuditResource.API_KEY, + resourceId: key.id, + }); + // The durable trail lives in container logs (AUDIT_SERVICE is a Noop in this + // build). No token material — the JWT is only ever returned in the response. + this.logger.log( + `API key created: id=${key.id} name=${JSON.stringify( + key.name, + )} actor=${user.id} expiresAt=${ + key.expiresAt ? new Date(key.expiresAt).toISOString() : 'never' + } ip=${this.clientIp(req)}`, + ); + + // Return the token ONCE (never retrievable again) and the computed expiry so + // the caller/UI can surface "expires " (the year-default time-bomb + // early-warning). + return { + token, + apiKey: { + id: key.id, + name: key.name, + expiresAt: key.expiresAt, + createdAt: key.createdAt, + }, + }; + } + + @HttpCode(HttpStatus.OK) + @Post('list') + async list( + @AuthUser() user: User, + @AuthWorkspace() workspace: Workspace, + @Req() req: FastifyRequest, + ) { + this.assertEnabled(); + this.rejectApiKeyPrincipal(req); + + const ability = this.workspaceAbility.createForUser(user, workspace); + // Admin (CASL Manage on API): every key in the workspace, attributed to its + // creator (closes "a leaked key of a departed employee is invisible"). A + // member sees only their own. + const keys = ability.can( + WorkspaceCaslAction.Manage, + WorkspaceCaslSubject.API, + ) + ? await this.apiKeyRepo.findAllInWorkspace(workspace.id) + : await this.apiKeyRepo.findByCreator(user.id, workspace.id); + + return keys.map((k) => ({ + id: k.id, + name: k.name, + expiresAt: k.expiresAt, + lastUsedAt: k.lastUsedAt, + createdAt: k.createdAt, + creator: k.creator, + })); + } + + @HttpCode(HttpStatus.OK) + @Post('revoke') + async revoke( + @Body() dto: RevokeApiKeyDto, + @AuthUser() user: User, + @AuthWorkspace() workspace: Workspace, + @Req() req: FastifyRequest, + ) { + this.assertEnabled(); + this.rejectApiKeyPrincipal(req); + + const key = await this.apiKeyRepo.findById(dto.id, workspace.id); + // Uniform 404 for a missing/already-revoked key regardless of who asks — no + // existence oracle for keys the caller may not own. + if (!key) { + throw new NotFoundException(); + } + + const ability = this.workspaceAbility.createForUser(user, workspace); + const canManageAll = ability.can( + WorkspaceCaslAction.Manage, + WorkspaceCaslSubject.API, + ); + // Admin may revoke any key in the workspace; a member only their own. + if (!canManageAll && key.creatorId !== user.id) { + throw new ForbiddenException(); + } + + await this.apiKeyService.revoke(key.id, workspace.id); + + this.auditService.log({ + event: AuditEvent.API_KEY_DELETED, + resourceType: AuditResource.API_KEY, + resourceId: key.id, + }); + this.logger.log( + `API key revoked: id=${key.id} name=${JSON.stringify( + key.name, + )} actor=${user.id} ip=${this.clientIp(req)}`, + ); + + return { success: true }; + } +} diff --git a/apps/server/src/core/api-key/api-key.module.ts b/apps/server/src/core/api-key/api-key.module.ts new file mode 100644 index 00000000..9c002f5b --- /dev/null +++ b/apps/server/src/core/api-key/api-key.module.ts @@ -0,0 +1,19 @@ +import { Module } from '@nestjs/common'; +import { ApiKeyService } from './api-key.service'; +import { ApiKeyController } from './api-key.controller'; +import { TokenModule } from '../auth/token.module'; + +// Core (non-EE) API-key feature: issuance REST endpoints + the shared validator +// consumed by jwt.strategy (REST) and McpService (the /mcp Bearer router). +// DatabaseModule (global) provides ApiKeyRepo/UserRepo/WorkspaceRepo; CaslModule +// (global) provides WorkspaceAbilityFactory; TokenModule provides TokenService +// (the no-exp api-key signer). ApiKeyService is exported so AuthModule (for +// jwt.strategy) and McpModule (for the /mcp router) can inject it directly, +// replacing the absent EE `ee/api-key` dynamic require. +@Module({ + imports: [TokenModule], + controllers: [ApiKeyController], + providers: [ApiKeyService], + exports: [ApiKeyService], +}) +export class ApiKeyModule {} diff --git a/apps/server/src/core/api-key/api-key.service.spec.ts b/apps/server/src/core/api-key/api-key.service.spec.ts new file mode 100644 index 00000000..248267ea --- /dev/null +++ b/apps/server/src/core/api-key/api-key.service.spec.ts @@ -0,0 +1,299 @@ +import { UnauthorizedException } from '@nestjs/common'; +import { ApiKeyService } from './api-key.service'; +import { JwtType } from '../auth/dto/jwt-payload'; + +/** + * Security contract for ApiKeyService.validate — the single validator shared by + * jwt.strategy (REST) and the /mcp Bearer router. + * + * Invariants under test: + * - Anti-enumeration: every DEFINITE deny (missing/revoked/expired row, disabled + * user, workspace mismatch, kill-switch off) throws the SAME bare + * UnauthorizedException — an agent cannot distinguish expired from revoked. + * - deny-on-decision / 5xx-on-infra: an UNEXPECTED (infra) error PROPAGATES as + * itself (→ 5xx), never masked as a 401. + * - Expiry is read from the ROW, never a JWT exp claim. + * - No validate cache (each call re-reads the row → immediate revocation). + */ + +const APP_SECRET = 'secret'; + +function makeDeps(over: Partial> = {}) { + const apiKeyRepo = { + findById: jest.fn(), + insert: jest.fn(), + softDelete: jest.fn(), + touchLastUsed: jest.fn().mockResolvedValue(undefined), + ...(over.apiKeyRepo ?? {}), + }; + const userRepo = { + findById: jest.fn(), + ...(over.userRepo ?? {}), + }; + const workspaceRepo = { + findById: jest.fn().mockResolvedValue({ id: 'ws-1' }), + ...(over.workspaceRepo ?? {}), + }; + const tokenService = { + generateApiToken: jest.fn().mockResolvedValue('minted.jwt.token'), + ...(over.tokenService ?? {}), + }; + const environmentService = { + isApiKeysEnabled: jest.fn().mockReturnValue(true), + getApiKeysEnabledRaw: jest.fn().mockReturnValue(undefined), + getAppSecret: jest.fn().mockReturnValue(APP_SECRET), + ...(over.environmentService ?? {}), + }; + const service = new (ApiKeyService as unknown as new (...a: any[]) => ApiKeyService)( + apiKeyRepo, + userRepo, + workspaceRepo, + tokenService, + environmentService, + ); + return { service, apiKeyRepo, userRepo, workspaceRepo, tokenService, environmentService }; +} + +const payload = (over: Record = {}) => ({ + sub: 'u-1', + workspaceId: 'ws-1', + apiKeyId: 'key-1', + type: JwtType.API_KEY, + ...over, +}); + +const activeRow = (over: Record = {}) => ({ + id: 'key-1', + name: 'k', + creatorId: 'u-1', + workspaceId: 'ws-1', + expiresAt: null, + lastUsedAt: null, + deletedAt: null, + ...over, +}); + +const activeUser = (over: Record = {}) => ({ + id: 'u-1', + workspaceId: 'ws-1', + deactivatedAt: null, + deletedAt: null, + isAgent: false, + ...over, +}); + +describe('ApiKeyService.validate', () => { + it('returns { user, workspace } for a valid active key', async () => { + const { service, apiKeyRepo, userRepo } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(activeRow()); + userRepo.findById.mockResolvedValue(activeUser()); + + const res = await service.validate(payload() as any); + + expect(res.user).toMatchObject({ id: 'u-1' }); + expect(res.workspace).toMatchObject({ id: 'ws-1' }); + // Loads isAgent so downstream provenance does not silently degrade. + expect(userRepo.findById).toHaveBeenCalledWith('u-1', 'ws-1', { + includeIsAgent: true, + }); + }); + + // --- Anti-enumeration: every deny is the SAME bare 401 ------------------- + const denyCases: Array<[string, (d: ReturnType) => void]> = [ + [ + 'missing/orphaned row', + (d) => d.apiKeyRepo.findById.mockResolvedValue(undefined), + ], + [ + 'revoked (soft-deleted → findById returns undefined)', + (d) => d.apiKeyRepo.findById.mockResolvedValue(undefined), + ], + [ + 'expired (expires_at in the past)', + (d) => { + d.apiKeyRepo.findById.mockResolvedValue( + activeRow({ expiresAt: new Date(Date.now() - 1000) }), + ); + d.userRepo.findById.mockResolvedValue(activeUser()); + }, + ], + [ + 'disabled user', + (d) => { + d.apiKeyRepo.findById.mockResolvedValue(activeRow()); + d.userRepo.findById.mockResolvedValue( + activeUser({ deactivatedAt: new Date() }), + ); + }, + ], + [ + 'user not found', + (d) => { + d.apiKeyRepo.findById.mockResolvedValue(activeRow()); + d.userRepo.findById.mockResolvedValue(undefined); + }, + ], + [ + 'creator/sub mismatch', + (d) => { + d.apiKeyRepo.findById.mockResolvedValue(activeRow({ creatorId: 'other' })); + d.userRepo.findById.mockResolvedValue(activeUser()); + }, + ], + [ + 'workspace row missing', + (d) => { + d.apiKeyRepo.findById.mockResolvedValue(activeRow()); + d.userRepo.findById.mockResolvedValue(activeUser()); + d.workspaceRepo.findById.mockResolvedValue(undefined); + }, + ], + [ + 'kill-switch OFF', + (d) => d.environmentService.isApiKeysEnabled.mockReturnValue(false), + ], + [ + 'malformed payload (missing apiKeyId)', + () => undefined, + ], + ]; + + it.each(denyCases)( + 'throws a bare UnauthorizedException with NO message for: %s', + async (label, arrange) => { + const deps = makeDeps(); + arrange(deps); + const p = + label === 'malformed payload (missing apiKeyId)' + ? (payload({ apiKeyId: undefined }) as any) + : (payload() as any); + + const err = await deps.service.validate(p).catch((e) => e); + expect(err).toBeInstanceOf(UnauthorizedException); + // Anti-enumeration: identical, class-less message for every failure mode. + expect(err.message).toBe('Unauthorized'); + }, + ); + + it('kill-switch OFF denies BEFORE any DB read', async () => { + const { service, apiKeyRepo, environmentService } = makeDeps(); + environmentService.isApiKeysEnabled.mockReturnValue(false); + + await expect(service.validate(payload() as any)).rejects.toBeInstanceOf( + UnauthorizedException, + ); + expect(apiKeyRepo.findById).not.toHaveBeenCalled(); + }); + + // --- Infra error propagates (5xx), NOT masked as 401 --------------------- + it('PROPAGATES an infra (DB) error instead of masking it as 401', async () => { + const { service, apiKeyRepo } = makeDeps(); + const boom = new Error('connection terminated'); + apiKeyRepo.findById.mockRejectedValue(boom); + + await expect(service.validate(payload() as any)).rejects.toBe(boom); + }); + + // --- No cache: revocation is immediate on the next call ------------------ + it('re-reads the row on every call (no validate cache → immediate revocation)', async () => { + const { service, apiKeyRepo, userRepo } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(activeRow()); + userRepo.findById.mockResolvedValue(activeUser()); + await service.validate(payload() as any); + + // Now the key is revoked (row invisible) — the very next call denies. + apiKeyRepo.findById.mockResolvedValue(undefined); + await expect(service.validate(payload() as any)).rejects.toBeInstanceOf( + UnauthorizedException, + ); + expect(apiKeyRepo.findById).toHaveBeenCalledTimes(2); + }); + + // --- last_used_at is throttled + fire-and-forget ------------------------- + it('touches last_used_at when stale and skips when fresh', async () => { + const { service, apiKeyRepo, userRepo } = makeDeps(); + userRepo.findById.mockResolvedValue(activeUser()); + + // Stale (never used) -> touch. + apiKeyRepo.findById.mockResolvedValue(activeRow({ lastUsedAt: null })); + await service.validate(payload() as any); + expect(apiKeyRepo.touchLastUsed).toHaveBeenCalledTimes(1); + + // Fresh (used 1 minute ago) -> skip. + apiKeyRepo.touchLastUsed.mockClear(); + apiKeyRepo.findById.mockResolvedValue( + activeRow({ lastUsedAt: new Date(Date.now() - 60_000) }), + ); + await service.validate(payload() as any); + expect(apiKeyRepo.touchLastUsed).not.toHaveBeenCalled(); + }); + + it('a failing last_used_at touch never fails the request', async () => { + const { service, apiKeyRepo, userRepo } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(activeRow({ lastUsedAt: null })); + userRepo.findById.mockResolvedValue(activeUser()); + apiKeyRepo.touchLastUsed.mockRejectedValue(new Error('write failed')); + + await expect(service.validate(payload() as any)).resolves.toMatchObject({ + user: { id: 'u-1' }, + }); + }); +}); + +describe('ApiKeyService.create (mint-then-insert, no exp)', () => { + it('mints the JWT BEFORE inserting the row and returns the token once', async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + const order: string[] = []; + tokenService.generateApiToken.mockImplementation(async () => { + order.push('mint'); + return 'tok'; + }); + apiKeyRepo.insert.mockImplementation(async (row: any) => { + order.push('insert'); + return { ...row, createdAt: new Date() }; + }); + + const user = { id: 'u-1', workspaceId: 'ws-1' } as any; + const res = await service.create(user, 'my key'); + + expect(order).toEqual(['mint', 'insert']); + expect(res.token).toBe('tok'); + // The id is generated before minting and reused for the row. + const mintArg = tokenService.generateApiToken.mock.calls[0][0]; + const insertArg = apiKeyRepo.insert.mock.calls[0][0]; + expect(mintArg.apiKeyId).toBe(insertArg.id); + }); + + it('applies the 1-year default when expiresAt is undefined', async () => { + const { service, apiKeyRepo } = makeDeps(); + apiKeyRepo.insert.mockImplementation(async (row: any) => row); + const user = { id: 'u-1', workspaceId: 'ws-1' } as any; + + await service.create(user, 'k'); + + const insertArg = apiKeyRepo.insert.mock.calls[0][0]; + const days = + (insertArg.expiresAt.getTime() - Date.now()) / (24 * 60 * 60 * 1000); + expect(days).toBeGreaterThan(364); + expect(days).toBeLessThan(367); + }); + + it('honors an explicit null (unlimited)', async () => { + const { service, apiKeyRepo } = makeDeps(); + apiKeyRepo.insert.mockImplementation(async (row: any) => row); + const user = { id: 'u-1', workspaceId: 'ws-1' } as any; + + await service.create(user, 'k', null); + + expect(apiKeyRepo.insert.mock.calls[0][0].expiresAt).toBeNull(); + }); + + it('does NOT insert a row when minting fails (mint-then-insert → inert)', async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + tokenService.generateApiToken.mockRejectedValue(new Error('mint failed')); + const user = { id: 'u-1', workspaceId: 'ws-1' } as any; + + await expect(service.create(user, 'k')).rejects.toThrow('mint failed'); + expect(apiKeyRepo.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/core/api-key/api-key.service.ts b/apps/server/src/core/api-key/api-key.service.ts new file mode 100644 index 00000000..797fee8d --- /dev/null +++ b/apps/server/src/core/api-key/api-key.service.ts @@ -0,0 +1,191 @@ +import { + Injectable, + Logger, + OnModuleInit, + UnauthorizedException, +} from '@nestjs/common'; +import { v7 as uuid7 } from 'uuid'; +import { ApiKeyRepo } from '@docmost/db/repos/api-key/api-key.repo'; +import { UserRepo } from '@docmost/db/repos/user/user.repo'; +import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo'; +import { TokenService } from '../auth/services/token.service'; +import { EnvironmentService } from '../../integrations/environment/environment.service'; +import { JwtApiKeyPayload } from '../auth/dto/jwt-payload'; +import { ApiKey, User, Workspace } from '@docmost/db/types/entity.types'; +import { isUserDisabled } from '../../common/helpers'; + +// Default lifetime for a new key when the caller does not specify one: 1 year. +// The owner runs a homelab where agents live for years; forcing rotation is +// operational pain, so an explicit `null` (unlimited) is also allowed. +const DEFAULT_LIFETIME_MS = 365 * 24 * 60 * 60 * 1000; + +// last_used_at is a best-effort forensics stamp, not an access record; we only +// refresh it when it is older than this to avoid a write on every request. The +// 1h resolution is a deliberate constant (forensics granularity, not accounting). +const LAST_USED_THROTTLE_MS = 60 * 60 * 1000; + +/** + * Core API-key lifecycle service. Owns minting (create), the single validator + * shared by BOTH the REST jwt.strategy path and the /mcp Bearer path (validate), + * and revocation (revoke). The `api_keys` ROW is the sole source of truth for a + * key's lifetime and revocation — never the JWT, which carries no `exp` claim. + */ +@Injectable() +export class ApiKeyService implements OnModuleInit { + private readonly logger = new Logger(ApiKeyService.name); + + constructor( + private readonly apiKeyRepo: ApiKeyRepo, + private readonly userRepo: UserRepo, + private readonly workspaceRepo: WorkspaceRepo, + private readonly tokenService: TokenService, + private readonly environmentService: EnvironmentService, + ) {} + + onModuleInit() { + // Boot log so the kill-switch state after each deploy is verifiable in logs. + const enabled = this.environmentService.isApiKeysEnabled(); + const raw = this.environmentService.getApiKeysEnabledRaw(); + this.logger.log( + `API keys: ${enabled ? 'ENABLED' : 'DISABLED'} (API_KEYS_ENABLED=${ + raw ?? 'unset' + })`, + ); + } + + /** + * Mint a new key for `user`. mint-then-insert ordering (R1): + * 1. generate the id first — it must be in the JWT payload before the row. + * 2. mint the JWT (no `exp` claim). A mint failure aborts before any row is + * written (inert), so a half-created key cannot exist. + * 3. insert the row last. A lost response leaves an orphaned row that is + * visible in `list` and self-heals (the user revokes it). + * The token is returned ONCE and never stored — the JWT is self-contained, so + * no token material lives in the table. + * + * `expiresAt`: `undefined` -> default 1 year; `null` -> unlimited (explicit); + * a Date -> that instant (a past date is rejected at the DTO layer). + */ + async create( + user: User, + name: string, + expiresAt?: Date | null, + ): Promise<{ token: string; key: ApiKey }> { + const resolvedExpiresAt = + expiresAt === undefined + ? new Date(Date.now() + DEFAULT_LIFETIME_MS) + : expiresAt; + + const apiKeyId = uuid7(); + + const token = await this.tokenService.generateApiToken({ + apiKeyId, + user, + workspaceId: user.workspaceId, + }); + + const key = await this.apiKeyRepo.insert({ + id: apiKeyId, + name, + creatorId: user.id, + workspaceId: user.workspaceId, + expiresAt: resolvedExpiresAt, + }); + + return { token, key }; + } + + /** + * The single validator for an api-key principal, shared by jwt.strategy and the + * /mcp Bearer router. Returns `{ user, workspace }` (the same shape the access + * path returns) so the AuthUser/AuthWorkspace decorators and MCP identity work + * unchanged. + * + * Failure semantics (R4, anti-enumeration): a DEFINITE negative fact — feature + * disabled, missing/revoked/expired row, workspace mismatch, disabled user — + * throws a bare `UnauthorizedException` (a single generic 401 for every case; + * an agent cannot distinguish expired from revoked, and its reaction is + * identical). An UNEXPECTED (infra) error is NOT caught here: it propagates so + * the surface returns 5xx, never a masked 401 (deny-on-decision / 5xx-on-infra). + * There is NO validate cache: it is 2–3 PK lookups (~1ms), two orders of + * magnitude cheaper than the bcrypt it replaces; a cache would only add a + * Date-serialization trap and a revocation lag. Revocation is immediate. + */ + async validate( + payload: JwtApiKeyPayload, + ): Promise<{ user: User; workspace: Workspace }> { + // Kill-switch OFF: deny unconditionally (same generic 401). Note the + // endpoints additionally 404 at the controller; here we deny the token. + if (!this.environmentService.isApiKeysEnabled()) { + throw new UnauthorizedException(); + } + + if (!payload?.apiKeyId || !payload?.sub || !payload?.workspaceId) { + throw new UnauthorizedException(); + } + + const row = await this.apiKeyRepo.findById( + payload.apiKeyId, + payload.workspaceId, + ); + // Absent row = revoked (soft-deleted, invisible to findById), orphaned + // (creator/workspace cascade-deleted), or never existed. All terminal deny. + if (!row) { + throw new UnauthorizedException(); + } + + // Expiry is read from the ROW, never an `exp` JWT claim. + if (row.expiresAt && row.expiresAt.getTime() <= Date.now()) { + throw new UnauthorizedException(); + } + + const user = await this.userRepo.findById( + payload.sub, + payload.workspaceId, + { includeIsAgent: true }, + ); + if (!user || isUserDisabled(user)) { + throw new UnauthorizedException(); + } + + // The key acts only as its creator (defence in depth against a token whose + // signed `sub` ever drifted from the row's owner). + if (row.creatorId !== user.id) { + throw new UnauthorizedException(); + } + + const workspace = await this.workspaceRepo.findById(payload.workspaceId); + if (!workspace) { + throw new UnauthorizedException(); + } + + // Best-effort, throttled, fire-and-forget forensics stamp AFTER all checks. + this.touchLastUsed(row); + + return { user, workspace }; + } + + /** + * Revoke (soft-delete) a key. Authorization/ownership is decided by the caller + * (the controller, via CASL); this only performs the terminal write. Idempotent: + * a second revoke is a no-op (the row is already invisible). + */ + async revoke(id: string, workspaceId: string): Promise { + await this.apiKeyRepo.softDelete(id, workspaceId); + } + + // Throttled best-effort last_used_at bump: skip if it was touched within the + // window; otherwise fire-and-forget so a stamp write never fails or slows the + // request (mirrors SessionActivityService.trackActivity). + private touchLastUsed(row: ApiKey): void { + const last = row.lastUsedAt ? new Date(row.lastUsedAt).getTime() : 0; + if (Date.now() - last < LAST_USED_THROTTLE_MS) return; + void this.apiKeyRepo.touchLastUsed(row.id).catch((err) => { + this.logger.warn( + `Failed to update api_key last_used_at for ${row.id}: ${ + (err as Error)?.message ?? err + }`, + ); + }); + } +} diff --git a/apps/server/src/core/api-key/dto/create-api-key.dto.ts b/apps/server/src/core/api-key/dto/create-api-key.dto.ts new file mode 100644 index 00000000..a3ee1bff --- /dev/null +++ b/apps/server/src/core/api-key/dto/create-api-key.dto.ts @@ -0,0 +1,51 @@ +import { + IsDateString, + IsNotEmpty, + IsOptional, + IsString, + MaxLength, + registerDecorator, + ValidationOptions, +} from 'class-validator'; + +/** + * `expiresAt` must be strictly in the future. Skips validation for `null` + * (explicit "unlimited") and `undefined` (server applies the 1-year default), + * so only an actually-supplied date is range-checked. Rejecting a PAST date at + * the DTO layer means a caller cannot mint an already-dead key. + */ +function IsFutureDateString(options?: ValidationOptions) { + return function (object: object, propertyName: string) { + registerDecorator({ + name: 'isFutureDateString', + target: object.constructor, + propertyName, + options: { + message: 'expiresAt must be a date in the future', + ...options, + }, + validator: { + validate(value: unknown) { + if (value === null || value === undefined) return true; + if (typeof value !== 'string') return false; + const t = Date.parse(value); + return !Number.isNaN(t) && t > Date.now(); + }, + }, + }); + }; +} + +export class CreateApiKeyDto { + @IsString() + @IsNotEmpty() + @MaxLength(255) + name: string; + + // undefined -> default 1 year (applied server-side); null -> unlimited + // (explicit); an ISO date string -> that instant, which must be in the future. + @IsOptional() + @IsDateString() + @IsFutureDateString() + expiresAt?: string | null; +} diff --git a/apps/server/src/core/api-key/dto/revoke-api-key.dto.ts b/apps/server/src/core/api-key/dto/revoke-api-key.dto.ts new file mode 100644 index 00000000..c304fa8f --- /dev/null +++ b/apps/server/src/core/api-key/dto/revoke-api-key.dto.ts @@ -0,0 +1,6 @@ +import { IsUUID } from 'class-validator'; + +export class RevokeApiKeyDto { + @IsUUID() + id: string; +} diff --git a/apps/server/src/core/auth/auth.controller.spec.ts b/apps/server/src/core/auth/auth.controller.spec.ts index 4746f249..428e538e 100644 --- a/apps/server/src/core/auth/auth.controller.spec.ts +++ b/apps/server/src/core/auth/auth.controller.spec.ts @@ -20,3 +20,71 @@ describe('AuthController', () => { expect(controller).toBeDefined(); }); }); + +// The collab-token handler is the ARM-SEAM of the #501 anti-laundering defense: +// it derives the api-key origin args ({ apiKeyId }) from the SIGNED-derived +// `req.raw` fields (stamped by jwt.strategy) and threads them into the collab +// token mint, forcing principal='api_key' so a later key revoke rejects NEW +// collab connections. A regression here (reading `body`, dropping `apiKeyId`, +// or inverting the ternary) would silently mint api-key requests as +// principal='session' and reopen the laundering hole with a green suite. These +// tests pin the EXACT args the controller passes to authService.getCollabToken. +describe('AuthController.collabToken arms the api-key origin (#501)', () => { + let controller: AuthController; + let getCollabToken: jest.Mock; + + const user = { id: 'u-1', workspaceId: 'ws-1' } as any; + const workspace = { id: 'ws-1' } as any; + + beforeEach(() => { + getCollabToken = jest.fn().mockResolvedValue({ token: 'ct' }); + controller = new AuthController( + { getCollabToken } as any, // authService + {} as any, // sessionService + {} as any, // environmentService + {} as any, // moduleRef + {} as any, // auditService + ); + }); + + it('threads { apiKeyId } when req.raw marks an api_key principal (ARMED)', async () => { + const req = { raw: { authType: 'api_key', apiKeyId: 'k-1' } } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledTimes(1); + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', { + apiKeyId: 'k-1', + }); + }); + + it('passes undefined for a normal session request (NOT armed)', async () => { + const req = { raw: {} } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledTimes(1); + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', undefined); + }); + + it('does NOT arm when api_key authType lacks an apiKeyId', async () => { + const req = { raw: { authType: 'api_key' } } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', undefined); + }); + + it('reads the SIGNED req.raw, never a spoofable client body', async () => { + // A client-supplied body claiming api_key must be ignored: only the + // jwt.strategy-stamped req.raw can arm the api-key origin. + const req = { + raw: {}, + body: { authType: 'api_key', apiKeyId: 'attacker' }, + } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', undefined); + }); +}); diff --git a/apps/server/src/core/auth/auth.controller.ts b/apps/server/src/core/auth/auth.controller.ts index f15d82e4..6991feff 100644 --- a/apps/server/src/core/auth/auth.controller.ts +++ b/apps/server/src/core/auth/auth.controller.ts @@ -207,8 +207,20 @@ export class AuthController { async collabToken( @AuthUser() user: User, @AuthWorkspace() workspace: Workspace, + @Req() req: FastifyRequest, ) { - return this.authService.getCollabToken(user, workspace.id); + // Thread the api-key origin (#501): when the requester authenticated with an + // api key (jwt.strategy stamped req.raw.authType/apiKeyId), the minted collab + // token carries principal='api_key' + apiKeyId so a later revoke of the key + // rejects NEW collab connections. A normal session request mints a + // principal='session' token. Reading the SIGNED-derived req.raw fields (never + // a client body) keeps it unspoofable. + const raw = req.raw as { authType?: string; apiKeyId?: string }; + const apiKey = + raw.authType === 'api_key' && raw.apiKeyId + ? { apiKeyId: raw.apiKeyId } + : undefined; + return this.authService.getCollabToken(user, workspace.id, apiKey); } @SkipThrottle({ [AUTH_THROTTLER]: true }) diff --git a/apps/server/src/core/auth/auth.module.ts b/apps/server/src/core/auth/auth.module.ts index 236f4dd5..3895fff0 100644 --- a/apps/server/src/core/auth/auth.module.ts +++ b/apps/server/src/core/auth/auth.module.ts @@ -5,9 +5,13 @@ import { JwtStrategy } from './strategies/jwt.strategy'; import { WorkspaceModule } from '../workspace/workspace.module'; import { SignupService } from './services/signup.service'; import { TokenModule } from './token.module'; +import { ApiKeyModule } from '../api-key/api-key.module'; @Module({ - imports: [TokenModule, WorkspaceModule], + // ApiKeyModule supplies ApiKeyService, injected into JwtStrategy so an + // api_key Bearer/cookie token is validated directly (replacing the absent EE + // `ee/api-key` dynamic require). + imports: [TokenModule, WorkspaceModule, ApiKeyModule], controllers: [AuthController], providers: [AuthService, SignupService, JwtStrategy], exports: [SignupService, AuthService], diff --git a/apps/server/src/core/auth/dto/jwt-payload.ts b/apps/server/src/core/auth/dto/jwt-payload.ts index b6a9f980..495f7610 100644 --- a/apps/server/src/core/auth/dto/jwt-payload.ts +++ b/apps/server/src/core/auth/dto/jwt-payload.ts @@ -33,6 +33,17 @@ export type JwtPayload = { aiChatId?: string | null; }; +// The AUTH-PRINCIPAL kind behind a collab token — distinct from `actor` +// (provenance). Stamped into EVERY newly-minted collab token (#501): 'session' +// for a normal user/session (incl. the internal AI agent, which is session- +// backed), 'api_key' when the token was minted by an api-key principal (an +// external MCP agent). The discriminator keys on the token's ORIGIN, so the +// collab seam can re-check a revoked api key on connect and reject a claimless +// token after the rollout grace window. NOT keyed on `actor:'agent'` — the +// internal agent is 'agent' but session-backed, so it must stay on the no-check +// path. +export type CollabPrincipal = 'session' | 'api_key'; + export type JwtCollabPayload = { sub: string; workspaceId: string; @@ -44,6 +55,13 @@ export type JwtCollabPayload = { // Nullable: an external MCP agent has no internal ai_chats row, so it carries // an 'agent' actor with a null aiChatId. aiChatId?: string | null; + // Auth-principal discriminator (#501). Present on every post-rollout token; + // its absence on a still-valid token past the grace window is treated as an + // error, not trust (fail-closed). + principal?: CollabPrincipal; + // Only when principal === 'api_key': the minting key's id, so the collab seam + // can row-check (and reject) a revoked key on connect. + apiKeyId?: string; }; export type JwtExchangePayload = { diff --git a/apps/server/src/core/auth/services/auth.service.ts b/apps/server/src/core/auth/services/auth.service.ts index 986084b9..e8d35186 100644 --- a/apps/server/src/core/auth/services/auth.service.ts +++ b/apps/server/src/core/auth/services/auth.service.ts @@ -375,10 +375,20 @@ export class AuthService { } } - async getCollabToken(user: User, workspaceId: string) { + async getCollabToken( + user: User, + workspaceId: string, + // Origin of the request minting this collab token (#501). When the caller is + // an api-key principal, its apiKeyId is threaded into the token so the collab + // seam can re-check the key on connect (closing api-key -> long-lived-collab + // laundering). Absent for a normal session/human request. + apiKey?: { apiKeyId: string }, + ) { const token = await this.tokenService.generateCollabToken( user, workspaceId, + undefined, + apiKey, ); return { token }; } diff --git a/apps/server/src/core/auth/services/token.service.behavior.spec.ts b/apps/server/src/core/auth/services/token.service.behavior.spec.ts index 32293c27..de2f44f0 100644 --- a/apps/server/src/core/auth/services/token.service.behavior.spec.ts +++ b/apps/server/src/core/auth/services/token.service.behavior.spec.ts @@ -1,4 +1,5 @@ import { ForbiddenException, UnauthorizedException } from '@nestjs/common'; +import * as jwt from 'jsonwebtoken'; import { TokenService } from './token.service'; import { JwtType } from '../dto/jwt-payload'; @@ -213,4 +214,175 @@ describe('TokenService.generateCollabToken', () => { aiChatId: 'chat-456', }); }); + + // #501 fail-closed discriminator: EVERY collab token carries a principal. + it("defaults principal to 'session' with NO apiKeyId (normal/internal-agent path)", async () => { + const { service, jwtService } = makeTokenService(); + await service.generateCollabToken(makeUser() as never, 'ws-1'); + const [payload] = jwtService.sign.mock.calls[0]; + expect(payload.principal).toBe('session'); + expect(payload).not.toHaveProperty('apiKeyId'); + }); + + it("the internal agent (provenance, NO apiKey) still gets principal='session'", async () => { + const { service, jwtService } = makeTokenService(); + await service.generateCollabToken( + makeUser() as never, + 'ws-1', + { actor: 'agent', aiChatId: 'chat-1' }, + ); + const [payload] = jwtService.sign.mock.calls[0]; + // Keyed on api-key ORIGIN, not actor: an is_agent session token is 'session'. + expect(payload.principal).toBe('session'); + expect(payload).not.toHaveProperty('apiKeyId'); + }); + + it("stamps principal='api_key' + apiKeyId when minted by an api-key principal", async () => { + const { service, jwtService } = makeTokenService(); + await service.generateCollabToken( + makeUser() as never, + 'ws-1', + undefined, + { apiKeyId: 'key-9' }, + ); + const [payload] = jwtService.sign.mock.calls[0]; + expect(payload.principal).toBe('api_key'); + expect(payload.apiKeyId).toBe('key-9'); + }); }); + +/** + * API-key token minting MUST carry NO `exp` claim — the ONLY source of truth for + * a key's lifetime/revocation is its `api_keys` row, checked on every request. + * + * This is a LIVE-bug regression: the shared JwtService is registered with a + * global `signOptions.expiresIn` (default '90d') that merges into every sign(), + * so an api-key minted through it silently gets exp=now+90d and an "unlimited" + * key dies in 90 days. TokenService.generateApiToken mints through a dedicated + * no-expiry signer instead. These tests construct the REAL signer (a real secret + * via the stubbed EnvironmentService) and decode the produced JWT to assert the + * observable property: no `exp`. + */ +describe('TokenService.generateApiToken (no exp claim ever)', () => { + const APP_SECRET_LOCAL = 'apikey-secret'; + + function makeRealSignerService() { + // Give the SHARED jwtService a global expiresIn so a regression (minting + // through it) would show up as an exp claim — the exact live bug. + const { JwtService } = require('@nestjs/jwt'); + const sharedJwt = new JwtService({ + secret: APP_SECRET_LOCAL, + signOptions: { expiresIn: '90d', issuer: 'Docmost' }, + }); + const environmentService = { + getAppSecret: () => APP_SECRET_LOCAL, + }; + const service = new (TokenService as unknown as new ( + ...args: unknown[] + ) => TokenService)(sharedJwt, environmentService); + return { service }; + } + + const user = makeUser({ id: 'svc-1', workspaceId: 'ws-1' }); + + it('mints an api-key JWT with NO exp claim and issuer Docmost', async () => { + const { service } = makeRealSignerService(); + + const token = await service.generateApiToken({ + apiKeyId: 'key-1', + user: user as never, + workspaceId: 'ws-1', + }); + + const decoded = jwt.decode(token) as Record; + // The observable security property: no expiry lives in the JWT. + expect(decoded.exp).toBeUndefined(); + expect(decoded).toMatchObject({ + sub: 'svc-1', + apiKeyId: 'key-1', + workspaceId: 'ws-1', + type: JwtType.API_KEY, + iss: 'Docmost', + }); + }); + + it('demonstrates the live bug it guards: the SHARED signer WOULD add exp', () => { + const { JwtService } = require('@nestjs/jwt'); + const sharedJwt = new JwtService({ + secret: APP_SECRET_LOCAL, + signOptions: { expiresIn: '90d', issuer: 'Docmost' }, + }); + // Even with empty per-call options the global expiresIn merges in. + const leaky = sharedJwt.sign({ sub: 'x', type: JwtType.API_KEY }, {}); + expect((jwt.decode(leaky) as Record).exp).toBeDefined(); + }); + + it('refuses to mint for a disabled user', async () => { + const { service } = makeRealSignerService(); + await expect( + service.generateApiToken({ + apiKeyId: 'key-1', + user: makeUser({ deactivatedAt: new Date() }) as never, + workspaceId: 'ws-1', + }), + ).rejects.toBeInstanceOf(ForbiddenException); + }); +}); + +/** + * verifyJwtOneOf is the type-routing primitive: verify the signature once and + * assert the token type is on an explicit allowlist. It must NOT degrade into a + * "return whatever type" helper — a token whose type is off the allowlist is + * rejected with the same generic error as a single-type mismatch. + */ +describe('TokenService.verifyJwtOneOf (allowlist type-routing)', () => { + it('returns the payload when the type is on the allowlist', async () => { + const verifyAsync = jest + .fn() + .mockResolvedValue({ type: JwtType.API_KEY, sub: 'u-1' }); + const { service } = makeTokenService({ verifyAsync }); + + const payload = await service.verifyJwtOneOf(token123(), [ + JwtType.ACCESS, + JwtType.API_KEY, + ]); + + expect(payload).toMatchObject({ type: JwtType.API_KEY, sub: 'u-1' }); + expect(verifyAsync).toHaveBeenCalledTimes(1); + }); + + it('accepts the OTHER allowed type too', async () => { + const verifyAsync = jest + .fn() + .mockResolvedValue({ type: JwtType.ACCESS, sub: 'u-1' }); + const { service } = makeTokenService({ verifyAsync }); + + await expect( + service.verifyJwtOneOf(token123(), [JwtType.ACCESS, JwtType.API_KEY]), + ).resolves.toMatchObject({ type: JwtType.ACCESS }); + }); + + it('rejects a token whose type is OFF the allowlist (confused-deputy guard)', async () => { + const verifyAsync = jest + .fn() + .mockResolvedValue({ type: JwtType.COLLAB, sub: 'u-1' }); + const { service } = makeTokenService({ verifyAsync }); + + await expect( + service.verifyJwtOneOf(token123(), [JwtType.ACCESS, JwtType.API_KEY]), + ).rejects.toBeInstanceOf(UnauthorizedException); + }); + + it('verifies the signature exactly ONCE', async () => { + const verifyAsync = jest + .fn() + .mockResolvedValue({ type: JwtType.ACCESS }); + const { service } = makeTokenService({ verifyAsync }); + await service.verifyJwtOneOf(token123(), [JwtType.ACCESS]); + expect(verifyAsync).toHaveBeenCalledTimes(1); + }); +}); + +function token123(): string { + return 'a.b.c'; +} diff --git a/apps/server/src/core/auth/services/token.service.ts b/apps/server/src/core/auth/services/token.service.ts index 33a20069..de202346 100644 --- a/apps/server/src/core/auth/services/token.service.ts +++ b/apps/server/src/core/auth/services/token.service.ts @@ -4,7 +4,6 @@ import { UnauthorizedException, } from '@nestjs/common'; import { JwtService } from '@nestjs/jwt'; -import type { StringValue } from 'ms'; import { EnvironmentService } from '../../../integrations/environment/environment.service'; import { JwtApiKeyPayload, @@ -62,6 +61,12 @@ export class TokenService { // token carries no actor/aiChatId and is treated as 'user' downstream. // aiChatId is nullable for an external agent with no internal ai_chats row. provenance?: { actor: 'agent'; aiChatId: string | null }, + // Optional api-key origin (#501). When the collab token is minted by an + // api-key principal (an external MCP agent), the caller passes the key id so + // the token carries principal='api_key' + apiKeyId and the collab seam can + // re-check the key on connect. Absent -> principal='session' (a normal + // user/session, including the internal session-backed AI agent). + apiKey?: { apiKeyId: string }, ): Promise { if (isUserDisabled(user)) { throw new ForbiddenException(); @@ -71,6 +76,10 @@ export class TokenService { sub: user.id, workspaceId, type: JwtType.COLLAB, + // Fail-closed discriminator on EVERY minted token: 'api_key' when minted by + // an api-key principal, else 'session'. + principal: apiKey ? 'api_key' : 'session', + ...(apiKey ? { apiKeyId: apiKey.apiKeyId } : {}), ...(provenance ? { actor: provenance.actor, aiChatId: provenance.aiChatId } : {}), @@ -123,9 +132,8 @@ export class TokenService { apiKeyId: string; user: User; workspaceId: string; - expiresIn?: StringValue | number; }): Promise { - const { apiKeyId, user, workspaceId, expiresIn } = opts; + const { apiKeyId, user, workspaceId } = opts; if (isUserDisabled(user)) { throw new ForbiddenException(); } @@ -137,7 +145,32 @@ export class TokenService { type: JwtType.API_KEY, }; - return this.jwtService.sign(payload, expiresIn ? { expiresIn } : {}); + // API-key tokens carry NO `exp` claim EVER — the ONLY source of truth for a + // key's lifetime and revocation is its `api_keys` row (checked on every + // request), not the JWT. This CANNOT use `this.jwtService`: TokenModule + // registers it with a global `signOptions.expiresIn` (JWT_TOKEN_EXPIRES_IN, + // default '90d'), which merges into EVERY sign() call — even `sign(payload, + // {})` — and `{ expiresIn: undefined }` THROWS rather than stripping it + // (verified empirically). So an "unlimited" key minted through the shared + // signer would silently get exp=now+90d and die in 90 days regardless of its + // row. We mint through a dedicated no-expiry signer, re-stamping only + // `issuer: 'Docmost'` for claim parity with the shared signer. + return this.apiKeyJwtService().sign(payload); + } + + // Lazily-built JWT signer for API-key tokens: same APP_SECRET, same 'Docmost' + // issuer, but WITHOUT the global `expiresIn` — so minted API-key tokens have no + // `exp` claim. Built once and cached. Verification still goes through the + // shared verifier (same secret); `verifyAsync` does not require an `exp`. + private _apiKeyJwtService?: JwtService; + private apiKeyJwtService(): JwtService { + if (!this._apiKeyJwtService) { + this._apiKeyJwtService = new JwtService({ + secret: this.environmentService.getAppSecret(), + signOptions: { issuer: 'Docmost' }, + }); + } + return this._apiKeyJwtService; } async generatePdfRenderToken( @@ -177,4 +210,31 @@ export class TokenService { return payload; } + + /** + * Verify a token's signature ONCE and assert its `type` is one of `allowed`. + * + * This is the type-routing primitive for surfaces that legitimately accept + * more than one token type on the same Bearer slot (the /mcp Bearer path + * accepts both an ACCESS and an API_KEY token). It is deliberately NOT a + * "verify-and-return-whatever-type" helper — that would be a reusable + * confused-deputy footgun (any caller could then feed an attachment/collab + * token where an access token is expected). An explicit allowlist preserves + * the type-pinning property of `verifyJwt`: a token whose `type` is not in the + * allowlist is rejected with the SAME generic error as a type mismatch, and + * the signature is verified exactly once (no double-verify). + */ + async verifyJwtOneOf(token: string, allowed: JwtType[]) { + const payload = await this.jwtService.verifyAsync(token, { + secret: this.environmentService.getAppSecret(), + }); + + if (!allowed.includes(payload.type)) { + throw new UnauthorizedException( + 'Invalid JWT token. Token type does not match.', + ); + } + + return payload; + } } 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 20a669ca..cbcb3fc9 100644 --- a/apps/server/src/core/auth/strategies/jwt.strategy.spec.ts +++ b/apps/server/src/core/auth/strategies/jwt.strategy.spec.ts @@ -22,7 +22,8 @@ describe('JwtStrategy — provenance derivation', () => { const userSessionRepo: any = { findActiveById: jest.fn() }; const sessionActivityService: any = { trackActivity: jest.fn() }; const environmentService: any = { getAppSecret: () => 'test-secret' }; - const moduleRef: any = {}; + // ACCESS-path tests never touch the api-key seam; a bare stub suffices. + const apiKeyService: any = { validate: jest.fn() }; const strategy = new JwtStrategy( userRepo, @@ -30,7 +31,7 @@ describe('JwtStrategy — provenance derivation', () => { userSessionRepo, sessionActivityService, environmentService, - moduleRef, + apiKeyService, ); return { strategy, userRepo }; } @@ -122,25 +123,29 @@ describe('JwtStrategy — provenance derivation', () => { }); /** - * Provenance derivation on the API-KEY path (jwt.strategy.validateApiKey, #486). + * Provenance derivation on the API-KEY path (jwt.strategy.validateApiKey, #486 + * + #501). * * The access-token path stamped provenance; the API-key path returned early * WITHOUT it, so an is_agent API key's REST writes recorded no 'agent' marker. * The API-key payload carries no signed claim, so provenance is resolved from the - * SERVER-SIDE user returned by ApiKeyService.validateApiKey: isAgent -> 'agent', + * SERVER-SIDE user returned by ApiKeyService.validate: isAgent -> 'agent', * otherwise 'user'; aiChatId is always null (an API key has no ai_chats row). * - * The enterprise ApiKeyService is not bundled in the OSS build, so the strategy - * loads it through an overridable `resolveApiKeyService` seam that we stub here. + * #501 wires the CORE ApiKeyService (the EE `ee/api-key` module is absent in the + * fork) directly into the strategy — no dynamic require. The strategy also stamps + * `req.raw.authType='api_key'` + `req.raw.apiKeyId` for the "a token cannot manage + * tokens" guard on the /api-keys surface. */ -describe('JwtStrategy — API-key provenance derivation (#486)', () => { - function makeApiKeyStrategy(validateApiKeyImpl: (p: any) => Promise) { +describe('JwtStrategy — API-key provenance derivation (#486/#501)', () => { + function makeApiKeyStrategy(validateImpl: (p: any) => Promise) { const userRepo: any = { findById: jest.fn() }; const workspaceRepo: any = { findById: jest.fn() }; const userSessionRepo: any = { findActiveById: jest.fn() }; const sessionActivityService: any = { trackActivity: jest.fn() }; const environmentService: any = { getAppSecret: () => 'test-secret' }; - const moduleRef: any = {}; + const validate = jest.fn(validateImpl); + const apiKeyService: any = { validate }; const strategy = new JwtStrategy( userRepo, @@ -148,14 +153,9 @@ describe('JwtStrategy — API-key provenance derivation (#486)', () => { userSessionRepo, sessionActivityService, environmentService, - moduleRef, + apiKeyService, ); - // Stub the EE ApiKeyService seam (the real module is not in the OSS build). - const validateApiKey = jest.fn(validateApiKeyImpl); - jest - .spyOn(strategy as any, 'resolveApiKeyService') - .mockReturnValue({ validateApiKey }); - return { strategy, validateApiKey }; + return { strategy, validate }; } const makeReq = () => ({ raw: {} as Record }); @@ -166,22 +166,23 @@ describe('JwtStrategy — API-key provenance derivation (#486)', () => { type: JwtType.API_KEY, }); - it("stamps actor='agent' for an is_agent API key (from the validated user)", async () => { + it("stamps actor='agent' + authType/apiKeyId for an is_agent API key", async () => { const validated = { user: { id: 'svc-1', isAgent: true }, workspace: { id: 'ws-1' }, }; - const { strategy, validateApiKey } = makeApiKeyStrategy( - async () => validated, - ); + const { strategy, validate } = makeApiKeyStrategy(async () => validated); const req = makeReq(); const result = await strategy.validate(req, apiKeyPayload() as any); - expect(validateApiKey).toHaveBeenCalledTimes(1); + expect(validate).toHaveBeenCalledTimes(1); expect(req.raw.actor).toBe('agent'); // API keys carry no internal ai_chats row -> null. expect(req.raw.aiChatId).toBeNull(); + // Principal-kind markers for the management-surface guard. + expect(req.raw.authType).toBe('api_key'); + expect(req.raw.apiKeyId).toBe('key-1'); // The validated auth object is returned unchanged (req.user shape preserved). expect(result).toBe(validated); }); @@ -197,25 +198,19 @@ describe('JwtStrategy — API-key provenance derivation (#486)', () => { expect(req.raw.actor).toBe('user'); expect(req.raw.aiChatId).toBeNull(); + expect(req.raw.authType).toBe('api_key'); }); - it('throws Unauthorized (and stamps nothing) when the EE module is missing', async () => { - const userRepo: any = { findById: jest.fn() }; - const strategy = new JwtStrategy( - userRepo, - { findById: jest.fn() } as any, - { findActiveById: jest.fn() } as any, - { trackActivity: jest.fn() } as any, - { getAppSecret: () => 'test-secret' } as any, - {} as any, - ); - // EE not bundled: the seam returns null. - jest.spyOn(strategy as any, 'resolveApiKeyService').mockReturnValue(null); + it('propagates a validate() rejection and stamps nothing', async () => { + const { strategy } = makeApiKeyStrategy(async () => { + throw new UnauthorizedException(); + }); const req = makeReq(); await expect( strategy.validate(req, apiKeyPayload() as any), ).rejects.toThrow(UnauthorizedException); expect(req.raw.actor).toBeUndefined(); + expect(req.raw.authType).toBeUndefined(); }); }); diff --git a/apps/server/src/core/auth/strategies/jwt.strategy.ts b/apps/server/src/core/auth/strategies/jwt.strategy.ts index 2abd65c2..ebb60283 100644 --- a/apps/server/src/core/auth/strategies/jwt.strategy.ts +++ b/apps/server/src/core/auth/strategies/jwt.strategy.ts @@ -1,4 +1,4 @@ -import { Injectable, Logger, UnauthorizedException } from '@nestjs/common'; +import { Injectable, UnauthorizedException } from '@nestjs/common'; import { PassportStrategy } from '@nestjs/passport'; import { Strategy } from 'passport-jwt'; import { EnvironmentService } from '../../../integrations/environment/environment.service'; @@ -9,20 +9,18 @@ import { UserSessionRepo } from '@docmost/db/repos/session/user-session.repo'; import { SessionActivityService } from '../../session/session-activity.service'; import { FastifyRequest } from 'fastify'; import { extractBearerTokenFromHeader, isUserDisabled } from '../../../common/helpers'; -import { ModuleRef } from '@nestjs/core'; import { resolveProvenance } from '../../../common/decorators/auth-provenance.decorator'; +import { ApiKeyService } from '../../api-key/api-key.service'; @Injectable() export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') { - private logger = new Logger('JwtStrategy'); - constructor( private userRepo: UserRepo, private workspaceRepo: WorkspaceRepo, private userSessionRepo: UserSessionRepo, private sessionActivityService: SessionActivityService, private readonly environmentService: EnvironmentService, - private moduleRef: ModuleRef, + private readonly apiKeyService: ApiKeyService, ) { super({ jwtFromRequest: (req: FastifyRequest) => { @@ -102,12 +100,17 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') { } private async validateApiKey(req: any, payload: JwtApiKeyPayload) { - const apiKeyService = this.resolveApiKeyService(); - if (!apiKeyService) { - throw new UnauthorizedException('Enterprise API Key module missing'); - } + // The fork ships the core `ApiKeyService` (the EE `ee/api-key` module is + // absent). `validate` throws a bare UnauthorizedException on any definite + // deny (missing/revoked/expired row, disabled user, kill-switch off) and + // propagates infra errors (→ 5xx) rather than masking them as a 401. + const result = await this.apiKeyService.validate(payload); - const result = await apiKeyService.validateApiKey(payload); + // Stamp the principal kind + key id so the /api-keys management surface can + // enforce "a token cannot manage tokens". Done in this branch because it + // returns before the shared ACCESS-path stamping below. + req.raw.authType = 'api_key'; + req.raw.apiKeyId = payload.apiKeyId; // Stamp the agent-edit provenance for the API-KEY path too (#486). Unlike the // access-token path above, it CANNOT be resolved before this point: the @@ -119,32 +122,10 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') { // SERVER-SIDE user (never a client field), so an 'agent' badge is unspoofable // — mirroring the access-token path. Passing `null` for the claim means the // actor is decided solely by user.isAgent. - const provenance = resolveProvenance((result as any)?.user, null); + const provenance = resolveProvenance(result.user, null); req.raw.actor = provenance.actor; req.raw.aiChatId = provenance.aiChatId; return result; } - - /** - * Resolve the enterprise ApiKeyService, or `null` when the EE module is not - * bundled in this build (community build). Extracted as an overridable seam so - * the API-key provenance stamping can be unit-tested without the EE package - * present (docmost is OSS + a separate EE bundle; `require` of the EE path - * throws here). Any load/resolve error is treated as "module missing". - */ - protected resolveApiKeyService(): { - validateApiKey: (payload: JwtApiKeyPayload) => Promise; - } | null { - try { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const ApiKeyModule = require('./../../../ee/api-key/api-key.service'); - return this.moduleRef.get(ApiKeyModule.ApiKeyService, { strict: false }); - } catch (err) { - this.logger.debug( - 'API Key module requested but enterprise module not bundled in this build', - ); - return null; - } - } } diff --git a/apps/server/src/core/core.module.ts b/apps/server/src/core/core.module.ts index e898a4a1..50d7f090 100644 --- a/apps/server/src/core/core.module.ts +++ b/apps/server/src/core/core.module.ts @@ -6,6 +6,7 @@ import { } from '@nestjs/common'; import { UserModule } from './user/user.module'; import { AuthModule } from './auth/auth.module'; +import { ApiKeyModule } from './api-key/api-key.module'; import { WorkspaceModule } from './workspace/workspace.module'; import { PageModule } from './page/page.module'; import { AttachmentModule } from './attachment/attachment.module'; @@ -29,6 +30,7 @@ import { ClsMiddleware } from 'nestjs-cls'; imports: [ UserModule, AuthModule, + ApiKeyModule, WorkspaceModule, PageModule, AttachmentModule, diff --git a/apps/server/src/database/database.module.ts b/apps/server/src/database/database.module.ts index 3e9218cc..8d16e5f9 100644 --- a/apps/server/src/database/database.module.ts +++ b/apps/server/src/database/database.module.ts @@ -37,6 +37,7 @@ import { AiProviderCredentialsRepo } from '@docmost/db/repos/ai-chat/ai-provider import { AiMcpServerRepo } from '@docmost/db/repos/ai-chat/ai-mcp-server.repo'; import { AiAgentRoleRepo } from '@docmost/db/repos/ai-agent-roles/ai-agent-roles.repo'; import { PageEmbeddingRepo } from '@docmost/db/repos/ai-chat/page-embedding.repo'; +import { ApiKeyRepo } from '@docmost/db/repos/api-key/api-key.repo'; import { PageListener } from '@docmost/db/listeners/page.listener'; import { PostgresJSDialect } from 'kysely-postgres-js'; import * as postgres from 'postgres'; @@ -129,6 +130,7 @@ import { firstSqlToken } from '../integrations/metrics/metrics.constants'; AiMcpServerRepo, AiAgentRoleRepo, PageEmbeddingRepo, + ApiKeyRepo, PageListener, ], exports: [ @@ -164,6 +166,7 @@ import { firstSqlToken } from '../integrations/metrics/metrics.constants'; AiMcpServerRepo, AiAgentRoleRepo, PageEmbeddingRepo, + ApiKeyRepo, ], }) export class DatabaseModule implements OnApplicationBootstrap { diff --git a/apps/server/src/database/repos/api-key/api-key.repo.ts b/apps/server/src/database/repos/api-key/api-key.repo.ts new file mode 100644 index 00000000..835f0ac0 --- /dev/null +++ b/apps/server/src/database/repos/api-key/api-key.repo.ts @@ -0,0 +1,110 @@ +import { Injectable } from '@nestjs/common'; +import { InjectKysely } from 'nestjs-kysely'; +import { KyselyDB, KyselyTransaction } from '@docmost/db/types/kysely.types'; +import { DB, Users } from '@docmost/db/types/db'; +import { dbOrTx } from '@docmost/db/utils'; +import { ApiKey, InsertableApiKey } from '@docmost/db/types/entity.types'; +import { jsonObjectFrom } from 'kysely/helpers/postgres'; +import { ExpressionBuilder } from 'kysely'; + +// Public-facing shape: the api_keys row plus a compact creator attribution used +// by the admin list (the workspace-wide view attributes each key to its author). +export type ApiKeyWithCreator = ApiKey & { + creator: Pick | null; +}; + +@Injectable() +export class ApiKeyRepo { + constructor(@InjectKysely() private readonly db: KyselyDB) {} + + // Compact creator attribution embedded in the admin list rows. A nested + // subquery (jsonObjectFrom) keeps it one round-trip; the token material is + // never stored, so nothing sensitive is joined in. + private withCreator(eb: ExpressionBuilder) { + return jsonObjectFrom( + eb + .selectFrom('users') + .select(['users.id', 'users.name', 'users.email', 'users.avatarUrl']) + .whereRef('users.id', '=', 'apiKeys.creatorId'), + ).as('creator'); + } + + async findById( + id: string, + workspaceId: string, + trx?: KyselyTransaction, + ): Promise { + const db = dbOrTx(this.db, trx); + return db + .selectFrom('apiKeys') + .selectAll() + .where('id', '=', id) + .where('workspaceId', '=', workspaceId) + // Convention (soft-delete): a revoked key is invisible to reads. + .where('deletedAt', 'is', null) + .executeTakeFirst(); + } + + async findByCreator( + creatorId: string, + workspaceId: string, + ): Promise { + return this.db + .selectFrom('apiKeys') + .selectAll('apiKeys') + .select((eb) => this.withCreator(eb)) + .where('creatorId', '=', creatorId) + .where('workspaceId', '=', workspaceId) + .where('deletedAt', 'is', null) + .orderBy('createdAt', 'desc') + .execute() as unknown as Promise; + } + + async findAllInWorkspace( + workspaceId: string, + ): Promise { + return this.db + .selectFrom('apiKeys') + .selectAll('apiKeys') + .select((eb) => this.withCreator(eb)) + .where('workspaceId', '=', workspaceId) + .where('deletedAt', 'is', null) + .orderBy('createdAt', 'desc') + .execute() as unknown as Promise; + } + + async insert( + insertable: InsertableApiKey, + trx?: KyselyTransaction, + ): Promise { + const db = dbOrTx(this.db, trx); + return db + .insertInto('apiKeys') + .values(insertable) + .returningAll() + .executeTakeFirst(); + } + + // Soft-delete = revoke. Terminal: the row stays for forensics but is invisible + // to every read above, so validate() denies it immediately (no grace window). + async softDelete(id: string, workspaceId: string): Promise { + await this.db + .updateTable('apiKeys') + .set({ deletedAt: new Date(), updatedAt: new Date() }) + .where('id', '=', id) + .where('workspaceId', '=', workspaceId) + .where('deletedAt', 'is', null) + .execute(); + } + + // Throttled best-effort forensics stamp. Not on the critical path: callers + // fire-and-forget and swallow errors, so it never fails a request. + async touchLastUsed(id: string): Promise { + await this.db + .updateTable('apiKeys') + .set({ lastUsedAt: new Date() }) + .where('id', '=', id) + .where('deletedAt', 'is', null) + .execute(); + } +} diff --git a/apps/server/src/integrations/environment/environment.service.ts b/apps/server/src/integrations/environment/environment.service.ts index cea4d00d..e1fc423c 100644 --- a/apps/server/src/integrations/environment/environment.service.ts +++ b/apps/server/src/integrations/environment/environment.service.ts @@ -70,6 +70,23 @@ export class EnvironmentService { return this.configService.get('JWT_TOKEN_EXPIRES_IN', '90d'); } + // Kill-switch for the agent API-key feature. Default ON (unset -> enabled): a + // deploy that never sets the variable must NOT silently kill every agent. The + // parse is STRICT — the value is validated by environment.validation to be + // exactly 'true' or 'false' (or absent), so a typo like `=0`/`=off`/`=False` + // fails at boot rather than being read as "enabled" (which would leave an + // operator who yanked the switch during an incident with it still on). When + // OFF: validate() denies every api-key token and the issuance endpoints 404. + isApiKeysEnabled(): boolean { + return this.configService.get('API_KEYS_ENABLED', 'true') !== 'false'; + } + + // Raw value (or undefined) for the boot log, so the state after each deploy is + // verifiable in container logs: `API keys: ENABLED/DISABLED (API_KEYS_ENABLED=...)`. + getApiKeysEnabledRaw(): string | undefined { + return this.configService.get('API_KEYS_ENABLED'); + } + getCookieExpiresIn(): Date { const expiresInStr = this.getJwtTokenExpiresIn(); let msUntilExpiry: number; diff --git a/apps/server/src/integrations/environment/environment.validation.ts b/apps/server/src/integrations/environment/environment.validation.ts index 476bc81c..3b2f65eb 100644 --- a/apps/server/src/integrations/environment/environment.validation.ts +++ b/apps/server/src/integrations/environment/environment.validation.ts @@ -103,6 +103,15 @@ export class EnvironmentVariables { @IsString() TYPESENSE_LOCALE: string; + // Agent API-key kill-switch. Optional (absent -> default ON). STRICT: only the + // literals 'true'/'false' are accepted, so `=0`/`=off`/`=False` fail at boot + // instead of being silently read as "enabled" — the switch must actually flip + // when an operator flips it. See EnvironmentService.isApiKeysEnabled. + @IsOptional() + @IsIn(['true', 'false']) + @IsString() + API_KEYS_ENABLED: string; + @IsOptional() @ValidateIf((obj) => obj.AI_DRIVER) @IsIn(['openai', 'openai-compatible', 'gemini', 'ollama']) diff --git a/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts b/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts new file mode 100644 index 00000000..ebe67431 --- /dev/null +++ b/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts @@ -0,0 +1,131 @@ +// p-limit and @sindresorhus/slugify are ESM-only and not in jest's transform +// allowlist; both are irrelevant to createDrawioSvg (a pure fs + string method), +// so they are mocked out to keep the module graph loadable under ts-jest. +jest.mock('p-limit', () => ({ + __esModule: true, + default: () => (fn: () => unknown) => fn(), +})); +jest.mock('@sindresorhus/slugify', () => ({ + __esModule: true, + default: (input: string) => String(input), +})); + +import { promises as fs } from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { ImportAttachmentService } from './import-attachment.service'; + +/** + * Unit test for ImportAttachmentService.createDrawioSvg (issue #507). + * + * The Confluence import wraps a `.drawio` file into a `.drawio.svg` attachment. + * The `content=` payload MUST be the mxfile XML entity-escaped (draw.io's native + * form), NOT base64 — draw.io's editor decodes a base64 content= via Latin-1 + * atob, mangling every non-ASCII char into mojibake. createDrawioSvg touches no + * injected dependency, so the service is built with placeholder deps. + */ +describe('ImportAttachmentService.createDrawioSvg (#507)', () => { + const service = new ImportAttachmentService( + {} as any, + {} as any, + {} as any, + ); + const call = (p: string): Promise => + (service as any).createDrawioSvg(p); + + let tmpDir: string; + beforeAll(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'drawio-507-')); + }); + afterAll(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); + }); + + const writeDrawio = async (name: string, xml: string): Promise => { + const p = path.join(tmpDir, name); + await fs.writeFile(p, xml, 'utf-8'); + return p; + }; + + it('writes content= as entity-encoded XML, not base64', async () => { + const drawio = + '' + + '' + + '' + + ''; + const p = await writeDrawio('cyrillic.drawio', drawio); + + const svg = (await call(p)).toString('utf-8'); + const content = /content="([^"]*)"/.exec(svg)?.[1]; + expect(content).toBeDefined(); + + // Entity-encoded XML form, starting with <mxfile — never a base64 blob. + expect(content).toMatch(/^<mxfile/); + expect(content).toContain('<'); + // Non-ASCII survives as raw UTF-8, with no Latin-1 mojibake. + expect(content).toContain('Старт-бит'); + expect(content).toContain('Схема — ёж'); + expect(content).not.toContain('Ð'); + + // Decoding the attribute (un-escaping) yields the original drawio file. + const decoded = content! + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/&/g, '&'); + expect(decoded).toBe(drawio); + }); + + it('encodes literal tab/newline/CR as numeric char-refs, not literal control chars (#507 F1)', async () => { + // A literal tab/newline/CR inside the mxfile XML would be collapsed to a + // single space by XML attribute-value normalization when the draw.io editor + // reads content=, silently flattening multi-line labels and tab-bearing + // values. They must be emitted as numeric char-refs instead. + const drawio = + '' + + '' + + '' + + '' + + ''; + const p = await writeDrawio('ctrl.drawio', drawio); + + const svg = (await call(p)).toString('utf-8'); + const content = /content="([^"]*)"/.exec(svg)?.[1]; + expect(content).toBeDefined(); + // No literal control chars survive in the attribute value. + expect(content).not.toMatch(/[\t\n\r]/); + // They round-trip as numeric char-refs. + expect(content).toContain(' '); + expect(content).toContain(' '); + expect(content).toContain(' '); + // Decoding (char-refs back to literal, entities back) recovers the file. + const decoded = content! + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/ /gi, '\t') + .replace(/ /gi, '\n') + .replace(/ /gi, '\r') + .replace(/&/g, '&'); + expect(decoded).toBe(drawio); + }); + + it('escapes XML metacharacters in the drawio payload', async () => { + const drawio = '"q" <x>'; + const p = await writeDrawio('meta.drawio', drawio); + + const svg = (await call(p)).toString('utf-8'); + const content = /content="([^"]*)"/.exec(svg)?.[1]; + expect(content).toBeDefined(); + // The attribute value must contain no bare `<`, `>` or `"` that would break + // out of the content="..." attribute or the SVG element. + expect(content).not.toMatch(/[<>"]/); + const decoded = content! + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/&/g, '&'); + expect(decoded).toBe(drawio); + }); +}); diff --git a/apps/server/src/integrations/import/services/import-attachment.service.ts b/apps/server/src/integrations/import/services/import-attachment.service.ts index 9100149b..3fa94838 100644 --- a/apps/server/src/integrations/import/services/import-attachment.service.ts +++ b/apps/server/src/integrations/import/services/import-attachment.service.ts @@ -8,6 +8,7 @@ import { createReadStream } from 'node:fs'; import { promises as fs } from 'fs'; import { Readable } from 'stream'; import { getMimeType, sanitizeFileName } from '../../../common/helpers'; +import { htmlEscape } from '../../../common/helpers/html-escaper'; import { v7 } from 'uuid'; import { FileTask } from '@docmost/db/types/entity.types'; import { getAttachmentFolderPath } from '../../../core/attachment/attachment.utils'; @@ -849,7 +850,12 @@ export class ImportAttachmentService { ): Promise { try { const drawioContent = await fs.readFile(drawioPath, 'utf-8'); - const drawioBase64 = Buffer.from(drawioContent).toString('base64'); + // Write the mxfile XML XML-entity-escaped (draw.io's native content= form), + // NOT base64. draw.io's editor decodes a base64 content= via Latin-1 atob + // (no UTF-8 step), turning every non-ASCII char (Cyrillic, ё, —) into + // mojibake; the entity-encoded form is decoded by the DOM as UTF-8 and + // opens intact. Docmost's own decoder reads both forms. + const drawioEscaped = this.xmlEscapeContent(drawioContent); let imageElement = ''; // If we have a PNG, include it in the SVG @@ -875,7 +881,7 @@ export class ImportAttachmentService { width="600" height="400" viewBox="0 0 600 400" - content="${drawioBase64}">${imageElement}`; + content="${drawioEscaped}">${imageElement}`; return Buffer.from(svgContent, 'utf-8'); } catch (error) { @@ -884,6 +890,24 @@ export class ImportAttachmentService { } } + /** + * Escape a string so it is safe as the value of a double-quoted XML attribute + * (the `content=` payload of a `.drawio.svg`). The shared `htmlEscape` covers + * `& < > " '` (a strict superset of what this attribute needs; the extra `'` + * escape is harmless in a `"`-delimited value). On top of that, the numeric + * char-refs for tab/newline/CR are required: a literal tab/newline/CR inside + * an attribute value is collapsed to a single space by XML attribute-value + * normalization on DOM read (both our decoder and the real draw.io editor), + * silently flattening multi-line labels and tab-bearing values. Char-refs + * survive that normalization (#507). + */ + private xmlEscapeContent(s: string): string { + return htmlEscape(s) + .replace(/\t/g, ' ') + .replace(/\n/g, ' ') + .replace(/\r/g, ' '); + } + private async uploadWithRetry(opts: { abs: string; storageFilePath: string; diff --git a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts index 31a1d8f1..e8b20880 100644 --- a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts +++ b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts @@ -304,37 +304,102 @@ export function clientIp(req: ClientIpRequest): string { return 'unknown'; } -// Minimal structural shape of the TokenService.verifyJwt method we depend on, -// so this module never imports the concrete TokenService (heavy graph). -export interface AccessJwtVerifier { - verifyJwt: ( - token: string, - type: JwtType, - ) => Promise<{ - sub?: string; - email?: string; - workspaceId?: string; - sessionId?: string; - }>; -} - -/** - * Bind a TokenService-like verifier into a one-arg `verifyJwt(token)` that - * ALWAYS enforces `JwtType.ACCESS`. This is the single place where the /mcp - * Bearer path pins the token type: a Bearer access token must be verified AS an - * access token (not refresh/exchange/collab/etc.), so the type literal is fixed - * here rather than at the call site. McpService.verifyMcpBearer delegates to - * this, keeping the `JwtType.ACCESS` choice testable without the heavy graph. - */ -export function bindAccessJwtVerifier( - tokenService: AccessJwtVerifier, -): (token: string) => Promise<{ +// The decoded payload shared by the /mcp Bearer allowlist. Carries the `type` +// discriminator and the API-key `apiKeyId`, on top of the access-token fields. +export interface McpBearerPayload { + type?: JwtType; sub?: string; email?: string; workspaceId?: string; sessionId?: string; -}> { - return (token: string) => tokenService.verifyJwt(token, JwtType.ACCESS); + apiKeyId?: string; +} + +// Minimal structural shape of the TokenService.verifyJwtOneOf method. +export interface OneOfJwtVerifier { + verifyJwtOneOf: ( + token: string, + allowed: JwtType[], + ) => Promise; +} + +/** + * Bind a TokenService-like verifier into a one-arg `verifyJwtOneOf(token)` that + * pins the /mcp Bearer ALLOWLIST to exactly {ACCESS, API_KEY}. This is the single + * place the /mcp Bearer path pins the token type: the /mcp Bearer slot + * legitimately accepts either an ACCESS token (a + * human's session token) OR an API_KEY token (an agent's key), but NOTHING else + * (collab/exchange/attachment/etc. are rejected with the generic type error). + * The allowlist is fixed here rather than at the call site, and the signature is + * verified exactly once (see verifyMcpBearer). + */ +export function bindMcpBearerVerifier( + tokenService: OneOfJwtVerifier, +): (token: string) => Promise { + return (token: string) => + tokenService.verifyJwtOneOf(token, [JwtType.ACCESS, JwtType.API_KEY]); +} + +// Deps for the /mcp Bearer router. `verifyJwtOneOf` is the one-arg verifier bound +// above (allowlist {ACCESS, API_KEY}); the ACCESS-specific revocation/disabled +// deps mirror BearerVerifyDeps; `validateApiKey` is the SHARED api-key row-check. +export interface McpBearerDeps + extends Omit { + verifyJwtOneOf: (token: string) => Promise; + // Row-check for an API_KEY principal — the SAME validator REST uses. Throws + // UnauthorizedException on a definite deny; PROPAGATES an infra error (→ 5xx), + // never masking it as a 401. Not a login attempt: the Basic limiter is not + // involved on this path. + validateApiKey: (payload: McpBearerPayload) => Promise; +} + +/** + * Verify a /mcp Bearer token that may be an ACCESS token OR an API_KEY token, and + * route by type. The signature is verified EXACTLY ONCE (verifyJwtOneOf); the + * result is reused so the ACCESS branch does not re-verify. + * + * - API_KEY -> bind to THIS instance's workspace FIRST (a token for another + * workspace is rejected), THEN run the shared `validateApiKey` row-check. + * No session/limiter involvement (an API key is not a login). + * - ACCESS -> the unchanged `verifyBearerAccess` (session-active + not-disabled + * checks), fed a closure over the already-verified payload so "verify once" + * and "helper unchanged" coexist. + * + * Throws UnauthorizedException on any auth failure (uniform generic message — no + * enumeration of why); propagates an infra error from `validateApiKey` as itself. + */ +export async function verifyMcpBearer( + token: string, + deps: McpBearerDeps, +): Promise<{ sub?: string; email?: string }> { + const generic = 'Invalid or expired token'; + const payload = await deps.verifyJwtOneOf(token); + + if (payload.type === JwtType.API_KEY) { + if (!payload.sub || !payload.workspaceId) { + throw new UnauthorizedException(generic); + } + // Instance-binding (mirrors verifyBearerAccess): reject an API_KEY token + // minted for a different workspace before touching the DB. + if ( + deps.expectedWorkspaceId && + payload.workspaceId !== deps.expectedWorkspaceId + ) { + throw new UnauthorizedException(generic); + } + // Shared row-check. A definite deny throws Unauthorized; an infra error + // propagates (→ 5xx), which the caller must NOT convert to a 401. + await deps.validateApiKey(payload); + return { sub: payload.sub }; + } + + // ACCESS: reuse verifyBearerAccess WITHOUT re-verifying the signature. + return verifyBearerAccess(token, { + verifyJwt: async () => payload, + expectedWorkspaceId: deps.expectedWorkspaceId, + findUser: deps.findUser, + findActiveSession: deps.findActiveSession, + }); } // Minimal shapes for the Bearer revocation/disabled check. Kept structural so @@ -728,18 +793,24 @@ export async function resolveMcpSessionConfig( }; } - // --- 2) fallback A: Bearer access-JWT (user-supplied token) --- + // --- 2) fallback A: Bearer JWT (user-supplied ACCESS or agent API_KEY) --- const bearer = extractBearer(authHeader); if (bearer) { let payload: { sub?: string; email?: string }; try { payload = await deps.verifyAccessJwt(bearer); } catch (err) { - const message = - err instanceof Error && err.message - ? err.message - : 'Invalid or expired token'; - throw new UnauthorizedException(message); + // Anti-enumeration (Bearer leg): EVERY auth failure surfaces the SAME + // generic 401 — expired/revoked/wrong-type/unknown are indistinguishable + // to the caller (its reaction is identical either way). But an UNEXPECTED + // (infra) error is NOT an auth verdict: rethrow it AS ITSELF so the surface + // maps it to 5xx (mapAuthResultToResponse), never masking a DB/Redis + // outage as a bad token. verifyMcpBearer throws UnauthorizedException on a + // definite deny and lets an infra error from validateApiKey propagate. + if (err instanceof UnauthorizedException) { + throw new UnauthorizedException('Invalid or expired token'); + } + throw err; } return { config: { apiUrl, getToken: async () => bearer }, diff --git a/apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts b/apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts index d55451cb..c299bad4 100644 --- a/apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts +++ b/apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts @@ -115,6 +115,7 @@ function makeService(opts: { undefined as never, // userRepo undefined as never, // userSessionRepo moduleRef as never, // moduleRef (read by the MFA branch) + undefined as never, // apiKeyService (unused by the login-gate path) undefined as never, // sandboxStore (unused by the login-gate path) ); // Stop the constructor's unref'd sweep timer leaking across tests. diff --git a/apps/server/src/integrations/mcp/mcp.module.ts b/apps/server/src/integrations/mcp/mcp.module.ts index e2a01381..779d7a1b 100644 --- a/apps/server/src/integrations/mcp/mcp.module.ts +++ b/apps/server/src/integrations/mcp/mcp.module.ts @@ -4,13 +4,16 @@ import { McpService } from './mcp.service'; import { DatabaseModule } from '@docmost/db/database.module'; import { AuthModule } from '../../core/auth/auth.module'; import { TokenModule } from '../../core/auth/token.module'; +import { ApiKeyModule } from '../../core/api-key/api-key.module'; // Community MCP feature: the server itself serves the Model Context Protocol // over HTTP at /mcp. DatabaseModule (global) provides WorkspaceRepo. AuthModule // supplies AuthService (per-user HTTP-Basic login validation) and TokenModule -// supplies TokenService (Bearer access-JWT verification for the token fallback). +// supplies TokenService (Bearer JWT verification for the token path). ApiKeyModule +// supplies ApiKeyService (the shared api-key row-check for the API_KEY Bearer +// branch, so an agent authenticates with a key instead of the bcrypt Basic path). @Module({ - imports: [DatabaseModule, AuthModule, TokenModule], + imports: [DatabaseModule, AuthModule, TokenModule, ApiKeyModule], controllers: [McpController], providers: [McpService], }) diff --git a/apps/server/src/integrations/mcp/mcp.service.spec.ts b/apps/server/src/integrations/mcp/mcp.service.spec.ts index 6894b97e..b2ee0236 100644 --- a/apps/server/src/integrations/mcp/mcp.service.spec.ts +++ b/apps/server/src/integrations/mcp/mcp.service.spec.ts @@ -6,9 +6,10 @@ import { isCredentialsFailure, isInitializeRequestBody, verifyBearerAccess, + verifyMcpBearer, + bindMcpBearerVerifier, sharedTokenMatches, clientIp, - bindAccessJwtVerifier, extractBearer, decideBasicGate, mapAuthResultToResponse, @@ -524,13 +525,28 @@ describe('resolveMcpSessionConfig', () => { expect(resolved.identity).toBe('bearer:user-9'); }); - it('Bearer invalid -> specific 401 from verifyAccessJwt', async () => { + it('Bearer invalid -> UNIFORM generic 401 (anti-enumeration, reason not leaked)', async () => { + // #501: the Bearer leg no longer surfaces the specific reason. Whether the + // token is expired, revoked, wrong-type or unknown, the caller sees ONE bare + // 'Invalid or expired token' — an agent's reaction is identical, and a leaked + // class would be an enumeration oracle. const verifyAccessJwt = jest .fn() .mockRejectedValue(new UnauthorizedException('jwt expired')); await expect( resolveMcpSessionConfig('Bearer expired', makeDeps({ verifyAccessJwt })), - ).rejects.toThrow('jwt expired'); + ).rejects.toThrow('Invalid or expired token'); + }); + + it('Bearer INFRA error -> propagates (NOT masked as 401)', async () => { + // A non-UnauthorizedException (e.g. a DB outage in the api-key row-check) is + // not an auth verdict: it must propagate so the surface maps it to 5xx. + const verifyAccessJwt = jest + .fn() + .mockRejectedValue(new Error('connection terminated')); + await expect( + resolveMcpSessionConfig('Bearer x', makeDeps({ verifyAccessJwt })), + ).rejects.toThrow('connection terminated'); }); it('no creds + env service account configured -> service-account config', async () => { @@ -1001,48 +1017,104 @@ describe('clientIp (XFF-fallback precedence, item 5)', () => { }); }); -describe('bindAccessJwtVerifier enforces JwtType.ACCESS (item 3)', () => { - it('calls TokenService.verifyJwt with JwtType.ACCESS as the second argument', async () => { - // Mock TokenService: assert the type literal is pinned to ACCESS so swapping - // to REFRESH (or omitting the type) breaks this test. - const verifyJwt = jest +describe('bindMcpBearerVerifier pins the {ACCESS, API_KEY} allowlist (#501)', () => { + it('calls verifyJwtOneOf with exactly [ACCESS, API_KEY]', async () => { + const verifyJwtOneOf = jest .fn() - .mockResolvedValue({ sub: 'user-1', workspaceId: 'ws-1' }); - const verify = bindAccessJwtVerifier({ verifyJwt }); + .mockResolvedValue({ type: JwtType.API_KEY, sub: 'u-1' }); + await bindMcpBearerVerifier({ verifyJwtOneOf })('the.jwt'); + expect(verifyJwtOneOf).toHaveBeenCalledWith('the.jwt', [ + JwtType.ACCESS, + JwtType.API_KEY, + ]); + // Pin the concrete enum values too. + expect(verifyJwtOneOf.mock.calls[0][1]).toEqual(['access', 'api_key']); + }); +}); - await verify('the.access.jwt'); - - expect(verifyJwt).toHaveBeenCalledTimes(1); - expect(verifyJwt).toHaveBeenCalledWith('the.access.jwt', JwtType.ACCESS); - // Pin the real enum value too, so renaming/repointing the enum member is caught. - expect(verifyJwt.mock.calls[0][1]).toBe('access'); +describe('verifyMcpBearer routes by token type (#501)', () => { + const accessDeps = (over: any = {}) => ({ + verifyJwtOneOf: jest.fn(), + expectedWorkspaceId: 'ws-1', + findUser: jest.fn().mockResolvedValue({ deactivatedAt: null }), + findActiveSession: jest + .fn() + .mockResolvedValue({ userId: 'u-1', workspaceId: 'ws-1' }), + validateApiKey: jest.fn(), + ...over, }); - it('passes through the verified payload', async () => { - const payload = { sub: 'user-9', email: 'u@e.com', workspaceId: 'ws-1' }; - const verifyJwt = jest.fn().mockResolvedValue(payload); - await expect( - bindAccessJwtVerifier({ verifyJwt })('t'), - ).resolves.toBe(payload); + it('API_KEY -> row-checks via validateApiKey and does NOT touch session/limiter', async () => { + const deps = accessDeps({ + verifyJwtOneOf: jest.fn().mockResolvedValue({ + type: JwtType.API_KEY, + sub: 'svc-1', + workspaceId: 'ws-1', + apiKeyId: 'k-1', + }), + validateApiKey: jest.fn().mockResolvedValue({ user: { id: 'svc-1' } }), + }); + const res = await verifyMcpBearer('tok', deps); + expect(deps.validateApiKey).toHaveBeenCalledTimes(1); + // No session lookup on the api-key path (not a login). + expect(deps.findActiveSession).not.toHaveBeenCalled(); + expect(res).toEqual({ sub: 'svc-1' }); }); - // The Bearer revocation/disabled checks (verifyBearerAccess) are covered above; - // this binds the ACCESS-type enforcement that verifyMcpBearer wires in. - it('feeds verifyBearerAccess so the whole Bearer chain enforces ACCESS', async () => { - const verifyJwt = jest.fn().mockResolvedValue({ - sub: 'user-1', + it('API_KEY for ANOTHER workspace -> rejected before the row-check', async () => { + const deps = accessDeps({ + verifyJwtOneOf: jest.fn().mockResolvedValue({ + type: JwtType.API_KEY, + sub: 'svc-1', + workspaceId: 'ws-OTHER', + apiKeyId: 'k-1', + }), + validateApiKey: jest.fn(), + }); + await expect(verifyMcpBearer('tok', deps)).rejects.toBeInstanceOf( + UnauthorizedException, + ); + expect(deps.validateApiKey).not.toHaveBeenCalled(); + }); + + it('API_KEY infra error from validateApiKey PROPAGATES (not masked)', async () => { + const boom = new Error('db down'); + const deps = accessDeps({ + verifyJwtOneOf: jest.fn().mockResolvedValue({ + type: JwtType.API_KEY, + sub: 'svc-1', + workspaceId: 'ws-1', + apiKeyId: 'k-1', + }), + validateApiKey: jest.fn().mockRejectedValue(boom), + }); + await expect(verifyMcpBearer('tok', deps)).rejects.toBe(boom); + }); + + it('ACCESS -> runs the session/disabled checks and does NOT call validateApiKey', async () => { + const deps = accessDeps({ + verifyJwtOneOf: jest.fn().mockResolvedValue({ + type: JwtType.ACCESS, + sub: 'u-1', + workspaceId: 'ws-1', + sessionId: 'sess-1', + email: 'u@e.com', + }), + }); + const res = await verifyMcpBearer('tok', deps); + expect(deps.findActiveSession).toHaveBeenCalledWith('sess-1'); + expect(deps.validateApiKey).not.toHaveBeenCalled(); + expect(res).toEqual({ sub: 'u-1', email: 'u@e.com' }); + }); + + it('verifies the signature exactly ONCE (single verifyJwtOneOf, no re-verify)', async () => { + const verifyJwtOneOf = jest.fn().mockResolvedValue({ + type: JwtType.ACCESS, + sub: 'u-1', workspaceId: 'ws-1', - sessionId: 'sess-1', }); - const res = await verifyBearerAccess('t', { - verifyJwt: bindAccessJwtVerifier({ verifyJwt }), - findUser: jest.fn().mockResolvedValue({ deactivatedAt: null }), - findActiveSession: jest - .fn() - .mockResolvedValue({ userId: 'user-1', workspaceId: 'ws-1' }), - }); - expect(verifyJwt).toHaveBeenCalledWith('t', JwtType.ACCESS); - expect(res).toEqual({ sub: 'user-1', email: undefined }); + await verifyMcpBearer('tok', accessDeps({ verifyJwtOneOf })); + expect(verifyJwtOneOf).toHaveBeenCalledTimes(1); }); }); @@ -1198,6 +1270,7 @@ describe('McpService.onModuleDestroy — CollabSession teardown (#486)', () => { {} as any, {} as any, {} as any, + {} as any, ); } diff --git a/apps/server/src/integrations/mcp/mcp.service.ts b/apps/server/src/integrations/mcp/mcp.service.ts index e72ee8ac..5b947c5e 100644 --- a/apps/server/src/integrations/mcp/mcp.service.ts +++ b/apps/server/src/integrations/mcp/mcp.service.ts @@ -14,16 +14,17 @@ import { UserSessionRepo } from '@docmost/db/repos/session/user-session.repo'; import { AuthService } from '../../core/auth/services/auth.service'; import { TokenService } from '../../core/auth/services/token.service'; import { validateSsoEnforcement } from '../../core/auth/auth.util'; -import { JwtPayload } from '../../core/auth/dto/jwt-payload'; +import { JwtApiKeyPayload } from '../../core/auth/dto/jwt-payload'; import { Workspace } from '@docmost/db/types/entity.types'; +import { ApiKeyService } from '../../core/api-key/api-key.service'; import { FailedLoginLimiter, resolveMcpSessionConfig, - verifyBearerAccess, + verifyMcpBearer, isInitializeRequestBody, sharedTokenMatches, clientIp, - bindAccessJwtVerifier, + bindMcpBearerVerifier, decideBasicGate, mapAuthResultToResponse, DocmostMcpConfig, @@ -105,6 +106,10 @@ export class McpService implements OnModuleDestroy { private readonly userRepo: UserRepo, private readonly userSessionRepo: UserSessionRepo, private readonly moduleRef: ModuleRef, + // Shared api-key row-check for the /mcp API_KEY Bearer branch (same validator + // REST uses). Also lets an agent authenticate to /mcp with an api key instead + // of the bcrypt Basic path, so parallel reads stop starving the limiter. + private readonly apiKeyService: ApiKeyService, // Shared singleton in-RAM blob store backing the stash tool. private readonly sandboxStore: SandboxStore, ) { @@ -194,37 +199,41 @@ export class McpService implements OnModuleDestroy { } } - // Bearer access-JWT verification for the /mcp token fallback. verifyJwt only - // checks signature/exp/type, but a logged-out (revoked) or disabled user can - // still hold an unexpired access JWT. JwtStrategy additionally checks the - // session is active and the user is not disabled; we mirror those exact checks - // here so the MCP Bearer path is not weaker than the normal cookie/header path. + // Bearer verification for the /mcp token path. The Bearer slot accepts EITHER + // an ACCESS token (a human session token) OR an API_KEY token (an agent's key) + // — the allowlist is pinned in bindMcpBearerVerifier. An ACCESS token is + // checked exactly as JwtStrategy does (signature/exp/type + session-active + + // not-disabled), so the MCP path is not weaker than the cookie/header path. An + // API_KEY token is HMAC-verified (microseconds) then row-checked via the shared + // ApiKeyService.validate — NOT a login attempt, so the Basic bcrypt path and + // its anti-brute-force limiter are never touched (the parallel-reads fix). private async verifyMcpBearer( token: string, ): Promise<{ sub?: string; email?: string }> { - // Resolve THIS instance's workspace so verifyBearerAccess can bind the - // token's `workspaceId` claim to it (mirrors JwtStrategy). The community - // build is single-workspace (findFirst), so this is the default workspace - // and the check is a no-op here; it only rejects a foreign-workspace token - // in a multi-workspace deployment. Undefined (no workspace configured) means - // no check — the credentials path would already have failed with no - // workspace, and an undefined here keeps the helper a no-op rather than - // rejecting every token. + // Resolve THIS instance's workspace so the router can bind the token's + // `workspaceId` claim to it (mirrors JwtStrategy). The community build is + // single-workspace (findFirst), so this is the default workspace and the + // check is a no-op here; it only rejects a foreign-workspace token in a + // multi-workspace deployment. Undefined (no workspace configured) means no + // check — the credentials path would already have failed with no workspace. const instanceWorkspace = await this.workspaceRepo.findFirst(); - // The revocation/disabled decision logic lives in the framework-free - // verifyBearerAccess helper (unit-testable without the heavy auth graph); - // this method only wires in the concrete TokenService + repos. - return verifyBearerAccess(token, { - // The JwtType.ACCESS enforcement lives in bindAccessJwtVerifier (a pure, - // testable seam) so the type literal cannot silently drift to REFRESH. - verifyJwt: bindAccessJwtVerifier(this.tokenService) as ( - t: string, - ) => Promise, + // The type-routing + revocation/disabled decision logic lives in the + // framework-free verifyMcpBearer helper (unit-testable without the heavy auth + // graph); this method only wires in the concrete TokenService + repos + the + // shared api-key validator. + return verifyMcpBearer(token, { + // The {ACCESS, API_KEY} allowlist enforcement lives in bindMcpBearerVerifier + // (a pure, testable seam) so the type set cannot silently drift. + verifyJwtOneOf: bindMcpBearerVerifier(this.tokenService), expectedWorkspaceId: instanceWorkspace?.id, findUser: (sub, workspaceId) => this.userRepo.findById(sub, workspaceId), findActiveSession: (sessionId) => this.userSessionRepo.findActiveById(sessionId), + // Shared with REST: a definite deny throws Unauthorized, an infra error + // propagates (→ 5xx). The /mcp bearer catch must preserve that distinction. + validateApiKey: (payload) => + this.apiKeyService.validate(payload as JwtApiKeyPayload), }); } diff --git a/packages/mcp/src/lib/drawio-xml.ts b/packages/mcp/src/lib/drawio-xml.ts index 84b2c5a3..620b12ea 100644 --- a/packages/mcp/src/lib/drawio-xml.ts +++ b/packages/mcp/src/lib/drawio-xml.ts @@ -178,10 +178,11 @@ function sliceModel(xml: string): string | null { // --- decode chain ---------------------------------------------------------- /** - * Read the `content=` attribute out of a `.drawio.svg` string. Docmost stores a - * base64 payload there (createDrawioSvg); draw.io's own SVG export may store the - * XML entity-encoded instead. The DOM decodes entities for us, so the caller - * only has to distinguish "starts with '<'" (raw XML) from base64. + * Read the `content=` attribute out of a `.drawio.svg` string. Docmost writes + * the mxfile XML entity-encoded there (buildDrawioSvg / createDrawioSvg), which + * is also how draw.io's own SVG export stores it; older attachments stored a + * base64 payload instead. The DOM decodes entities for us, so the caller only + * has to distinguish "starts with '<'" (raw XML) from base64. */ export function extractContentAttr(svg: string): string { const { doc, error } = parseXml(svg); @@ -195,12 +196,23 @@ export function extractContentAttr(svg: string): string { // value itself never contains a double-quote (base64 / entity-encoded XML). const m = /content="([^"]*)"/.exec(svg); if (m) { - // Decode the handful of XML entities a raw regex would leave encoded. + // Decode the handful of XML entities a raw regex would leave encoded. The + // numeric char-refs for tab/newline/CR MUST be decoded here too: the DOM + // path above turns them back into the literal control chars, so this + // regex fallback has to agree or the two decode paths diverge (#507). + // `&` is decoded last so an escaped `&#x9;` reads back as the + // literal text ` `, not a tab. return m[1] .replace(/</g, "<") .replace(/>/g, ">") .replace(/"/g, '"') .replace(/'/g, "'") + .replace(/ /gi, "\t") + .replace(/ /g, "\t") + .replace(/ /gi, "\n") + .replace(/ /g, "\n") + .replace(/ /gi, "\r") + .replace(/ /g, "\r") .replace(/&/g, "&"); } throw new Error("drawio: SVG has no content= attribute to decode"); @@ -307,9 +319,16 @@ export function encodeDrawioFile(modelXml: string, title = "Page-1"): string { /** * Build the `diagram.drawio.svg` attachment. Mirrors the import service's * createDrawioSvg contract exactly: - * ${inner} + * ${inner} * plus width/height/viewBox from the diagram bounding box and the schematic * preview as the visible children (`inner`). + * + * The `content=` value is the mxfile XML XML-entity-escaped (draw.io's own + * native form), NOT base64. draw.io's editor decodes a base64 content= via + * Latin-1 atob (no UTF-8 step), turning every non-ASCII char (e.g. Cyrillic, + * ё, —) into mojibake; the entity-encoded form is decoded by the DOM as UTF-8 + * and opens intact. Our decoder (decodeDrawioSvg) reads both forms, so old + * base64 attachments still round-trip. */ export function buildDrawioSvg( modelXml: string, @@ -318,14 +337,14 @@ export function buildDrawioSvg( title = "Page-1", ): string { const file = encodeDrawioFile(modelXml, title); - const base64 = Buffer.from(file, "utf-8").toString("base64"); + const content = xmlEscape(file); const w = Math.max(1, Math.round(bbox.width)); const h = Math.max(1, Math.round(bbox.height)); return ( `${inner}` + `content="${content}">${inner}` ); } @@ -334,7 +353,15 @@ function xmlEscape(s: string): string { .replace(/&/g, "&") .replace(//g, ">") - .replace(/"/g, """); + .replace(/"/g, """) + // A literal tab/newline/CR inside an attribute value is collapsed to a + // single space by XML attribute-value normalization on DOM read (both jsdom + // here and the real draw.io editor), silently flattening multi-line labels + // and tab-bearing values. Numeric char-refs survive that normalization, so + // emit them the way draw.io's own native export does (#507). + .replace(/\t/g, " ") + .replace(/\n/g, " ") + .replace(/\r/g, " "); } // --- normalization + hash -------------------------------------------------- diff --git a/packages/mcp/test/unit/drawio-xml.test.mjs b/packages/mcp/test/unit/drawio-xml.test.mjs index 8d6e83ec..3295b0f8 100644 --- a/packages/mcp/test/unit/drawio-xml.test.mjs +++ b/packages/mcp/test/unit/drawio-xml.test.mjs @@ -51,6 +51,14 @@ const hasRule = (issues, rule, cellId) => (i) => i.rule === rule && (cellId === undefined || i.cellId === cellId), ); +// Reverse the attribute-value XML escaping used in the `content=` payload. +const unescapeAttr = (s) => + s + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, '"') + .replace(/&/g, "&"); + // --- style parsing --------------------------------------------------------- test("parseStyle: base stylename + key=value pairs", () => { @@ -335,16 +343,20 @@ test("encode/build: a title with < > \" & round-trips without corrupting the SVG // The outer content="..." attribute must not be broken by the title: the raw // title metacharacters never appear literally in the SVG markup (they are - // base64-encoded inside content=, and escaped inside the file XML). + // entity-escaped inside content=, doubly so where the file XML already escaped + // them inside name="..."). const contentMatch = /content="([^"]*)"/.exec(svg); assert.ok(contentMatch, "SVG has a single well-formed content= attribute"); // The diagram model still decodes losslessly despite the exotic title. assert.equal(decodeDrawioSvg(svg), model); + // content= is now entity-encoded XML (draw.io's native form), never base64. + assert.match(contentMatch[1], /^<mxfile/); + // The file XML is well-formed: the title lives in name="..." as escaped // entities, so unescaping recovers the original title byte-for-byte. - const fileXml = Buffer.from(contentMatch[1], "base64").toString("utf-8"); + const fileXml = unescapeAttr(contentMatch[1]); const nameMatch = //.exec(fileXml); assert.ok(nameMatch, "the diagram name attribute is intact and quote-safe"); const decodedTitle = nameMatch[1] @@ -358,3 +370,112 @@ test("encode/build: a title with < > \" & round-trips without corrupting the SVG const file = encodeDrawioFile(model, title); assert.match(file, /name="A < B > C " D & E">/); }); + +// --- #507: content= is entity-encoded XML, never base64 -------------------- + +// A model whose cell values carry Cyrillic, ё and an em dash — exactly the +// characters draw.io's Latin-1 atob mangles when content= is base64. +const CYRILLIC_MODEL = + "" + + '' + + '' + + '' + + ""; + +test("#507: buildDrawioSvg writes content= as entity-encoded XML (not base64)", () => { + const model = normalizeXml(CYRILLIC_MODEL); + const svg = buildDrawioSvg(model, "", { width: 200, height: 120 }, "Диаграмма"); + + const contentMatch = /content="([^"]*)"/.exec(svg); + assert.ok(contentMatch, "SVG has a single well-formed content= attribute"); + const content = contentMatch[1]; + + // The content= value is the entity-encoded mxfile XML — starts with `<mxfile`. + assert.match(content, /^<mxfile/, "content= is entity-encoded mxfile XML"); + // It must NOT be a base64 blob: base64 has no XML entities and no literal `<`. + assert.ok(content.includes("<"), "content= carries XML entities, not base64"); + + // Cyrillic / ё / — survive verbatim in the attribute (raw UTF-8, not atob-mangled). + assert.ok(content.includes("Старт-бит — ёж"), "non-ASCII value is raw UTF-8 in content="); + assert.ok(content.includes("Диаграмма"), "non-ASCII title is raw UTF-8 in content="); + // The mojibake that base64+atob would have produced must be absent. + assert.ok(!content.includes("Ð"), "no Latin-1 mojibake in content="); +}); + +test("#507: Cyrillic model round-trips byte-stable through buildDrawioSvg -> decodeDrawioSvg", () => { + const model = normalizeXml(CYRILLIC_MODEL); + const svg = buildDrawioSvg(model, "", { width: 200, height: 120 }, "Заголовок — ё"); + assert.equal(decodeDrawioSvg(svg), model); +}); + +test("#507 back-compat: an OLD base64-form .drawio.svg still decodes losslessly", () => { + const model = normalizeXml(CYRILLIC_MODEL); + // Reproduce the pre-fix write path: encodeDrawioFile -> base64 in content=. + const file = encodeDrawioFile(model, "Старая диаграмма"); + const base64 = Buffer.from(file, "utf-8").toString("base64"); + const svg = + ``; + // No XML entities, purely base64 alphabet — this is the legacy form. + assert.ok(!base64.includes("<") && !base64.includes("&")); + assert.equal(decodeDrawioSvg(svg), model); +}); + +test("#507 negative: non-ASCII in a cell value AND in the title round-trip clean", () => { + const model = normalizeXml(CYRILLIC_MODEL); + const title = "Тест — ёмкость № 5"; + const svg = buildDrawioSvg(model, "", { width: 200, height: 120 }, title); + + // Model recovered byte-for-byte. + assert.equal(decodeDrawioSvg(svg), model); + + // Title lands in the of the decoded file XML, intact. + const content = /content="([^"]*)"/.exec(svg)[1]; + const fileXml = unescapeAttr(content); + const nameMatch = //.exec(fileXml); + assert.ok(nameMatch, "diagram name attribute present"); + assert.equal(unescapeAttr(nameMatch[1]), title); +}); + +// --- #507 F1: literal tab/newline/CR in an attribute value survive the +// content= round-trip. The whole mxfile XML lives in one content="..." attr; +// XML attribute-value normalization collapses a LITERAL tab/newline/CR to a +// single space on DOM read, so the escape must emit numeric char-refs instead. +const CTRL_MODEL = + "" + + '' + + '' + + '' + + '' + + '' + + ""; + +test("#507 F1: literal tab/newline/CR in a value round-trip byte-stable (DOM decode)", () => { + const svg = buildDrawioSvg(CTRL_MODEL, "", { width: 200, height: 200 }); + const content = /content="([^"]*)"/.exec(svg)[1]; + // The tab/newline/CR must be emitted as numeric char-refs, never as literal + // control chars (which DOM attribute-value normalization would eat). + assert.ok( + !/[\t\n\r]/.test(content), + "no literal tab/newline/CR survive in the content= attribute", + ); + assert.ok( + content.includes(" ") && + content.includes(" ") && + content.includes(" "), + "tab/newline/CR are emitted as numeric char-refs", + ); + // Full DOM decode recovers the model byte-for-byte, control chars intact. + assert.equal(decodeDrawioSvg(svg), CTRL_MODEL); +}); + +test("#507 F1: regex fallback decodes tab/newline/CR char-refs (agrees with DOM path)", () => { + const svg = buildDrawioSvg(CTRL_MODEL, "", { width: 200, height: 200 }); + const content = /content="([^"]*)"/.exec(svg)[1]; + // A bare `&` makes the SVG wrapper malformed, forcing extractContentAttr onto + // its regex fallback branch. That branch must decode the tab/newline/CR + // char-refs exactly like the DOM path, or the two decoders diverge. + const malformedSvg = `&`; + assert.equal(decodeDrawioSvg(malformedSvg), CTRL_MODEL); + // The well-formed (DOM) path yields the identical result. + assert.equal(decodeDrawioSvg(svg), CTRL_MODEL); +});