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/.github/workflows/develop.yml b/.github/workflows/develop.yml index 224a1e43..2831d643 100644 --- a/.github/workflows/develop.yml +++ b/.github/workflows/develop.yml @@ -226,6 +226,13 @@ jobs: - name: Build mcp run: pnpm --filter @docmost/mcp build + # apps/server imports @docmost/token-estimate at runtime (history-budget.ts, + # #490); its dist/ is gitignored and `test:e2e` type-checks + runs the code, + # so build it here or tsc fails with TS2307 Cannot find module + # '@docmost/token-estimate' (mirrors the editor-ext / mcp build steps above). + - name: Build token-estimate + run: pnpm --filter @docmost/token-estimate build + - name: Run migrations run: pnpm --filter ./apps/server migration:latest diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 509ad6be..fef6b189 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -159,6 +159,14 @@ jobs: - name: Build prosemirror-markdown run: pnpm --filter @docmost/prosemirror-markdown build + # @docmost/token-estimate is a shared workspace package the client vitest + # suite resolves via its dist build (main: ./dist/index.js); dist/ is + # gitignored and `pnpm -r test` does NOT honour nx `dependsOn: ^build`, so + # build it before the recursive test run or the client suite fails with + # "Failed to resolve import '@docmost/token-estimate'" (#490). + - name: Build token-estimate + run: pnpm --filter @docmost/token-estimate build + - name: Run unit tests run: pnpm -r test diff --git a/CHANGELOG.md b/CHANGELOG.md index fc7ae18d..53dfdcd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 new resync path re-reads the live anchor so the suggestion applies against the current text, and orphaned anchors (whose marked run was deleted) are reconciled rather than left blocking. (#496) +- **Save intentional page versions.** Press `Cmd/Ctrl+S` (or use the page menu) + to save a named version of a page. The history panel now distinguishes + intentional versions (a "Saved" / "Agent version" badge) from automatic + snapshots, dims autosaves, and offers an "Only versions" filter. Automatic + snapshots switched from a fixed interval to a trailing idle-flush with a + max-wait ceiling, and a boundary snapshot is pinned whenever the editing source + changes (e.g. a person's edits followed by the AI agent). (#370) - **Place several images side by side in a row.** A new "Inline (side by side)" alignment mode in the image bubble menu renders consecutive inline @@ -385,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/Dockerfile b/Dockerfile index 280e5b55..792bf0e5 100644 --- a/Dockerfile +++ b/Dockerfile @@ -59,6 +59,14 @@ COPY --from=builder /app/packages/mcp/data /app/packages/mcp/data COPY --from=builder /app/packages/prosemirror-markdown/build /app/packages/prosemirror-markdown/build COPY --from=builder /app/packages/prosemirror-markdown/package.json /app/packages/prosemirror-markdown/package.json +# apps/server imports @docmost/token-estimate (workspace:*) at runtime +# (history-budget.ts, #490). tsc emits only dist/ and dist/ is gitignored, so the +# prod install would resolve a broken workspace symlink and the server would die +# with ERR_MODULE_NOT_FOUND on the first history-budget call. Ship the built +# package + its manifest, mirroring prosemirror-markdown above. +COPY --from=builder /app/packages/token-estimate/dist /app/packages/token-estimate/dist +COPY --from=builder /app/packages/token-estimate/package.json /app/packages/token-estimate/package.json + # Copy root package files COPY --from=builder /app/package.json /app/package.json COPY --from=builder /app/pnpm*.yaml /app/ diff --git a/README.md b/README.md index bf6a46ae..07e9fde9 100644 --- a/README.md +++ b/README.md @@ -206,6 +206,137 @@ start the new migrations apply on top of your existing schema (`CREATE EXTENSION existing pages are indexed on their next edit. pgvector is still required for the migration to apply at all. +## Local embeddings server + +The AI agent's semantic (RAG) search needs an **embeddings model**. Instead of paying a cloud +provider (e.g. OpenAI `text-embedding-3-*`) to embed every page, you can run a small open-weights +model yourself with Hugging Face +[Text Embeddings Inference](https://github.com/huggingface/text-embeddings-inference) (TEI), which +serves an OpenAI-compatible `/v1/embeddings` endpoint. `intfloat/multilingual-e5-small` is a good +default: multilingual, 384-dim, and comfortable on CPU (~1–2 GB RAM, 1–2 vCPU). Point Gitmost at it +under **Workspace settings → AI → Embeddings**. + +### Option A — local (same Docker network as Gitmost) + +Run TEI as a container on the network Gitmost is already on. The port is never published, so the +endpoint stays internal and needs no authentication. + +```yaml +services: + embeddings: + image: ghcr.io/huggingface/text-embeddings-inference:cpu-1.9 # pin version; use a cuda-* tag for GPU + container_name: embeddings + restart: unless-stopped + networks: + - gitmost_net # same network Gitmost is on + command: + - "--model-id" + - "intfloat/multilingual-e5-small" + - "--auto-truncate" # clamp over-long inputs instead of returning 413 + volumes: + - tei-models:/data # weights are downloaded once and cached here + +networks: + gitmost_net: + external: true # the network Gitmost already uses + +volumes: + tei-models: +``` + +Gitmost settings (**Workspace settings → AI → Embeddings**): + +| Field | Value | +|-------------------|-----------------------------------| +| Model | `intfloat/multilingual-e5-small` | +| Base URL | `http://embeddings:80/v1/` | +| Embedding API key | — (leave empty) | + +> `embeddings` is the container name — Gitmost resolves it over DNS inside the Docker network. +> The port is not published, so the endpoint is reachable only by containers on that network and +> no authorization is required. + +### Option B — separate host (public via Traefik + Let's Encrypt) + +This assumes the host already runs Traefik with an ACME resolver (the example below uses +`letsEncrypt`, the `websecure` entrypoint and a shared `docker_main_net` network). Replace the +domain / network / resolver with your own. + +**DNS:** add an A record `embeddings.example.com` → the IP of your Traefik host (same +challenge / port 80 as the rest of your sites). + +```yaml +services: + embeddings: + image: ghcr.io/huggingface/text-embeddings-inference:cpu-1.9 # pin version; cuda-* tag for GPU + container_name: embeddings + restart: unless-stopped + networks: + - docker_main_net # the network Traefik is attached to + command: + - "--model-id" + - "intfloat/multilingual-e5-small" + - "--auto-truncate" + - "--api-key" + - "sk-emb-REPLACE_WITH_YOUR_KEY" + volumes: + - tei-models:/data + labels: + traefik.enable: "true" + traefik.http.routers.embeddings.rule: "Host(`embeddings.example.com`)" + traefik.http.routers.embeddings.entrypoints: "websecure" + traefik.http.routers.embeddings.tls: "true" + traefik.http.routers.embeddings.tls.certresolver: "letsEncrypt" + traefik.http.routers.embeddings.service: "embeddings" + traefik.http.services.embeddings.loadbalancer.server.port: "80" + # TEI enforces the Bearer key itself; Traefik only rate-limits to protect the CPU + traefik.http.routers.embeddings.middlewares: "embeddings-rl" + traefik.http.middlewares.embeddings-rl.ratelimit.average: "20" + traefik.http.middlewares.embeddings-rl.ratelimit.burst: "40" + traefik.http.middlewares.embeddings-rl.ratelimit.period: "1s" + +networks: + docker_main_net: + external: true + +volumes: + tei-models: +``` + +Gitmost settings (**Workspace settings → AI → Embeddings**): + +| Field | Value | +|-------------------|---------------------------------------| +| Model | `intfloat/multilingual-e5-small` | +| Base URL | `https://embeddings.example.com/v1/` | +| Embedding API key | your `sk-emb-…` | + +Check it from outside: + +```bash +curl -s https://embeddings.example.com/v1/embeddings \ + -H "Authorization: Bearer sk-emb-REPLACE_WITH_YOUR_KEY" \ + -H "Content-Type: application/json" \ + -d '{"model":"intfloat/multilingual-e5-small","input":"query: hello"}' \ + | python3 -c 'import sys,json;print("dims:",len(json.load(sys.stdin)["data"][0]["embedding"]))' +# -> dims: 384 +``` + +### Embeddings server notes + +- **Vector dimension is 384.** If this Gitmost was previously embedded with a different model + (e.g. `text-embedding-3-large` = 3072-dim), the old pgvector rows won't match the new dimension — + clear the existing embeddings / re-index before switching. Gitmost only compares vectors of the + same dimension, so mixed-dimension rows are silently ignored rather than searched. +- **First start downloads the weights** (hundreds of MB) from `huggingface.co` into the + `tei-models` volume; every start after that reads from the volume. +- **Pin the version.** Pin the image, and optionally the model: add `--revision ` to + `command` (the sha is on the model's page on Hugging Face). +- **Air-gapped / no egress:** seed the `tei-models` volume ahead of time and add + `environment: [HF_HUB_OFFLINE=1]`. +- **GPU:** use the cuda tag of the same release (e.g. + `ghcr.io/huggingface/text-embeddings-inference:cuda-1.9`) and start the container with `gpus: all`. + ## Features - Real-time collaboration diff --git a/README.ru.md b/README.ru.md index 517c4802..13ee8498 100644 --- a/README.ru.md +++ b/README.ru.md @@ -193,6 +193,137 @@ dump/restore, существующий каталог данных переис > неизменным и бэкапьте вместе с базой данных. +## Локальный сервер эмбеддингов + +Семантическому (RAG) поиску AI-агента нужна **модель эмбеддингов**. Вместо оплаты облачного +провайдера (например, OpenAI `text-embedding-3-*`) за эмбеддинг каждой страницы можно запустить +небольшую open-weights модель у себя через Hugging Face +[Text Embeddings Inference](https://github.com/huggingface/text-embeddings-inference) (TEI) — он +отдаёт OpenAI-совместимый эндпоинт `/v1/embeddings`. Хороший дефолт — `intfloat/multilingual-e5-small`: +многоязычная, 384-мерная, комфортно работает на CPU (~1–2 ГБ RAM, 1–2 vCPU). Пропишите её в +**Настройки воркспейса → AI → Эмбеддинги**. + +### Вариант A — локально (та же Docker-сеть, что и Gitmost) + +Запустите TEI контейнером в той же сети, где уже работает Gitmost. Порт наружу не публикуется, +поэтому эндпоинт остаётся внутренним и не требует авторизации. + +```yaml +services: + embeddings: + image: ghcr.io/huggingface/text-embeddings-inference:cpu-1.9 # pin version; use a cuda-* tag for GPU + container_name: embeddings + restart: unless-stopped + networks: + - gitmost_net # same network Gitmost is on + command: + - "--model-id" + - "intfloat/multilingual-e5-small" + - "--auto-truncate" # clamp over-long inputs instead of returning 413 + volumes: + - tei-models:/data # weights are downloaded once and cached here + +networks: + gitmost_net: + external: true # the network Gitmost already uses + +volumes: + tei-models: +``` + +Настройки Gitmost (**Настройки воркспейса → AI → Эмбеддинги**): + +| Поле | Значение | +|-------------------|-----------------------------------| +| Model | `intfloat/multilingual-e5-small` | +| Base URL | `http://embeddings:80/v1/` | +| Embedding API key | — (оставить пустым) | + +> `embeddings` — имя контейнера, Gitmost резолвит его по DNS внутри Docker-сети. +> Наружу порт не публикуется, эндпоинт доступен только контейнерам этой сети, поэтому +> авторизация не нужна. + +### Вариант B — на отдельном хосте (наружу через Traefik + Let's Encrypt) + +Предполагается, что на хосте уже есть Traefik с ACME-резолвером (в примере ниже — `letsEncrypt`, +entrypoint `websecure`, общая сеть `docker_main_net`). Замените домен / сеть / резолвер на свои. + +**DNS:** заведите A-запись `embeddings.example.com` → IP хоста с Traefik (тот же challenge / порт 80, +что и у остальных сайтов). + +```yaml +services: + embeddings: + image: ghcr.io/huggingface/text-embeddings-inference:cpu-1.9 # pin version; cuda-* tag for GPU + container_name: embeddings + restart: unless-stopped + networks: + - docker_main_net # the network Traefik is attached to + command: + - "--model-id" + - "intfloat/multilingual-e5-small" + - "--auto-truncate" + - "--api-key" + - "sk-emb-REPLACE_WITH_YOUR_KEY" + volumes: + - tei-models:/data + labels: + traefik.enable: "true" + traefik.http.routers.embeddings.rule: "Host(`embeddings.example.com`)" + traefik.http.routers.embeddings.entrypoints: "websecure" + traefik.http.routers.embeddings.tls: "true" + traefik.http.routers.embeddings.tls.certresolver: "letsEncrypt" + traefik.http.routers.embeddings.service: "embeddings" + traefik.http.services.embeddings.loadbalancer.server.port: "80" + # TEI enforces the Bearer key itself; Traefik only rate-limits to protect the CPU + traefik.http.routers.embeddings.middlewares: "embeddings-rl" + traefik.http.middlewares.embeddings-rl.ratelimit.average: "20" + traefik.http.middlewares.embeddings-rl.ratelimit.burst: "40" + traefik.http.middlewares.embeddings-rl.ratelimit.period: "1s" + +networks: + docker_main_net: + external: true + +volumes: + tei-models: +``` + +Настройки Gitmost (**Настройки воркспейса → AI → Эмбеддинги**): + +| Поле | Значение | +|-------------------|---------------------------------------| +| Model | `intfloat/multilingual-e5-small` | +| Base URL | `https://embeddings.example.com/v1/` | +| Embedding API key | ваш `sk-emb-…` | + +Проверка снаружи: + +```bash +curl -s https://embeddings.example.com/v1/embeddings \ + -H "Authorization: Bearer sk-emb-REPLACE_WITH_YOUR_KEY" \ + -H "Content-Type: application/json" \ + -d '{"model":"intfloat/multilingual-e5-small","input":"query: hello"}' \ + | python3 -c 'import sys,json;print("dims:",len(json.load(sys.stdin)["data"][0]["embedding"]))' +# -> dims: 384 +``` + +### Заметки про сервер эмбеддингов + +- **Размерность вектора — 384.** Если раньше этот Gitmost эмбеддился другой моделью + (например, `text-embedding-3-large` = 3072-dim), старые строки в pgvector не совпадут по + размерности — очистите существующие эмбеддинги / переиндексируйте перед переключением. Gitmost + сравнивает только вектора одной размерности, поэтому строки другой размерности не участвуют в + поиске, а не ломают его. +- **Первый старт тянет веса** (сотни МБ) с `huggingface.co` в том `tei-models`; дальше — из тома. +- **Пин версии.** Пиньте образ, а при желании и модель: добавьте в `command` `--revision ` + (sha берётся со страницы модели на Hugging Face). +- **Без egress (air-gapped):** засейте том `tei-models` заранее и добавьте + `environment: [HF_HUB_OFFLINE=1]`. +- **GPU:** возьмите cuda-тег того же релиза (например, + `ghcr.io/huggingface/text-embeddings-inference:cuda-1.9`) и запустите контейнер с `gpus: all`. + + ## Возможности - Совместная работа в реальном времени diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 837217a3..9b57f724 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -1418,5 +1418,14 @@ "The commented text changed since this suggestion was made; it was not applied.": "The commented text changed since this suggestion was made; it was not applied.", "Dismiss": "Dismiss", "Suggestion dismissed": "Suggestion dismissed", - "Failed to dismiss suggestion": "Failed to dismiss suggestion" + "Failed to dismiss suggestion": "Failed to dismiss suggestion", + "Save version": "Save version", + "Ctrl+S": "Ctrl+S", + "Version saved": "Version saved", + "Already saved as the latest version": "Already saved as the latest version", + "Agent version": "Agent version", + "Boundary": "Boundary", + "Autosave": "Autosave", + "Only versions": "Only versions", + "No saved versions yet.": "No saved versions yet." } diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 9723e108..e22d8746 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -1433,5 +1433,14 @@ "The commented text changed since this suggestion was made; it was not applied.": "Прокомментированный текст изменился после создания предложения; оно не было применено.", "Dismiss": "Не применять", "Suggestion dismissed": "Предложение отклонено", - "Failed to dismiss suggestion": "Не удалось отклонить предложение" + "Failed to dismiss suggestion": "Не удалось отклонить предложение", + "Save version": "Сохранить версию", + "Ctrl+S": "Ctrl+S", + "Version saved": "Версия сохранена", + "Already saved as the latest version": "Уже сохранено как последняя версия", + "Agent version": "Версия агента", + "Boundary": "Граница", + "Autosave": "Автосейв", + "Only versions": "Только версии", + "No saved versions yet.": "Пока нет сохранённых версий." } diff --git a/apps/client/src/features/editor/atoms/editor-atoms.ts b/apps/client/src/features/editor/atoms/editor-atoms.ts index d39a2395..3e1f19fa 100644 --- a/apps/client/src/features/editor/atoms/editor-atoms.ts +++ b/apps/client/src/features/editor/atoms/editor-atoms.ts @@ -3,11 +3,20 @@ import { atom } from "jotai"; // import would drag the whole @tiptap/core engine into the eager graph of every // shell component that reads one of these atoms. import type { Editor } from "@tiptap/core"; +import type { HocuspocusProvider } from "@hocuspocus/provider"; import { PageEditMode } from "@/features/user/types/user.types.ts"; import type { DictationUnavailableReason } from "@/features/dictation/dictation-status"; export const pageEditorAtom = atom(null); +// #370 — the active page's collab provider, published by the page editor so the +// header menu can emit the "save-version" stateless signal (Cmd+S / button). +// Null when the page is read-only / collab isn't connected. A typed initial +// value (rather than an explicit generic) keeps jotai's overload resolution on +// the writable PrimitiveAtom branch. +const initialCollabProvider: HocuspocusProvider | null = null; +export const collabProviderAtom = atom(initialCollabProvider); + export const titleEditorAtom = atom(null); export const readOnlyEditorAtom = atom(null); diff --git a/apps/client/src/features/editor/page-editor.tsx b/apps/client/src/features/editor/page-editor.tsx index 82744161..676ec972 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/apps/client/src/features/editor/page-editor.tsx @@ -31,11 +31,18 @@ import { useAtom, useAtomValue, useSetAtom } from "jotai"; import useCollaborationUrl from "@/features/editor/hooks/use-collaboration-url"; import { currentUserAtom } from "@/features/user/atoms/current-user-atom"; import { + collabProviderAtom, currentPageEditModeAtom, dictationAvailabilityAtom, pageEditorAtom, yjsConnectionStatusAtom, } from "@/features/editor/atoms/editor-atoms"; +import { notifications } from "@mantine/notifications"; +import { + VERSION_SAVED_MESSAGE_TYPE, + type VersionSavedMessage, + saveVersionPending, +} from "@/features/page-history/version-messages"; import { asideStateAtom } from "@/components/layouts/global/hooks/atoms/sidebar-atom"; import { activeCommentIdAtom, @@ -124,6 +131,7 @@ export default function PageEditor({ const [currentUser] = useAtom(currentUserAtom); const [, setEditor] = useAtom(pageEditorAtom); + const setCollabProvider = useSetAtom(collabProviderAtom); const [, setAsideState] = useAtom(asideStateAtom); const [, setActiveCommentId] = useAtom(activeCommentIdAtom); const [showCommentPopup, setShowCommentPopup] = useAtom(showCommentPopupAtom); @@ -181,6 +189,24 @@ export default function PageEditor({ const onStatelessHandler = ({ payload }: onStatelessParameters) => { try { const message = JSON.parse(payload); + // #370 — a version was saved somewhere; live-refresh the history panel + // on every client. Only the client that pressed Save (tracked by the + // module-level flag) shows the confirmation toast. + if (message?.type === VERSION_SAVED_MESSAGE_TYPE) { + const versionMsg = message as VersionSavedMessage; + queryClient.invalidateQueries({ + queryKey: ["page-history-list"], + }); + if (saveVersionPending.current) { + saveVersionPending.current = false; + notifications.show({ + message: versionMsg.alreadySaved + ? t("Already saved as the latest version") + : t("Version saved"), + }); + } + return; + } if (message?.type !== "page.updated" || !message.updatedAt) return; const pageData = queryClient.getQueryData(["pages", slugId]); if (pageData) { @@ -238,12 +264,16 @@ export default function PageEditor({ local.on("synced", onLocalSyncedHandler); providersRef.current = { socket, local, remote }; + // #370 — publish the provider so the header menu can emit save-version. + setCollabProvider(remote); setProvidersReady(true); } else { + setCollabProvider(providersRef.current.remote); setProvidersReady(true); } // Only destroy on final unmount return () => { + setCollabProvider(null); providersRef.current?.socket.destroy(); providersRef.current?.remote.destroy(); providersRef.current?.local.destroy(); diff --git a/apps/client/src/features/page-history/components/history-item.tsx b/apps/client/src/features/page-history/components/history-item.tsx index bc810eca..aa76b4f2 100644 --- a/apps/client/src/features/page-history/components/history-item.tsx +++ b/apps/client/src/features/page-history/components/history-item.tsx @@ -1,4 +1,11 @@ -import { Text, Group, UnstyledButton, Avatar, Tooltip } from "@mantine/core"; +import { + Text, + Group, + UnstyledButton, + Avatar, + Tooltip, + Badge, +} from "@mantine/core"; import { CustomAvatar } from "@/components/ui/custom-avatar.tsx"; import { AgentAvatarStack } from "@/components/ui/agent-avatar-stack.tsx"; import { formattedDate } from "@/lib/time"; @@ -7,36 +14,59 @@ import clsx from "clsx"; import { IPageHistory } from "@/features/page-history/types/page.types"; import { memo, useCallback } from "react"; import { useSetAtom } from "jotai"; +import { useTranslation } from "react-i18next"; import { historyAtoms } from "@/features/page-history/atoms/history-atoms.ts"; const MAX_VISIBLE_AVATARS = 5; +/** + * #370 — map a snapshot's intentionality tier to its badge. `version: true` + * marks the intentional points (manual / agent); autosaves (boundary / idle / + * legacy null) are non-versions and get dimmed in the list. + */ +type HistoryKindMeta = { labelKey: string; color: string; version: boolean }; +export function historyKindMeta(kind?: string | null): HistoryKindMeta { + switch (kind) { + case "manual": + return { labelKey: "Saved", color: "blue", version: true }; + case "agent": + return { labelKey: "Agent version", color: "violet", version: true }; + case "boundary": + return { labelKey: "Boundary", color: "gray", version: false }; + default: // "idle" | null | undefined (legacy autosave) + return { labelKey: "Autosave", color: "gray", version: false }; + } +} + interface HistoryItemProps { historyItem: IPageHistory; - index: number; - onSelect: (id: string, index: number) => void; - onHover?: (id: string, index: number) => void; + // The previous snapshot for diff/restore is resolved by id from the FULL list + // in the parent (resolvePrevSnapshotId), so the item only needs to report its + // own id — never a list index (which would be the filtered-view index). + onSelect: (id: string) => void; + onHover?: (id: string) => void; onHoverEnd?: () => void; isActive: boolean; } const HistoryItem = memo(function HistoryItem({ historyItem, - index, onSelect, onHover, onHoverEnd, isActive, }: HistoryItemProps) { const setHistoryModalOpen = useSetAtom(historyAtoms); + const { t } = useTranslation(); + const kindMeta = historyKindMeta(historyItem.kind); const handleClick = useCallback(() => { - onSelect(historyItem.id, index); - }, [onSelect, historyItem.id, index]); + onSelect(historyItem.id); + }, [onSelect, historyItem.id]); const handleMouseEnter = useCallback(() => { - onHover?.(historyItem.id, index); - }, [onHover, historyItem.id, index]); + onHover?.(historyItem.id); + }, [onHover, historyItem.id]); const contributors = historyItem.contributors; const hasContributors = contributors && contributors.length > 0; @@ -49,8 +79,20 @@ const HistoryItem = memo(function HistoryItem({ onMouseEnter={handleMouseEnter} onMouseLeave={onHoverEnd} className={clsx(classes.history, { [classes.active]: isActive })} + // #370 — dim autosnapshots so intentional versions stand out. + style={{ opacity: kindMeta.version ? 1 : 0.55 }} > - {formattedDate(new Date(historyItem.createdAt))} + + {formattedDate(new Date(historyItem.createdAt))} + + {t(kindMeta.labelKey)} + + {hasContributors ? ( diff --git a/apps/client/src/features/page-history/components/history-list.tsx b/apps/client/src/features/page-history/components/history-list.tsx index 4024901b..01143beb 100644 --- a/apps/client/src/features/page-history/components/history-list.tsx +++ b/apps/client/src/features/page-history/components/history-list.tsx @@ -2,14 +2,16 @@ import { usePageHistoryListQuery, prefetchPageHistory, } from "@/features/page-history/queries/page-history-query"; -import HistoryItem from "@/features/page-history/components/history-item"; +import HistoryItem, { + historyKindMeta, +} from "@/features/page-history/components/history-item"; import { activeHistoryIdAtom, activeHistoryPrevIdAtom, historyAtoms, } from "@/features/page-history/atoms/history-atoms"; import { useAtom, useSetAtom } from "jotai"; -import { useCallback, useEffect, useMemo, useRef } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { Button, ScrollArea, @@ -17,9 +19,12 @@ import { Divider, Loader, Center, + Switch, + Text, } from "@mantine/core"; import { useTranslation } from "react-i18next"; import { useHistoryRestore } from "@/features/page-history/hooks"; +import { resolvePrevSnapshotId } from "@/features/page-history/utils/resolve-prev-snapshot"; const PREFETCH_DELAY_MS = 150; @@ -47,6 +52,22 @@ function HistoryList({ pageId }: Props) { [pageHistoryData], ); + // #370 — "only versions" filter: hide autosnapshots (idle/boundary/legacy + // null), keep only intentional points (manual/agent). Filtering is over the + // already-loaded pages; the diff/restore still targets the true previous + // snapshot, so items carry their index within the FULL list. + const [onlyVersions, setOnlyVersions] = useState(false); + // Reuse historyKindMeta().version — the SAME predicate the badge (HistoryItem) + // uses to mark intentional points — so the "Only versions" filter and the badge + // can never drift apart when a future intentional kind is added. + const visibleItems = useMemo( + () => + onlyVersions + ? historyItems.filter((item) => historyKindMeta(item.kind).version) + : historyItems, + [historyItems, onlyVersions], + ); + const loadMoreRef = useRef(null); const prefetchTimeoutRef = useRef | null>(null); @@ -60,11 +81,13 @@ function HistoryList({ pageId }: Props) { }, []); const handleHover = useCallback( - (historyId: string, index: number) => { + (historyId: string) => { clearPrefetchTimeout(); prefetchTimeoutRef.current = setTimeout(() => { prefetchPageHistory(historyId); - const prevId = historyItems[index + 1]?.id; + // The true previous snapshot in the FULL list (not the previous visible + // one under the "only versions" filter). + const prevId = resolvePrevSnapshotId(historyItems, historyId); if (prevId) { prefetchPageHistory(prevId); } @@ -78,9 +101,11 @@ function HistoryList({ pageId }: Props) { }, [clearPrefetchTimeout]); const handleSelect = useCallback( - (id: string, index: number) => { + (id: string) => { setActiveHistoryId(id); - setActiveHistoryPrevId(historyItems[index + 1]?.id ?? ""); + // Baseline = true previous snapshot in the FULL list, so the "only + // versions" filter never diffs/restores against the wrong item. + setActiveHistoryPrevId(resolvePrevSnapshotId(historyItems, id)); }, [historyItems, setActiveHistoryId, setActiveHistoryPrevId], ); @@ -128,12 +153,27 @@ function HistoryList({ pageId }: Props) { return (
+ + setOnlyVersions(e.currentTarget.checked)} + label={t("Only versions")} + /> + + - {historyItems.map((historyItem, index) => ( + {onlyVersions && visibleItems.length === 0 && ( +
+ + {t("No saved versions yet.")} + +
+ )} + {visibleItems.map((historyItem) => ( { + // Newest-first, as the history list stores it: a version, then two autosaves, + // then an older version. + const full = [ + { id: "v2", kind: "manual" }, + { id: "a2", kind: "idle" }, + { id: "a1", kind: "boundary" }, + { id: "v1", kind: "manual" }, + { id: "a0", kind: null }, + ]; + + it("returns the immediate FULL-list successor, not the previous visible version", () => { + // Selecting v2 while filtered to versions-only must baseline against a2 (the + // real chronological predecessor), NOT v1 (the previous visible version). + expect(resolvePrevSnapshotId(full, "v2")).toBe("a2"); + }); + + it("resolves an autosnapshot's predecessor by full-list order", () => { + expect(resolvePrevSnapshotId(full, "a1")).toBe("v1"); + }); + + it("returns '' for the oldest item (no predecessor)", () => { + expect(resolvePrevSnapshotId(full, "a0")).toBe(""); + }); + + it("returns '' for an id not in the list", () => { + expect(resolvePrevSnapshotId(full, "missing")).toBe(""); + }); + + it("does not depend on a filtered subset — same result whatever is visible", () => { + // The helper only ever sees the full list; a filtered view cannot change the + // baseline it computes. + expect(resolvePrevSnapshotId(full, "v1")).toBe("a0"); + }); +}); diff --git a/apps/client/src/features/page-history/utils/resolve-prev-snapshot.ts b/apps/client/src/features/page-history/utils/resolve-prev-snapshot.ts new file mode 100644 index 00000000..73320cd2 --- /dev/null +++ b/apps/client/src/features/page-history/utils/resolve-prev-snapshot.ts @@ -0,0 +1,22 @@ +/** + * #370 — resolve the TRUE previous snapshot for a history item. + * + * The history panel can be filtered to "only versions" (manual/agent), but diff + * and restore must always compare against the immediately-preceding snapshot in + * the FULL, unfiltered list — NOT the previous VISIBLE item. Comparing against + * the previous visible version would silently skip the autosnapshots between two + * versions and diff/restore the wrong baseline. + * + * Given the full (newest-first) list and an item id, this returns the id of the + * item right after it in the full list (its chronological predecessor), or "" if + * it is the oldest / not found. Pure and list-order-preserving so it can be unit + * tested without mounting the component. + */ +export function resolvePrevSnapshotId( + fullItems: ReadonlyArray<{ id: string }>, + id: string, +): string { + const index = fullItems.findIndex((item) => item.id === id); + if (index === -1) return ""; + return fullItems[index + 1]?.id ?? ""; +} diff --git a/apps/client/src/features/page-history/version-messages.ts b/apps/client/src/features/page-history/version-messages.ts new file mode 100644 index 00000000..1e944397 --- /dev/null +++ b/apps/client/src/features/page-history/version-messages.ts @@ -0,0 +1,28 @@ +/** + * #370 — page-version stateless wire formats. Kept in one place so the client + * emitter (Save hotkey / button) and the client listener (page-editor) agree + * with the server (PersistenceExtension) on the message shapes. + */ + +/** Client → server: "save a version now". The server derives the tier + * (manual/agent) from the signed connection actor, never from this payload. */ +export const SAVE_VERSION_MESSAGE_TYPE = "save-version"; + +/** Server → all clients: a version was saved (or promoted / already existed). */ +export const VERSION_SAVED_MESSAGE_TYPE = "version.saved"; + +export interface VersionSavedMessage { + type: typeof VERSION_SAVED_MESSAGE_TYPE; + historyId: string; + kind: "manual" | "agent"; + /** True when the latest snapshot was already a manual version (a no-op save). */ + alreadySaved: boolean; +} + +/** + * Cross-component coordination flag so only the client that pressed Save shows + * the confirmation toast, while every other client silently refreshes its + * history panel on the broadcast. A module-level ref avoids stale-closure + * pitfalls in the editor's long-lived stateless handler. + */ +export const saveVersionPending = { current: false }; diff --git a/apps/client/src/features/page/components/header/page-header-menu.tsx b/apps/client/src/features/page/components/header/page-header-menu.tsx index 5b1b38ec..844ef09a 100644 --- a/apps/client/src/features/page/components/header/page-header-menu.tsx +++ b/apps/client/src/features/page/components/header/page-header-menu.tsx @@ -3,6 +3,7 @@ import { IconArrowRight, IconArrowsHorizontal, IconClockHour4, + IconDeviceFloppy, IconDots, IconEye, IconEyeOff, @@ -17,7 +18,7 @@ import { IconTrash, IconWifiOff, } from "@tabler/icons-react"; -import React, { useEffect, useRef, useState } from "react"; +import React, { useCallback, useEffect, useRef, useState } from "react"; import { useAsideTriggerProps } from "@/hooks/use-toggle-aside.tsx"; import { useAtom, useAtomValue } from "jotai"; import { historyAtoms } from "@/features/page-history/atoms/history-atoms.ts"; @@ -39,9 +40,14 @@ import { Trans, useTranslation } from "react-i18next"; import ExportModal from "@/components/common/export-modal"; import { convertProseMirrorToMarkdown } from "@docmost/prosemirror-markdown/browser"; import { + collabProviderAtom, pageEditorAtom, yjsConnectionStatusAtom, } from "@/features/editor/atoms/editor-atoms.ts"; +import { + SAVE_VERSION_MESSAGE_TYPE, + saveVersionPending, +} from "@/features/page-history/version-messages.ts"; import { formattedDate } from "@/lib/time.ts"; import { PageEditModeToggle } from "@/features/user/components/page-state-pref.tsx"; import MovePageModal from "@/features/page/components/move-page-modal.tsx"; @@ -72,9 +78,34 @@ export default function PageHeaderMenu({ readOnly }: PageHeaderMenuProps) { }); const isDeleted = !!page?.deletedAt; const [workspace] = useAtom(workspaceAtom); + const collabProvider = useAtomValue(collabProviderAtom); // Community public-sharing entry point (replaces the removed EE PageShareModal) const workspaceSharingDisabled = workspace?.settings?.sharing?.disabled === true; + // #370 — explicit "save a version" (Cmd+S / Save button). One path for the + // human; the server derives the tier from the signed actor. Readers can't save + // (the button is hidden and the collab connection is read-only server-side). + const handleSaveVersion = useCallback(() => { + if (readOnly || !collabProvider) return; + // Flag this client as the initiator so only it shows the confirmation toast; + // a safety timeout clears it if no broadcast comes back (e.g. offline). + saveVersionPending.current = true; + window.setTimeout(() => { + saveVersionPending.current = false; + }, 5000); + collabProvider.sendStateless( + JSON.stringify({ type: SAVE_VERSION_MESSAGE_TYPE }), + ); + }, [readOnly, collabProvider]); + + // mod+S must also block the browser's "Save page" dialog. `triggerOnContent- + // Editable` + empty ignore-list so it fires while typing in the editor/title. + useHotkeys( + [["mod+S", handleSaveVersion, { preventDefault: true }]], + [], + true, + ); + useHotkeys( [ [ @@ -133,15 +164,16 @@ export default function PageHeaderMenu({ readOnly }: PageHeaderMenuProps) { - + ); } interface PageActionMenuProps { readOnly?: boolean; + onSaveVersion?: () => void; } -function PageActionMenu({ readOnly }: PageActionMenuProps) { +function PageActionMenu({ readOnly, onSaveVersion }: PageActionMenuProps) { const { t } = useTranslation(); const [, setHistoryModalOpen] = useAtom(historyAtoms); const clipboard = useClipboard({ timeout: 500 }); @@ -303,6 +335,20 @@ function PageActionMenu({ readOnly }: PageActionMenuProps) { + {!readOnly && ( + } + onClick={onSaveVersion} + rightSection={ + + {t("Ctrl+S")} + + } + > + {t("Save version")} + + )} + } onClick={openHistoryModal} 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/constants.ts b/apps/server/src/collaboration/constants.ts index 9401d973..d6c49255 100644 --- a/apps/server/src/collaboration/constants.ts +++ b/apps/server/src/collaboration/constants.ts @@ -1,10 +1,36 @@ -export const HISTORY_INTERVAL = 5 * 60 * 1000; -export const HISTORY_FAST_INTERVAL = 60 * 1000; -export const HISTORY_FAST_THRESHOLD = 5 * 60 * 1000; - // #348 — debounce window for the per-page RAG re-embed job. Repeated saves // within this window collapse to a single delayed job (coalesced by a stable // jobId), so active editing does not pile up expensive re-embeds (external API // + page_embeddings rewrite, concurrency 1). The worker reads the CURRENT page // state at run time, so the last content within the window wins. export const EMBED_DEBOUNCE_MS = 30 * 1000; + +/** + * #370 — page-history intentionality tiers. Domain of `page_history.kind`. + * - 'manual' / 'agent' → Tier 1 versions (intentional points) + * - 'idle' / 'boundary' → Tier 0 autosnapshots (safety net) + * A legacy `null` kind is treated as an autosave. + */ +export type PageHistoryKind = 'manual' | 'agent' | 'idle' | 'boundary'; + +/** + * #370 — trailing idle-flush windows. A page's pending idle snapshot is + * re-armed on every store and fires this long after edits go quiet, so a burst + * of edits collapses into a single autosnapshot instead of one-per-store. Human + * sessions are noisier and less risky, so they flush less often than the agent. + */ +export const IDLE_INTERVAL_USER = 60 * 60 * 1000; // 60m +export const IDLE_INTERVAL_AGENT = 15 * 60 * 1000; // 15m + +/** + * #370 — max-wait ceiling for the idle flush. Pure trailing debounce starves the + * safety net: hocuspocus stores at least every ~45s, so a CONTINUOUS editing + * session would re-arm the trailing timer forever and never take an idle + * snapshot until edits finally go quiet (up to IDLE_INTERVAL_USER = 60m). This + * ceiling bounds the actual wait from the FIRST edit of a burst, so an idle + * snapshot fires at least this often during a long unbroken session — restoring + * a recovery point cadence closer to the old heuristic without one-per-store + * noise. Mirrors hocuspocus's own maxDebounce idea. + */ +export const IDLE_MAX_WAIT_USER = 10 * 60 * 1000; // 10m +export const IDLE_MAX_WAIT_AGENT = 5 * 60 * 1000; // 5m 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/collaboration/extensions/compute-history-job.spec.ts b/apps/server/src/collaboration/extensions/compute-history-job.spec.ts index aa21f14f..be42040f 100644 --- a/apps/server/src/collaboration/extensions/compute-history-job.spec.ts +++ b/apps/server/src/collaboration/extensions/compute-history-job.spec.ts @@ -1,84 +1,93 @@ +import { computeHistoryJob, resolveSource } from './persistence.extension'; import { - computeHistoryJob, - resolveSource, -} from './persistence.extension'; -import { - HISTORY_FAST_INTERVAL, - HISTORY_FAST_THRESHOLD, - HISTORY_INTERVAL, + IDLE_INTERVAL_AGENT, + IDLE_INTERVAL_USER, + IDLE_MAX_WAIT_AGENT, + IDLE_MAX_WAIT_USER, } from '../constants'; -// A fixed clock + fixed createdAt make pageAge deterministic. -const NOW = 1_700_000_000_000; const PAGE_ID = '550e8400-e29b-41d4-a716-446655440000'; -// Build a minimal page whose age (NOW - createdAt) is exactly `ageMs`. -const pageAged = (ageMs: number) => ({ - id: PAGE_ID, - createdAt: new Date(NOW - ageMs), -}); +const page = { id: PAGE_ID }; -describe('computeHistoryJob', () => { - it('agent edit → delay MUST be 0 and job id is source-keyed', () => { - // INVARIANT (§15 H2 / persistence.extension): the agent delay MUST stay 0. - // The worker re-reads the page row at run time, so any non-zero delay risks - // snapshotting content a later human edit has already overwritten. This is - // the load-bearing assertion of this spec — do not relax it. - const { jobId, delay } = computeHistoryJob(pageAged(0), 'agent', NOW); - expect(delay).toBe(0); - expect(jobId).toBe(`${PAGE_ID}-agent`); - }); - - it('agent edit on an OLD page is still delay 0 (age never applies to agents)', () => { - // Even when the page is far older than the fast threshold, the agent path - // must short-circuit to 0 — age-based debounce is a human-only concern. - const { jobId, delay } = computeHistoryJob( - pageAged(HISTORY_FAST_THRESHOLD + 60_000), - 'agent', - NOW, - ); - expect(delay).toBe(0); - expect(jobId).toBe(`${PAGE_ID}-agent`); - }); - - it('human edit on a YOUNG page (age < threshold) → fast interval, bare job id', () => { - const { jobId, delay } = computeHistoryJob( - pageAged(HISTORY_FAST_THRESHOLD - 1), - 'user', - NOW, - ); - expect(delay).toBe(HISTORY_FAST_INTERVAL); +describe('computeHistoryJob (#370 — shared trailing idle pipeline)', () => { + it('human edit → user idle window, bare page.id job', () => { + // Humans and the agent now share ONE idle job per page (jobId = page.id). + // The agent's old delay=0 fast path is GONE — intentional agent points now + // arrive via the explicit save-version signal, not a zero-delay snapshot. + const { jobId, delay } = computeHistoryJob(page, 'user'); + expect(delay).toBe(IDLE_INTERVAL_USER); expect(jobId).toBe(PAGE_ID); }); - it('human edit on an OLD page (age > threshold) → standard interval', () => { - const { jobId, delay } = computeHistoryJob( - pageAged(HISTORY_FAST_THRESHOLD + 1), - 'user', - NOW, - ); - expect(delay).toBe(HISTORY_INTERVAL); + it('agent edit → agent idle window (shorter), still the bare page.id job', () => { + const { jobId, delay } = computeHistoryJob(page, 'agent'); + expect(delay).toBe(IDLE_INTERVAL_AGENT); + // No `-agent` suffix anymore: the agent joins the common idle pipeline. expect(jobId).toBe(PAGE_ID); }); - it('boundary: pageAge EXACTLY === threshold takes the slow branch (the `<` is strict)', () => { - // Off-by-one guard: the condition is `pageAge < HISTORY_FAST_THRESHOLD`, so - // an age of exactly the threshold is NOT "fast" — it must use HISTORY_INTERVAL. - const { delay } = computeHistoryJob( - pageAged(HISTORY_FAST_THRESHOLD), - 'user', - NOW, - ); - expect(delay).toBe(HISTORY_INTERVAL); + it('agent flushes sooner than a human', () => { + expect(IDLE_INTERVAL_AGENT).toBeLessThan(IDLE_INTERVAL_USER); }); - it('treats any non-"agent" source string as human', () => { - // resolveSource only ever yields 'agent' | 'user', but guard the contract: - // the agent branch keys strictly on === 'agent'. - const { jobId, delay } = computeHistoryJob(pageAged(0), 'user', NOW); - expect(delay).toBe(HISTORY_FAST_INTERVAL); + it('treats any non-"agent" source string as human (keys strictly on === agent)', () => { + const { jobId, delay } = computeHistoryJob(page, 'user'); + expect(delay).toBe(IDLE_INTERVAL_USER); expect(jobId).toBe(PAGE_ID); }); + + // #370 review round-1 WARNING: the max-wait ceiling prevents autosnapshot + // starvation during a continuous editing session (the trailing timer would + // otherwise re-arm forever and never fire). + describe('max-wait ceiling', () => { + const T0 = 1_000_000; // arbitrary fixed epoch for deterministic tests + + it('once a burst is armed, delay clamps to the remaining max-wait budget', () => { + // 1 minute into the burst the USER interval (60m) far exceeds the remaining + // max-wait budget (10m - 1m = 9m), so the delay is clamped DOWN to that + // remaining budget — the full interval is NOT used once a ceiling applies. + const { delay } = computeHistoryJob(page, 'user', T0, T0 + 60_000); + expect(delay).toBe(IDLE_MAX_WAIT_USER - 60_000); + }); + + it('never waits longer than the max-wait budget from the burst start', () => { + // A store arriving right at the ceiling → delay 0 (fire promptly). + const { delay } = computeHistoryJob( + page, + 'user', + T0, + T0 + IDLE_MAX_WAIT_USER, + ); + expect(delay).toBe(0); + }); + + it('past the ceiling never returns a negative delay', () => { + const { delay } = computeHistoryJob( + page, + 'user', + T0, + T0 + IDLE_MAX_WAIT_USER + 5 * 60_000, + ); + expect(delay).toBe(0); + }); + + it('the agent ceiling is shorter than the user ceiling', () => { + expect(IDLE_MAX_WAIT_AGENT).toBeLessThan(IDLE_MAX_WAIT_USER); + const { delay } = computeHistoryJob( + page, + 'agent', + T0, + T0 + IDLE_MAX_WAIT_AGENT, + ); + expect(delay).toBe(0); + }); + + it('without a burstStart there is no ceiling (backward-compatible)', () => { + expect(computeHistoryJob(page, 'user').delay).toBe(IDLE_INTERVAL_USER); + expect(computeHistoryJob(page, 'agent').delay).toBe(IDLE_INTERVAL_AGENT); + }); + }); }); describe('resolveSource (truth table)', () => { diff --git a/apps/server/src/collaboration/extensions/persistence-store.spec.ts b/apps/server/src/collaboration/extensions/persistence-store.spec.ts index 122356bb..c6ed09eb 100644 --- a/apps/server/src/collaboration/extensions/persistence-store.spec.ts +++ b/apps/server/src/collaboration/extensions/persistence-store.spec.ts @@ -40,11 +40,12 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' let pageHistoryRepo: { saveHistory: jest.Mock; findPageLastHistory: jest.Mock; + updateHistoryKind: jest.Mock; }; let aiQueue: { add: jest.Mock }; - let historyQueue: { add: jest.Mock }; + let historyQueue: { add: jest.Mock; remove: jest.Mock }; let notificationQueue: { add: jest.Mock }; - let collabHistory: { addContributors: jest.Mock }; + let collabHistory: { addContributors: jest.Mock; popContributors: jest.Mock }; let transclusionService: { syncPageTransclusions: jest.Mock; syncPageReferences: jest.Mock; @@ -93,13 +94,22 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' pageHistoryRepo = { saveHistory: jest.fn().mockImplementation(async () => { callOrder.push('saveHistory'); + return { id: 'history-1' }; }), findPageLastHistory: jest.fn().mockResolvedValue(null), + updateHistoryKind: jest.fn().mockResolvedValue(undefined), }; aiQueue = { add: jest.fn().mockResolvedValue(undefined) }; - historyQueue = { add: jest.fn().mockResolvedValue(undefined) }; + historyQueue = { + add: jest.fn().mockResolvedValue(undefined), + // #370 — enqueuePageHistory now removes any pending idle job before re-adding. + remove: jest.fn().mockResolvedValue(undefined), + }; notificationQueue = { add: jest.fn().mockResolvedValue(undefined) }; - collabHistory = { addContributors: jest.fn().mockResolvedValue(undefined) }; + collabHistory = { + addContributors: jest.fn().mockResolvedValue(undefined), + popContributors: jest.fn().mockResolvedValue([]), + }; transclusionService = { syncPageTransclusions: jest.fn().mockResolvedValue(undefined), syncPageReferences: jest.fn().mockResolvedValue(undefined), @@ -165,6 +175,50 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' expect(pageRepo.updatePage.mock.calls[0][0].lastUpdatedSource).toBe('user'); }); + // #370 review round-1 SUGGESTION: the boundary was GENERALIZED from a + // user→agent special-case to ANY lastUpdatedSource transition. These pin the + // generalized behaviour it was rebuilt for. + describe('generalized boundary — any source transition', () => { + // Same persisted page but with an explicit prior source. + const pageWithPriorSource = (prior: string | null) => ({ + ...persistedHumanPage('NEW CONTENT'), + lastUpdatedSource: prior, + }); + + it('agent→user transition fires the boundary (pins the prior agent revision)', async () => { + const document = ydocFor(doc('NEW CONTENT')); + pageRepo.findById.mockResolvedValue(pageWithPriorSource('agent')); + pageHistoryRepo.findPageLastHistory.mockResolvedValue(null); + + await ext.onStoreDocument(buildData(document, 'user') as any); + + expect(pageHistoryRepo.saveHistory).toHaveBeenCalledTimes(1); + expect(callOrder).toEqual(['saveHistory', 'updatePage']); + expect(pageRepo.updatePage.mock.calls[0][0].lastUpdatedSource).toBe('user'); + }); + + it('git→user transition fires the boundary (git-sync overwrite is a source change)', async () => { + const document = ydocFor(doc('NEW CONTENT')); + pageRepo.findById.mockResolvedValue(pageWithPriorSource('git')); + pageHistoryRepo.findPageLastHistory.mockResolvedValue(null); + + await ext.onStoreDocument(buildData(document, 'user') as any); + + expect(pageHistoryRepo.saveHistory).toHaveBeenCalledTimes(1); + expect(callOrder).toEqual(['saveHistory', 'updatePage']); + }); + + it('a null prior source (first-ever edit) does NOT fire the boundary', async () => { + const document = ydocFor(doc('NEW CONTENT')); + pageRepo.findById.mockResolvedValue(pageWithPriorSource(null)); + + await ext.onStoreDocument(buildData(document, 'agent') as any); + + expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); + expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); + }); + }); + it('idempotency: unchanged content → no updatePage, no history, no queues', async () => { // The Y.Doc content equals the persisted content deeply → early skip. // A Y.Doc round-trip normalizes attrs (e.g. paragraph indent), so derive @@ -479,4 +533,231 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' // Contributors keyed by the UUID so they match the PAGE_HISTORY job (page.id). expect(collabHistory.addContributors.mock.calls[0][0]).toBe(PAGE_ID); }); + + // #370 — explicit save-version (Cmd+S / agent save tool) over the stateless + // seam. The tier is derived from the SIGNED connection actor, the store path + // is reused, and promote-not-dup avoids duplicating heavy content rows. + describe('save-version (#370)', () => { + const emitSave = (document: any, actor: 'user' | 'agent') => + ext.onStateless({ + connection: { + readOnly: false, + context: { user: { id: USER_ID, name: 'Alice' }, actor }, + } as any, + documentName: `page.${PAGE_ID}`, + document: document as any, + payload: JSON.stringify({ type: 'save-version' }), + } as any); + + // findById returns a page whose content already equals the live doc, so the + // store path is a no-op and we isolate the versioning decision. + const pageMatchingDoc = (document: any) => ({ + ...persistedHumanPage('IGNORED'), + content: TiptapTransformer.fromYdoc(document, 'default'), + }); + + it('human save with no prior snapshot → writes a manual version + broadcasts', async () => { + const document = ydocFor(doc('VERSION ME')); + pageRepo.findById.mockResolvedValue(pageMatchingDoc(document)); + pageHistoryRepo.findPageLastHistory.mockResolvedValue(null); + + await emitSave(document, 'user'); + + expect(pageHistoryRepo.saveHistory).toHaveBeenCalledTimes(1); + expect(pageHistoryRepo.saveHistory.mock.calls[0][1]).toEqual( + expect.objectContaining({ kind: 'manual' }), + ); + // The pending idle autosnapshot is cancelled by the explicit version. + expect(historyQueue.remove).toHaveBeenCalledWith(PAGE_ID); + const msg = JSON.parse( + (document as any).broadcastStateless.mock.calls[(document as any).broadcastStateless.mock.calls.length - 1][0], + ); + expect(msg).toMatchObject({ + type: 'version.saved', + kind: 'manual', + alreadySaved: false, + }); + }); + + it('agent save derives kind=agent from the signed actor', async () => { + const document = ydocFor(doc('AGENT VERSION')); + pageRepo.findById.mockResolvedValue(pageMatchingDoc(document)); + pageHistoryRepo.findPageLastHistory.mockResolvedValue(null); + + await emitSave(document, 'agent'); + + expect(pageHistoryRepo.saveHistory.mock.calls[pageHistoryRepo.saveHistory.mock.calls.length - 1][1]).toEqual( + expect.objectContaining({ kind: 'agent' }), + ); + }); + + it('promote-not-dup: latest snapshot is an autosave with identical content → upgrades in place', async () => { + const document = ydocFor(doc('SAME')); + const page = pageMatchingDoc(document); + pageRepo.findById.mockResolvedValue(page); + pageHistoryRepo.findPageLastHistory.mockResolvedValue({ + id: 'auto-1', + content: page.content, + kind: 'idle', + }); + + await emitSave(document, 'user'); + + // No heavy new content row — the existing autosave is promoted to manual. + expect(pageHistoryRepo.updateHistoryKind).toHaveBeenCalledWith( + 'auto-1', + 'manual', + expect.anything(), + ); + expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); + const msg = JSON.parse( + (document as any).broadcastStateless.mock.calls[(document as any).broadcastStateless.mock.calls.length - 1][0], + ); + expect(msg).toMatchObject({ historyId: 'auto-1', alreadySaved: false }); + }); + + it('no-op when the latest snapshot is already a manual version of this content', async () => { + const document = ydocFor(doc('ALREADY SAVED')); + const page = pageMatchingDoc(document); + pageRepo.findById.mockResolvedValue(page); + pageHistoryRepo.findPageLastHistory.mockResolvedValue({ + id: 'ver-1', + content: page.content, + kind: 'manual', + }); + + await emitSave(document, 'user'); + + expect(pageHistoryRepo.updateHistoryKind).not.toHaveBeenCalled(); + expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); + const msg = JSON.parse( + (document as any).broadcastStateless.mock.calls[(document as any).broadcastStateless.mock.calls.length - 1][0], + ); + expect(msg).toMatchObject({ alreadySaved: true, kind: 'manual' }); + }); + + it('a read-only connection cannot save a version', async () => { + const document = ydocFor(doc('READER')); + pageRepo.findById.mockResolvedValue(pageMatchingDoc(document)); + + await ext.onStateless({ + connection: { + readOnly: true, + context: { user: { id: USER_ID }, actor: 'user' }, + } as any, + documentName: `page.${PAGE_ID}`, + document: document as any, + payload: JSON.stringify({ type: 'save-version' }), + } as any); + + expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); + expect(pageHistoryRepo.updateHistoryKind).not.toHaveBeenCalled(); + }); + + // #370 F8-twin — a COMMIT abort (serialization/deadlock/conn-drop) rejects + // OUTSIDE the tx callback, AFTER the destructive popContributors (SPOP) and + // saveHistory ran but the INSERT rolled back. onStateless has no retry, so + // the outer catch MUST re-add (SADD) the popped set or attribution is lost + // irrecoverably. MUTATION: drop the outer catch → addContributors is never + // called → this reddens. + it('restores popped contributors when the commit aborts after the callback', async () => { + const document = ydocFor(doc('VERSION ME')); + pageRepo.findById.mockResolvedValue(pageMatchingDoc(document)); + // No matching snapshot → fresh version branch → pops contributors. + pageHistoryRepo.findPageLastHistory.mockResolvedValue(null); + collabHistory.popContributors.mockResolvedValue(['u1', 'u2']); + + // A db whose commit REJECTS after the callback body resolved: the SPOP and + // saveHistory already ran, then the tx aborts. onStoreDocument's flush uses + // the same db but its content matches (no-op branch) and its own retry loop + // swallows the throw, so only the versioning tx exercises the restore. + const commitFailingDb = { + transaction: () => ({ + execute: async (fn: (trx: any) => Promise) => { + await fn(trxStub); + throw new Error('commit aborted (serialization_failure)'); + }, + }), + }; + const ext2 = new PersistenceExtension( + pageRepo as any, + pageHistoryRepo as any, + commitFailingDb as any, + aiQueue as any, + historyQueue as any, + notificationQueue as any, + collabHistory as any, + transclusionService as any, + ); + jest.spyOn(ext2['logger'], 'debug').mockImplementation(() => undefined); + jest.spyOn(ext2['logger'], 'warn').mockImplementation(() => undefined); + jest.spyOn(ext2['logger'], 'error').mockImplementation(() => undefined); + + await expect( + ext2.onStateless({ + connection: { + readOnly: false, + context: { user: { id: USER_ID, name: 'Alice' }, actor: 'user' }, + } as any, + documentName: `page.${PAGE_ID}`, + document: document as any, + payload: JSON.stringify({ type: 'save-version' }), + } as any), + ).rejects.toThrow(); + + // Attribution preserved: the popped set is SADD-restored, keyed by the page + // UUID it was popped under. + expect(collabHistory.addContributors).toHaveBeenCalledWith(PAGE_ID, [ + 'u1', + 'u2', + ]); + }); + + // #370 #260 — for a `page.` document the idle job is armed under the + // page UUID (computeHistoryJob's jobId = page.id), so the supersede-remove + // must target page.id, not the raw slugId doc-name id, or it silently misses. + it('cancels the superseded idle job by the page UUID for a slugId doc', async () => { + const SLUG = 'slug-1'; // persistedHumanPage.slugId + const document = ydocFor(doc('VERSION ME')); + pageRepo.findById.mockResolvedValue(pageMatchingDoc(document)); + pageHistoryRepo.findPageLastHistory.mockResolvedValue(null); + + await ext.onStateless({ + connection: { + readOnly: false, + context: { user: { id: USER_ID, name: 'Alice' }, actor: 'user' }, + } as any, + documentName: `page.${SLUG}`, + document: document as any, + payload: JSON.stringify({ type: 'save-version' }), + } as any); + + // remove() keyed by the UUID (the real jobId), never the slugId. + expect(historyQueue.remove).toHaveBeenCalledWith(PAGE_ID); + expect(historyQueue.remove).not.toHaveBeenCalledWith(SLUG); + }); + }); + + // #370 — the in-memory idle-burst marker must be dropped on doc unload (like + // its sibling per-document maps) or it grows unbounded for every page that was + // edited but never manually saved. MUTATION: drop the afterUnloadDocument + // delete → the entry survives → this reddens. + describe('idleBurstStart housekeeping', () => { + it('afterUnloadDocument clears the idle-burst marker armed by a store', async () => { + const document = ydocFor(doc('EDIT')); + pageRepo.findById.mockResolvedValue(persistedHumanPage('EDIT')); + + await ext.onStoreDocument(buildData(document, 'user') as any); + + const map = ext['idleBurstStart'] as Map; + // Keyed by documentName (buildData uses `page.${PAGE_ID}`). + expect(map.has(`page.${PAGE_ID}`)).toBe(true); + + await ext.afterUnloadDocument({ + documentName: `page.${PAGE_ID}`, + } as any); + + expect(map.has(`page.${PAGE_ID}`)).toBe(false); + }); + }); }); diff --git a/apps/server/src/collaboration/extensions/persistence.extension.ts b/apps/server/src/collaboration/extensions/persistence.extension.ts index 666b0c40..49a9fd9e 100644 --- a/apps/server/src/collaboration/extensions/persistence.extension.ts +++ b/apps/server/src/collaboration/extensions/persistence.extension.ts @@ -37,9 +37,11 @@ import { Page } from '@docmost/db/types/entity.types'; import { CollabHistoryService } from '../services/collab-history.service'; import { EMBED_DEBOUNCE_MS, - HISTORY_FAST_INTERVAL, - HISTORY_FAST_THRESHOLD, - HISTORY_INTERVAL, + IDLE_INTERVAL_AGENT, + IDLE_INTERVAL_USER, + IDLE_MAX_WAIT_AGENT, + IDLE_MAX_WAIT_USER, + PageHistoryKind, } from '../constants'; import { TransclusionService } from '../../core/page/transclusion/transclusion.service'; import { @@ -56,6 +58,16 @@ import { hasTransclusionFamilyNodes } from '../../core/page/transclusion/utils/t */ export const INTENTIONAL_CLEAR_MESSAGE_TYPE = 'intentional-clear'; +/** + * #370 — wire format of the client→server "save a version" signal. Sent by the + * human (Cmd+S / Save button) and by the agent's explicit save tool over the + * SAME stateless channel. The intentionality tier ('manual' vs 'agent') is + * derived SERVER-SIDE from the signed connection actor, never from this + * payload, so a version's type is unforgeable. The document is taken from the + * connection (not the payload), so the signal cannot be aimed at another page. + */ +export const SAVE_VERSION_MESSAGE_TYPE = 'save-version'; + /** * #251 — how long an intentional-clear signal stays "pending" before it is * ignored. The signal is set on the clearing keystroke but consumed by the @@ -92,35 +104,39 @@ export function resolveSource( } /** - * Compute the BullMQ job id + delay for a page-history snapshot job. Pure so - * the data-loss-sensitive timing arithmetic is unit-testable; `now` is injected - * (caller passes `Date.now()`) for determinism. + * #370 — compute the BullMQ job id + delay for a page's trailing idle-flush + * autosnapshot. Pure so the timing is unit-testable. * - * - Agent edits: delay 0 and a source-keyed job id `${page.id}-agent`. The - * delay MUST stay 0 — the worker re-reads the page row at run time, so any - * delay risks reading content a later human edit has already overwritten - * (mis-tagged snapshot). 0 minimizes that window. The `-agent` suffix keeps - * the job from coalescing with the bare-page.id human job. - * - Human edits: age-based debounce so rapid human edits coalesce into one - * snapshot; job id is the bare `page.id`. - * - * BullMQ forbids ':' in custom job ids (Redis key separator), so '-' is used; - * page.id is a UUID, so `${page.id}-agent` cannot collide with a human job. + * Both humans and the agent now share ONE idle pipeline (the agent's old + * `delay=0` fast path is gone — intentional agent points arrive via the + * explicit save-version signal instead). The job id is the bare `page.id`, so a + * page has at most one pending idle job; the caller removes-and-re-adds it on + * every store to keep it debounced to the trailing edge of an edit burst. The + * window differs by source only: the agent flushes sooner than a human. */ export function computeHistoryJob( - page: Pick, + page: Pick, source: string, - now: number, + // Epoch ms of the FIRST edit in the current burst (when the pending idle job + // was first armed). Used to enforce the max-wait ceiling so a continuous + // editing session cannot re-arm the trailing timer forever. `now` is injectable + // for tests; both default to a live clock / no ceiling when omitted. + burstStart?: number, + now: number = Date.now(), ): { jobId: string; delay: number } { const isAgent = source === 'agent'; - const pageAge = now - new Date(page.createdAt).getTime(); - const delay = isAgent - ? 0 - : pageAge < HISTORY_FAST_THRESHOLD - ? HISTORY_FAST_INTERVAL - : HISTORY_INTERVAL; - const jobId = isAgent ? `${page.id}-agent` : page.id; - return { jobId, delay }; + const interval = isAgent ? IDLE_INTERVAL_AGENT : IDLE_INTERVAL_USER; + const maxWait = isAgent ? IDLE_MAX_WAIT_AGENT : IDLE_MAX_WAIT_USER; + + let delay = interval; + if (burstStart !== undefined) { + // Time already elapsed since the burst's first edit; the snapshot must fire + // no later than `maxWait` after that, so shrink the trailing delay to the + // remaining budget (never negative, so BullMQ fires it promptly). + const remaining = burstStart + maxWait - now; + delay = Math.max(0, Math.min(interval, remaining)); + } + return { jobId: page.id, delay }; } @Injectable() @@ -132,6 +148,28 @@ export class PersistenceExtension implements Extension { // coalescing window" per document and OR it across all edits in the window, // so the snapshot is marked 'agent' regardless of who wrote last. private agentTouched: Map = new Map(); + // #370 — epoch ms of the FIRST edit in the current idle-flush burst. Keyed by + // documentName (like its sibling per-document maps above), NOT by page.id, so + // it can be cleaned in afterUnloadDocument alongside `contributors` / + // `agentTouched` / `intentionalClear` when the doc unloads — otherwise any page + // that was edited but never manually saved (the common case) would keep its + // entry forever and the Map would grow unbounded in this long-lived process. + // Set when the pending idle job is first armed (empty entry), read to enforce + // the max-wait ceiling in computeHistoryJob, and cleared on doc unload or when + // a manual save cancels the idle job so the next burst starts a fresh window. + // + // Single-process assumption (like `contributors` / `agentTouched` above): this + // lives only in THIS collab process's memory. A restart, or a page's ownership + // moving to another node, loses the burst-start marker. Consequence: a burst + // that spans the restart looks like a fresh burst to the surviving process, so + // its max-wait ceiling is re-anchored to the first post-restart edit — a single + // continuous session straddling a restart can therefore wait up to ~2× the cap + // for its idle snapshot (once for the lost pre-restart window, once for the new + // one). Bounded and benign (it only DELAYS a safety-net autosnapshot; manual + // saves are unaffected and the next quiet period always flushes), but the + // assumption and its consequence are recorded here so no one mistakes the + // in-memory marker for a durable, cross-process guarantee. + private idleBurstStart: Map = new Map(); // #251 — per-document "intentional clear pending" flags. Keyed by // documentName, value = expiry timestamp (ms). Set by onStateless when the // client reports a deliberate clear; consumed once by the next @@ -363,20 +401,19 @@ export class PersistenceExtension implements Extension { //this.logger.debug('Contributors error:' + err?.['message']); } - // Approach A — boundary snapshot before the agent's first edit. - // When this store is the agent's and the page's currently persisted - // state was authored by a human, pin that human state as its own - // history version BEFORE the agent overwrites it. `page` still holds - // the OLD content/provenance here, so saveHistory(page) captures the - // pre-agent state tagged 'user'. The agent's new content is - // snapshotted later by the debounced PAGE_HISTORY job ('agent'). Skip - // if the prior state is already agent-authored (boundary already - // pinned on the user->agent transition), if the page is effectively - // empty, or if the latest existing snapshot already equals this human - // state (avoid duplicates). + // #370 — boundary snapshot on ANY source transition. When the store + // flips the page's provenance (user↔agent↔git), pin the OUTGOING + // state as its own history version BEFORE the incoming source + // overwrites it. `page` still holds the OLD content/provenance here, + // so saveHistory(page) captures the pre-transition state tagged with + // its own source, kind='boundary'. The incoming content is snapshotted + // later by the debounced idle job. Skip if the page is effectively + // empty or if the latest existing snapshot already equals this state + // (the shared isDeepStrictEqual gate — avoids duplicates). Generalizing + // beyond the old user→agent special-case also covers git-sync for free. if ( - lastUpdatedSource === 'agent' && - page.lastUpdatedSource !== 'agent' + page.lastUpdatedSource && + page.lastUpdatedSource !== lastUpdatedSource ) { // pageHistory.pageId is uuid-typed; use page.id (never the doc-name // slugId) so a `page.` doc cannot throw 22P02 here (#260). @@ -384,15 +421,13 @@ export class PersistenceExtension implements Extension { page.id, { includeContent: true, trx }, ); - const humanBaselineMissing = + const baselineMissing = !lastHistory || !isDeepStrictEqual(lastHistory.content, page.content); - if ( - !isEmptyParagraphDoc(page.content as any) && - humanBaselineMissing - ) { + if (!isEmptyParagraphDoc(page.content as any) && baselineMissing) { await this.pageHistoryRepo.saveHistory(page, { contributorIds: page.contributorIds ?? undefined, + kind: 'boundary', trx, }); } @@ -522,7 +557,7 @@ export class PersistenceExtension implements Extension { { jobId: `embed-${page.id}`, delay: EMBED_DEBOUNCE_MS }, ); - await this.enqueuePageHistory(page, lastUpdatedSource); + await this.enqueuePageHistory(page, documentName, lastUpdatedSource); } // #402 — report the serialized size for the store histogram's size_bucket. @@ -554,6 +589,14 @@ export class PersistenceExtension implements Extension { return; // unrelated / malformed stateless message } + // #370 — explicit "save a version" (human Cmd+S / agent save tool). Edit + // rights are already enforced by the readOnly reject above (a reader can't + // create a version), exactly as intentional-clear requires. + if (message?.type === SAVE_VERSION_MESSAGE_TYPE) { + await this.handleSaveVersion(data); + return; + } + if (message?.type !== INTENTIONAL_CLEAR_MESSAGE_TYPE) return; this.intentionalClear.set( @@ -562,6 +605,160 @@ export class PersistenceExtension implements Extension { ); } + /** + * #370 — persist an intentional version from the live in-memory ydoc. + * + * One stateless path serves BOTH the human and the agent; the tier is derived + * SERVER-SIDE from the signed connection actor ('agent' → 'agent', anything + * else → 'manual'), so the version type cannot be spoofed by the client. We + * take the fresh ydoc from the collab process memory and run it through the + * EXISTING store path first (so pages.content/ydoc reflect the exact content + * being versioned — a REST endpoint would race the up-to-10s-stale page row), + * then snapshot it into page_history with the intentional kind. + * + * Promote-not-dup: if the latest history row already holds this exact content + * and it is an autosave (idle/boundary/legacy-null), upgrade its kind in place + * instead of duplicating a heavy content row; if it is already 'manual', it is + * a no-op (the client shows an "already saved" toast). Otherwise a fresh + * version row is written, popping the aggregated contributors from Redis. + */ + private async handleSaveVersion(data: onStatelessPayload): Promise { + const { connection, document, documentName } = data; + const context = connection?.context; + const pageId = getPageId(documentName); + // Unforgeable: 'agent' only for a signed agent connection, else 'manual'. + const kind: PageHistoryKind = + context?.actor === 'agent' ? 'agent' : 'manual'; + + // Flush the live ydoc through the normal store path so the page row + ydoc + // hold exactly what we are about to version (also fires the idle enqueue we + // supersede below, plus any source-transition boundary). onStoreDocument + // only needs document/documentName/context. + await this.onStoreDocument({ + document, + documentName, + context, + } as onStoreDocumentPayload); + + let result: + | { historyId: string; kind: PageHistoryKind; alreadySaved: boolean } + | undefined; + + // #370 F8-twin — the contributor set popped from Redis (destructive SPOP) + // must be restored if the version row does not durably land. The inner + // try/catch below only covers a throw INSIDE the callback; but executeTx + // COMMITS after the callback, so a commit-abort (serialization/deadlock/ + // connection drop — the transient class the epic retries in the processor) + // rejects OUTSIDE the callback, after saveHistory already ran and the SPOP + // already happened, while the INSERT rolls back. onStateless does NOT retry, + // so an unrestored pop is a one-shot irrecoverable attribution loss (the + // processor got exactly this fix: poppedForRestore + an outer catch). We + // track the popped set here (keyed by the page UUID it was popped by — never + // the doc-name id, which may be a slugId, #260) and restore it in the outer + // catch. addContributors is an idempotent Redis SADD, so a double-restore is + // harmless. versionedPageId is also reused below to remove the superseded + // idle job by its real jobId (page.id). + let poppedForRestore: string[] = []; + let versionedPageId: string | undefined; + + try { + await executeTx(this.db, async (trx) => { + const page = await this.pageRepo.findById(pageId, { + withLock: true, + includeContent: true, + trx, + }); + if (!page) return; + versionedPageId = page.id; + // Never version an effectively-empty page (mirrors the processor's + // first-history guard); there is nothing intentional to pin. + if (isEmptyParagraphDoc(page.content as any)) return; + + const lastHistory = await this.pageHistoryRepo.findPageLastHistory( + page.id, + { includeContent: true, trx }, + ); + + if ( + lastHistory && + isDeepStrictEqual(lastHistory.content, page.content) + ) { + // Content is already snapshotted. Promote-not-dup. + if (lastHistory.kind === 'manual') { + result = { + historyId: lastHistory.id, + kind: 'manual', + alreadySaved: true, + }; + return; + } + await this.pageHistoryRepo.updateHistoryKind( + lastHistory.id, + kind, + trx, + ); + result = { historyId: lastHistory.id, kind, alreadySaved: false }; + return; + } + + // Fresh version row. Pop the contributors aggregated since the last + // snapshot (SPOP); restore them if the write fails so they aren't lost. + const contributorIds = await this.collabHistory.popContributors( + page.id, + ); + poppedForRestore = contributorIds; + try { + const saved = await this.pageHistoryRepo.saveHistory(page, { + contributorIds, + kind, + trx, + }); + result = { historyId: saved.id, kind, alreadySaved: false }; + } catch (err) { + await this.collabHistory.addContributors(page.id, contributorIds); + poppedForRestore = []; + throw err; + } + }); + } catch (err) { + // A throw here means the tx did NOT commit (callback threw, or the commit + // itself failed and rolled back). If we popped contributors and the inner + // catch did not already restore them, restore now so attribution is not + // lost — onStateless has no retry to recover it. Restore by the page UUID + // the pop was keyed under (versionedPageId is always set before the pop). + if (poppedForRestore.length && versionedPageId) { + await this.collabHistory.addContributors( + versionedPageId, + poppedForRestore, + ); + } + throw err; + } + + // Housekeeping: this explicit version supersedes the page's pending idle + // autosnapshot, so cancel it and end the current idle burst so the next edit + // starts a fresh max-wait window. Remove the idle job by its REAL jobId + // (page.id UUID — computeHistoryJob arms it under page.id), not the raw + // doc-name id which may be a slugId for a `page.` doc (#260), or the + // remove silently misses. The burst marker is keyed by documentName (like its + // sibling per-document maps), and is also cleaned in afterUnloadDocument. + if (versionedPageId) { + await this.historyQueue.remove(versionedPageId).catch(() => undefined); + } + this.idleBurstStart.delete(documentName); + + if (result) { + document.broadcastStateless( + JSON.stringify({ + type: 'version.saved', + historyId: result.historyId, + kind: result.kind, + alreadySaved: result.alreadySaved, + }), + ); + } + } + async onChange(data: onChangePayload) { const documentName = data.documentName; const userId = data.context?.user?.id; @@ -586,6 +783,10 @@ export class PersistenceExtension implements Extension { this.contributors.delete(documentName); this.agentTouched.delete(documentName); this.intentionalClear.delete(documentName); + // #370 — drop the idle-burst marker with the other per-document maps so it + // cannot accumulate across the process lifetime for never-manually-saved + // pages. The pending idle job (if any) is a self-expiring BullMQ delayed job. + this.idleBurstStart.delete(documentName); } private consumeContributors(documentName: string): string[] { @@ -617,19 +818,80 @@ export class PersistenceExtension implements Extension { private async enqueuePageHistory( page: Page, + documentName: string, lastUpdatedSource: string, ): Promise { - // Job id + delay arithmetic lives in the pure `computeHistoryJob` (see its - // doc comment for the agent-delay-0 / age-based-debounce invariants). + // #370 — trailing idle debounce with a max-wait ceiling. One pending idle + // job per page (jobId = page.id); on every store we remove the pending + // delayed job and re-add it, so the snapshot lands `delay` after edits go + // quiet rather than once per store (precedent: workspace.service.ts). + // remove() on a delayed job simply deletes it (0 if absent, no throw); if the + // job is already ACTIVE and the remove is a no-op, the add still de-dups and + // the processor's isDeepStrictEqual gate collapses the duplicate content. + // + // The FIRST arm of a burst records `burstStart`; computeHistoryJob shrinks + // the delay to the remaining max-wait budget from that point, so a continuous + // session cannot re-arm the trailing timer forever and starve the snapshot. + // A burst marker older than THIS TIER's max-wait means the previous idle job + // has already fired — start a fresh window instead of firing immediately on + // the next edit. Must use the SAME source-specific max-wait computeHistoryJob + // uses (agent 5m / user 10m): a hardcoded USER ceiling would leave an agent + // burst's marker stale for 5..10m, forcing delay=0 on every store in that + // window and writing one idle row per store — exactly the per-store bloat the + // debounce exists to prevent, on the continuous-agent path. + const maxWait = + lastUpdatedSource === 'agent' ? IDLE_MAX_WAIT_AGENT : IDLE_MAX_WAIT_USER; + const now = Date.now(); + // Keyed by documentName (see the map declaration) so afterUnloadDocument can + // clean it; the queue jobId stays page.id (computeHistoryJob) as required. + let burstStart = this.idleBurstStart.get(documentName); + if (burstStart === undefined || now - burstStart >= maxWait) { + burstStart = now; + this.idleBurstStart.set(documentName, burstStart); + } + const { jobId, delay } = computeHistoryJob( page, lastUpdatedSource, - Date.now(), + burstStart, + now, ); + // remove-then-add trailing-debounce idiom, and its ONE race. We delete the + // pending delayed job and re-add it under the same jobId so the timer resets + // to the trailing edge of the burst. The race is the small window between + // these two awaits: if the delayed job's `delay` elapses in that gap it goes + // ACTIVE, and then: + // - remove() on an active/locked job is a no-op (BullMQ won't yank a job a + // worker holds), and our `.catch(() => undefined)` swallows that too; and + // - add() with a jobId that already exists (the now-active job's id) is + // DROPPED by BullMQ — a duplicate add is a no-op. + // So this store fails to re-arm the trailing job: the just-fired snapshot + // captured content up to the moment it went active, and THIS edit is left + // without a pending trailing job. It is bounded and self-healing — the NEXT + // store re-arms a fresh delayed job (the id is free again once the active job + // completes / removeOnComplete frees it), and the processor's + // isDeepStrictEqual gate collapses any content-identical duplicate. The only + // uncovered case is when the racing store was the LAST in the session: the + // tail edits made after the job went active get NO trailing snapshot until + // the next edit re-arms one. That is an acceptable safety-net gap (a manual + // Save, a source-transition boundary, or simply the next edit all still cover + // it), which is why the reviewer accepts documenting it here rather than + // adding a post-add "did the add actually arm a job?" re-check. + // + // NOTE — do NOT "unify" this with the neighbouring embed-debounce idiom + // (aiQueue.add of PAGE_CONTENT_UPDATED above): that one uses a STABLE jobId + // and NO remove(), relying purely on BullMQ coalescing a repeated add under + // the same id, because a re-embed only needs to eventually run once on the + // latest content and re-anchoring its delay on every keystroke is undesirable. + // THIS idiom deliberately removes-then-adds precisely to PUSH the delay back + // to the trailing edge on every store (a true debounce), which coalescing + // alone cannot do. Collapsing them would silently change the history cadence. + await this.historyQueue.remove(jobId).catch(() => undefined); + await this.historyQueue.add( QueueJob.PAGE_HISTORY, - { pageId: page.id } as IPageHistoryJob, + { pageId: page.id, kind: 'idle' } as IPageHistoryJob, { jobId, delay }, ); } diff --git a/apps/server/src/collaboration/processors/history.processor.spec.ts b/apps/server/src/collaboration/processors/history.processor.spec.ts index bdcf846e..4e87a322 100644 --- a/apps/server/src/collaboration/processors/history.processor.spec.ts +++ b/apps/server/src/collaboration/processors/history.processor.spec.ts @@ -66,6 +66,15 @@ describe('HistoryProcessor.process', () => { notificationQueue = { add: jest.fn().mockResolvedValue(undefined) }; generalQueue = { add: jest.fn().mockResolvedValue(undefined) }; + // #370 F3 — the processor now serializes its find+save under a page-row lock + // via executeTx. A db whose transaction().execute(fn) runs fn with a trx stub + // drives the real executeTx() helper without a database. + const db = { + transaction: () => ({ + execute: (fn: (trx: any) => Promise) => fn({ __trx: true }), + }), + }; + // WorkerHost's constructor reads `this.worker`; passing repos positionally // matches the constructor and avoids the Nest DI container. proc = new HistoryProcessor( @@ -73,6 +82,7 @@ describe('HistoryProcessor.process', () => { pageRepo as any, collabHistory as any, watcherService as any, + db as any, notificationQueue as any, generalQueue as any, ); @@ -126,15 +136,26 @@ describe('HistoryProcessor.process', () => { await proc.process(buildJob()); expect(collabHistory.popContributors).toHaveBeenCalledWith(PAGE_ID); + // #370 F3/F9 — the snapshot decision runs under a page-row lock. Pin the lock + // structurally so a refactor that drops withLock/trx (silently reintroducing + // the TOCTOU double-insert) turns this red. The tx stub is { __trx: true }. + expect(pageRepo.findById).toHaveBeenCalledWith( + PAGE_ID, + expect.objectContaining({ withLock: true, trx: { __trx: true } }), + ); + // #370 F7 — addPageWatchers MUST receive the trx, or its FK-check runs on a + // separate connection and self-deadlocks against our FOR UPDATE. Asserting + // the trx arg here is exactly what would have caught that regression. expect(watcherService.addPageWatchers).toHaveBeenCalledWith( ['u1', 'u2'], PAGE_ID, SPACE_ID, WORKSPACE_ID, + { __trx: true }, ); expect(pageHistoryRepo.saveHistory).toHaveBeenCalledWith( expect.objectContaining({ id: PAGE_ID }), - { contributorIds: ['u1', 'u2'] }, + { contributorIds: ['u1', 'u2'], kind: 'idle', trx: { __trx: true } }, ); expect(generalQueue.add).toHaveBeenCalledWith( QueueJob.PAGE_BACKLINKS, @@ -186,6 +207,48 @@ describe('HistoryProcessor.process', () => { ]); }); + it('COMMIT failure (throw outside the tx callback) → contributors RESTORED', async () => { + // #370 F8 — a commit-time failure throws OUTSIDE the callback, so the inner + // try/catch does not run; the outer catch must restore the popped set (else a + // BullMQ retry writes an unattributed version). Use a db whose execute() runs + // the callback THEN throws, simulating a commit abort. + pageHistoryRepo.findPageLastHistory.mockResolvedValue({ + content: { type: 'doc', content: [] }, + }); + const commitFail = { + transaction: () => ({ + execute: async (fn: (trx: any) => Promise) => { + await fn({ __trx: true }); // callback succeeds (saveHistory ok) + throw new Error('commit aborted'); // ...but the COMMIT fails + }, + }), + }; + const procCommitFail = new HistoryProcessor( + pageHistoryRepo as any, + pageRepo as any, + collabHistory as any, + watcherService as any, + commitFail as any, + notificationQueue as any, + generalQueue as any, + ); + jest + .spyOn(procCommitFail['logger'], 'error') + .mockImplementation(() => undefined); + + await expect(procCommitFail.process(buildJob())).rejects.toThrow( + 'commit aborted', + ); + // The inner catch did NOT run (save succeeded), so only the outer catch can + // restore — assert it did. + expect(collabHistory.addContributors).toHaveBeenCalledWith(PAGE_ID, [ + 'u1', + 'u2', + ]); + // And the post-snapshot queue work must NOT have run (we rethrew). + expect(generalQueue.add).not.toHaveBeenCalled(); + }); + it('backlinks + notification queue failures are swallowed (history still committed)', async () => { pageHistoryRepo.findPageLastHistory.mockResolvedValue({ content: { type: 'doc', content: [] }, diff --git a/apps/server/src/collaboration/processors/history.processor.ts b/apps/server/src/collaboration/processors/history.processor.ts index 6f26d3fa..9c4bf7a6 100644 --- a/apps/server/src/collaboration/processors/history.processor.ts +++ b/apps/server/src/collaboration/processors/history.processor.ts @@ -19,6 +19,9 @@ import { isDeepStrictEqual } from 'node:util'; import { CollabHistoryService } from '../services/collab-history.service'; import { WatcherService } from '../../core/watcher/watcher.service'; import { isEmptyParagraphDoc } from '../collaboration.util'; +import { InjectKysely } from 'nestjs-kysely'; +import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { executeTx } from '@docmost/db/utils'; @Processor(QueueName.HISTORY_QUEUE) export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { @@ -29,6 +32,7 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { private readonly pageRepo: PageRepo, private readonly collabHistory: CollabHistoryService, private readonly watcherService: WatcherService, + @InjectKysely() private readonly db: KyselyDB, @InjectQueue(QueueName.NOTIFICATION_QUEUE) private notificationQueue: Queue, @InjectQueue(QueueName.GENERAL_QUEUE) private generalQueue: Queue, ) { @@ -41,6 +45,9 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { try { const { pageId } = job.data; + // Read the page WITHOUT a lock first, only to bail early on the two cheap + // no-write cases (page gone / empty first snapshot) without opening a + // transaction. The authoritative check-then-write happens locked below. const page = await this.pageRepo.findById(pageId, { includeContent: true, }); @@ -51,40 +58,109 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { return; } - const lastHistory = await this.pageHistoryRepo.findPageLastHistory( - pageId, - { includeContent: true }, - ); + // #370 F3 — the snapshot decision (findPageLastHistory → saveHistory) must + // be serialized against manual-save/boundary writers, which run under a + // page-row lock in onStoreDocument. Without it, this processor and a + // concurrent manual-save each read the same lastHistory (MVCC), both see + // content != lastHistory, and both insert — producing two page_history rows + // with IDENTICAL content (one 'idle', one 'manual'), defeating + // promote-not-dup and the version-vs-autosave split. Taking the same + // page-row lock makes the second writer observe the first's committed row so + // the isDeepStrictEqual gate collapses the duplicate. Only the read+write + // is transacted; the post-snapshot queue work stays outside. + let contributorIds: string[] = []; + let snapshotWritten = false; + let lastHistoryContent: unknown; + // #370 F8 — the contributor set popped from Redis (destructive SPOP) must be + // restored if the snapshot does not durably land. The inner try/catch only + // covers a throw INSIDE the callback; a COMMIT failure (connection drop, + // serialization/deadlock abort on commit — the transient class the epic + // already retries) throws OUTSIDE it, rolling the snapshot back while the + // pop is already gone. We track the popped set here and restore it in the + // outer catch so a BullMQ retry re-attributes the version. addContributors + // is an idempotent Redis SADD, so a double-restore is harmless. + let poppedForRestore: string[] = []; - if (!lastHistory && isEmptyParagraphDoc(page.content as any)) { - this.logger.debug( - `Skipping first history for page ${pageId}: empty content`, - ); - await this.collabHistory.clearContributors(pageId); + try { + await executeTx(this.db, async (trx) => { + const lockedPage = await this.pageRepo.findById(pageId, { + includeContent: true, + withLock: true, + trx, + }); + if (!lockedPage) return; + + const lastHistory = await this.pageHistoryRepo.findPageLastHistory( + pageId, + { includeContent: true, trx }, + ); + lastHistoryContent = lastHistory?.content; + + if (!lastHistory && isEmptyParagraphDoc(lockedPage.content as any)) { + this.logger.debug( + `Skipping first history for page ${pageId}: empty content`, + ); + return; + } + + if ( + lastHistory && + isDeepStrictEqual(lastHistory.content, lockedPage.content) + ) { + return; // already snapshotted at this content — nothing to write + } + + contributorIds = await this.collabHistory.popContributors(pageId); + poppedForRestore = contributorIds; + try { + // Pass `trx` so the watcher insert's FK check (FOR KEY SHARE on + // pages[pageId]) runs on the SAME connection that already holds the + // FOR UPDATE lock from findById — otherwise it takes the FK lock on a + // separate pool connection and self-deadlocks against our own tx. + await this.watcherService.addPageWatchers( + contributorIds, + pageId, + lockedPage.spaceId, + lockedPage.workspaceId, + trx, + ); + + // #370 — every job on this queue is a trailing idle-flush autosnapshot. + await this.pageHistoryRepo.saveHistory(lockedPage, { + contributorIds, + kind: job.data.kind ?? 'idle', + trx, + }); + snapshotWritten = true; + this.logger.debug(`History created for page: ${pageId}`); + } catch (err) { + await this.collabHistory.addContributors(pageId, contributorIds); + poppedForRestore = []; + throw err; + } + }); + } catch (err) { + // A throw here means the tx did NOT commit (callback threw, or the commit + // itself failed and rolled back). If we popped contributors and the inner + // catch did not already restore them, restore now so the retry keeps + // attribution. snapshotWritten is irrelevant: it is set before commit, so + // it can be true even when the commit rolled the snapshot back. + if (poppedForRestore.length) { + await this.collabHistory.addContributors(pageId, poppedForRestore); + } + throw err; + } + + // No snapshot written (page vanished / empty-first / unchanged content) → + // clear the contributor set for the skip cases and stop. + if (!snapshotWritten) { + if (!lastHistoryContent && isEmptyParagraphDoc(page.content as any)) { + await this.collabHistory.clearContributors(pageId); + } return; } - if ( - !lastHistory || - !isDeepStrictEqual(lastHistory.content, page.content) - ) { - const contributorIds = await this.collabHistory.popContributors(pageId); - - try { - await this.watcherService.addPageWatchers( - contributorIds, - pageId, - page.spaceId, - page.workspaceId, - ); - - await this.pageHistoryRepo.saveHistory(page, { contributorIds }); - this.logger.debug(`History created for page: ${pageId}`); - } catch (err) { - await this.collabHistory.addContributors(pageId, contributorIds); - throw err; - } - + { const mentions = extractMentions(page.content); const pageMentions = extractPageMentions(mentions); const internalLinkSlugIds = extractInternalLinkSlugIds(page.content); @@ -102,7 +178,7 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { ); }); - if (contributorIds.length > 0 && lastHistory?.content) { + if (contributorIds.length > 0 && lastHistoryContent) { await this.notificationQueue .add(QueueJob.PAGE_UPDATED, { pageId, diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index 201c4cf8..0cd86cde 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -190,10 +190,11 @@ export function stepBudgetWarning(stepNumber: number): string { // // `system` is the in-scope system prompt; we CONCATENATE so the original // persona/context is preserved — a bare `system` override would REPLACE the -// whole system prompt for the step. `activatedTools` is PER-TURN mutable state -// owned by the streaming loop (a closure Set grown by loadTools); it is passed -// in (not module-global, not persisted) so this stays a pure function of its -// arguments. +// whole system prompt for the step. `activatedTools` is a closure Set grown by +// loadTools and owned by the streaming loop; the caller seeds it from and +// persists it to the chat's metadata across turns (#490), but this function only +// READS the Set it is handed, so it stays a pure function of its arguments (not +// module-global). // // NOTE: at AI SDK v7 the per-step `system` field is renamed to `instructions`. // On v6 (`^6.0.134`) `system` is the correct field — adjust when bumping. @@ -1417,10 +1418,11 @@ export class AiChatService implements OnModuleInit, OnModuleDestroy { const baseTools = { ...external.tools, ...docmostTools }; // Deferred tool loading state (#332), scoped to THIS streaming loop: - // - `activatedTools` is per-TURN mutable state — a fresh closure Set created - // per streamText call, NOT module-global and NOT persisted, so a new turn - // starts cold. loadTools.execute adds to it; prepareAgentStep reads it to - // widen `activeTools` on the NEXT step. + // - `activatedTools` is a fresh closure Set per streamText call (not + // module-global), SEEDED from the chat's persisted metadata.activatedTools + // (#490, just below) so activation carries across turns. loadTools.execute + // adds to it; prepareAgentStep reads it to widen `activeTools` on the NEXT + // step; turn end persists it back. // - `validDeferredNames` = every tool that is NOT core (the in-app deferred // tools + ALL external MCP tools), computed from the ACTUAL toolset so an // external tool is loadable by its namespaced name. loadTools rejects any 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 71f87d14..22b4962c 100644 --- a/apps/server/src/database/database.module.ts +++ b/apps/server/src/database/database.module.ts @@ -38,6 +38,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'; @@ -131,6 +132,7 @@ import { firstSqlToken } from '../integrations/metrics/metrics.constants'; AiMcpServerRepo, AiAgentRoleRepo, PageEmbeddingRepo, + ApiKeyRepo, PageListener, ], exports: [ @@ -167,6 +169,7 @@ import { firstSqlToken } from '../integrations/metrics/metrics.constants'; AiMcpServerRepo, AiAgentRoleRepo, PageEmbeddingRepo, + ApiKeyRepo, ], }) export class DatabaseModule implements OnApplicationBootstrap { diff --git a/apps/server/src/database/migrations/20260707T120000-page-history-kind.ts b/apps/server/src/database/migrations/20260707T120000-page-history-kind.ts new file mode 100644 index 00000000..c2292e6f --- /dev/null +++ b/apps/server/src/database/migrations/20260707T120000-page-history-kind.ts @@ -0,0 +1,27 @@ +import { type Kysely } from 'kysely'; + +/** + * #370 — page-versioning intentionality tier on a history snapshot. + * + * Adds `page_history.kind`, the three-tier "how intentional was this snapshot" + * marker that lets versions (intentional points) be told apart from autosaves: + * - 'manual' — a human explicitly saved a version (Cmd+S / Save button) + * - 'agent' — the AI agent explicitly saved a version + * - 'idle' — trailing idle-flush autosnapshot (safety net) + * - 'boundary' — autosnapshot pinned on a source transition (user↔agent↔git) + * + * Nullable with NO default (mirrors last_updated_source in the agent-provenance + * migration): legacy rows predate the marker and read back as `null`, which the + * client renders as a plain autosave. Stored as a short varchar to stay + * forward-compatible without an enum migration. + */ +export async function up(db: Kysely): Promise { + await db.schema + .alterTable('page_history') + .addColumn('kind', 'varchar(20)', (col) => col) + .execute(); +} + +export async function down(db: Kysely): Promise { + await db.schema.alterTable('page_history').dropColumn('kind').execute(); +} 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/database/repos/page/page-history.repo.ts b/apps/server/src/database/repos/page/page-history.repo.ts index e6ba1d61..6eb5b145 100644 --- a/apps/server/src/database/repos/page/page-history.repo.ts +++ b/apps/server/src/database/repos/page/page-history.repo.ts @@ -13,6 +13,7 @@ import { jsonArrayFrom, jsonObjectFrom } from 'kysely/helpers/postgres'; import { ExpressionBuilder, sql } from 'kysely'; import { DB } from '@docmost/db/types/db'; import { resolveAgentProvenance } from '../agent-provenance'; +import { PageHistoryKind } from '../../../collaboration/constants'; /** * Role-resolution subquery for a page-history row's bound AI chat (#300). Joins @@ -46,6 +47,9 @@ export class PageHistoryRepo { 'lastUpdatedById', 'lastUpdatedSource', 'lastUpdatedAiChatId', + // #370 — intentionality tier ('manual' | 'agent' | 'idle' | 'boundary'); + // null on legacy rows (= autosave). Selected so callers can read/promote it. + 'kind', 'contributorIds', 'spaceId', 'workspaceId', @@ -85,9 +89,15 @@ export class PageHistoryRepo { async saveHistory( page: Page, - opts?: { contributorIds?: string[]; trx?: KyselyTransaction }, - ): Promise { - await this.insertPageHistory( + opts?: { + contributorIds?: string[]; + // #370 — intentionality tier for this snapshot. Omitted → null (legacy + // autosave semantics). Callers derive it server-side, never from a client. + kind?: PageHistoryKind; + trx?: KyselyTransaction; + }, + ): Promise { + return await this.insertPageHistory( { pageId: page.id, slugId: page.slugId, @@ -99,6 +109,7 @@ export class PageHistoryRepo { // Copy the provenance marker off the page row, as for lastUpdatedById. lastUpdatedSource: page.lastUpdatedSource, lastUpdatedAiChatId: page.lastUpdatedAiChatId, + kind: opts?.kind ?? null, contributorIds: opts?.contributorIds, spaceId: page.spaceId, workspaceId: page.workspaceId, @@ -107,6 +118,25 @@ export class PageHistoryRepo { ); } + /** + * #370 — promote an existing snapshot's intentionality tier in place. Used by + * the manual-save "promote-not-dup" path: when the latest history row already + * holds the exact content being versioned, we upgrade its `kind` instead of + * duplicating a heavy content row. + */ + async updateHistoryKind( + pageHistoryId: string, + kind: PageHistoryKind, + trx?: KyselyTransaction, + ): Promise { + const db = dbOrTx(this.db, trx); + await db + .updateTable('pageHistory') + .set({ kind }) + .where('id', '=', pageHistoryId) + .execute(); + } + async findPageHistoryByPageId(pageId: string, pagination: PaginationOptions) { const query = this.db .selectFrom('pageHistory') diff --git a/apps/server/src/database/types/db.d.ts b/apps/server/src/database/types/db.d.ts index e1495e74..5530db56 100644 --- a/apps/server/src/database/types/db.d.ts +++ b/apps/server/src/database/types/db.d.ts @@ -280,6 +280,7 @@ export interface PageHistory { createdAt: Generated; icon: string | null; id: Generated; + kind: string | null; lastUpdatedAiChatId: string | null; lastUpdatedById: string | null; lastUpdatedSource: string | null; 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/apps/server/src/integrations/queue/constants/queue.interface.ts b/apps/server/src/integrations/queue/constants/queue.interface.ts index bcd84889..81975577 100644 --- a/apps/server/src/integrations/queue/constants/queue.interface.ts +++ b/apps/server/src/integrations/queue/constants/queue.interface.ts @@ -20,6 +20,10 @@ export interface IStripeSeatsSyncJob { export interface IPageHistoryJob { pageId: string; + // #370 — intentionality tier the worker stamps on the snapshot. All jobs on + // this queue are trailing idle-flush autosnapshots, so this is 'idle' (absent + // → treated as 'idle' by the processor). + kind?: 'idle'; } /** diff --git a/apps/server/test/integration/ai-chat-stream.int-spec.ts b/apps/server/test/integration/ai-chat-stream.int-spec.ts index bd2a8098..0ddbfdcc 100644 --- a/apps/server/test/integration/ai-chat-stream.int-spec.ts +++ b/apps/server/test/integration/ai-chat-stream.int-spec.ts @@ -322,20 +322,21 @@ describe('AiChatService.stream [integration]', () => { }); /** - * #332 deferred tool loading, the ON path. The riskiest property is that the - * per-turn `activatedTools` Set is created FRESH inside each stream() call, so a - * tool a previous turn activated via loadTools is NOT still active when the next - * turn starts — the new turn begins "cold" (CORE + loadTools only). The unit - * tests only exercise pure prepareAgentStep with hand-fed Sets; this pins the - * real wiring end-to-end (loadTools.execute -> activatedTools -> prepareStep -> - * per-step activeTools) against the real streamText loop, and proves there is no - * cross-turn leak. We drive a MockLanguageModelV3 whose step 1 calls - * loadTools(['createPage']) and assert, via the model's recorded per-step - * CallOptions.tools (the AI SDK filters the provider tool list by activeTools), - * that the deferred tool becomes active on the SAME turn's next step but NOT on a - * fresh turn's first step. + * #332 + #490 deferred tool loading, the ON path. Turn 1 starts COLD (CORE + + * loadTools only) and activates a deferred tool via loadTools; that activation + * is PERSISTED into the chat's metadata.activatedTools (#490) so the NEXT turn + * SEEDS from it and the tool is active from the fresh turn's FIRST step — the + * model never re-runs loadTools to re-activate the same tool. The unit tests + * only exercise pure prepareAgentStep with hand-fed Sets; this pins the real + * wiring end-to-end (loadTools.execute -> activatedTools -> persist -> next-turn + * seed -> prepareStep -> per-step activeTools) against the real streamText loop. + * We drive a MockLanguageModelV3 whose step 1 calls loadTools(['createPage']) + * and assert, via the model's recorded per-step CallOptions.tools (the AI SDK + * filters the provider tool list by activeTools), that the deferred tool becomes + * active on the SAME turn's next step AND, seeded from metadata, on the next + * turn's first step. */ - describe('deferred tool loading ON — per-turn activation, no leak (#332)', () => { + describe('deferred tool loading ON — cross-turn activation persistence (#332 + #490)', () => { // A stub deferred (non-core) tool the agent can activate. Its execute is never // called — the model only needs to SEE it become active — but it must be a // valid AI-SDK tool so the SDK includes it in a step's tool list once active. @@ -451,7 +452,7 @@ describe('AiChatService.stream [integration]', () => { } as any); } - it('activates a deferred tool for the SAME turn, and a NEW turn starts cold (no leak)', async () => { + it('activates a deferred tool for the SAME turn, and a NEW turn SEEDS it from persisted chat metadata (#490)', async () => { const chatId = (await createChat(db, { workspaceId, creatorId: userId })).id; // --- Turn 1: loadTools(createPage) on step 1, then answer on step 2. --- @@ -474,7 +475,7 @@ describe('AiChatService.stream [integration]', () => { // Step 2 of the SAME turn sees the just-activated deferred tool. expect(step2Tools).toContain('createPage'); - // --- Turn 2 on the SAME chat: must start cold again. --- + // --- Turn 2 on the SAME chat: seeds the persisted activation (#490). --- const model2 = new MockLanguageModelV3({ doStream: async () => ({ stream: successStream() }), } as any); @@ -485,9 +486,10 @@ describe('AiChatService.stream [integration]', () => { const nextTurnFirstStep = toolNames(model2.doStreamCalls[0]); expect(nextTurnFirstStep).toContain('loadTools'); - // The activated set is per-turn: the prior turn's createPage did NOT leak, - // so the fresh turn's first step sees it deferred again. - expect(nextTurnFirstStep).not.toContain('createPage'); + // #490: activation PERSISTS across turns — turn 1 wrote createPage into the + // chat's metadata.activatedTools, so the next turn seeds from it and the + // deferred tool is active from the FIRST step (no need to re-run loadTools). + expect(nextTurnFirstStep).toContain('createPage'); }); }); }); diff --git a/apps/server/test/integration/page-history-idle-pipeline.int-spec.ts b/apps/server/test/integration/page-history-idle-pipeline.int-spec.ts new file mode 100644 index 00000000..80540809 --- /dev/null +++ b/apps/server/test/integration/page-history-idle-pipeline.int-spec.ts @@ -0,0 +1,162 @@ +import { randomUUID } from 'node:crypto'; +import { Queue, Worker } from 'bullmq'; +import { PersistenceExtension } from '../../src/collaboration/extensions/persistence.extension'; + +/** + * #370 — integration property of the idle-snapshot pipeline against REAL BullMQ. + * + * This is deliberately NOT a unit test of computeHistoryJob (that lives in + * compute-history-job.spec.ts). The point here is the OBSERVABLE end-to-end + * behaviour of the production `enqueuePageHistory` remove-then-add debounce + * driving a real Redis-backed delayed queue + worker (the #431→#439 class: a + * locally-correct function whose queue/timer property was never exercised): + * + * - a CONTINUOUS burst of stores lasting several caps yields periodic idle + * snapshots — at least one per max-wait cap, NOT one-per-store; and + * - an INTERMITTENT burst (a few stores, then quiet) yields exactly ONE + * trailing snapshot. + * + * We shrink the idle windows to milliseconds (jest.mock of collaboration + * constants) so real BullMQ delayed jobs actually promote within the test — + * fake timers cannot advance Redis's own delayed-set clock, so the intervals + * must be real but tiny. The production method under test is called verbatim. + */ + +// NOTE: jest.mock is hoisted above the module's const initializers, so its +// factory cannot close over MAX_WAIT_MS/INTERVAL_MS — the literals are inlined +// here and MUST stay in sync with the consts below (a single source of truth is +// impossible across the hoist boundary). +jest.mock('../../src/collaboration/constants', () => { + const actual = jest.requireActual('../../src/collaboration/constants'); + return { + ...actual, + IDLE_MAX_WAIT_USER: 300, + IDLE_MAX_WAIT_AGENT: 300, + IDLE_INTERVAL_USER: 1000, + IDLE_INTERVAL_AGENT: 1000, + }; +}); + +// Mirrors the mocked IDLE_MAX_WAIT_* above (IDLE_INTERVAL_* is 1000 > this, so +// the max-wait ceiling is what actually governs the trailing delay). +const MAX_WAIT_MS = 300; + +const REDIS_CONNECTION = { + host: process.env.TEST_REDIS_HOST ?? '127.0.0.1', + port: Number(process.env.TEST_REDIS_PORT ?? 6379), +}; + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +describe('#370 idle-snapshot pipeline (real BullMQ)', () => { + let queue: Queue; + let worker: Worker; + let extension: PersistenceExtension; + // Every processed snapshot, tagged by pageId so the two scenarios stay isolated. + const processed: Array<{ pageId: string; kind: string; at: number }> = []; + const queueName = `history-idle-int-${randomUUID()}`; + + beforeAll(async () => { + queue = new Queue(queueName, { + connection: REDIS_CONNECTION, + // Mirror the production default (BullModule.forRoot removeOnComplete): the + // enqueue idiom relies on the jobId being freed once a job completes so the + // next burst can re-arm the same id. + defaultJobOptions: { removeOnComplete: true, removeOnFail: true }, + }); + await queue.waitUntilReady(); + + worker = new Worker( + queueName, + async (job) => { + processed.push({ + pageId: job.data?.pageId, + kind: job.data?.kind, + at: Date.now(), + }); + }, + { connection: REDIS_CONNECTION }, + ); + await worker.waitUntilReady(); + + // Construct the real extension; only historyQueue (5th ctor arg) and the + // internal idleBurstStart map are exercised by enqueuePageHistory, so the + // other collaborators can be null — the constructor only assigns fields. + extension = new PersistenceExtension( + null as any, // pageRepo + null as any, // pageHistoryRepo + null as any, // db + null as any, // aiQueue + queue as any, // historyQueue + null as any, // notificationQueue + null as any, // collabHistory + null as any, // transclusionService + ); + }); + + afterAll(async () => { + // Force-close and fully drain so no BullMQ background activity (delayed-set + // polling, blocking BRPOPLPUSH) bleeds into later suites in this single + // shared jest worker (maxWorkers: 1). + await worker?.close(true).catch(() => undefined); + await queue?.obliterate({ force: true }).catch(() => undefined); + await queue?.close(); + // Let the redis sockets settle before the next suite starts. + await sleep(150); + }); + + const arm = (pageId: string) => + (extension as any).enqueuePageHistory({ id: pageId }, 'user'); + + it('continuous burst over several caps → periodic idle snapshots (≥1 per cap, not one-per-store)', async () => { + const pageId = randomUUID(); + const runMs = 6 * MAX_WAIT_MS; // ~6 caps of unbroken editing + // Store cadence that does NOT evenly divide the cap: real hocuspocus stores + // are not aligned to cap boundaries, so a boundary job promotes in the gap + // before the next store's remove(). A cap-aligned cadence would instead land + // a store exactly on every boundary and lose the snapshot to the documented + // remove-vs-active race — an artefact of the test clock, not the pipeline. + const stepMs = 70; + const stores = Math.floor(runMs / stepMs); + + const start = Date.now(); + let count = 0; + while (Date.now() - start < runMs) { + await arm(pageId); + count++; + await sleep(stepMs); + } + // Let the final armed job flush. + await sleep(2 * MAX_WAIT_MS); + + const snaps = processed.filter((p) => p.pageId === pageId); + + // Every autosnapshot is an idle-kind row. + expect(snaps.every((s) => s.kind === 'idle')).toBe(true); + // Periodic: at least one per cap over a multi-cap burst (lower-bounded loosely + // to stay robust; the property is "fires at least every cap", not a single + // trailing snapshot). + expect(snaps.length).toBeGreaterThanOrEqual(3); + // But NOT one-per-store: ~`stores` stores were issued; the debounce must + // collapse them to a small multiple of the cap count, nowhere near per-store. + expect(snaps.length).toBeLessThanOrEqual(Math.ceil(stores / 2)); + }); + + it('intermittent burst (a few stores, then quiet) → exactly ONE trailing snapshot', async () => { + const pageId = randomUUID(); + + // A short burst well within a single cap window, then silence. + await arm(pageId); + await sleep(40); + await arm(pageId); + await sleep(40); + await arm(pageId); + + // Wait comfortably past the cap so the single pending trailing job fires. + await sleep(4 * MAX_WAIT_MS); + + const snaps = processed.filter((p) => p.pageId === pageId); + expect(snaps).toHaveLength(1); + expect(snaps[0].kind).toBe('idle'); + }); +}); diff --git a/docs/features/page-work-time_design.md b/docs/features/page-work-time_design.md index 24c86b24..46a3e999 100644 --- a/docs/features/page-work-time_design.md +++ b/docs/features/page-work-time_design.md @@ -387,7 +387,7 @@ bucketByDay(sessions, tz): факты ниже — ground truth, можно дозапросить файлы через gitea MCP по указанному SHA): - `page_history.kind` — `varchar(20)`, NULLABLE, БЕЗ дефолта (migration - `20260705T120000-page-history-kind.ts`). Домен: `manual`/`agent`/`idle`/`boundary`; + `20260707T120000-page-history-kind.ts`). Домен: `manual`/`agent`/`idle`/`boundary`; legacy `null` = автосейв (`collaboration/constants.ts`, `PageHistoryKind`). - `kind` УЖЕ включён в `PageHistoryRepo.baseFields` (`page-history.repo.ts`) — читается всеми выборками истории. `saveHistory({kind})` и `updateHistoryKind(id, kind)` существуют. 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); +});