diff --git a/.env.example b/.env.example index bb189adc..51fc4d4d 100644 --- a/.env.example +++ b/.env.example @@ -251,16 +251,6 @@ 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/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 0b90b2c7..855b3b33 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -13,8 +13,16 @@ "Add space members": "Add space members", "Add to favorites": "Add to favorites", "Admin": "Admin", + "API key copied to clipboard": "API key copied to clipboard", "API key created": "API key created", "API key revoked": "API key revoked", + "Confirm your password": "Confirm your password", + "Copy API key": "Copy API key", + "Copy to clipboard": "Copy to clipboard", + "Copy {{name}}": "Copy {{name}}", + "Enter your password to copy the API key \"{{name}}\" to your clipboard.": "Enter your password to copy the API key \"{{name}}\" to your clipboard.", + "Failed to copy API key": "Failed to copy API key", + "Incorrect password": "Incorrect password", "API keys across the workspace.": "API keys across the workspace.", "Are you sure you want to delete this group? Members will lose access to resources this group has access to.": "Are you sure you want to delete this group? Members will lose access to resources this group has access to.", "Are you sure you want to delete this page?": "Are you sure you want to delete this page?", diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 2aa13e5b..985c9d1c 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -13,8 +13,16 @@ "Add space members": "Добавить участников пространства", "Add to favorites": "Добавить в избранное", "Admin": "Администратор", + "API key copied to clipboard": "API ключ скопирован в буфер обмена", "API key created": "API ключ создан", "API key revoked": "API ключ отозван", + "Confirm your password": "Подтвердите пароль", + "Copy API key": "Скопировать API ключ", + "Copy to clipboard": "Скопировать в буфер обмена", + "Copy {{name}}": "Скопировать {{name}}", + "Enter your password to copy the API key \"{{name}}\" to your clipboard.": "Введите пароль, чтобы скопировать API ключ «{{name}}» в буфер обмена.", + "Failed to copy API key": "Не удалось скопировать API ключ", + "Incorrect password": "Неверный пароль", "API keys across the workspace.": "API ключи всего рабочего пространства.", "Are you sure you want to delete this group? Members will lose access to resources this group has access to.": "Вы уверены, что хотите удалить эту группу? Участники потеряют доступ к материалам, к которым у этой группы есть доступ.", "Are you sure you want to delete this page?": "Вы уверены, что хотите удалить эту страницу?", diff --git a/apps/client/src/features/api-key/components/api-keys-manager.test.tsx b/apps/client/src/features/api-key/components/api-keys-manager.test.tsx index dec6fea0..dcf39744 100644 --- a/apps/client/src/features/api-key/components/api-keys-manager.test.tsx +++ b/apps/client/src/features/api-key/components/api-keys-manager.test.tsx @@ -20,12 +20,14 @@ vi.mock("@/features/api-key/services/api-key-service", () => ({ getApiKeys: vi.fn(), createApiKey: vi.fn(), revokeApiKey: vi.fn(), + revealApiKey: vi.fn(), })); import { getApiKeys, createApiKey, revokeApiKey, + revealApiKey, } from "@/features/api-key/services/api-key-service"; import ApiKeysManager from "./api-keys-manager"; @@ -181,9 +183,9 @@ describe("ApiKeysManager — revoke (acceptance #4)", () => { }); }); -describe("ApiKeysManager — show-once token (acceptance #1 & #2)", () => { - it("shows the token once, then discards it from the UI, localStorage and query cache", async () => { - const SECRET = "gm_secret-token-value-xyz"; +describe("ApiKeysManager — create no longer shows the token (copy replaces show-once)", () => { + it("create closes the modal + toasts without ever rendering the token", async () => { + const SECRET = "gm_secret-created-value"; vi.mocked(getApiKeys).mockResolvedValue([]); vi.mocked(createApiKey).mockResolvedValue({ token: SECRET, @@ -198,7 +200,6 @@ describe("ApiKeysManager — show-once token (acceptance #1 & #2)", () => { const { queryClient } = renderManager(UserRole.MEMBER); await screen.findByText("No API keys yet"); - // Open create modal (use the header CTA), fill the name, submit. fireEvent.click( screen.getAllByRole("button", { name: "Create API key" })[0], ); @@ -206,22 +207,13 @@ describe("ApiKeysManager — show-once token (acceptance #1 & #2)", () => { fireEvent.change(nameInput, { target: { value: "My key" } }); fireEvent.click(screen.getByRole("button", { name: "Create" })); - // Token is shown exactly once in the show-once modal. - const tokenEl = await screen.findByTestId("api-key-token"); - expect(tokenEl.textContent).toBe(SECRET); + await waitFor(() => expect(createApiKey).toHaveBeenCalled()); + // The token is never rendered (no more show-once modal). + expect(screen.queryByTestId("api-key-token")).toBeNull(); + expect(screen.queryByText(SECRET)).toBeNull(); - // Close the modal → token discarded from the DOM. - fireEvent.click(screen.getByRole("button", { name: "Done" })); - await waitFor(() => - expect(screen.queryByTestId("api-key-token")).toBeNull(), - ); - - // Acceptance #2: the secret is nowhere in localStorage or the react-query - // caches (query cache never carried it; the mutation copy was reset()). - // Non-vacuous: currentUser IS in storage, so the dump is exercised. - const dump = storageDump(); - expect(dump).toContain("currentUser"); - expect(dump).not.toContain(SECRET); + // The created token never lands in localStorage or the react-query caches. + expect(storageDump()).not.toContain(SECRET); const cacheDump = JSON.stringify( queryClient.getQueryCache().getAll().map((q) => q.state.data), ); @@ -232,3 +224,73 @@ describe("ApiKeysManager — show-once token (acceptance #1 & #2)", () => { expect(mutationDump).not.toContain(SECRET); }); }); + +describe("ApiKeysManager — copy under step-up (acceptance #9)", () => { + it("copies a re-minted token to the clipboard and leaks it nowhere else", async () => { + const SECRET = "gm_revealed-token-value-xyz"; + const writeText = vi.fn().mockResolvedValue(undefined); + // jsdom has no clipboard; install a stub the copy helper will use. + Object.defineProperty(navigator, "clipboard", { + value: { writeText }, + configurable: true, + }); + + vi.mocked(getApiKeys).mockResolvedValue([makeKey({ id: "k1", name: "CI token" })]); + vi.mocked(revealApiKey).mockResolvedValue(SECRET); + + const { queryClient } = renderManager(UserRole.MEMBER); + await screen.findByText("CI token"); + + // Click the per-row Copy action → password step-up modal. + fireEvent.click(screen.getByLabelText("Copy CI token")); + const pwInput = await screen.findByLabelText("Password"); + fireEvent.change(pwInput, { target: { value: "hunter2" } }); + fireEvent.click(screen.getByRole("button", { name: "Copy to clipboard" })); + + // The reveal request carried the id + password; the token was written to the + // clipboard and NOWHERE else. + await waitFor(() => + expect(revealApiKey).toHaveBeenCalledWith({ + id: "k1", + password: "hunter2", + }), + ); + await waitFor(() => expect(writeText).toHaveBeenCalledWith(SECRET)); + + // The revealed secret is never in the DOM, localStorage, or either cache. + expect(screen.queryByText(SECRET)).toBeNull(); + expect(storageDump()).not.toContain(SECRET); + const cacheDump = JSON.stringify( + queryClient.getQueryCache().getAll().map((q) => q.state.data), + ); + expect(cacheDump).not.toContain(SECRET); + const mutationDump = JSON.stringify( + queryClient.getMutationCache().getAll().map((m) => m.state.data), + ); + expect(mutationDump).not.toContain(SECRET); + }); + + it("a wrong password keeps the modal open and does not copy", async () => { + const writeText = vi.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, "clipboard", { + value: { writeText }, + configurable: true, + }); + vi.mocked(getApiKeys).mockResolvedValue([makeKey({ id: "k1", name: "CI token" })]); + // Server returns a 401 for a wrong step-up password. + vi.mocked(revealApiKey).mockRejectedValue({ response: { status: 401 } }); + + renderManager(UserRole.MEMBER); + await screen.findByText("CI token"); + + fireEvent.click(screen.getByLabelText("Copy CI token")); + const pwInput = await screen.findByLabelText("Password"); + fireEvent.change(pwInput, { target: { value: "wrong" } }); + fireEvent.click(screen.getByRole("button", { name: "Copy to clipboard" })); + + await waitFor(() => expect(revealApiKey).toHaveBeenCalled()); + // Nothing copied; the password field is still on screen for a retry. + expect(writeText).not.toHaveBeenCalled(); + expect(await screen.findByLabelText("Password")).toBeDefined(); + }); +}); diff --git a/apps/client/src/features/api-key/components/api-keys-manager.tsx b/apps/client/src/features/api-key/components/api-keys-manager.tsx index ba134166..4192c222 100644 --- a/apps/client/src/features/api-key/components/api-keys-manager.tsx +++ b/apps/client/src/features/api-key/components/api-keys-manager.tsx @@ -13,27 +13,26 @@ import { } from "@mantine/core"; import { modals } from "@mantine/modals"; import { notifications } from "@mantine/notifications"; -import { IconAlertTriangle, IconKey, IconTrash } from "@tabler/icons-react"; +import { IconAlertTriangle, IconCopy, IconKey, IconTrash } from "@tabler/icons-react"; import { useTranslation } from "react-i18next"; import useUserRole from "@/hooks/use-user-role.tsx"; import { formatLocalized, useDateFnsLocale } from "@/lib/date-locale"; import { timeAgo } from "@/lib/time"; +import { copyToClipboard } from "@/lib/copy-to-clipboard"; import { useApiKeysQuery, useCreateApiKeyMutation, + useRevealApiKeyMutation, useRevokeApiKeyMutation, } from "@/features/api-key/queries/api-key-query"; -import { - IApiKey, - ICreateApiKeyResponse, -} from "@/features/api-key/types/api-key.types"; +import { IApiKey } from "@/features/api-key/types/api-key.types"; import { isExpired, isExpiringSoon, lastUsedBucket, } from "@/features/api-key/utils"; import { CreateApiKeyModal } from "./create-api-key-modal"; -import { ShowTokenModal } from "./show-token-modal"; +import { RevealKeyModal } from "./reveal-key-modal"; export default function ApiKeysManager() { const { t } = useTranslation(); @@ -46,27 +45,27 @@ export default function ApiKeysManager() { const { data: keys, isLoading, isError } = useApiKeysQuery(); const createMutation = useCreateApiKeyMutation(); const revokeMutation = useRevokeApiKeyMutation(); + const revealMutation = useRevealApiKeyMutation(); const [createOpened, setCreateOpened] = useState(false); - // SECURITY: the show-once token lives ONLY here, in this component's local - // state. It is never written to localStorage, the query cache or a log. Closing - // the modal sets it back to null (handleCloseToken) — discarded forever. - const [createdKey, setCreatedKey] = useState( - null, - ); + // SECURITY: only the key METADATA is held here (for the modal title) — never a + // token. The token is copied straight to the clipboard in handleCopy and is + // never stored in state, the query cache or localStorage. + const [revealTarget, setRevealTarget] = useState(null); const handleCreate = async (values: { name: string; expiresAt: string | null; }): Promise => { try { - const res = await createMutation.mutateAsync(values); - setCreateOpened(false); - // Move the token into local state, then immediately purge react-query's - // own copy of the mutation result so the secret does not linger in the - // mutation cache. - setCreatedKey(res); + await createMutation.mutateAsync(values); + // The create response carries a token, but it is now retrievable any time + // via the per-row Copy action (reveal), so we DISCARD it here: purge + // react-query's copy immediately and never move it into state. The user + // copies the key from the list via a password step-up. createMutation.reset(); + setCreateOpened(false); + notifications.show({ message: t("API key created") }); return true; } catch { notifications.show({ @@ -77,9 +76,36 @@ export default function ApiKeysManager() { } }; - const handleCloseToken = () => { - // Token discarded — the only copy the UI ever held is dropped here. - setCreatedKey(null); + // Password step-up accepted -> re-mint + copy. The token exists only as a local + // `const` for the single clipboard write, then react-query's copy is reset(). + // It never enters component state, the query cache or localStorage. + const handleCopy = async (password: string): Promise => { + const target = revealTarget; + if (!target) return false; + try { + const token = await revealMutation.mutateAsync({ + id: target.id, + password, + }); + await copyToClipboard(token); + revealMutation.reset(); + notifications.show({ message: t("API key copied to clipboard") }); + return true; + } catch (err) { + revealMutation.reset(); + // 401 = wrong password (keep the modal open to retry); anything else is a + // uniform failure (the server does not distinguish key states). + const status = (err as { response?: { status?: number } })?.response + ?.status; + notifications.show({ + message: + status === 401 + ? t("Incorrect password") + : t("Failed to copy API key"), + color: "red", + }); + return false; + } }; const openRevokeModal = (key: IApiKey) => @@ -186,16 +212,28 @@ export default function ApiKeysManager() { )} - - openRevokeModal(key)} - > - - - + + + setRevealTarget(key)} + > + + + + + openRevokeModal(key)} + > + + + + ); @@ -258,7 +296,13 @@ export default function ApiKeysManager() { loading={createMutation.isPending} /> - + setRevealTarget(null)} + onConfirm={handleCopy} + loading={revealMutation.isPending} + /> ); } diff --git a/apps/client/src/features/api-key/components/reveal-key-modal.tsx b/apps/client/src/features/api-key/components/reveal-key-modal.tsx new file mode 100644 index 00000000..3b4b8987 --- /dev/null +++ b/apps/client/src/features/api-key/components/reveal-key-modal.tsx @@ -0,0 +1,85 @@ +import { useState } from "react"; +import { + Button, + Group, + Modal, + PasswordInput, + Stack, + Text, +} from "@mantine/core"; +import { useTranslation } from "react-i18next"; + +interface Props { + // The key being copied (metadata only — never its token). `null` closes. + keyName: string | null; + opened: boolean; + onClose: () => void; + // Step-up: resolves true when the password was accepted, the token was copied + // to the clipboard and the modal should close; false to keep it open (wrong + // password) so the user can retry. The password is passed straight through and + // never persisted here. + onConfirm: (password: string) => Promise; + loading?: boolean; +} + +// Password step-up before a copyable key is re-minted + copied. This modal only +// ever holds the PASSWORD (transient, cleared on close) — never the revealed +// token, which the parent writes straight to the clipboard. +export function RevealKeyModal({ + keyName, + opened, + onClose, + onConfirm, + loading, +}: Props) { + const { t } = useTranslation(); + const [password, setPassword] = useState(""); + + const close = () => { + setPassword(""); + onClose(); + }; + + const handleSubmit = async (e: React.FormEvent) => { + e.preventDefault(); + if (password.length === 0) return; + const ok = await onConfirm(password); + if (ok) close(); + // On failure keep the modal open; clear the field so a retry starts clean. + else setPassword(""); + }; + + return ( + +
+ + + {t( + 'Enter your password to copy the API key "{{name}}" to your clipboard.', + { name: keyName ?? "" }, + )} + + setPassword(e.currentTarget.value)} + /> + + + + + +
+ + ); +} diff --git a/apps/client/src/features/api-key/components/show-token-modal.tsx b/apps/client/src/features/api-key/components/show-token-modal.tsx deleted file mode 100644 index 20c1f35f..00000000 --- a/apps/client/src/features/api-key/components/show-token-modal.tsx +++ /dev/null @@ -1,107 +0,0 @@ -import { - Alert, - Button, - Code, - Group, - Modal, - Stack, - Text, -} from "@mantine/core"; -import { IconAlertTriangle, IconCheck, IconCopy } from "@tabler/icons-react"; -import { useTranslation } from "react-i18next"; -import { CopyButton } from "@/components/common/copy-button"; -import { formatLocalized, useDateFnsLocale } from "@/lib/date-locale"; -import { ICreateApiKeyResponse } from "@/features/api-key/types/api-key.types"; - -interface Props { - // The freshly-created key incl. its token. Owned by the parent; this modal - // only renders it and never copies it into its own persistent state. - created: ICreateApiKeyResponse | null; - // Closing MUST discard the token in the parent (set the `created` prop back to - // null) — the token is shown exactly once. - onClose: () => void; -} - -export function ShowTokenModal({ created, onClose }: Props) { - const { t } = useTranslation(); - const locale = useDateFnsLocale(); - - const expiresAt = created?.apiKey.expiresAt ?? null; - - return ( - - {created && ( - - } - variant="light" - > - {t( - "Copy your API key now and store it somewhere safe. For security reasons it will not be shown again.", - )} - - -
- - {t("Token")} - - - - {created.token} - - - {({ copied, copy }) => ( - - )} - - -
- - - {expiresAt - ? t("Expires {{date}}", { - date: formatLocalized( - new Date(expiresAt), - "MMM dd, yyyy", - "PP", - locale, - ), - }) - : t("This key never expires")} - - - - - - - )} - - ); -} diff --git a/apps/client/src/features/api-key/queries/api-key-query.ts b/apps/client/src/features/api-key/queries/api-key-query.ts index d9738047..d6faffd5 100644 --- a/apps/client/src/features/api-key/queries/api-key-query.ts +++ b/apps/client/src/features/api-key/queries/api-key-query.ts @@ -9,12 +9,14 @@ import { useTranslation } from "react-i18next"; import { createApiKey, getApiKeys, + revealApiKey, revokeApiKey, } from "@/features/api-key/services/api-key-service"; import { IApiKey, ICreateApiKey, ICreateApiKeyResponse, + IRevealApiKey, } from "@/features/api-key/types/api-key.types"; export const API_KEYS_QUERY_KEY = ["api-keys"]; @@ -47,6 +49,23 @@ export function useCreateApiKeyMutation() { }); } +/** + * Reveal (copy) mutation. + * + * SECURITY (mirrors useCreateApiKeyMutation): the resolved value is the raw + * token. This hook deliberately stashes it NOWHERE — the caller reads it from + * `mutateAsync`, writes it straight to the clipboard, then calls + * `mutation.reset()` to purge react-query's own copy. `gcTime: 0` is the second + * belt so nothing lingers in the mutation cache after the observer unmounts. + * There is no `onSuccess` list invalidation: reveal does not change the list. + */ +export function useRevealApiKeyMutation() { + return useMutation({ + mutationFn: (data) => revealApiKey(data), + gcTime: 0, + }); +} + export function useRevokeApiKeyMutation() { const { t } = useTranslation(); const queryClient = useQueryClient(); diff --git a/apps/client/src/features/api-key/services/api-key-service.test.ts b/apps/client/src/features/api-key/services/api-key-service.test.ts index 177a03f7..66852293 100644 --- a/apps/client/src/features/api-key/services/api-key-service.test.ts +++ b/apps/client/src/features/api-key/services/api-key-service.test.ts @@ -15,6 +15,7 @@ vi.mock("@/lib/api-client", () => ({ import { createApiKey, getApiKeys, + revealApiKey, } from "@/features/api-key/services/api-key-service"; beforeEach(() => { @@ -75,4 +76,23 @@ describe("api-key-service response-contract unwrap", () => { expect(result).toHaveLength(2); expect(post).toHaveBeenCalledWith("/api-keys/list"); }); + + it("revealApiKey unwraps the envelope to the bare token string", async () => { + const SECRET = "gm_revealed-token"; + post.mockResolvedValue({ + data: { token: SECRET }, + success: true, + status: 200, + }); + + const result = await revealApiKey({ id: "key-1", password: "pw" }); + + // The service returns ONLY the token string (not the { token } wrapper), so + // the caller can copy it straight to the clipboard. + expect(result).toBe(SECRET); + expect(post).toHaveBeenCalledWith("/api-keys/reveal", { + id: "key-1", + password: "pw", + }); + }); }); diff --git a/apps/client/src/features/api-key/services/api-key-service.ts b/apps/client/src/features/api-key/services/api-key-service.ts index e76909e0..8697d6eb 100644 --- a/apps/client/src/features/api-key/services/api-key-service.ts +++ b/apps/client/src/features/api-key/services/api-key-service.ts @@ -3,6 +3,7 @@ import { IApiKey, ICreateApiKey, ICreateApiKeyResponse, + IRevealApiKey, } from "@/features/api-key/types/api-key.types"; // Mint a new key. The response carries the token ONCE — the caller must move it @@ -28,3 +29,13 @@ export async function getApiKeys(): Promise { export async function revokeApiKey(id: string): Promise { await api.post("/api-keys/revoke", { id }); } + +// Reveal (re-mint) an existing key's token under a password step-up. Returns the +// bare token string — the SECURITY contract (mirrors create): the caller must +// write it straight to the clipboard and never stash it in state, the query +// cache or localStorage. See useRevealApiKeyMutation (gcTime: 0 + reset-after- +// read) and api-keys-manager.tsx `handleCopy`. +export async function revealApiKey(data: IRevealApiKey): Promise { + const res = await api.post<{ token: string }>("/api-keys/reveal", data); + return (res.data as { token: string }).token; +} diff --git a/apps/client/src/features/api-key/types/api-key.types.ts b/apps/client/src/features/api-key/types/api-key.types.ts index 84fcfb13..dbc448b5 100644 --- a/apps/client/src/features/api-key/types/api-key.types.ts +++ b/apps/client/src/features/api-key/types/api-key.types.ts @@ -39,10 +39,20 @@ export interface ICreatedApiKey { createdAt: string; } -// Response of `POST /api/api-keys/create`. `token` is the ONLY time the secret -// is ever returned — it must live only in the show-once modal's local state and -// must never be cached, persisted or logged. +// Response of `POST /api/api-keys/create`. `token` is returned on create; it is +// ALSO retrievable later via reveal (deterministic re-mint under a step-up), so +// it is no longer "show once". It must never be cached, persisted or logged — +// the create flow discards it (the user copies via the per-row Copy action). export interface ICreateApiKeyResponse { token: string; apiKey: ICreatedApiKey; } + +// Payload for `POST /api/api-keys/reveal`: the key id + the caller's current +// password (step-up). The response is `{ token }` — a re-minted, byte-identical +// copy of the key's token, which must be written straight to the clipboard and +// never held in state, cache or storage. +export interface IRevealApiKey { + id: string; + password: string; +} diff --git a/apps/client/src/lib/copy-to-clipboard.ts b/apps/client/src/lib/copy-to-clipboard.ts index e30de66e..4a4d3f53 100644 --- a/apps/client/src/lib/copy-to-clipboard.ts +++ b/apps/client/src/lib/copy-to-clipboard.ts @@ -14,3 +14,19 @@ export function execCommandCopy(text: string): void { document.execCommand("copy"); document.body.removeChild(textarea); } + +// Stateless one-shot copy: write `text` to the clipboard without ever storing it +// in React state (unlike useClipboard, which keeps a `copied` flag AND holds the +// last value). Used by the api-key reveal/copy flow, where the secret must touch +// nothing but the clipboard — no component state, no cache, no localStorage. +export async function copyToClipboard(text: string): Promise { + if (typeof navigator !== "undefined" && "clipboard" in navigator) { + try { + await navigator.clipboard.writeText(text); + return; + } catch { + // Fall through to the execCommand fallback (e.g. insecure context). + } + } + execCommandCopy(text); +} diff --git a/apps/server/src/common/events/audit-events.ts b/apps/server/src/common/events/audit-events.ts index bb377847..27bbad3e 100644 --- a/apps/server/src/common/events/audit-events.ts +++ b/apps/server/src/common/events/audit-events.ts @@ -22,6 +22,9 @@ export const AuditEvent = { API_KEY_CREATED: 'api_key.created', API_KEY_UPDATED: 'api_key.updated', API_KEY_DELETED: 'api_key.deleted', + // A copyable key was re-minted and returned to its owner under a password + // step-up (see ApiKeyController.reveal). Durable via DatabaseAuditService. + API_KEY_REVEALED: 'api_key.revealed', // SCIM Tokens SCIM_TOKEN_CREATED: 'scim_token.created', 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 index 3cf3d94c..4fd1682b 100644 --- a/apps/server/src/core/api-key/api-key.controller.spec.ts +++ b/apps/server/src/core/api-key/api-key.controller.spec.ts @@ -1,4 +1,8 @@ -import { ForbiddenException, NotFoundException } from '@nestjs/common'; +import { + ForbiddenException, + NotFoundException, + UnauthorizedException, +} from '@nestjs/common'; import { ApiKeyController } from './api-key.controller'; import { WorkspaceCaslAction, @@ -12,7 +16,8 @@ import { * — 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). + * - reveal: password step-up (401 on wrong password), owner-only even for admin + * (404), api_key principal 403 — the copyable-key surface (#557). */ function makeController(over: any = {}) { @@ -22,6 +27,7 @@ function makeController(over: any = {}) { key: { id: 'k1', name: 'n', expiresAt: null, createdAt: new Date() }, }), revoke: jest.fn().mockResolvedValue(undefined), + reveal: jest.fn().mockResolvedValue('remint.tok'), ...(over.apiKeyService ?? {}), }; const apiKeyRepo = { @@ -36,16 +42,16 @@ function makeController(over: any = {}) { }), ...(over.workspaceAbility ?? {}), }; - const environmentService = { - isApiKeysEnabled: jest.fn().mockReturnValue(over.enabled ?? true), - ...(over.environmentService ?? {}), + const authService = { + verifyUserCredentials: jest.fn().mockResolvedValue({ id: 'u-1' }), + ...(over.authService ?? {}), }; const auditService = { log: jest.fn() }; const controller = new ApiKeyController( apiKeyService as any, apiKeyRepo as any, workspaceAbility as any, - environmentService as any, + authService as any, auditService as any, ); return { @@ -53,12 +59,12 @@ function makeController(over: any = {}) { apiKeyService, apiKeyRepo, workspaceAbility, - environmentService, + authService, auditService, }; } -const user = { id: 'u-1', workspaceId: 'ws-1' } as any; +const user = { id: 'u-1', email: 'u@x.io', 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 = () => @@ -86,20 +92,86 @@ describe('ApiKeyController — a token cannot manage tokens', () => { controller.revoke({ id: 'k1' } as any, user, workspace, reqApiKey()), ).rejects.toBeInstanceOf(ForbiddenException); }); + + it('403 on reveal for an api_key principal (a key cannot reveal its siblings)', async () => { + const { controller, authService, apiKeyService } = makeController(); + await expect( + controller.reveal( + { id: 'k1', password: 'pw' } as any, + user, + workspace, + reqApiKey(), + ), + ).rejects.toBeInstanceOf(ForbiddenException); + // Rejected BEFORE the step-up and the key lookup even run. + expect(authService.verifyUserCredentials).not.toHaveBeenCalled(); + expect(apiKeyService.reveal).not.toHaveBeenCalled(); + }); }); -describe('ApiKeyController — kill-switch OFF → 404', () => { - it('create/list/revoke all 404 when disabled', async () => { - const { controller } = makeController({ enabled: false }); +describe('ApiKeyController — reveal (copyable key under step-up)', () => { + it('re-mints and returns the token + logs API_KEY_REVEALED on success', async () => { + const { controller, apiKeyService, authService, auditService } = + makeController(); + const res = await controller.reveal( + { id: 'k1', password: 'correct-horse' } as any, + user, + workspace, + reqAccess(), + ); + + expect(res).toEqual({ token: 'remint.tok' }); + // Step-up ran with the caller's email + supplied password. + expect(authService.verifyUserCredentials).toHaveBeenCalledWith( + { email: 'u@x.io', password: 'correct-horse' }, + 'ws-1', + ); + expect(apiKeyService.reveal).toHaveBeenCalledWith({ + apiKeyId: 'k1', + user, + workspaceId: 'ws-1', + }); + expect(auditService.log).toHaveBeenCalledWith( + expect.objectContaining({ event: 'api_key.revealed' }), + ); + }); + + it('401 on a wrong password — step-up runs BEFORE the key lookup (no oracle)', async () => { + const { controller, apiKeyService } = makeController({ + authService: { + verifyUserCredentials: jest + .fn() + .mockRejectedValue(new UnauthorizedException()), + }, + }); await expect( - controller.create({ name: 'x' } as any, user, reqAccess()), - ).rejects.toBeInstanceOf(NotFoundException); + controller.reveal( + { id: 'k1', password: 'wrong' } as any, + user, + workspace, + reqAccess(), + ), + ).rejects.toBeInstanceOf(UnauthorizedException); + // The key is never looked up / re-minted when step-up fails. + expect(apiKeyService.reveal).not.toHaveBeenCalled(); + }); + + it("propagates the service's uniform 404 (absent/revoked/expired/non-owner)", async () => { + const { controller, auditService } = makeController({ + apiKeyService: { + reveal: jest.fn().mockRejectedValue(new NotFoundException()), + }, + }); await expect( - controller.list(user, workspace, reqAccess()), - ).rejects.toBeInstanceOf(NotFoundException); - await expect( - controller.revoke({ id: 'k1' } as any, user, workspace, reqAccess()), + controller.reveal( + { id: 'k1', password: 'correct' } as any, + user, + workspace, + reqAccess(), + ), ).rejects.toBeInstanceOf(NotFoundException); + // No audit event on a failed reveal. + expect(auditService.log).not.toHaveBeenCalled(); }); }); diff --git a/apps/server/src/core/api-key/api-key.controller.ts b/apps/server/src/core/api-key/api-key.controller.ts index 455d64d0..4eab95bf 100644 --- a/apps/server/src/core/api-key/api-key.controller.ts +++ b/apps/server/src/core/api-key/api-key.controller.ts @@ -2,6 +2,7 @@ import { Body, Controller, ForbiddenException, + forwardRef, HttpCode, HttpStatus, Inject, @@ -17,6 +18,7 @@ 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 { RevealApiKeyDto } from './dto/reveal-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'; @@ -31,7 +33,7 @@ import { AUDIT_SERVICE, IAuditService, } from '../../integrations/audit/audit.service'; -import { EnvironmentService } from '../../integrations/environment/environment.service'; +import { AuthService } from '../auth/services/auth.service'; import { UserThrottlerGuard } from '../../integrations/throttle/user-throttler.guard'; import { AUTH_THROTTLER } from '../../integrations/throttle/throttler-names'; @@ -44,18 +46,13 @@ export class ApiKeyController { private readonly apiKeyService: ApiKeyService, private readonly apiKeyRepo: ApiKeyRepo, private readonly workspaceAbility: WorkspaceAbilityFactory, - private readonly environmentService: EnvironmentService, + // forwardRef breaks the AuthModule <-> ApiKeyModule cycle (AuthModule imports + // ApiKeyModule for JwtStrategy). Only used for the reveal step-up. + @Inject(forwardRef(() => AuthService)) + private readonly authService: AuthService, @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). @@ -78,7 +75,6 @@ export class ApiKeyController { @AuthUser() user: User, @Req() req: FastifyRequest, ) { - this.assertEnabled(); this.rejectApiKeyPrincipal(req); // undefined -> service default (1 year); null -> unlimited; string -> Date. @@ -100,8 +96,9 @@ export class ApiKeyController { 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. + // Durable audit is via DatabaseAuditService (#496); this structured log is a + // second, container-log trail. 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, @@ -131,7 +128,6 @@ export class ApiKeyController { @AuthWorkspace() workspace: Workspace, @Req() req: FastifyRequest, ) { - this.assertEnabled(); this.rejectApiKeyPrincipal(req); const ability = this.workspaceAbility.createForUser(user, workspace); @@ -163,7 +159,6 @@ export class ApiKeyController { @AuthWorkspace() workspace: Workspace, @Req() req: FastifyRequest, ) { - this.assertEnabled(); this.rejectApiKeyPrincipal(req); const key = await this.apiKeyRepo.findById(dto.id, workspace.id); @@ -198,4 +193,64 @@ export class ApiKeyController { return { success: true }; } + + /** + * Reveal (re-mint) a copyable token for an existing key, under a password + * step-up. The token is hashed nowhere and stored nowhere — "reveal" re-mints + * the SAME deterministic value (see TokenService.generateApiToken / + * ApiKeyService.reveal) and returns it in the body only. + * + * Defence layers (triple, against exfiltration through a hijacked session): + * - rejectApiKeyPrincipal: an api_key Bearer caller is 403 — a leaked key must + * NOT be able to reveal its siblings (key laundering); a token doesn't manage + * tokens. + * - step-up: the password is re-verified (verifyUserCredentials) BEFORE any key + * lookup, so a wrong/absent password is a uniform 401 that leaks nothing about + * the key. SSO/MFA-only users (no local password) are out of scope (homelab). + * - AUTH throttler: same limiter as create. + * + * Owner-only, EVEN for an admin, and every key-state negative (absent / revoked / + * expired / another user's / creator disabled) is a UNIFORM 404 (see + * ApiKeyService.reveal) — no existence/state oracle. + */ + @HttpCode(HttpStatus.OK) + @UseGuards(JwtAuthGuard, UserThrottlerGuard) + @Throttle({ [AUTH_THROTTLER]: { limit: 10, ttl: 60_000 } }) + @Post('reveal') + async reveal( + @Body() dto: RevealApiKeyDto, + @AuthUser() user: User, + @AuthWorkspace() workspace: Workspace, + @Req() req: FastifyRequest, + ) { + this.rejectApiKeyPrincipal(req); + + // Step-up FIRST — before any key lookup — so the 401 for a bad password never + // depends on (and never leaks) the key's existence/state. Reuses the exact + // timing-safe, SSO-aware credential check used by login (throws a uniform + // 401 on wrong/absent/SSO-only password). + await this.authService.verifyUserCredentials( + { email: user.email, password: dto.password }, + workspace.id, + ); + + const token = await this.apiKeyService.reveal({ + apiKeyId: dto.id, + user, + workspaceId: workspace.id, + }); + + // Durable audit (DatabaseAuditService, #496) + a structured container-log + // trail. NEVER the token material. + this.auditService.log({ + event: AuditEvent.API_KEY_REVEALED, + resourceType: AuditResource.API_KEY, + resourceId: dto.id, + }); + this.logger.log( + `API key revealed: id=${dto.id} actor=${user.id} ip=${this.clientIp(req)}`, + ); + + return { token }; + } } diff --git a/apps/server/src/core/api-key/api-key.module.ts b/apps/server/src/core/api-key/api-key.module.ts index 9c002f5b..cfe55391 100644 --- a/apps/server/src/core/api-key/api-key.module.ts +++ b/apps/server/src/core/api-key/api-key.module.ts @@ -1,7 +1,8 @@ -import { Module } from '@nestjs/common'; +import { forwardRef, Module } from '@nestjs/common'; import { ApiKeyService } from './api-key.service'; import { ApiKeyController } from './api-key.controller'; import { TokenModule } from '../auth/token.module'; +import { AuthModule } from '../auth/auth.module'; // Core (non-EE) API-key feature: issuance REST endpoints + the shared validator // consumed by jwt.strategy (REST) and McpService (the /mcp Bearer router). @@ -11,7 +12,10 @@ import { TokenModule } from '../auth/token.module'; // jwt.strategy) and McpModule (for the /mcp router) can inject it directly, // replacing the absent EE `ee/api-key` dynamic require. @Module({ - imports: [TokenModule], + // forwardRef(AuthModule): the reveal endpoint's password step-up uses + // AuthService.verifyUserCredentials. AuthModule already imports ApiKeyModule + // (for JwtStrategy), so the two form a cycle that forwardRef resolves. + imports: [TokenModule, forwardRef(() => AuthModule)], controllers: [ApiKeyController], providers: [ApiKeyService], exports: [ApiKeyService], 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 index 248267ea..87c663a2 100644 --- a/apps/server/src/core/api-key/api-key.service.spec.ts +++ b/apps/server/src/core/api-key/api-key.service.spec.ts @@ -1,4 +1,8 @@ -import { UnauthorizedException } from '@nestjs/common'; +import { + ForbiddenException, + NotFoundException, + UnauthorizedException, +} from '@nestjs/common'; import { ApiKeyService } from './api-key.service'; import { JwtType } from '../auth/dto/jwt-payload'; @@ -16,8 +20,6 @@ import { JwtType } from '../auth/dto/jwt-payload'; * - No validate cache (each call re-reads the row → immediate revocation). */ -const APP_SECRET = 'secret'; - function makeDeps(over: Partial> = {}) { const apiKeyRepo = { findById: jest.fn(), @@ -38,20 +40,13 @@ function makeDeps(over: Partial> = {}) { 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 }; + return { service, apiKeyRepo, userRepo, workspaceRepo, tokenService }; } const payload = (over: Record = {}) => ({ @@ -148,10 +143,6 @@ describe('ApiKeyService.validate', () => { d.workspaceRepo.findById.mockResolvedValue(undefined); }, ], - [ - 'kill-switch OFF', - (d) => d.environmentService.isApiKeysEnabled.mockReturnValue(false), - ], [ 'malformed payload (missing apiKeyId)', () => undefined, @@ -175,14 +166,16 @@ describe('ApiKeyService.validate', () => { }, ); - it('kill-switch OFF denies BEFORE any DB read', async () => { - const { service, apiKeyRepo, environmentService } = makeDeps(); - environmentService.isApiKeysEnabled.mockReturnValue(false); + // --- Kill-switch removed: keys work with no env variable ------------------ + it('validates with no API_KEYS_ENABLED env (kill-switch fully removed)', async () => { + // The service no longer takes EnvironmentService — there is no env gate left. + const { service, apiKeyRepo, userRepo } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(activeRow()); + userRepo.findById.mockResolvedValue(activeUser()); - await expect(service.validate(payload() as any)).rejects.toBeInstanceOf( - UnauthorizedException, - ); - expect(apiKeyRepo.findById).not.toHaveBeenCalled(); + await expect(service.validate(payload() as any)).resolves.toMatchObject({ + user: { id: 'u-1' }, + }); }); // --- Infra error propagates (5xx), NOT masked as 401 --------------------- @@ -297,3 +290,90 @@ describe('ApiKeyService.create (mint-then-insert, no exp)', () => { expect(apiKeyRepo.insert).not.toHaveBeenCalled(); }); }); + +/** + * ApiKeyService.reveal — the KEY-STATE gate for the copyable-key flow. Owner-only + * (even for an admin), and EVERY negative is a uniform NotFoundException so there + * is no existence/state oracle. Re-mint is deterministic (delegated to + * TokenService); here we only assert the gate + normalization. + */ +describe('ApiKeyService.reveal', () => { + const caller = (over: Record = {}) => + ({ id: 'u-1', email: 'u@x.io', workspaceId: 'ws-1', ...over }) as any; + + it('re-mints and returns the token for a live key owned by the caller', async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(activeRow()); + tokenService.generateApiToken.mockResolvedValue('remint.tok'); + + const token = await service.reveal({ + apiKeyId: 'key-1', + user: caller(), + workspaceId: 'ws-1', + }); + + expect(token).toBe('remint.tok'); + // Re-mints against the SAME id + the caller (== creator) so the value matches. + expect(tokenService.generateApiToken).toHaveBeenCalledWith({ + apiKeyId: 'key-1', + user: expect.objectContaining({ id: 'u-1' }), + workspaceId: 'ws-1', + }); + }); + + it('404 for a missing/revoked key (findById returns undefined)', async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(undefined); + + await expect( + service.reveal({ apiKeyId: 'key-1', user: caller(), workspaceId: 'ws-1' }), + ).rejects.toBeInstanceOf(NotFoundException); + // No re-mint on any negative. + expect(tokenService.generateApiToken).not.toHaveBeenCalled(); + }); + + it("404 for another user's key — owner-only, even for an admin", async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + // The caller is a workspace admin (irrelevant here — reveal ignores CASL). + apiKeyRepo.findById.mockResolvedValue(activeRow({ creatorId: 'someone-else' })); + + await expect( + service.reveal({ apiKeyId: 'key-1', user: caller(), workspaceId: 'ws-1' }), + ).rejects.toBeInstanceOf(NotFoundException); + expect(tokenService.generateApiToken).not.toHaveBeenCalled(); + }); + + it('404 for an expired key (expiry read from the row) — no dead token issued', async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue( + activeRow({ expiresAt: new Date(Date.now() - 1000) }), + ); + + await expect( + service.reveal({ apiKeyId: 'key-1', user: caller(), workspaceId: 'ws-1' }), + ).rejects.toBeInstanceOf(NotFoundException); + expect(tokenService.generateApiToken).not.toHaveBeenCalled(); + }); + + it('normalizes a disabled-creator ForbiddenException (403) to a uniform 404', async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(activeRow()); + // generateApiToken throws ForbiddenException when isUserDisabled(user). + tokenService.generateApiToken.mockRejectedValue(new ForbiddenException()); + + await expect( + service.reveal({ apiKeyId: 'key-1', user: caller(), workspaceId: 'ws-1' }), + ).rejects.toBeInstanceOf(NotFoundException); + }); + + it('propagates an UNEXPECTED re-mint error (not masked as 404)', async () => { + const { service, apiKeyRepo, tokenService } = makeDeps(); + apiKeyRepo.findById.mockResolvedValue(activeRow()); + const boom = new Error('signer exploded'); + tokenService.generateApiToken.mockRejectedValue(boom); + + await expect( + service.reveal({ apiKeyId: 'key-1', user: caller(), workspaceId: 'ws-1' }), + ).rejects.toBe(boom); + }); +}); diff --git a/apps/server/src/core/api-key/api-key.service.ts b/apps/server/src/core/api-key/api-key.service.ts index 797fee8d..52a43222 100644 --- a/apps/server/src/core/api-key/api-key.service.ts +++ b/apps/server/src/core/api-key/api-key.service.ts @@ -1,7 +1,8 @@ import { + ForbiddenException, Injectable, Logger, - OnModuleInit, + NotFoundException, UnauthorizedException, } from '@nestjs/common'; import { v7 as uuid7 } from 'uuid'; @@ -9,7 +10,6 @@ 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'; @@ -31,7 +31,7 @@ const LAST_USED_THROTTLE_MS = 60 * 60 * 1000; * key's lifetime and revocation — never the JWT, which carries no `exp` claim. */ @Injectable() -export class ApiKeyService implements OnModuleInit { +export class ApiKeyService { private readonly logger = new Logger(ApiKeyService.name); constructor( @@ -39,20 +39,8 @@ export class ApiKeyService implements OnModuleInit { 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. @@ -101,8 +89,8 @@ export class ApiKeyService implements OnModuleInit { * 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 — + * Failure semantics (R4, anti-enumeration): a DEFINITE negative fact — + * 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 @@ -114,12 +102,6 @@ export class ApiKeyService implements OnModuleInit { 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(); } @@ -174,6 +156,67 @@ export class ApiKeyService implements OnModuleInit { await this.apiKeyRepo.softDelete(id, workspaceId); } + /** + * Re-mint (reveal) the token for an EXISTING key so its owner can copy it again. + * The token material is never stored, so "reveal" = re-generate the same value: + * generateApiToken is deterministic (no `iat`/`exp`, fixed payload), so the + * re-minted token is byte-identical to the original for the same key. + * + * Authorization/step-up (principal rejection + password) is the caller's job + * (the controller). This method owns the KEY-STATE gate and the re-mint. Every + * negative is a UNIFORM `NotFoundException` — there is NO existence/state oracle: + * a caller cannot tell "absent" from "revoked" from "expired" from "another + * user's key" from "creator disabled". Only a live key owned by `user` re-mints. + * + * `user` is the authenticated caller AND (after the owner check) the key's + * creator, so the re-minted token's `sub` matches the original mint — passing + * `user` directly avoids re-fetching the creator row. + */ + async reveal(opts: { + apiKeyId: string; + user: User; + workspaceId: string; + }): Promise { + const { apiKeyId, user, workspaceId } = opts; + + const row = await this.apiKeyRepo.findById(apiKeyId, workspaceId); + // Absent = revoked (soft-deleted), orphaned, or never existed -> uniform 404. + if (!row) { + throw new NotFoundException(); + } + + // Owner-only, EVEN for an admin: a working revealed token is an impersonation + // of the creator, so unlike list/revoke, admin must NOT reveal others' keys. + // Someone else's key looks exactly like a missing one -> uniform 404. + if (row.creatorId !== user.id) { + throw new NotFoundException(); + } + + // Expiry is read from the ROW (findById filters only deletedAt, so an expired + // row is still returned). A dead key must not hand out a working token, and a + // uniform 404 keeps the anti-enumeration property. + if (row.expiresAt && row.expiresAt.getTime() <= Date.now()) { + throw new NotFoundException(); + } + + try { + // Deterministic re-mint (byte-identical to the original for this key). + return await this.tokenService.generateApiToken({ + apiKeyId: row.id, + user, + workspaceId, + }); + } catch (err) { + // A disabled creator makes generateApiToken throw ForbiddenException (403). + // Normalize to the SAME 404 so reveal never leaks "user is blocked" vs "key + // does not exist". Unexpected errors propagate (5xx), never masked. + if (err instanceof ForbiddenException) { + throw new NotFoundException(); + } + throw err; + } + } + // 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). diff --git a/apps/server/src/core/api-key/dto/reveal-api-key.dto.ts b/apps/server/src/core/api-key/dto/reveal-api-key.dto.ts new file mode 100644 index 00000000..12d3c058 --- /dev/null +++ b/apps/server/src/core/api-key/dto/reveal-api-key.dto.ts @@ -0,0 +1,14 @@ +import { IsNotEmpty, IsString, IsUUID } from 'class-validator'; + +// Step-up payload for `POST /api-keys/reveal`: the key to re-mint + the caller's +// current password. The password is re-verified server-side (AuthService. +// verifyUserCredentials) before the key is ever looked up, so a copyable, +// non-expiring token cannot be exfiltrated through a merely-hijacked session. +export class RevealApiKeyDto { + @IsUUID() + id: string; + + @IsString() + @IsNotEmpty() + password: string; +} diff --git a/apps/server/src/core/auth/auth.module.ts b/apps/server/src/core/auth/auth.module.ts index 3895fff0..4539329f 100644 --- a/apps/server/src/core/auth/auth.module.ts +++ b/apps/server/src/core/auth/auth.module.ts @@ -1,4 +1,4 @@ -import { Module } from '@nestjs/common'; +import { forwardRef, Module } from '@nestjs/common'; import { AuthController } from './auth.controller'; import { AuthService } from './services/auth.service'; import { JwtStrategy } from './strategies/jwt.strategy'; @@ -10,8 +10,9 @@ import { ApiKeyModule } from '../api-key/api-key.module'; @Module({ // 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], + // `ee/api-key` dynamic require). forwardRef: ApiKeyModule imports AuthModule + // back (for the reveal step-up), so the two form a cycle. + imports: [TokenModule, WorkspaceModule, forwardRef(() => ApiKeyModule)], controllers: [AuthController], providers: [AuthService, SignupService, JwtStrategy], exports: [SignupService, AuthService], 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 de2f44f0..3d0d2f3a 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 @@ -306,6 +306,41 @@ describe('TokenService.generateApiToken (no exp claim ever)', () => { }); }); + // #557: the copyable-key contract. noTimestamp suppresses `iat`, so the token + // is a pure deterministic function of (payload, secret) — re-minting the SAME + // key yields a BYTE-IDENTICAL value, which is what makes "reveal" a safe + // re-mint rather than a stored secret. + it('mints an api-key JWT with NEITHER exp NOR iat (deterministic)', 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; + expect(decoded.exp).toBeUndefined(); + expect(decoded.iat).toBeUndefined(); + }); + + it('two mints of the SAME key are byte-identical (re-mint = reveal)', async () => { + const { service } = makeRealSignerService(); + const opts = { + apiKeyId: 'key-1', + user: user as never, + workspaceId: 'ws-1', + }; + + const first = await service.generateApiToken(opts); + // A different time (and a fresh signer instance) must not change the bytes. + await new Promise((r) => setTimeout(r, 1100)); + const { service: service2 } = makeRealSignerService(); + const second = await service2.generateApiToken(opts); + + expect(second).toBe(first); + }); + it('demonstrates the live bug it guards: the SHARED signer WOULD add exp', () => { const { JwtService } = require('@nestjs/jwt'); const sharedJwt = new JwtService({ diff --git a/apps/server/src/core/auth/services/token.service.ts b/apps/server/src/core/auth/services/token.service.ts index de202346..aba2b763 100644 --- a/apps/server/src/core/auth/services/token.service.ts +++ b/apps/server/src/core/auth/services/token.service.ts @@ -155,19 +155,31 @@ export class TokenService { // 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. + // + // The dedicated signer ALSO sets `noTimestamp: true`, so the token carries + // neither `exp` nor `iat`. The payload is a fixed literal { sub, apiKeyId, + // workspaceId, type } in a stable order, so the HS256 signature is a pure + // deterministic function of (sub, apiKeyId, workspaceId, APP_SECRET): minting + // the SAME key twice yields a BYTE-IDENTICAL token. This is what makes the key + // "copyable" — the reveal endpoint (#557) re-mints the same value under a + // step-up without ever persisting the token material. 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`. + // `exp` claim. `noTimestamp: true` additionally suppresses the default `iat` + // claim jsonwebtoken would otherwise add, making the minted token a fully + // deterministic function of its payload + secret (byte-identical re-mints, the + // basis of the copyable/reveal flow). Built once and cached. Verification still + // goes through the shared verifier (same secret); `verifyAsync` does not + // require an `exp` or `iat`. private _apiKeyJwtService?: JwtService; private apiKeyJwtService(): JwtService { if (!this._apiKeyJwtService) { this._apiKeyJwtService = new JwtService({ secret: this.environmentService.getAppSecret(), - signOptions: { issuer: 'Docmost' }, + signOptions: { issuer: 'Docmost', noTimestamp: true }, }); } return this._apiKeyJwtService; diff --git a/apps/server/src/integrations/environment/environment.service.ts b/apps/server/src/integrations/environment/environment.service.ts index e1fc423c..cea4d00d 100644 --- a/apps/server/src/integrations/environment/environment.service.ts +++ b/apps/server/src/integrations/environment/environment.service.ts @@ -70,23 +70,6 @@ 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 3b2f65eb..476bc81c 100644 --- a/apps/server/src/integrations/environment/environment.validation.ts +++ b/apps/server/src/integrations/environment/environment.validation.ts @@ -103,15 +103,6 @@ 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'])