diff --git a/.claude/skills/runner-playwright-e2e/SKILL.md b/.claude/skills/runner-playwright-e2e/SKILL.md index 90fc3b02a..1574acfed 100644 --- a/.claude/skills/runner-playwright-e2e/SKILL.md +++ b/.claude/skills/runner-playwright-e2e/SKILL.md @@ -1,6 +1,6 @@ --- name: runner-playwright-e2e -description: Use when writing or modifying Playwright E2E specs for the demo runner (runner/e2e/*.spec.ts) - the deterministic-by-default suite, the env-gate taxonomy (E2E_LIVE, E2E_BASE_URL, E2E_BROKER_TOKEN, E2E_AI, E2E_STARTER_MATRIX), the shared helpers, data-* test contracts, CodeMirror and Sandpack gotchas, and container-pool hygiene. NOT for pipeline unit tests (node --test in runner/pipeline/). +description: Use when writing or modifying Playwright E2E specs for the demo runner (runner/e2e/*.spec.ts) - the deterministic-by-default suite, the env-gate taxonomy (E2E_LIVE, E2E_BASE_URL, E2E_API_TOKEN, E2E_AI, E2E_STARTER_MATRIX), the shared helpers, data-* test contracts, CodeMirror and Sandpack gotchas, and container-pool hygiene. NOT for pipeline unit tests (node --test in runner/pipeline/). --- # Runner Playwright E2E authoring @@ -22,7 +22,7 @@ path. Anything that needs the real world takes the **narrowest gate** that covers the dependency — `E2E_LIVE` (real preview mount), `E2E_BASE_URL` (worker routes), -`E2E_BROKER_TOKEN` (authed round-trip), `E2E_AI` (LLM spend), +`E2E_API_TOKEN` (authed round-trip), `E2E_AI` (LLM spend), `E2E_STARTER_MATRIX` (container matrix). Two hard rules: the spec self-skips with instructions (`test.skip(cond, "set X=1 to …")`), and every gated spec is named in a workflow that actually runs it — in the same PR. Full taxonomy: diff --git a/.github/workflows/e2e-live.yml b/.github/workflows/e2e-live.yml index 920a82997..1494815f6 100644 --- a/.github/workflows/e2e-live.yml +++ b/.github/workflows/e2e-live.yml @@ -168,9 +168,13 @@ jobs: E2E_LIVE: '1' run: pnpm e2e e2e/style-apply.spec.ts --workers=1 -g "react|vue|javascript|switching" - # The authed share round-trip needs a hand-refreshed broker token - # (AGENTS.md § E2E). An absent or expired token is a notice, not a red - # run — the secret rots by design and rot is not a product failure. + # The authed share round-trip needs a persistent API token, minted from + # the app's API tokens page (ADR-0037). Absent, the step is skipped with a + # notice — the secret is optional by design. Present and NOT validating is + # a red run: this credential does not expire, so a refusal means revoked, + # deleted, or broken, none of which should pass quietly. That is the whole + # point of DEV-2583 — the broker token it replaced rotted hourly, so its + # failure had to be a warning, and the step therefore never ran. - name: E2E — authed share round-trip (secret-gated) if: ${{ !inputs.smoke && env.BASE_URL != '' }} env: @@ -179,17 +183,22 @@ jobs: # the auth-gated /api/admin/sessions listing, so it rides this # token-gated step rather than the anonymous suites. E2E_API_BASE: ${{ env.BASE_URL }} - E2E_BROKER_TOKEN: ${{ secrets.E2E_BROKER_TOKEN }} + E2E_API_TOKEN: ${{ secrets.E2E_API_TOKEN }} run: | - if [ -z "$E2E_BROKER_TOKEN" ]; then - echo "::notice::E2E_BROKER_TOKEN is not set — the authed share round-trip and the session-leak spec were skipped." + if [ -z "$E2E_API_TOKEN" ]; then + echo "::notice::E2E_API_TOKEN is not set — the authed share round-trip and the session-leak spec were skipped." exit 0 fi - code=$(curl -s -o /dev/null -w "%{http_code}" -H "Authorization: Bearer $E2E_BROKER_TOKEN" \ - "https://mcp-auth-proxy-j0tb.onrender.com/broker/userinfo" || true) + # Probed against our own API rather than the broker, which knows + # nothing about this credential. `/api/profile` rather than + # `/api/tokens`: a token is fenced off token management entirely + # (ADR-0037), and verifying the bearer is itself an `api_tokens` read, + # so this exercises the new path either way. + code=$(curl -s -o /dev/null -w "%{http_code}" -H "Authorization: Bearer $E2E_API_TOKEN" \ + "$BASE_URL/api/profile" || true) if [ "$code" != "200" ]; then - echo "::warning::E2E_BROKER_TOKEN no longer validates against the broker ($code) — refresh the secret to run the authed share round-trip." - exit 0 + echo "::error::E2E_API_TOKEN did not validate ($code). A persistent token does not expire, so this means revoked, deleted, or broken — mint a new one on /api-tokens." + exit 1 fi # Secret hygiene (Bugbot, #189): this run puts a live session JWT in # sessionStorage and an Authorization header — a Playwright trace diff --git a/runner/AGENTS.md b/runner/AGENTS.md index c1cd61069..9ebfdb34e 100644 --- a/runner/AGENTS.md +++ b/runner/AGENTS.md @@ -125,11 +125,16 @@ The quick list — what a green E2E run does and does not prove: (`FIXTURE_ID` in the spec — currently `r-react-18-0-0`). Never revoke it; if it is lost, mint a replacement titled "E2E fixture — do not revoke" from any signed-in session and update the constant. -- **The authed write round-trip needs `E2E_BROKER_TOKEN`** (`share-create-live.spec.ts`): - a fresh `sessionStorage.hot_token` from a signed-in session on the deployed app. Broker - tokens expire and cannot be minted programmatically, so the spec self-skips without one - and the workflow treats an expired token as a warning, not a failure. It creates one - real demo and revokes it in `finally` (the 410 doubles as the revocation assertion). +- **The authed write round-trip needs `E2E_API_TOKEN`** (`share-create-live.spec.ts`, and + the session-leak spec rides the same step): a persistent API token minted on the + deployed app's `/api-tokens` page (DEV-2583, ADR-0037). It never expires, so the spec + self-skips only when the secret is *absent* — a token that no longer validates fails the + run, because that means somebody revoked it. The spec injects it as + `sessionStorage.hot_token` and the client resolves identity for it against our own API, + which is what keeps the real Share button under test. It creates one real demo and + revokes it in an `afterEach` (the 410 doubles as the revocation assertion). + Because the credential has no expiry, that step keeps `--trace off` and scrubs its + artifacts — a leaked trace from this repo, which is public, would leak a live token. - **`E2E_AI=1` gates the live LLM answer checks** (`ai-live.spec.ts`): two API-level calls per run, real budget, shared 8/min-per-IP rate bucket — a 429 skips rather than fails. diff --git a/runner/apps/authoring/src/ApiTokens.tsx b/runner/apps/authoring/src/ApiTokens.tsx new file mode 100644 index 000000000..ac47c9f77 --- /dev/null +++ b/runner/apps/authoring/src/ApiTokens.tsx @@ -0,0 +1,604 @@ +// The API tokens page (DEV-2583, ADR-0037) — mint, see, and revoke the +// persistent credentials that stand in for a broker login in CI. +// +// Wholly undesigned; no frame models any of it (ADR-0023 rule 1). The frame this +// page borrows is Settings': the same top bar with a static pill, the same left +// nav, the same heading, the same card. The judgment calls: +// +// * The plaintext is shown in a callout above the list, not in a dialog. A +// dialog is dismissed by a stray Escape, and this is the only moment the +// token exists — the callout stays until the page is left, and says so. +// * The list is the whole team's tokens, because revocation is (ADR-0037), and +// each row names its creator so "whose is this" never needs asking. +// * Revoked rows stay, greyed, with who killed them. They are the audit trail; +// hiding them would make the list look like it had never had a problem. +// * Revoke is confirmed in a `Dialog`, copying My Demos' delete: it breaks +// something running elsewhere and cannot be undone. +// * No last-used-precise-time, only the date: the stamp is coarsened to the +// hour on the server, so rendering minutes would be a lie about precision. +// * A session running *on* a token gets an explanation instead of the page. +// Tokens are fenced off token management entirely (ADR-0037), so every +// control here would 403 — the same reasoning that hides Ask AI and Style +// from such a session, except this page cannot be hidden: it is reachable by +// URL, and the account menu row is disabled rather than absent. + +import { useEffect, useState, type CSSProperties } from "react"; +import { + Dialog, + IconCopy, + SideNav, + Spinner, + TopBar, + formatCreated, + shellStyles, + theme, +} from "@handsontable/demo-editor-shell"; +import { isTokenSession, login, logout, type User } from "./auth.js"; +import { isSessionExpired } from "./apiError.js"; +import { + fetchTokens, + mintApiToken, + revokeApiToken, + type ApiToken, + type MintedToken, +} from "./tokens.js"; +import { useProfile } from "./useProfile.js"; +import { + fieldInput, + fieldLabel as label, + ghostButton, + primaryButton, +} from "./formStyles.js"; +import { reportError } from "./sentry.js"; + +/** What the server accepts (`MAX_TOKEN_NAME` in the Worker's `token.ts`). + * Mirrored only to stop the form sending something it knows will 400. */ +const MAX_TOKEN_NAME = 64; + +/** The display form, and the only one there is — the server has no masking helper + * of its own, because it never renders a token. The id is public by design and + * the secret is gone the moment the mint response has been read. */ +const masked = (id: string) => `hot_pat_${id}_${"•".repeat(8)}`; + +export interface ApiTokensPageProps { + apiBase: string; + user: User; +} + +export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { + const profile = useProfile(apiBase, user.email); + // Read once: it can only change by a reload (see `App.tsx`). + const [tokenSession] = useState(isTokenSession); + const [tokens, setTokens] = useState(null); + const [name, setName] = useState(""); + // The one and only sight of a plaintext token. Deliberately not persisted + // anywhere — a reload is meant to lose it, because the server already has. + const [minted, setMinted] = useState(null); + const [confirming, setConfirming] = useState(null); + const [busy, setBusy] = useState(null); + // Distinct from `tokens === null`, which means "still loading". A failed read + // must not render the empty state: this page's whole job is enumerating live + // credentials, and "No tokens yet" is the one thing it then cannot know. + const [loadFailed, setLoadFailed] = useState(false); + const [copied, setCopied] = useState(false); + const [error, setError] = useState(null); + + useEffect(() => { + // A token may not read the listing, and asking anyway would put a 403 on the + // page under an explanation that already says so. + if (tokenSession) return; + let live = true; + fetchTokens(apiBase) + .then((list) => { if (live) setTokens(list); }) + .catch((e) => { + if (!live) return; + if (isSessionExpired(e)) return login(); + fail(e, "tokens-list"); + setLoadFailed(true); + }); + return () => { live = false; }; + }, [apiBase, tokenSession]); + + function fail(e: unknown, context: string) { + reportError(e, context); + setError(e instanceof Error ? e.message : String(e)); + } + + async function mint(event: React.FormEvent) { + event.preventDefault(); + const trimmed = name.trim(); + if (!trimmed || busy) return; + setBusy("mint"); + setError(null); + try { + const created = await mintApiToken(apiBase, trimmed); + setMinted(created); + setCopied(false); + setName(""); + // The mint response is itself a listing row, so the table updates without + // a second round trip — and `token` is dropped on the way in, so the + // plaintext lives in exactly one piece of state. + const { token: _plaintext, ...row } = created; + setTokens((current) => [row, ...(current ?? [])]); + } catch (e) { + if (isSessionExpired(e)) return login(); + fail(e, "tokens-mint"); + } finally { + setBusy(null); + } + } + + async function revoke(target: ApiToken) { + setBusy("revoke"); + setError(null); + try { + await revokeApiToken(apiBase, target.id); + const now = new Date().toISOString(); + setTokens((current) => + (current ?? []).map((row) => + row.id === target.id ? { ...row, revoked_at: now, revoked_by: user.email } : row, + )); + // The plaintext on screen may be the one just killed; it is no longer + // worth offering to copy. + if (minted?.id === target.id) setMinted(null); + setConfirming(null); + } catch (e) { + // Kept on this page rather than swallowed: a revoke that failed because a + // colleague got there first (404) or because the session went stale (401) + // must not read as a revoke that worked. + if (isSessionExpired(e)) return login(); + fail(e, "tokens-revoke"); + } finally { + setBusy(null); + } + } + + async function copy() { + if (!minted) return; + try { + await navigator.clipboard.writeText(minted.token); + setCopied(true); + setTimeout(() => setCopied(false), 1500); + } catch { + /* clipboard blocked; the field is selectable, so it can still be copied by hand */ + } + } + + return ( +
+ + API tokens +
+ } + accountEmail={user.email} + accountDisplayName={profile?.display_name} + accountAvatarUrl={profile?.avatar_url} + onMyDemos={() => { location.href = "/my-demos"; }} + onSettings={() => { location.href = "/settings"; }} + onApiTokens={tokenSession ? undefined : () => { location.href = "/api-tokens"; }} + onGuide={() => { location.href = "/guide"; }} + onLogout={() => logout("/")} + /> + +
+ logout("/")} /> + +
+

API tokens

+

+ A token authenticates against this API the way you do, and never expires — the + nightly test matrix uses one. It cannot use the AI features, change the + guardrail settings, or manage tokens. Every token here belongs to the team: + anyone can see one, and anyone can revoke one. +

+ + {tokenSession && ( +

+ This page is not available to a session signed in with an API token. A token + cannot create, list or revoke tokens — sign in with your Google account to + manage them. +

+ )} + + {error && !confirming &&

{error}

} + + {minted && ( +
+

Copy your token now

+

+ This is the only time it is shown. Once you leave this page there is no way + to see it again — the server keeps a hash, not the token. +

+
+ e.currentTarget.select()} + /> + +
+
+ )} + + {!tokenSession && ( +
void mint(e)}> + + setName(e.target.value)} + /> +

+ What this token is for, so the next person can tell whether revoking it will + break something. +

+
+ {/* Also inert until the listing has landed: minting first would + prepend the new row, then the in-flight GET would resolve and + replace state with the pre-mint snapshot, dropping the token + whose plaintext is still on screen. */} + +
+
+ )} + + {/* A notice beside the list rather than instead of it. Minting stays + available after a failed read, so its row has to be reachable — a + credential that exists and cannot be revoked from this page is the + worst state the feature has (Bugbot, #252). The notice stays up + regardless, because one row known locally is not the same as the + list, and this page must not imply otherwise. */} + {!tokenSession && loadFailed && ( +

+ The token list could not be loaded, so tokens that already exist may be + missing from it. Reload to try again. +

+ )} + + {tokenSession + ? null + : tokens === null + ? (loadFailed ? null :

Loading tokens…

) + : tokens.length === 0 + ? (loadFailed ? null :

No tokens yet.

) + : ( +
    + {tokens.map((token) => ( + setConfirming(token)} + /> + ))} +
+ )} +
+
+ + {confirming && ( + { if (busy !== "revoke") setConfirming(null); }} + > + {error &&

{error}

} +

+ {confirming.name} stops working on its very next request. + Anything using it — a nightly workflow, a script, somebody's shell — starts + failing with 401. This can't be undone; mint a new token instead. +

+
+ + {/* `data-autofocus`, not React's `autoFocus`: `Dialog` focuses the + content's first focusable from an effect that runs *after* the + layout-phase autoFocus, so it would win and land focus on Revoke + — where Enter or Space revokes a live credential without the + question having been answered. The marker is the hatch Dialog + documents for exactly this, and what every other destructive + confirm in the app uses. */} + +
+
+ )} + + ); +} + +function TokenRow({ + token, + busy, + onRevoke, +}: { + token: ApiToken; + busy: boolean; + onRevoke: () => void; +}) { + const revoked = token.revoked_at !== null; + return ( +
  • +
    + {token.name} + {masked(token.id)} +
    +
    + {token.created_by} + created {formatCreated(token.created_at) ?? token.created_at} + + {token.last_used_at + ? `last used ${formatCreated(token.last_used_at) ?? token.last_used_at}` + : "never used"} + + {revoked && ( + + revoked{token.revoked_by ? ` by ${token.revoked_by}` : ""} + + )} +
    + {!revoked && ( + + )} +
  • + ); +} + +// ---- styles ---------------------------------------------------------------- +// The frame is Settings' (`114:26833`) — same body grid, same content padding, +// same heading and card. Only what this page adds is spelled out. + +const body: CSSProperties = { + display: "grid", + gridTemplateColumns: "320px minmax(0, 1fr)", + minHeight: 0, + overflow: "hidden", +}; + +const content: CSSProperties = { + padding: theme.space(4), + overflowY: "auto", + background: theme.color.surface, +}; + +const heading: CSSProperties = { + margin: `0 0 ${theme.space(2)}`, + fontFamily: theme.font.ui, + fontSize: 20, + fontWeight: 600, + color: theme.color.text, +}; + +const intro: CSSProperties = { + maxWidth: 620, + margin: `0 0 ${theme.space(4)}`, + fontFamily: theme.font.ui, + fontSize: 13, + lineHeight: 1.5, + color: theme.color.textMuted, +}; + +const card: CSSProperties = { + maxWidth: 520, + padding: theme.space(4), + borderRadius: theme.radius.md, + background: theme.color.surfaceMuted, +}; + +const cardFooter: CSSProperties = { display: "flex", marginTop: theme.space(4) }; + +const input: CSSProperties = { ...fieldInput, boxSizing: "border-box" }; + +const hint: CSSProperties = { + margin: `${theme.space(2)} 0 0`, + fontFamily: theme.font.ui, + fontSize: 12, + lineHeight: 1.45, + color: theme.color.textMuted, +}; + +// `accent`, not `danger`: minting is not a warning, it is a one-time reveal. The +// border is what stops it reading as another card. +const callout: CSSProperties = { + maxWidth: 620, + marginBottom: theme.space(4), + padding: theme.space(4), + border: `1px solid ${theme.color.accent}`, + borderRadius: theme.radius.md, + background: theme.color.surfaceMuted, +}; + +const calloutTitle: CSSProperties = { + margin: `0 0 ${theme.space(1)}`, + fontFamily: theme.font.ui, + fontSize: 13, + fontWeight: 600, + color: theme.color.text, +}; + +const calloutBody: CSSProperties = { + margin: `0 0 ${theme.space(3)}`, + fontFamily: theme.font.ui, + fontSize: 12.5, + lineHeight: 1.45, + color: theme.color.textMuted, +}; + +// `controlBorder`, not `border`: this is an outlined control on `surfaceSunken`, +// and in dark those two tokens are #353535 against #222222 — the plain `border` +// reads as no edge at all, on the one field that has to stay obvious after a +// mint. The shared `fieldInput` uses `controlBorder` for exactly this reason +// (Bugbot, #252). +const field: CSSProperties = { + display: "flex", + alignItems: "center", + gap: theme.space(1), + padding: `0 ${theme.space(1)} 0 ${theme.space(3)}`, + border: `1px solid ${theme.color.controlBorder}`, + borderRadius: theme.radius.md, + background: theme.color.surfaceSunken, +}; + +const fieldValue: CSSProperties = { + flex: 1, + minWidth: 0, + height: 32, + border: "none", + outline: "none", + background: "transparent", + color: theme.color.text, + fontFamily: theme.font.mono, + fontSize: 12.5, +}; + +const copyButton: CSSProperties = { + display: "inline-flex", + alignItems: "center", + justifyContent: "center", + width: 28, + height: 28, + flex: "0 0 auto", + border: "none", + borderRadius: theme.radius.sm, + color: theme.color.textMuted, + cursor: "pointer", +}; + +const list: CSSProperties = { + maxWidth: 620, + margin: `${theme.space(4)} 0 0`, + padding: 0, + listStyle: "none", + display: "flex", + flexDirection: "column", + gap: theme.space(2), +}; + +const row = (revoked: boolean): CSSProperties => ({ + display: "grid", + gridTemplateColumns: "minmax(0, 1fr) auto", + gridTemplateRows: "auto auto", + alignItems: "center", + gap: `${theme.space(1)} ${theme.space(3)}`, + padding: theme.space(3), + borderRadius: theme.radius.md, + background: theme.color.surfaceMuted, + // Greyed rather than hidden: a revoked row is the audit trail. + opacity: revoked ? 0.6 : 1, +}); + +const rowMain: CSSProperties = { + display: "flex", + alignItems: "baseline", + gap: theme.space(2), + minWidth: 0, +}; + +const rowName: CSSProperties = { + fontFamily: theme.font.ui, + fontSize: 13, + fontWeight: 600, + color: theme.color.text, +}; + +const rowId: CSSProperties = { + fontFamily: theme.font.mono, + fontSize: 12, + color: theme.color.textMuted, + whiteSpace: "nowrap", + overflow: "hidden", + textOverflow: "ellipsis", +}; + +const rowMeta: CSSProperties = { + display: "flex", + flexWrap: "wrap", + gap: `0 ${theme.space(3)}`, + gridColumn: "1", + fontFamily: theme.font.ui, + fontSize: 12, + color: theme.color.textMuted, +}; + +const revokedTag: CSSProperties = { color: theme.color.danger }; + +const rowAction: CSSProperties = { + ...ghostButton, + gridColumn: "2", + gridRow: "1 / span 2", +}; + +const muted: CSSProperties = { + display: "flex", + alignItems: "center", + gap: theme.space(2), + marginTop: theme.space(4), + fontFamily: theme.font.ui, + fontSize: 13, + color: theme.color.textMuted, +}; + +const errorText: CSSProperties = { + margin: `0 0 ${theme.space(3)}`, + fontFamily: theme.font.ui, + fontSize: 13, + color: theme.color.danger, +}; + +const confirmBody: CSSProperties = { + margin: 0, + fontFamily: theme.font.ui, + fontSize: 13, + lineHeight: 1.5, + color: theme.color.textMuted, +}; + +const confirmFooter: CSSProperties = { + display: "flex", + gap: theme.space(2), + marginTop: theme.space(5), +}; + +const dangerButton: CSSProperties = { + ...ghostButton, + border: `1px solid ${theme.color.danger}`, + background: theme.color.danger, + color: theme.color.accentContrast, + fontWeight: 600, +}; diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index c330b3e89..6de984f41 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -47,7 +47,7 @@ import { } from "./docs-catalog.js"; import { loadStarterExample, toPlaceholderEntry } from "./starter-catalog.js"; import { DocsCascader, type CascaderLeaf } from "./DocsCascader.js"; -import { currentUser, login, logout, getToken, type User } from "./auth.js"; +import { currentUser, isTokenSession, login, logout, getToken, type User } from "./auth.js"; import { assertApiOk, readApiJson } from "./api.js"; import { isSessionExpired } from "./apiError.js"; import { formFooter, ghostButton, primaryButton } from "./formStyles.js"; @@ -64,6 +64,7 @@ import { elapsedBucket } from "./sessionDiagnostics.js"; import { Markdown } from "./markdown.js"; import { MyDemosPage } from "./MyDemos.js"; import { SettingsPage } from "./Settings.js"; +import { ApiTokensPage } from "./ApiTokens.js"; import { useProfile } from "./useProfile.js"; import { monitorDemos, reportDemoEvent, reportError, reportingEnabled, Sentry } from "./sentry.js"; import { isMonitorPayload } from "@handsontable/demo-runtime/monitor"; @@ -360,6 +361,7 @@ type AppRoute = | { mode: "myDemos" } | { mode: "allDemos" } | { mode: "settings" } + | { mode: "apiTokens" } | { mode: "guide" }; function parseRoute(): AppRoute { @@ -371,6 +373,10 @@ function parseRoute(): AppRoute { // serves index.html for it (`not_found_handling: "single-page-application"`), // so it never 404'd, it just showed the wrong thing. if (/^\/settings\/?$/.test(location.pathname)) return { mode: "settings" }; + // `/api-tokens` (DEV-2583, ADR-0037). Same shape and the same hazard as + // `/settings`: above the editor fallthrough, or the SPA fallback renders the + // playground for it instead of the page. + if (/^\/api-tokens\/?$/.test(location.pathname)) return { mode: "apiTokens" }; // `/guide` (DEV-2503) — the in-app how-to. Same shape as the two above: matched // before the editor fallthrough, which would otherwise read it as a demo id. // `/guide` and `/guide/` (DEV-2522): one route, because the page reads the @@ -417,6 +423,7 @@ function fullModeId(route: AppRoute): string | null { route.mode === "myDemos" || route.mode === "allDemos" || route.mode === "settings" || + route.mode === "apiTokens" || route.mode === "guide" ) return null; return new URLSearchParams(location.search).get("mode") === "full" ? route.id : null; @@ -453,6 +460,8 @@ export function App() { if (route.mode === "allDemos") return ; // Same story as My demos: auth-gated, renders no runtime, boots no container. if (route.mode === "settings") return ; + // Same again: auth-gated, no runtime, no container. + if (route.mode === "apiTokens") return ; // Login-gated like the two above: the guide describes what signing in unlocks, // and it is the account menu that offers it. if (route.mode === "guide") return ; @@ -499,6 +508,24 @@ function SettingsRoute() { return ; } +/** `/api-tokens` (DEV-2583, ADR-0037). A token acts as one person, and the + * listing is the team's, so the page needs an identity before it shows + * anything — the same login-on-anonymous contract as `/settings`. */ +function ApiTokensRoute() { + const [user, setUser] = useState(undefined); + useEffect(() => { + currentUser().then(setUser); + }, []); + useEffect(() => { + if (user === null) login(); // return_to preserves /api-tokens + }, [user]); + useDocumentTitle("API tokens"); + + if (user === undefined) return ; + if (user === null) return ; + return ; +} + /** `/guide` and `/guide/` (DEV-2503, tracks in DEV-2522). The content is the * markdown in `runner/docs/guide/`; this route only gates and frames it. */ function GuideRoute() { @@ -1069,6 +1096,12 @@ function Authoring({ * `16.2` is a version both the pencil and `?v=` accept, and the raw-string * reading this replaced found no `\d+\.` in either, answered null, and so * handed a v16 core the prerelease pass-through (DEV-2571). */ + /** Is this tab authenticated by a persistent API token rather than a login + * (ADR-0037)? Read once per render rather than held in state: the value can + * only change by a reload, since nothing in the app writes `hot_token` after + * `currentUser()` has consumed the redirect. */ + const tokenSession = isTokenSession(); + const themingSupported = (() => { // A ref the validator refuses is not themeable either. `selectedReleaseMajor` // answers null for one — the same null that waves prereleases through — so @@ -2694,6 +2727,9 @@ function Authoring({ // account menu that holds it renders only for an identified user. onUsage={() => { location.href = "/admin"; }} onSettings={() => { location.href = "/settings"; }} + // Disabled, not hidden, for a token session: the page explains itself + // when reached by URL, and a row that vanished would read as a bug. + onApiTokens={tokenSession ? undefined : () => { location.href = "/api-tokens"; }} onGuide={() => { location.href = "/guide"; }} // `edit` is auth-gated — `Gate` answers a null user with `login()`, so a // plain reload would bounce straight back to the broker. `play` and @@ -2718,16 +2754,24 @@ function Authoring({ // exactly what a shared link invites. Mutually exclusive: since DEV-2209 // they are literally the same surface — one `Drawer`, one `DRAWER_WIDTH` // (400) — on the same edge of the screen. + // Both are hidden outright for a session running on a persistent API + // token: the Worker fences that credential off `/api/chat` and + // `/api/theme` (ADR-0037), so the controls could only ever open a panel + // whose first request comes back 403. A disabled pair with a tooltip + // would be the kinder treatment for a person, but nobody arrives in a + // token session by accident — they pasted the token in. secondaryActions={ - <> - { setChatOpen((v) => !v); setStyleOpen(false); }} /> - { setStyleOpen((v) => !v); setChatOpen(false); }} - disabled={!themingSupported} - disabledReason={`Theming needs Handsontable ${THEME_API_MIN_MAJOR} or newer — this demo is on ${version}.`} - /> - + tokenSession ? null : ( + <> + { setChatOpen((v) => !v); setStyleOpen(false); }} /> + { setStyleOpen((v) => !v); setChatOpen(false); }} + disabled={!themingSupported} + disabledReason={`Theming needs Handsontable ${THEME_API_MIN_MAJOR} or newer — this demo is on ${version}.`} + /> + + ) } // ---- chrome (T2) -------------------------------------------------- examplePill={ diff --git a/runner/apps/authoring/src/Guide.tsx b/runner/apps/authoring/src/Guide.tsx index cfff9332b..0ce9547ce 100644 --- a/runner/apps/authoring/src/Guide.tsx +++ b/runner/apps/authoring/src/Guide.tsx @@ -21,7 +21,7 @@ import everyoneMarkdown from "../../../docs/guide/everyone.md?raw"; import supportMarkdown from "../../../docs/guide/support.md?raw"; import devrelMarkdown from "../../../docs/guide/devrel.md?raw"; import developersMarkdown from "../../../docs/guide/developers.md?raw"; -import { logout, type User } from "./auth.js"; +import { isTokenSession, logout, type User } from "./auth.js"; import { Markdown } from "./markdown.js"; import { GUIDE_TRACKS, @@ -67,6 +67,10 @@ export function GuidePage({ apiBase, user }: GuidePageProps) { accountAvatarUrl={profile?.avatar_url} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} + // Disabled for a session running on an API token: tokens are fenced off + // token management entirely, so the row would only lead to a page that + // explains it cannot be used (ADR-0037, Bugbot #252). + onApiTokens={isTokenSession() ? undefined : () => { location.href = "/api-tokens"; }} onGuide={() => { location.href = "/guide"; }} // Public target, as on Settings: this page sends a null user to `login()`, // so logging out to `/guide` would walk them straight back to the broker. diff --git a/runner/apps/authoring/src/MyDemos.tsx b/runner/apps/authoring/src/MyDemos.tsx index 6057ee03f..ea04532b9 100644 --- a/runner/apps/authoring/src/MyDemos.tsx +++ b/runner/apps/authoring/src/MyDemos.tsx @@ -42,7 +42,7 @@ import { Markdown } from "./markdown.js"; import { filterByOwner, isOwnedBy, ownerNameFromSlug, ownerOptions } from "./demoOwners.js"; import { assertApiOk, readApiJson } from "./api.js"; import { isSessionExpired } from "./apiError.js"; -import { getToken, login, logout, type User } from "./auth.js"; +import { getToken, isTokenSession, login, logout, type User } from "./auth.js"; import { displayNameFromEmail, initialFromEmail } from "./displayName.js"; import { fieldInput, fieldLabel, formFooter, ghostButton, primaryButton } from "./formStyles.js"; import { useProfile } from "./useProfile.js"; @@ -270,6 +270,10 @@ export function MyDemosPage({ apiBase, user, scope = "mine" }: MyDemosPageProps) accountAvatarUrl={ownerAvatar} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} + // Disabled for a session running on an API token: tokens are fenced off + // token management entirely, so the row would only lead to a page that + // explains it cannot be used (ADR-0037, Bugbot #252). + onApiTokens={isTokenSession() ? undefined : () => { location.href = "/api-tokens"; }} onGuide={() => { location.href = "/guide"; }} // Never a bare reload here: `/my-demos` answers a null user with // `login()`, so logging out in place would re-enter the broker. diff --git a/runner/apps/authoring/src/Settings.tsx b/runner/apps/authoring/src/Settings.tsx index f81f3b42c..a67e701d6 100644 --- a/runner/apps/authoring/src/Settings.tsx +++ b/runner/apps/authoring/src/Settings.tsx @@ -27,7 +27,7 @@ import { shellStyles, theme, } from "@handsontable/demo-editor-shell"; -import { logout, type User } from "./auth.js"; +import { isTokenSession, logout, type User } from "./auth.js"; import { displayNameFromEmail } from "./displayName.js"; import { removeAvatar, @@ -185,6 +185,10 @@ export function SettingsPage({ apiBase, user }: SettingsPageProps) { accountAvatarUrl={profile?.avatar_url} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} + // Disabled for a session running on an API token: tokens are fenced off + // token management entirely, so the row would only lead to a page that + // explains it cannot be used (ADR-0037, Bugbot #252). + onApiTokens={isTokenSession() ? undefined : () => { location.href = "/api-tokens"; }} onGuide={() => { location.href = "/guide"; }} // Public target, always: this page answers a null user with `login()`, // so a bare reload would walk the user who just logged out straight back diff --git a/runner/apps/authoring/src/apiError.ts b/runner/apps/authoring/src/apiError.ts index 917e0f28e..67736502c 100644 --- a/runner/apps/authoring/src/apiError.ts +++ b/runner/apps/authoring/src/apiError.ts @@ -32,6 +32,11 @@ export type ApiFailureKind = "session-expired" | "forbidden" | "other"; export const SESSION_EXPIRED_MESSAGE = "Your session expired. Sign in again to continue."; export const FORBIDDEN_MESSAGE = "This demo belongs to someone else — only its owner can change it."; +/** The fallback for a capability refusal that arrived without its `detail` + * (DEV-2583). Every route that sends `token_forbidden` sends one, but a proxy + * or a truncated body must not produce an empty toast. */ +export const CAPABILITY_DENIED_MESSAGE = + "An API token cannot do this. Sign in to continue."; /** Whatever JSON the Worker put in the error body. Both fields are optional * because a 401 from a proxy, or a body that failed to parse, has neither. */ @@ -108,10 +113,19 @@ export function describeApiFailure( return new ApiError(SESSION_EXPIRED_MESSAGE, status, "session-expired", false); } if (status === 403) { + const detail = typeof body.detail === "string" ? body.detail.trim() : ""; + // A persistent API token's capability fence shares the status with the + // ownership checks and means something else entirely: nothing belongs to + // anybody else, this credential simply may not do this (ADR-0037). So the + // detail becomes the whole sentence rather than a parenthetical on the + // ownership copy, and it is not reportable — the fence refusing is the fence + // working, not the UI and the server disagreeing about who owns a row. + if (body.error === "token_forbidden") { + return new ApiError(detail || CAPABILITY_DENIED_MESSAGE, status, "forbidden", false); + } // The ownership refusals the browser actually hits send a bare // `{"error":"forbidden"}` (index.ts:887 and :954), so ownership is the // primary sentence and `detail` — sent only by the MCP route — refines it. - const detail = typeof body.detail === "string" ? body.detail.trim() : ""; const message = detail ? `${FORBIDDEN_MESSAGE} (${detail})` : FORBIDDEN_MESSAGE; return new ApiError(message, status, "forbidden", true); } diff --git a/runner/apps/authoring/src/auth.ts b/runner/apps/authoring/src/auth.ts index 9ead02279..809e72dd8 100644 --- a/runner/apps/authoring/src/auth.ts +++ b/runner/apps/authoring/src/auth.ts @@ -1,11 +1,22 @@ // Handsontable Google login broker client (ADR-0007). Internal team only — // the broker rejects non-@handsontable.com accounts. The token is per-user, // per-session; kept in sessionStorage, never persisted or logged. +// +// Since ADR-0037 a session may instead hold a persistent API token, which the +// broker knows nothing about — identity for one is resolved against our own API. +// That is what lets the live share spec drive the real Share button with a +// credential that does not expire; the cost, stated in the ADR, is that such a +// token is a browser session in a string, so it is fenced off the AI features +// and the admin writes on the server side. import { reportError } from "./sentry.js"; const BROKER = import.meta.env.VITE_LOGIN_BROKER_URL || "https://mcp-auth-proxy-j0tb.onrender.com"; +const API_BASE = import.meta.env.VITE_API_BASE || "http://localhost:8787"; const TOKEN_KEY = "hot_token"; +/** Mirrors `PAT_PREFIX` in the Worker's `token.ts` — the two must agree, and a + * cross-package import from the app into `workers/` does not exist. */ +const PAT_PREFIX = "hot_pat_"; /** The cached profile (DEV-2166). Declared beside the token because it shares * the token's lifetime, and because `logout()` has to know to drop it — see * `profile.ts` for why the cache exists at all. */ @@ -21,6 +32,16 @@ export function getToken(): string | null { return sessionStorage.getItem(TOKEN_KEY); } +/** + * Is this session running on a persistent API token rather than a login? + * + * Read by the surfaces the server fences off (Ask AI, the style generator): a + * feature that would answer 403 is better not offered than offered and refused. + */ +export function isTokenSession(): boolean { + return getToken()?.startsWith(PAT_PREFIX) ?? false; +} + /** Resolve the current user: consume a fresh #token from the broker redirect, * else use the stored token. Returns null if not signed in / token invalid. */ export async function currentUser(): Promise { @@ -41,8 +62,16 @@ export async function currentUser(): Promise { } if (!token) return null; + // A persistent API token is ours, not the broker's, so identity comes from our + // own API. `GET /api/profile` already answers with the caller's verified + // address for any authenticated request, which is exactly what a `User` needs + // — hence no separate identity endpoint (ADR-0037). + const endpoint = token.startsWith(PAT_PREFIX) + ? `${API_BASE}/api/profile` + : `${BROKER}/broker/userinfo`; + try { - const res = await fetch(`${BROKER}/broker/userinfo`, { + const res = await fetch(endpoint, { headers: { Authorization: `Bearer ${token}` }, }); if (!res.ok) { diff --git a/runner/apps/authoring/src/tokens.ts b/runner/apps/authoring/src/tokens.ts new file mode 100644 index 000000000..7a7de305b --- /dev/null +++ b/runner/apps/authoring/src/tokens.ts @@ -0,0 +1,63 @@ +// Client for the persistent API tokens routes (DEV-2583, ADR-0037). +// +// Shaped like `profile.ts`: inline `fetch`, the shared `readApiJson` / +// `assertApiOk` describers, and the same `authHeaders()` idiom. There is no +// caching here on purpose — a listing of live credentials must never be painted +// from a stale copy, because the one question it answers is "what is still able +// to act as us right now". + +import { assertApiOk, readApiJson } from "./api.js"; +import { getToken } from "./auth.js"; + +/** A token as the listing shows it. No digest, no plaintext — the server never + * selects the former and only ever answers the latter once, on mint. */ +export interface ApiToken { + id: string; + name: string; + created_by: string; + created_at: string; + last_used_at: string | null; + revoked_at: string | null; + revoked_by: string | null; +} + +/** The mint response: the listing row plus the one and only sight of the token. */ +export interface MintedToken extends ApiToken { + token: string; +} + +function authHeaders(): Record { + const token = getToken(); + return token ? { Authorization: `Bearer ${token}` } : {}; +} + +const FALLBACK = (status: number) => `Request failed (${status}).`; + +export async function fetchTokens(apiBase: string): Promise { + const body = await readApiJson<{ tokens: ApiToken[] }>( + await fetch(`${apiBase}/api/tokens`, { headers: authHeaders(), cache: "no-store" }), + FALLBACK, + ); + return body.tokens; +} + +export async function mintApiToken(apiBase: string, name: string): Promise { + return readApiJson( + await fetch(`${apiBase}/api/tokens`, { + method: "POST", + headers: { "Content-Type": "application/json", ...authHeaders() }, + body: JSON.stringify({ name }), + }), + FALLBACK, + ); +} + +export async function revokeApiToken(apiBase: string, id: string): Promise { + await assertApiOk( + await fetch(`${apiBase}/api/tokens/${encodeURIComponent(id)}`, { + method: "DELETE", + headers: authHeaders(), + }), + FALLBACK, + ); +} diff --git a/runner/apps/authoring/vite.config.ts b/runner/apps/authoring/vite.config.ts index e3a898018..8cbb90e33 100644 --- a/runner/apps/authoring/vite.config.ts +++ b/runner/apps/authoring/vite.config.ts @@ -108,11 +108,17 @@ export default defineConfig({ // // Set `VITE_API_BASE=http://localhost:5173` to route through this. An empty // value does not work — `App.tsx` falls back to :8787 on any falsy value. - // `/d` is a regex, not a prefix string: a bare "/d" key matches every path - // that *starts* with it, which swallows `public/docs-examples/` (the docs - // snapshots the picker loads) and 404s it against the worker. + // `/d` and `/api` are regexes, not prefix strings: a bare key matches every + // path that *starts* with it. For `/d` that swallowed `public/docs-examples/` + // (the docs snapshots the picker loads) and 404'd it against the worker; for + // `/api` it swallowed the `/api-tokens` page (DEV-2583), which proxied to a + // worker that has no such route and 500'd where production serves the SPA. + // The production route really is `demos.handsontable.com/api/*` (see the + // `--routes` flags in workers/api/package.json), so the bare prefix was + // always wider here than on the deployment it stands in for. `/embed` has + // the same shape but nothing is named as a sibling of it today. proxy: { - "/api": { target: "http://localhost:8787" }, + "^/api(?:/|$)": { target: "http://localhost:8787" }, "^/d(?:/|$)": { target: "http://localhost:8787" }, "/embed": { target: "http://localhost:8787" }, }, diff --git a/runner/docs/TESTING.md b/runner/docs/TESTING.md index c14c70ce9..b722977a9 100644 --- a/runner/docs/TESTING.md +++ b/runner/docs/TESTING.md @@ -105,7 +105,7 @@ covers the dependency: | *(none)* | Only the built SPA — shell, routing, stubbed network | `ci.yml` on every PR | The default. If you can stub it, don't gate it. | | `E2E_LIVE=1` | A real preview mount — the hosted Sandpack bundler or a Tier-2 container | `e2e-live.yml` (manual + canary/smoke once [#189](https://github.com/handsontable/examples/pull/189) lands) | Off by default so an external-bundler outage never blocks merges. | | `E2E_BASE_URL` | Worker routes (`/api`, `/d`, `/embed`) — `vite preview` has none, so the spec self-skips without it | `e2e-live.yml` pointed at a deployment | `share-view.spec.ts` also needs the permanent fixture demo (`FIXTURE_ID`) — never revoke it. | -| `E2E_BROKER_TOKEN` | An authed write round-trip against the real broker (`share-create-live.spec.ts`, lands with [#186](https://github.com/handsontable/examples/pull/186)) | `e2e-live.yml` | Broker tokens expire and cannot be minted programmatically; an expired token is a warning, not a failure. | +| `E2E_API_TOKEN` | An authed write round-trip against the deployed API (`share-create-live.spec.ts`, `session-abandoned-create.spec.ts`) | `e2e-live.yml` | A persistent API token minted on `/api-tokens` (ADR-0037). It does not expire, so a token that stops validating **fails** the run — it means revoked or broken. Absent, the step is skipped. | | `E2E_AI=1` | A live LLM answer (`ai-live.spec.ts`, lands with [#187](https://github.com/handsontable/examples/pull/187)) | `e2e-live.yml` weekly canary | Real budget, shared 8/min-per-IP rate bucket — a 429 skips rather than fails. | | `E2E_STARTER_MATRIX=1` | Every starter × major through a live container session | `e2e-starter-matrix.yml` (manual + monthly) | Serialized against the global container cap; never fold matrix cases into the PR suite. | diff --git a/runner/docs/adr/0037-persistent-api-tokens.md b/runner/docs/adr/0037-persistent-api-tokens.md new file mode 100644 index 000000000..97ca438a8 --- /dev/null +++ b/runner/docs/adr/0037-persistent-api-tokens.md @@ -0,0 +1,150 @@ +# ADR-0037: Persistent API tokens, verified in the Worker rather than by the broker + +**Status:** Accepted (DEV-2583; amends the scope of 0007) + +## Context + +The nightly live canary has one step it almost never runs. `e2e-live.yml` guards the +authed share round-trip — the only test that drives the builder, R2 and D1 end to end +against production — behind `secrets.E2E_BROKER_TOKEN`, and that secret is a **per-user +browser session JWT copied by hand out of `sessionStorage.hot_token`**. It expires in +about an hour. The workflow knows this and treats a dead token as a `::notice::` and a +green run, because "the secret rots by design and rot is not a product failure". The +honest reading is that the step is skipped every night and the coverage is theoretical. + +There is no mint path to automate. Every credential this runner accepts today is a +broker session token: `authenticate()` does not verify a JWT at all, it forwards the +bearer verbatim to the login broker's `/broker/userinfo` and trusts the address that +comes back (ADR-0007). Nothing in the system can issue a credential, and nothing in the +system can validate one it did not get from the broker. + +Two properties of the surrounding code constrain any answer: + +- **There is no user table and no organization model.** The organization is the string + test `email.endsWith("@handsontable.com")`; ownership is `sameOwner(created_by, email)`; + `profiles` is keyed on the address because "email is the only stable identifier we + hold" (`0005_profiles.sql`). A token has to live in that world rather than found a + parallel one. +- **Any team member is already an admin.** `PUT /api/admin/settings` changes the spend + ceiling and the enforcement switch behind nothing but `authenticate() !== null`, which + `admin.ts` records as deliberate — spend figures are internal, not secret. That is a + defensible position for a human who signed in through Google this morning. It is a + different proposition for a never-expiring string sitting in a **public** repository's + secrets. + +The rejected alternative was to give the CI job a longer-lived broker token, or a broker +service account. It was rejected for the reason ADR-0033 gives for the MCP: that hands a +machine the broker's own authority, and widens the set of things that break when the +broker does. What CI needs is permission to exercise this API, not the ability to present +itself as a person to every system that trusts the broker. + +## Decision + +**A first-party credential — `hot_pat__` — minted from the app, stored as a +hash, verified inside the Worker, and revocable by anyone on the team.** + +- **The token is `hot_pat_` + a public id + a secret.** The id is 16 hex characters from + 8 random bytes and the secret is base64url over 32 more, both from + `crypto.getRandomValues`. The id is deliberately public: it is the D1 primary key, it + is what the UI shows for the rest of the token's life, and it is what + `DELETE /api/tokens/:id` names. Nothing anywhere needs to handle the plaintext twice. +- **The prefix is checked before anything touches the network, and the two paths never + fall through to one another.** This is a security property rather than a latency win: a + bearer that reached the broker's `/broker/userinfo` because the local lookup missed + would have shipped our own permanent credential to a third-party host on Render, and + the failure would look like a slow success. +- **Only a SHA-256 hex digest of the token string is stored.** The plaintext is in the + mint response and nowhere else — not in the row, not in a log, not in the listing. The + reflex objection here is bcrypt or argon2, and the answer is that this is a 256-bit + random secret rather than a password: there is no dictionary to run, no low-entropy + guess space to grind, and the id makes the lookup a primary-key hit, so there is + nothing a work factor would be defending. A stretched hash would cost every + authenticated request and buy nothing. +- **Verification is one indexed read plus the existing constant-time compare.** + `secretsMatch()` already exists for `MCP_SHARED_SECRET` and is reused unchanged over + the two digests. Revocation is read in the same row, so it takes effect on the next + request with no cache to invalidate. +- **A token acts as its creator's address.** The token path returns + `{ email: row.created_by, via: row.id }`, so `sameOwner()`, `created_by`, `?scope=mine` + and "My demos" keep working with no new identity shape — and, more to the point, no new + address that `endsWith("@handsontable.com")` would have to be taught to accept. The + `via` field is the audit trail and the thing the capability guard reads; nothing that + only wants `identity.email` changes at all. The cost is stated plainly: a demo created + by CI is indistinguishable in the listing from one its owner built by hand, and it + outlives their involvement. +- **A token may do what a person may, minus four things.** No `PUT`/`DELETE` on + `/api/admin/*`, no `POST /api/chat`, no `POST /api/theme`, and no token management at + all — reads included. So a leaked token cannot raise the spend ceiling, cannot turn + enforcement off, cannot burn AI budget, and — the one that matters most — cannot mint + itself a successor or revoke the tokens that would be used to kill it. `GET + /api/tokens` is fenced along with the writes rather than left open for the CI + preflight's convenience: the listing carries no digests, but it names every credential + in the organization and its owner, and that is reconnaissance rather than a credential + check. The preflight asks `/api/profile` instead, which verifies the bearer — itself an + `api_tokens` read — without enumerating anything. `GET /api/admin/*` does stay open, + because the session-leak spec reads `/api/admin/sessions` and reading internal spend + figures is what `admin.ts` already says it is. +- **The fence is a fixed rule, not a scope field.** Per-token scopes were considered and + dropped: this repo has never had a permission model, one consumer exists, and a + configurable fence is a thing to get wrong at mint time. When a second consumer needs + something different, that is the moment to design scopes. +- **Every token is visible to, and revocable by, the whole team.** `GET /api/tokens` + lists all of them with creator, timestamps and who revoked what; any signed-in team + member may revoke any of them. This is the one place where the codebase's standing + warning — that `?scope=all` is visibility and must never quietly become permission + (`demos-list.ts`) — is knowingly set aside rather than overlooked. A permanent + credential nobody but its author can kill is worse than one anybody on the team can: + the person who minted it will eventually be on holiday, and the token will not expire + on their behalf. +- **`last_used_at` is coarsened to the hour and written atomically.** A single + `UPDATE … WHERE id = ? AND (last_used_at IS NULL OR last_used_at < ?)` bounds the hot + auth path to one effective write per token per hour, needs no `ctx` threaded through + the two dozen `authenticate()` call sites, and has no read-then-write race to reason + about under concurrency. +- **The client accepts one too.** `currentUser()` prefix-branches to the API's own + `GET /api/profile` instead of the broker's `/broker/userinfo`; `ProfileView` already + carries `email`, taken straight from the verified identity, which is exactly what the + caller needs. No new `/api/me` route. This is what keeps the live spec exercising the + real Share button rather than being rewritten into an API script, and it is the part of + this decision with the sharpest edge — see below. + +## Consequences + +The nightly canary can run unattended, and the credential it uses can be killed from the +app by whoever notices first. The token path also costs one D1 read where the broker path +costs a cross-Atlantic fetch to Render, so it is faster and it does not care whether the +broker is up. + +**Trade-offs and follow-ups:** + +- **A token is a browser session in a string.** Because the client accepts it, anyone + holding one can paste it into `sessionStorage` in a console and have a signed-in tab + that never expires. The capability fence, org-wide revocation and the fact that the + domain-suffix gate already means "team member equals broad authority" bound this; they + do not remove it. Choosing the other branch — a server-only credential — would have + removed it at the price of rewriting the one spec that proves the UI path works, and + the coverage was judged worth more than the theoretical narrowing. +- **The `e2e-live.yml` trace scrubbing is now load-bearing rather than tidy.** That run + puts the credential in `sessionStorage` and in an `Authorization` header, a Playwright + trace records both, GitHub does not redact secrets inside artifact zips, and this repo + is public. Before, a leaked trace exposed a token with an hour to live. Now it exposes + one with no expiry at all. `--trace off` and the artifact `rm -rf` stay, and the reason + they exist has gone up in severity. +- **Token failure stops being rot.** A broker token that stopped validating meant a week + had passed; a persistent token that stops validating means it was revoked, deleted, or + something is broken. So the workflow's preflight fails the run instead of warning and + passing — which is the point of the ticket, and also means the first thing this feature + can do is turn a permanently-green step red. +- **A token session in the SPA will 403 on Chat and the theme generator, and the existing + classifier calls every 403 an ownership problem.** `apiError.ts` renders "This demo + belongs to someone else" and marks it reportable, so an unguarded capability denial + would show the wrong sentence and open a Sentry issue on every click. The session is at + least not wrongly cleared — only a 401 does that. The answer is that these routes send + a `detail`, which that branch already appends, and that the UI does not offer AI + features to a token session in the first place. +- **The admin model is fenced, not fixed.** "Any team member is an admin" is untouched + here; this decision only declines to extend it to machines. Introducing real roles is + its own ticket, and this ADR is not a substitute for it. +- **No expiry, no rotation endpoint, no per-token scopes.** Rotation is mint-then-revoke, + which is two clicks and needs no code. An optional expiry is the obvious first + extension if these ever leave CI and start living in people's shell profiles. diff --git a/runner/docs/adr/README.md b/runner/docs/adr/README.md index ba3fc4a1d..60468b6df 100644 --- a/runner/docs/adr/README.md +++ b/runner/docs/adr/README.md @@ -41,3 +41,4 @@ once Accepted. | [0034](0034-role-based-guide-tracks.md) | The guide is four role-based tracks, not one document | Accepted (supersedes the DEV-2503 single-document shape) | | [0035](0035-the-shell-drives-the-preview-colour-scheme.md) | The shell drives the preview's colour scheme, unless the demo declares one | Accepted (supersedes part of 0028) | | [0036](0036-the-api-owns-the-handsontable-version.md) | The API owns the Handsontable version — derive from the payload, never default to a dist-tag | Accepted (amends 0005 with who applies it) | +| [0037](0037-persistent-api-tokens.md) | Persistent API tokens, verified in the Worker rather than by the broker | Accepted (DEV-2583; amends 0007) | diff --git a/runner/docs/run-and-deploy.md b/runner/docs/run-and-deploy.md index 4aee0d1dc..f16acab00 100644 --- a/runner/docs/run-and-deploy.md +++ b/runner/docs/run-and-deploy.md @@ -41,6 +41,7 @@ npx wrangler d1 execute handsontable-demos --local --file=migrations/0002_buildk npx wrangler d1 execute handsontable-demos --local --file=migrations/0003_cost_ledger.sql -y npx wrangler d1 execute handsontable-demos --local --file=migrations/0004_settings_and_analytics.sql -y npx wrangler d1 execute handsontable-demos --local --file=migrations/0005_profiles.sql -y +npx wrangler d1 execute handsontable-demos --local --file=migrations/0006_api_tokens.sql -y npx wrangler dev --port 8787 # builds the container images # then run the authoring app pointing at it: cd ../../apps/authoring @@ -85,6 +86,7 @@ npx wrangler d1 execute handsontable-demos --remote --file=migrations/0002_build npx wrangler d1 execute handsontable-demos --remote --file=migrations/0003_cost_ledger.sql -y npx wrangler d1 execute handsontable-demos --remote --file=migrations/0004_settings_and_analytics.sql -y npx wrangler d1 execute handsontable-demos --remote --file=migrations/0005_profiles.sql -y +npx wrangler d1 execute handsontable-demos --remote --file=migrations/0006_api_tokens.sql -y pnpm run deploy # wrangler deploy --routes … (attaches the demos.handsontable.com routes) # -> https://demos.handsontable.com (plus the account's own *.workers.dev URL) diff --git a/runner/e2e/api-tokens.spec.ts b/runner/e2e/api-tokens.spec.ts new file mode 100644 index 000000000..deef14575 --- /dev/null +++ b/runner/e2e/api-tokens.spec.ts @@ -0,0 +1,382 @@ +import { test, expect, type Page } from "@playwright/test"; + +// The API tokens page (DEV-2583, ADR-0037) — mint, reveal once, revoke. +// +// Deterministic — no `E2E_LIVE=1`: the page renders no example, so nothing here +// needs the bundler. The API is stubbed in the browser-side route handler and +// holds its state there, so a revoke is read back the way the server would +// answer it rather than asserted against the response the page already has. +// +// Sign-in is faked at the *token* layer, for the reason `settings.spec.ts` sets +// out: a build-layer `VITE_DEV_USER` bypass leaks into production builds through +// `.env.local`, which would make the anonymous case below pass while proving +// nothing. + +const EMAIL = "dev@handsontable.com"; +const OTHER = "someone.else@handsontable.com"; + +type TokenRow = { + id: string; + name: string; + created_by: string; + created_at: string; + last_used_at: string | null; + revoked_at: string | null; + revoked_by: string | null; +}; + +const row = (over: Partial = {}): TokenRow => ({ + id: "0123456789abcdef", + name: "nightly e2e", + created_by: EMAIL, + created_at: "2026-08-01T09:00:00.000Z", + last_used_at: null, + revoked_at: null, + revoked_by: null, + ...over, +}); + +/** The plaintext the stub mints. Shaped like the real thing (`hot_pat_` + 16 hex + * + `_` + 43 base64url chars) so an assertion about "the secret" is about a + * string the Worker would actually have produced. */ +const MINTED_ID = "fedcba9876543210"; +const MINTED_SECRET = "A".repeat(43); +const MINTED = `hot_pat_${MINTED_ID}_${MINTED_SECRET}`; + +async function signIn(page: Page) { + await page.addInitScript(() => sessionStorage.setItem("hot_token", "e2e-token")); + await page.route("**/broker/userinfo", (route) => route.fulfill({ json: { email: EMAIL } })); + await page.route("**/broker/login**", (route) => route.abort()); +} + +async function stubProfile(page: Page) { + await page.route("**/api/profile", (route) => + route.fulfill({ + json: { + email: EMAIL, + display_name: "Dev", + saved_name: null, + description: null, + avatar_url: null, + initial: "D", + }, + })); +} + +/** A tokens server living in the route handler. Returns the calls it saw, so a + * test can assert the page actually asked rather than rendered from its own + * optimism. */ +async function stubTokensApi(page: Page, seed: TokenRow[] = []) { + const state = [...seed]; + const calls: string[] = []; + + await page.route("**/api/tokens", async (route) => { + const method = route.request().method(); + calls.push(`${method} /api/tokens`); + if (method === "GET") return route.fulfill({ json: { tokens: state } }); + if (method === "POST") { + const { name } = JSON.parse(route.request().postData() ?? "{}") as { name: string }; + const created = row({ id: MINTED_ID, name, created_at: "2026-08-21T10:00:00.000Z" }); + state.unshift(created); + // The mint response is the row plus the one and only sight of the token. + return route.fulfill({ status: 201, json: { ...created, token: MINTED } }); + } + return route.fallback(); + }); + + await page.route("**/api/tokens/*", async (route) => { + const id = new URL(route.request().url()).pathname.split("/").pop()!; + calls.push(`${route.request().method()} /api/tokens/${id}`); + const target = state.find((t) => t.id === id); + if (!target) return route.fulfill({ status: 404, json: { error: "not found" } }); + target.revoked_at = "2026-08-21T11:00:00.000Z"; + target.revoked_by = EMAIL; + return route.fulfill({ status: 204, body: "" }); + }); + + return { calls, state }; +} + +const nameField = (page: Page) => page.getByLabel("Name"); +const createButton = (page: Page) => page.getByRole("button", { name: "Create token" }); + +test.describe("/api-tokens", () => { + test("the page frames itself like the other account pages", async ({ page }) => { + await signIn(page); + await stubProfile(page); + await stubTokensApi(page); + await page.goto("/api-tokens"); + + await expect(page.getByRole("heading", { name: "API tokens" })).toBeVisible(); + const nav = page.getByRole("navigation", { name: "Account" }); + await expect(nav.getByRole("link", { name: "My demos" })).toBeVisible(); + await expect(nav.getByRole("link", { name: "API tokens" })).toHaveAttribute("aria-current", "page"); + await expect(nameField(page)).toBeVisible(); + }); + + test("the account menu and the left nav both reach the page", async ({ page }) => { + await signIn(page); + await stubProfile(page); + await stubTokensApi(page); + await page.route("**/api/demos?scope=*", (route) => route.fulfill({ json: { demos: [] } })); + await page.goto("/my-demos"); + + const nav = page.getByRole("navigation", { name: "Account" }); + const link = nav.getByRole("link", { name: "API tokens" }); + await expect(link).not.toHaveAttribute("aria-current", "page"); + await link.click(); + await expect(page).toHaveURL(/\/api-tokens$/); + await expect(page.getByRole("heading", { name: "API tokens" })).toBeVisible(); + + await page.getByRole("button", { name: `Account: ${EMAIL}` }).click(); + await expect(page.getByRole("menuitem", { name: "API tokens" })).toBeVisible(); + }); + + test("minting shows the token once, and the list only ever shows the masked form", async ({ page }) => { + await signIn(page); + await stubProfile(page); + const api = await stubTokensApi(page); + await page.goto("/api-tokens"); + + await nameField(page).fill("nightly e2e"); + await createButton(page).click(); + + // The reveal: the whole plaintext, exactly once, with the warning that says so. + const reveal = page.getByRole("textbox", { name: "Your new API token" }); + await expect(reveal).toHaveValue(MINTED); + await expect(page.getByText(/only time it is shown/i)).toBeVisible(); + + // The row is in the list, masked — the secret is not on the page twice. + await expect(page.getByText(`hot_pat_${MINTED_ID}_••••••••`)).toBeVisible(); + expect(api.calls).toContain("POST /api/tokens"); + + // A reload is meant to lose it: nothing persisted the plaintext, and the + // server cannot answer with it again. + await page.reload(); + await expect(page.getByRole("textbox", { name: "Your new API token" })).toHaveCount(0); + await expect(page.getByText(`hot_pat_${MINTED_ID}_••••••••`)).toBeVisible(); + expect(await page.content()).not.toContain(MINTED_SECRET); + }); + + test("Create is inert until the token has a name", async ({ page }) => { + await signIn(page); + await stubProfile(page); + await stubTokensApi(page); + await page.goto("/api-tokens"); + + await expect(createButton(page)).toBeDisabled(); + await nameField(page).fill(" "); + await expect(createButton(page), "whitespace is not a name").toBeDisabled(); + await nameField(page).fill("nightly e2e"); + await expect(createButton(page)).toBeEnabled(); + }); + + test("revoking asks first, then says who killed it", async ({ page }) => { + await signIn(page); + await stubProfile(page); + const api = await stubTokensApi(page, [row()]); + await page.goto("/api-tokens"); + + await page.getByRole("button", { name: "Revoke" }).click(); + const dialog = page.getByRole("dialog", { name: "Revoke this token?" }); + await expect(dialog).toBeVisible(); + + // Cancel leaves it alone — the confirmation is a real gate, not decoration. + await dialog.getByRole("button", { name: "Cancel" }).click(); + await expect(dialog).toBeHidden(); + expect(api.calls.filter((c) => c.startsWith("DELETE"))).toEqual([]); + + await page.getByRole("button", { name: "Revoke" }).click(); + await page.getByRole("dialog").getByRole("button", { name: "Revoke" }).click(); + + await expect(page.getByText(`revoked by ${EMAIL}`)).toBeVisible(); + // The row stays: it is the audit trail, so revoking must not hide it. + await expect(page.getByText("nightly e2e")).toBeVisible(); + // And there is nothing left to revoke on it. + await expect(page.getByRole("button", { name: "Revoke" })).toHaveCount(0); + expect(api.calls).toContain("DELETE /api/tokens/0123456789abcdef"); + }); + + test("the revoke confirmation opens on Cancel, so Enter cannot revoke unanswered", async ({ page }) => { + // `Dialog` focuses the content's first focusable unless something is marked + // `data-autofocus`, and the first control here is the destructive one. Without + // the marker, opening the dialog and pressing Enter revokes a live credential + // without the question ever being answered. + await signIn(page); + await stubProfile(page); + const api = await stubTokensApi(page, [row()]); + await page.goto("/api-tokens"); + + await page.getByRole("button", { name: "Revoke" }).click(); + await expect(page.getByRole("dialog", { name: "Revoke this token?" })).toBeVisible(); + + await expect( + page.getByRole("dialog").getByRole("button", { name: "Cancel" }), + "focus lands on Cancel, not on Revoke", + ).toBeFocused(); + + await page.keyboard.press("Enter"); + expect(api.calls.filter((c) => c.startsWith("DELETE")), "Enter did not revoke").toEqual([]); + await expect(page.getByRole("dialog")).toBeHidden(); + }); + + test("a listing that fails says so instead of claiming there are no tokens", async ({ page }) => { + // The page's whole job is enumerating live credentials. Telling a reader + // "No tokens yet" when the request failed asserts the one thing it cannot know. + await signIn(page); + await stubProfile(page); + await page.route("**/api/tokens", (route) => + route.fulfill({ status: 500, json: { error: "boom" } })); + + await page.goto("/api-tokens"); + await expect(page.getByText(/could not be loaded/i)).toBeVisible(); + await expect(page.getByText("No tokens yet.")).toHaveCount(0); + }); + + test("a token minted after a failed listing is still shown, so it can be revoked", async ({ page }) => { + // Create is deliberately live after a load failure, so the row it produces + // has to be reachable: a credential that exists and cannot be revoked from + // the page is the worst state this feature can be in. The failure notice + // stays too — the list is still not known to be complete. + await signIn(page); + await stubProfile(page); + const calls: string[] = []; + await page.route("**/api/tokens", (route) => { + const method = route.request().method(); + calls.push(method); + if (method === "GET") return route.fulfill({ status: 500, json: { error: "boom" } }); + const created = row({ id: MINTED_ID, name: "after failure" }); + return route.fulfill({ status: 201, json: { ...created, token: MINTED } }); + }); + + await page.goto("/api-tokens"); + await expect(page.getByText(/could not be loaded/i)).toBeVisible(); + + await nameField(page).fill("after failure"); + await createButton(page).click(); + + await expect(page.getByRole("textbox", { name: "Your new API token" })).toHaveValue(MINTED); + await expect(page.getByText("after failure")).toBeVisible(); + await expect(page.getByText(`hot_pat_${MINTED_ID}_••••••••`)).toBeVisible(); + await expect( + page.getByRole("button", { name: "Revoke" }), + "the token just minted can be revoked", + ).toBeEnabled(); + await expect( + page.getByText(/could not be loaded/i), + "and the page still admits the list is incomplete", + ).toBeVisible(); + }); + + test("somebody else's token is listed, and revocable, because revocation is team-wide", async ({ page }) => { + // The deliberate departure recorded in ADR-0037: a permanent credential only + // its author can kill is worse than one anybody on the team can. + await signIn(page); + await stubProfile(page); + await stubTokensApi(page, [row({ id: "aaaabbbbccccdddd", name: "their script", created_by: OTHER })]); + await page.goto("/api-tokens"); + + await expect(page.getByText("their script")).toBeVisible(); + await expect(page.getByText(OTHER)).toBeVisible(); + await expect(page.getByRole("button", { name: "Revoke" })).toBeEnabled(); + }); + + test("a token session is not offered the AI features", async ({ page }) => { + // The Worker fences a token off /api/chat and /api/theme, so the controls + // would only ever open a panel whose first request comes back 403. + await page.addInitScript((token) => sessionStorage.setItem("hot_token", token), MINTED); + await page.route("**/api/profile", (route) => route.fulfill({ json: { email: EMAIL, display_name: "Dev", saved_name: null, description: null, avatar_url: null, initial: "D" } })); + // The broker must never be asked about this credential — that is the whole + // point of resolving a token's identity against our own API. + let brokerCalls = 0; + await page.route("**/broker/userinfo", (route) => { brokerCalls += 1; return route.fulfill({ status: 401, json: {} }); }); + + await page.goto("/"); + await expect(page.getByRole("button", { name: `Account: ${EMAIL}` })).toBeVisible(); + expect(brokerCalls, "a token's identity comes from /api/profile, not the broker").toBe(0); + // `exact` on both: role-name matching is substring by default, and the file + // tree's `styles.css` button matches a bare "Style". + await expect(page.getByRole("button", { name: "Ask AI", exact: true })).toHaveCount(0); + await expect(page.getByRole("button", { name: "Style", exact: true })).toHaveCount(0); + }); + + test("a token session gets an explanation here, not controls that would 403", async ({ page }) => { + // A token is fenced off token management entirely (ADR-0037), so the page + // must not offer a form and a list whose every request comes back 403. + await page.addInitScript((token) => sessionStorage.setItem("hot_token", token), MINTED); + await stubProfile(page); + await page.route("**/broker/userinfo", (route) => route.fulfill({ status: 401, json: {} })); + // A tripwire, not a stub: the page must not ask at all. + let listCalls = 0; + await page.route("**/api/tokens", (route) => { + listCalls += 1; + return route.fulfill({ status: 403, json: { error: "token_forbidden", detail: "An API token cannot read the token list." } }); + }); + + await page.goto("/api-tokens"); + await expect(page.getByRole("heading", { name: "API tokens" })).toBeVisible(); + await expect(page.getByText(/not available to a session signed in with an API token/i)).toBeVisible(); + await expect(createButton(page)).toHaveCount(0); + await expect(nameField(page)).toHaveCount(0); + expect(listCalls, "the page does not ask for a listing it may not have").toBe(0); + }); + + test("Create waits for the listing, so a mint cannot be overwritten by it", async ({ page }) => { + // Minting before the mount GET resolves would prepend the new row and then + // have the older response replace state with the pre-mint snapshot. + await signIn(page); + await stubProfile(page); + let release = () => {}; + const held = new Promise((r) => { release = r; }); + await page.route("**/api/tokens", async (route) => { + if (route.request().method() === "GET") { + await held; + return route.fulfill({ json: { tokens: [] } }); + } + return route.fallback(); + }); + + await page.goto("/api-tokens"); + await nameField(page).fill("nightly e2e"); + await expect(createButton(page), "inert while the listing is in flight").toBeDisabled(); + release(); + await expect(createButton(page)).toBeEnabled(); + }); + + test("a login session still gets the AI features", async ({ page }) => { + // The control for the test above: without it, a bug that hid Ask AI from + // everybody would read as the fence working. + await signIn(page); + await stubProfile(page); + await page.goto("/"); + + await expect(page.getByRole("button", { name: `Account: ${EMAIL}` })).toBeVisible(); + await expect(page.getByRole("button", { name: "Ask AI", exact: true })).toBeVisible(); + await expect(page.getByRole("button", { name: "Style", exact: true })).toBeVisible(); + }); + + test("an anonymous visitor is sent to the broker with /api-tokens preserved", async ({ page }) => { + await page.route("**/broker/userinfo", (route) => route.fulfill({ status: 401, json: {} })); + const brokerUrls: string[] = []; + await page.route("**/broker/login**", (route) => { + brokerUrls.push(route.request().url()); + return route.abort(); + }); + + await page.goto("/api-tokens"); + await expect.poll(() => brokerUrls.length, { timeout: 5_000 }).toBeGreaterThan(0); + expect(decodeURIComponent(brokerUrls[0]!)).toContain("/api-tokens"); + }); + + test("a hard load of /api-tokens resolves to the page, not the playground", async ({ page }) => { + // The `parseRoute()` ordering hazard: the SPA fallback serves index.html for + // every path, so a missing regex renders the editor instead of 404ing. + await signIn(page); + await stubProfile(page); + await stubTokensApi(page); + await page.goto("/api-tokens"); + + await expect(page.getByRole("heading", { name: "API tokens" })).toBeVisible(); + await expect(page.getByRole("button", { name: "Share this demo" })).toHaveCount(0); + }); +}); diff --git a/runner/e2e/session-abandoned-create.spec.ts b/runner/e2e/session-abandoned-create.spec.ts index 9136b09eb..706dc706c 100644 --- a/runner/e2e/session-abandoned-create.spec.ts +++ b/runner/e2e/session-abandoned-create.spec.ts @@ -62,8 +62,8 @@ const refFor = (sessionId: string): string => * green. That is the one failure mode a regression test must not have. */ async function meteredRefs(request: APIRequestContext): Promise { - const headers = process.env.E2E_BROKER_TOKEN - ? { Authorization: `Bearer ${process.env.E2E_BROKER_TOKEN}` } + const headers = process.env.E2E_API_TOKEN + ? { Authorization: `Bearer ${process.env.E2E_API_TOKEN}` } : undefined; const limit = 200; // SESSIONS_MAX_PAGE_SIZE; a larger ask is clamped to it const refs: string[] = []; diff --git a/runner/e2e/share-create-live.spec.ts b/runner/e2e/share-create-live.spec.ts index ddd81d850..c234c2996 100644 --- a/runner/e2e/share-create-live.spec.ts +++ b/runner/e2e/share-create-live.spec.ts @@ -7,21 +7,27 @@ import { workspaceFiles } from "./helpers"; // (share-view.spec.ts); this is the one test that exercises the builder, R2 // and D1 end to end. // -// It needs a real broker token — there is no test bypass in the deployed -// worker, by design (auth.ts re-validates every bearer against the broker and -// requires @handsontable.com). Broker tokens are per-user session JWTs with an -// expiry and no programmatic mint path, so the token arrives as a secret you -// refresh by hand when you want this to run: +// It needs a real credential — there is no test bypass in the deployed worker, +// by design (auth.ts requires @handsontable.com on both of its paths). Since +// DEV-2583 that credential is a persistent API token, which does not expire and +// can be minted from the app, so this spec finally runs unattended: // -// 1. Sign in on the deployed app, then in the console: sessionStorage.hot_token -// 2. E2E_BROKER_TOKEN= E2E_BASE_URL=https://demos.handsontable.com \ +// 1. Sign in on the deployed app, go to /api-tokens, mint one, copy it +// 2. E2E_API_TOKEN= E2E_BASE_URL=https://demos.handsontable.com \ // pnpm e2e e2e/share-create-live.spec.ts --workers=1 // +// The token goes into `sessionStorage.hot_token` exactly as a login token would +// — the client resolves identity for it against our own API rather than the +// broker (ADR-0037), which is what keeps this spec driving the real Share +// button instead of being rewritten into an API script. It is also why the +// workflow's trace scrubbing matters more than it used to: this credential has +// no expiry to limit the damage of a leaked artifact. +// // Cost and hygiene: one BuilderSandbox boot (pool of 3, shared) and one D1 // row per run. The revoke lives in an afterEach, not the test body — see the // note at `demoId` below. -const TOKEN = process.env.E2E_BROKER_TOKEN; +const TOKEN = process.env.E2E_API_TOKEN; // Written by the test, read by the afterEach below. Module scope on purpose: // the revoke must not live in the test body's `finally` — a body that hits its @@ -46,14 +52,16 @@ test.afterEach(async ({ request, baseURL }) => { test("a demo shared today is a page a client can open — until it is revoked", async ({ page }) => { test.skip(!process.env.E2E_BASE_URL, "needs a deployed API origin"); - test.skip(!TOKEN, "set E2E_BROKER_TOKEN to a fresh sessionStorage.hot_token from a signed-in session"); + test.skip(!TOKEN, "set E2E_API_TOKEN to a token minted on /api-tokens"); // Headroom above the sum of the wait ceilings (30+30+300+60s) — the previous // 420s equalled it exactly, so a build that used its whole dialog budget // timed the body out before the view assertions ran (Bugbot, #186). All // waits below are condition-bound with ceilings, never sleeps. test.setTimeout(480_000); - // The real token, the real broker, no stubs. + // The real token, the real deployment, no stubs — and deliberately not the + // real broker any more: identity for a `hot_pat_` resolves against our own + // /api/profile (ADR-0037), which is what makes this spec runnable at all. await page.addInitScript((token) => sessionStorage.setItem("hot_token", token), TOKEN!); // The id is captured off the network, not the dialog: a locator throwing @@ -72,7 +80,7 @@ test("a demo shared today is a page a client can open — until it is revoked", // acting, so an expired token reads as "token expired", not a dead button. await expect( page.getByRole("button", { name: "Fork", exact: true }), - "no authed top bar — is E2E_BROKER_TOKEN expired?", + "no authed top bar — has E2E_API_TOKEN been revoked?", ).toBeVisible({ timeout: 30_000 }); // Auth is not the only precondition: the workspace starts as an empty diff --git a/runner/packages/editor-shell/src/AccountMenu.tsx b/runner/packages/editor-shell/src/AccountMenu.tsx index edd794b0f..2a50f05a9 100644 --- a/runner/packages/editor-shell/src/AccountMenu.tsx +++ b/runner/packages/editor-shell/src/AccountMenu.tsx @@ -15,7 +15,7 @@ // `displayName`, and the app owns fetching both. import { useEffect, useRef, useState, type CSSProperties } from "react"; -import { IconBook, IconChartBar, IconListDetails, IconLogin2, IconSettings2 } from "./icons/index.js"; +import { IconBook, IconChartBar, IconKey, IconListDetails, IconLogin2, IconSettings2 } from "./icons/index.js"; import { theme } from "./theme.js"; export interface AccountMenuProps { @@ -35,6 +35,9 @@ export interface AccountMenuProps { /** `/settings` (DEV-2166). Optional for the same reason `onMyDemos` is a * callback at all: this package does no navigation. */ onSettings?: () => void; + /** `/api-tokens` (DEV-2583, ADR-0037) — the persistent API tokens. Optional for + * the same reason the rest are: this package navigates nothing itself. */ + onApiTokens?: () => void; /** `/guide` (DEV-2503) — the in-app how-to. Optional for the same reason the two * above are: this package navigates nothing itself. */ onGuide?: () => void; @@ -48,6 +51,7 @@ export function AccountMenu({ onMyDemos, onUsage, onSettings, + onApiTokens, onGuide, onLogout, }: AccountMenuProps) { @@ -117,6 +121,18 @@ export function AccountMenu({ disabled={!onSettings} title={onSettings ? "Your name, description and avatar" : "Profile settings are not available here"} /> + {/* `/api-tokens` (DEV-2583). Follows Settings, which is where the thing + it manages belongs: a credential that acts as you is account state, + not a demo. */} + } + label="API tokens" + onClick={onApiTokens ? () => { setOpen(false); onApiTokens(); } : undefined} + disabled={!onApiTokens} + title={onApiTokens + ? "Persistent tokens for scripts and CI" + : "API tokens are not available here"} + /> {/* `/guide` (DEV-2503). Last before the rule: it is the row you want on your first day and never again, so it sits below the ones you use daily rather than above them. */} diff --git a/runner/packages/editor-shell/src/EditorShell.tsx b/runner/packages/editor-shell/src/EditorShell.tsx index b516af996..3fa8546c7 100644 --- a/runner/packages/editor-shell/src/EditorShell.tsx +++ b/runner/packages/editor-shell/src/EditorShell.tsx @@ -116,6 +116,8 @@ export interface EditorShellProps { onUsage?: () => void; /** `/settings`, the profile page. Reaches the account menu. */ onSettings?: () => void; + /** `/api-tokens` (DEV-2583) — reaches the account menu. */ + onApiTokens?: () => void; /** `/guide` (DEV-2503) — reaches the account menu. */ onGuide?: () => void; onLogout?: () => void; @@ -397,6 +399,7 @@ export function EditorShell(props: EditorShellProps) { onMyDemos={props.fullMode ? undefined : props.onMyDemos} onUsage={props.fullMode ? undefined : props.onUsage} onSettings={props.fullMode ? undefined : props.onSettings} + onApiTokens={props.fullMode ? undefined : props.onApiTokens} onGuide={props.fullMode ? undefined : props.onGuide} onLogout={props.fullMode ? undefined : props.onLogout} // The mode action is resolved here, not in `TopBar`, and off `authed` diff --git a/runner/packages/editor-shell/src/SideNav.tsx b/runner/packages/editor-shell/src/SideNav.tsx index abda4084e..8b8aa54ce 100644 --- a/runner/packages/editor-shell/src/SideNav.tsx +++ b/runner/packages/editor-shell/src/SideNav.tsx @@ -12,7 +12,7 @@ // the stylesheet's hover and the rows would look dead. import type { CSSProperties } from "react"; -import { IconBook, IconListDetails, IconLogin2, IconSettings2, IconUsers } from "./icons/index.js"; +import { IconBook, IconKey, IconListDetails, IconLogin2, IconSettings2, IconUsers } from "./icons/index.js"; import { theme } from "./theme.js"; /** A child row under one of the sections — the guide's tracks (DEV-2522). */ @@ -24,7 +24,7 @@ export interface SideNavSubItem { export interface SideNavProps { /** Which row is the current page. */ - active: "myDemos" | "allDemos" | "settings" | "guide"; + active: "myDemos" | "allDemos" | "settings" | "apiTokens" | "guide"; /** Rows nested under Guide. Only drawn when Guide is the current page: a * four-item sub-list on My demos would be navigation for a page you are not on. */ guideSubItems?: SideNavSubItem[]; @@ -40,6 +40,10 @@ export function SideNav({ active, guideSubItems, onLogout }: SideNavProps) { `WHERE`, and `IconUsers` says "other people's" without a word. */} } label="All demos" /> } label="Settings" /> + {/* `/api-tokens` (DEV-2583, ADR-0037) — the persistent credentials. Under + Settings because it is account-shaped rather than demo-shaped: what it + manages is who may act as you, not anything you built. */} + } label="API tokens" /> {/* `/guide` (DEV-2503) — the in-app how-to. `IconBook` is already in the set (it heads a README row elsewhere) and reads as documentation. */} } label="Guide" /> diff --git a/runner/packages/editor-shell/src/TopBar.tsx b/runner/packages/editor-shell/src/TopBar.tsx index a8e194135..4b14791f7 100644 --- a/runner/packages/editor-shell/src/TopBar.tsx +++ b/runner/packages/editor-shell/src/TopBar.tsx @@ -46,6 +46,9 @@ export interface TopBarProps { * in the account menu rather than the bar: the pre-redesign bar had it as a * loose `Usage` link beside `My demos`, and My demos is now a menu row. */ onUsage?: () => void; + /** `/api-tokens` — the persistent API tokens (DEV-2583, ADR-0037). Absent + * leaves the menu row disabled, like the two below. */ + onApiTokens?: () => void; /** `/settings` — the profile page (DEV-2166). Absent leaves the menu row * disabled, which is what the anonymous-adjacent surfaces want. */ onSettings?: () => void; @@ -76,6 +79,7 @@ export function TopBar({ onMyDemos, onUsage, onSettings, + onApiTokens, onGuide, onLogout, onFork, @@ -168,6 +172,7 @@ export function TopBar({ onMyDemos={onMyDemos} onUsage={onUsage} onSettings={onSettings} + onApiTokens={onApiTokens} onGuide={onGuide} onLogout={onLogout} /> diff --git a/runner/packages/editor-shell/src/icons/ui.tsx b/runner/packages/editor-shell/src/icons/ui.tsx index 148d41080..e03a4e33d 100644 --- a/runner/packages/editor-shell/src/icons/ui.tsx +++ b/runner/packages/editor-shell/src/icons/ui.tsx @@ -49,6 +49,12 @@ // rather than a modified marker. Filled is the // truthful import; the layer name is the slip. // +// And one from no frame either, added by DEV-2583 for the API tokens page: +// `IconKey` — the account menu + side nav row for +// `/api-tokens`. Persistent API tokens postdate +// every frame, and a key is the one glyph the +// set has nothing else for. +// // The wrapper exists to pin the design's 16px/2px rendering (tabler defaults to // 24px) and to mark icons `aria-hidden` — labels live on the enclosing button. // Both are overridable via props. @@ -68,6 +74,7 @@ import { IconDownload as TablerDownload, IconExternalLink as TablerExternalLink, IconFolderPlus as TablerFolderPlus, + IconKey as TablerKey, IconLayoutSidebarLeftCollapse as TablerLayoutSidebarLeftCollapse, IconLayoutSidebarLeftExpand as TablerLayoutSidebarLeftExpand, IconListDetails as TablerListDetails, @@ -106,6 +113,7 @@ function ui(displayName: string, Base: TablerIcon) { } export const IconBook = ui("IconBook", TablerBook); +export const IconKey = ui("IconKey", TablerKey); export const IconBrandGithub = ui("IconBrandGithub", TablerBrandGithub); export const IconBrandReactNative = ui("IconBrandReactNative", TablerBrandReactNative); export const IconChartBar = ui("IconChartBar", TablerChartBar); diff --git a/runner/pipeline/api-error.test.mjs b/runner/pipeline/api-error.test.mjs index 5786189fc..263a7322e 100644 --- a/runner/pipeline/api-error.test.mjs +++ b/runner/pipeline/api-error.test.mjs @@ -111,3 +111,35 @@ test("isSessionExpired is true only for the 401 ApiError", () => { assert.equal(isSessionExpired(null), false); assert.equal(isSessionExpired("session-expired"), false); }); + +test("a capability refusal reads as itself, not as an ownership problem (DEV-2583)", () => { + // A persistent API token is fenced off AI features and the admin writes + // (ADR-0037). Those refusals share the 403 status with the ownership checks, + // and the ownership sentence is the wrong explanation for them: nothing + // belongs to anybody else, the credential simply may not do this. + const failure = describeApiFailure( + 403, + { error: "token_forbidden", detail: "an API token cannot use the AI features" }, + "ask failed (403)", + ); + + assert.equal(failure.kind, "forbidden"); + assert.equal(failure.status, 403); + assert.equal(failure.message, "an API token cannot use the AI features"); + assert.doesNotMatch(failure.message, /belongs to someone else/i); + assert.doesNotMatch(failure.message, /token_forbidden/i, "never the wire string"); + // Not reportable: unlike an ownership 403, this is the fence working as + // designed, so it is not a UI/server disagreement worth a Sentry issue. + assert.equal(failure.reportable, false); + assert.equal(isSessionExpired(failure), false, "a fence is not an expired session"); +}); + +test("a token_forbidden without a detail still says something useful", () => { + // Defensive: every route that sends this code sends a detail, but a proxy or + // a truncated body must not produce an empty toast. + const failure = describeApiFailure(403, { error: "token_forbidden" }, "ask failed (403)"); + + assert.equal(failure.kind, "forbidden"); + assert.ok(failure.message.length > 0); + assert.doesNotMatch(failure.message, /token_forbidden/i); +}); diff --git a/runner/pipeline/api-token.test.mjs b/runner/pipeline/api-token.test.mjs new file mode 100644 index 000000000..ee60b3869 --- /dev/null +++ b/runner/pipeline/api-token.test.mjs @@ -0,0 +1,138 @@ +// The persistent API token's own rules (DEV-2583, ADR-0037), tested against the +// real `token.ts` — the module the Worker's auth path calls, not a local copy of +// its regex. `token.ts` imports nothing and touches no binding for exactly this +// reason (the rule demos-list.ts records), so it loads directly under +// `node --experimental-strip-types` with no module hooks and no fakes. +// +// What is proved here is the credential's shape and the arithmetic around it. +// That the router actually enforces any of it is token-routes.test.mjs's job: +// a shape check that only ever runs in this file would stay green with the +// branch deleted from auth.ts (the #201 lesson). +// +// Run: node --experimental-strip-types --test pipeline/api-token.test.mjs + +import test from "node:test"; +import assert from "node:assert/strict"; + +const { + PAT_PREFIX, + MAX_TOKEN_NAME, + hashToken, + isTokenBearerValue, + mintToken, + normalizeTokenName, + parseTokenId, + touchThreshold, +} = await import("../workers/api/src/token.ts"); + +test("a minted token carries its own public id and is shaped for parsing", async () => { + const { id, token } = mintToken(); + assert.match(id, /^[0-9a-f]{16}$/, "the id is 16 hex characters"); + assert.ok(token.startsWith(PAT_PREFIX), `a token announces itself: ${token}`); + assert.equal( + parseTokenId(token), + id, + "the id the caller stores is the id read back out of the token string", + ); +}); + +test("two mints never collide, and the secret is not derived from the id", () => { + const a = mintToken(); + const b = mintToken(); + assert.notEqual(a.id, b.id); + assert.notEqual(a.token, b.token); + // Same id, different secret must be a different token — otherwise the public + // half would be enough to reconstruct the credential. + const secretOf = (t) => t.slice(PAT_PREFIX.length + 16 + 1); + assert.notEqual(secretOf(a.token), secretOf(b.token)); + assert.equal(secretOf(a.token).length, 43, "base64url over 32 random bytes"); +}); + +test("only the exact token shape parses", () => { + const { id, token } = mintToken(); + const secret = token.slice(PAT_PREFIX.length + 16 + 1); + + assert.equal(parseTokenId(token), id, "the control case parses"); + + for (const [label, raw] of [ + ["a broker JWT", "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.e30.sig"], + ["the empty string", ""], + ["the prefix alone", PAT_PREFIX], + ["a near-miss prefix", `hot_pat${id}_${secret}`], + ["an uppercase prefix", `HOT_PAT_${id}_${secret}`], + ["a non-hex id", `${PAT_PREFIX}${"g".repeat(16)}_${secret}`], + ["an uppercase id", `${PAT_PREFIX}${id.toUpperCase()}_${secret}`], + ["a short id", `${PAT_PREFIX}${id.slice(0, 15)}_${secret}`], + ["a long id", `${PAT_PREFIX}${id}a_${secret}`], + ["no separator", `${PAT_PREFIX}${id}${secret}`], + ["a short secret", `${PAT_PREFIX}${id}_${secret.slice(0, 42)}`], + ["a long secret", `${PAT_PREFIX}${id}_${secret}a`], + ["a secret with a dot", `${PAT_PREFIX}${id}_${`.${secret.slice(1)}`}`], + ["trailing whitespace", `${token} `], + ["a leading Bearer", `Bearer ${token}`], + ]) { + assert.equal(parseTokenId(raw), null, `${label} is not a token`); + } +}); + +test("the bearer test is the prefix and nothing else — it never validates", () => { + // This is what decides whether a credential goes to the local lookup or to the + // broker, so it must answer on the prefix alone: a malformed `hot_pat_…` has to + // be refused here rather than fall through and be posted to a third-party host. + assert.equal(isTokenBearerValue(mintToken().token), true); + assert.equal(isTokenBearerValue(`${PAT_PREFIX}nonsense`), true, "malformed but ours"); + assert.equal(isTokenBearerValue("eyJhbGciOiJSUzI1NiJ9.e30.sig"), false, "a broker JWT is not"); + assert.equal(isTokenBearerValue(""), false); +}); + +test("the stored digest is a SHA-256 of the whole token, not of its id", async () => { + const { id, token } = mintToken(); + const digest = await hashToken(token); + assert.match(digest, /^[0-9a-f]{64}$/); + assert.equal(digest, await hashToken(token), "stable for the same input"); + assert.notEqual(digest, await hashToken(id), "the public half does not produce the digest"); + assert.notEqual(digest, await hashToken(mintToken().token)); + // The known-answer case, so a future refactor cannot quietly change algorithm. + assert.equal( + await hashToken("abc"), + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad", + ); +}); + +test("a token name is required, trimmed and capped", () => { + assert.deepEqual(normalizeTokenName({ name: " nightly e2e " }), { ok: true, value: "nightly e2e" }); + assert.equal(normalizeTokenName({ name: "x".repeat(MAX_TOKEN_NAME) }).ok, true, "the cap itself fits"); + + for (const [label, body] of [ + ["a missing body", null], + ["a missing name", {}], + ["an empty name", { name: "" }], + ["a whitespace-only name", { name: " " }], + ["a non-string name", { name: 42 }], + ["an over-long name", { name: "x".repeat(MAX_TOKEN_NAME + 1) }], + ]) { + const result = normalizeTokenName(body); + assert.equal(result.ok, false, `${label} is refused`); + assert.equal(typeof result.error, "string"); + assert.ok(result.error.length > 0, "the refusal says why"); + } +}); + +test("last_used_at is bounded to one write per clock hour", () => { + // The threshold is the bound in `UPDATE … WHERE last_used_at IS NULL OR + // last_used_at < ?`. A stamp from this hour must not pass it; anything older + // must. Asserting the comparison, not just the string, because the string + // format is only load-bearing insofar as ISO8601 sorts lexicographically. + const now = "2026-08-21T13:47:12.345Z"; + const threshold = touchThreshold(now); + assert.equal(threshold, "2026-08-21T13:00:00.000Z"); + + assert.ok(!("2026-08-21T13:05:00.000Z" < threshold), "a stamp from this hour does not re-write"); + assert.ok(!(threshold < threshold), "the hour boundary itself does not re-write"); + assert.ok("2026-08-21T12:59:59.999Z" < threshold, "the previous hour does"); + assert.ok("2026-08-20T23:00:00.000Z" < threshold, "so does yesterday"); + + // Midnight and the turn of the year, where a naive slice-and-concat breaks. + assert.equal(touchThreshold("2026-01-01T00:00:00.000Z"), "2026-01-01T00:00:00.000Z"); + assert.equal(touchThreshold("2025-12-31T23:59:59.999Z"), "2025-12-31T23:00:00.000Z"); +}); diff --git a/runner/pipeline/fixtures/worker-harness.mjs b/runner/pipeline/fixtures/worker-harness.mjs index bb85531e5..ca842866d 100644 --- a/runner/pipeline/fixtures/worker-harness.mjs +++ b/runner/pipeline/fixtures/worker-harness.mjs @@ -11,6 +11,11 @@ // the `demos//__source.json` snapshot). The build_cache read always hits, // steering createDemo()/updateDemo() through their cached-artifact branch so // no route under test ever asks for a container. +// +// D1 also models `api_tokens` (DEV-2583), including the two conditional UPDATEs +// the token store relies on — the idempotent revoke and the hour-coarsened +// `last_used_at`. Those conditions are the behaviour under test, so a fake that +// applied every UPDATE unconditionally would hand back a false pass. import assert from "node:assert/strict"; @@ -37,12 +42,19 @@ export function parseDemosInsert(sql, binds) { * always hits so createDemo() takes its cached-artifact branch. Unmatched * reads answer empty, which the budget code treats as "no spend yet". */ -export function fakeD1(seedRows = []) { +export function fakeD1(seedRows = [], seedTokens = []) { const writes = []; const demos = new Map(seedRows.map((row) => [row.id, row])); + const tokens = new Map(seedTokens.map((row) => [row.id, { ...row }])); const prepare = (sql) => { const bound = (binds) => ({ async first() { + if (/FROM api_tokens WHERE id = \?/.test(sql)) { + const row = tokens.get(binds[0]); + // A copy, so a caller cannot mutate the store by holding its row — + // and so a route that forgets to write a change cannot appear to. + return row ? { ...row } : null; + } if (/FROM demos WHERE id = \?/.test(sql)) return demos.get(binds[0]) ?? null; if (/FROM build_cache/.test(sql)) return { r2_prefix: "demos/_prior-identical-build/" }; return null; @@ -51,33 +63,103 @@ export function fakeD1(seedRows = []) { writes.push({ sql, binds }); const inserted = parseDemosInsert(sql, binds); if (inserted) demos.set(inserted.id, inserted); + applyTokenWrite(tokens, sql, binds); return { success: true, meta: {} }; }, async all() { + if (/FROM api_tokens ORDER BY created_at DESC/.test(sql)) { + const results = [...tokens.values()] + .map(({ token_hash, ...view }) => view) + .sort((a, b) => (a.created_at < b.created_at ? 1 : -1)); + return { success: true, results }; + } return { success: true, results: [] }; }, }); return { bind: (...binds) => bound(binds), ...bound([]) }; }; - return { db: { prepare }, writes, demos }; + return { db: { prepare }, writes, demos, tokens }; +} + +/** + * Apply an `api_tokens` write, honouring the WHERE clause. + * + * The conditions are the point: the revoke is idempotent because of + * `revoked_at IS NULL`, and `last_used_at` is bounded to one write per clock + * hour because of `last_used_at < ?`. A fake that ignored them would make both + * properties untestable while looking like it had tested them. + */ +function applyTokenWrite(tokens, sql, binds) { + if (/INSERT INTO api_tokens/.test(sql)) { + const [id, name, token_hash, created_by, created_at] = binds; + tokens.set(id, { + id, + name, + token_hash, + created_by, + created_at, + last_used_at: null, + revoked_at: null, + revoked_by: null, + }); + return; + } + if (/UPDATE api_tokens SET revoked_at/.test(sql)) { + const [now, revokedBy, id] = binds; + const row = tokens.get(id); + if (row && row.revoked_at === null) { + row.revoked_at = now; + row.revoked_by = revokedBy; + } + return; + } + if (/UPDATE api_tokens SET last_used_at/.test(sql)) { + const [now, id, threshold] = binds; + const row = tokens.get(id); + if (row && (row.last_used_at === null || row.last_used_at < threshold)) { + row.last_used_at = now; + } + } } /** * KV fake — a Map, ignoring TTL options (nothing under test outlives one). + * + * `list()` answers the real shape — `{ keys: [{ name, metadata }], list_complete, + * cursor }` — because `adminSessions()` reads all three fields, and a fake that + * returned only the names would make its paging branch untestable. Metadata is + * carried on `put(key, value, { metadata })`, the way the meter writes it. */ export function fakeKV() { const store = new Map(); + const meta = new Map(); return { async get(key, type) { const value = store.get(key); if (value === undefined) return null; return type === "json" ? JSON.parse(value) : value; }, - async put(key, value) { + async put(key, value, options) { store.set(key, String(value)); + if (options?.metadata !== undefined) meta.set(key, options.metadata); }, async delete(key) { store.delete(key); + meta.delete(key); + }, + async list({ prefix = "", limit = 1000, cursor } = {}) { + // KV lists in UTF-8 key order and pages with an opaque cursor; the cursor + // here is the index it stands for, which is opaque enough for a fake. + const all = [...store.keys()].filter((k) => k.startsWith(prefix)).sort(); + const start = cursor ? Number(cursor) : 0; + const page = all.slice(start, start + limit); + const end = start + page.length; + const complete = end >= all.length; + return { + keys: page.map((name) => ({ name, metadata: meta.get(name) })), + list_complete: complete, + ...(complete ? {} : { cursor: String(end) }), + }; }, }; } @@ -113,8 +195,8 @@ export const AUTHOR = "dev@handsontable.com"; * layered by the caller; none are set here so the broker path stays the one * under test on the browser routes. */ -export function makeEnv(seedRows = []) { - const { db, writes, demos } = fakeD1(seedRows); +export function makeEnv(seedRows = [], seedTokens = []) { + const { db, writes, demos, tokens } = fakeD1(seedRows, seedTokens); const artifacts = fakeR2(); const env = { Sandbox: {}, @@ -130,7 +212,7 @@ export function makeEnv(seedRows = []) { // Not the production host, so the Sentry gate in index.ts stays inert. PREVIEW_HOST: "localhost:8787", }; - return { env, writes, demos, artifacts }; + return { env, writes, demos, artifacts, tokens }; } export const ctx = { diff --git a/runner/pipeline/mcp-create.test.mjs b/runner/pipeline/mcp-create.test.mjs index 11c8d5930..eb8fc6a6b 100644 --- a/runner/pipeline/mcp-create.test.mjs +++ b/runner/pipeline/mcp-create.test.mjs @@ -1,21 +1,32 @@ // Headless demo creation over the MCP service path (DEV-2501, ADR-0033). // +// The imports are dynamic, behind the `.js`->`.ts` module hooks, because +// `auth.ts` stopped being an import-free leaf when the persistent-token path +// landed (DEV-2583): it now pulls in `token.js` / `token-store.js` by the +// repo's NodeNext-style specifier, which bare `--experimental-strip-types` +// cannot resolve. Nothing here drives the token path — `authenticateService` is +// the subject — but the module graph has to load all the same. +// // Run: node --experimental-strip-types --test pipeline/*.test.mjs import test from "node:test"; import assert from "node:assert/strict"; import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; -import { +import { register } from "node:module"; + +register("./fixtures/worker-hooks.mjs", import.meta.url); + +const { MAX_MCP_BYTES, MAX_MCP_FILES, isMcpCreated, isMcpValidationError, isTeamEmail, validateMcpFiles, -} from "../workers/api/src/mcp-create.ts"; -import { authenticateService, normalizeEmail, sameOwner } from "../workers/api/src/auth.ts"; -import { demoListQuery } from "../workers/api/src/demos-list.ts"; +} = await import("../workers/api/src/mcp-create.ts"); +const { authenticateService, normalizeEmail, sameOwner } = await import("../workers/api/src/auth.ts"); +const { demoListQuery } = await import("../workers/api/src/demos-list.ts"); const ok = { "/package.json": '{"name":"demo"}', "/index.js": "console.log(1)" }; diff --git a/runner/pipeline/token-routes.test.mjs b/runner/pipeline/token-routes.test.mjs new file mode 100644 index 000000000..dffcecd5f --- /dev/null +++ b/runner/pipeline/token-routes.test.mjs @@ -0,0 +1,504 @@ +// Persistent API tokens at the route level (DEV-2583, ADR-0037), driven through +// the REAL router — the default export of workers/api/src/index.ts — so every +// property asserted here is a property of the deployed Worker rather than of a +// re-declared copy of its checks (the #201 lesson). +// +// Three things are only provable here, not in api-token.test.mjs: +// +// 1. A minted token authenticates, and does it WITHOUT the broker. The stub +// below counts calls, so "verified locally" is an assertion rather than an +// inference. The same counter proves the security property in ADR-0037: +// a malformed `hot_pat_…` is refused here and never forwarded to a +// third-party host. +// 2. The capability fence. Delete a `tokenForbidden` line from a route and the +// matching case below goes red, because the request really travels through +// that route. +// 3. That the broker path still works, unchanged, beside the new one. +// +// Bindings are the in-memory fakes from fixtures/worker-harness.mjs, whose D1 +// models `api_tokens` including the two conditional UPDATEs the store relies on. +// +// Run: node --experimental-strip-types --test pipeline/token-routes.test.mjs + +import test, { after, beforeEach } from "node:test"; +import assert from "node:assert/strict"; +import { register } from "node:module"; +import { AUTHOR, ctx, makeEnv } from "./fixtures/worker-harness.mjs"; + +register("./fixtures/worker-hooks.mjs", import.meta.url); + +const { default: worker } = await import("../workers/api/src/index.ts"); +const { PAT_PREFIX, hashToken, mintToken } = await import("../workers/api/src/token.ts"); +const { touchToken, verifyToken } = await import("../workers/api/src/token-store.ts"); + +// ---- the broker stub / network tripwire --------------------------------------- + +const OTHER = "someone.else@handsontable.com"; +const REAL_FETCH = globalThis.fetch; +let brokerCalls = 0; + +globalThis.fetch = async (input, init) => { + const url = typeof input === "string" ? input : input.url; + if (url.startsWith("https://login.invalid")) { + brokerCalls += 1; + const presented = init?.headers?.Authorization; + if (presented === "Bearer test-token") return Response.json({ email: AUTHOR, sub: "u1" }); + if (presented === "Bearer other-token") return Response.json({ email: OTHER, sub: "u2" }); + return new Response("no", { status: 401 }); + } + throw new Error(`unexpected network fetch in token-routes.test.mjs: ${url}`); +}; + +after(() => { + globalThis.fetch = REAL_FETCH; +}); + +// ---- fixtures ---------------------------------------------------------------- + +const HOST = "https://demos.handsontable.com"; + +let env; +let tokens; + +beforeEach(() => { + ({ env, tokens } = makeEnv()); + brokerCalls = 0; +}); + +const asPerson = (token = "test-token") => ({ Authorization: `Bearer ${token}` }); + +const req = (method, path, { headers = {}, body } = {}) => + new Request(`${HOST}${path}`, { + method, + headers: body ? { "Content-Type": "application/json", ...headers } : headers, + body: body === undefined ? undefined : JSON.stringify(body), + }); + +/** Mint through the real route, and hand back the plaintext it answered with. */ +async function mintViaRoute(name = "nightly e2e", who = "test-token") { + const res = await worker.fetch( + req("POST", "/api/tokens", { headers: asPerson(who), body: { name } }), + env, + ctx, + ); + assert.equal(res.status, 201, "the mint route answers 201"); + const body = await res.json(); + return body; +} + +// ---- minting ----------------------------------------------------------------- + +test("minting returns the plaintext once and stores only its digest", async () => { + const body = await mintViaRoute("nightly e2e"); + + assert.ok(body.token.startsWith(PAT_PREFIX), `the response carries a token: ${body.token}`); + assert.equal(body.name, "nightly e2e"); + assert.equal(body.created_by, AUTHOR, "the token acts as the person who minted it"); + assert.equal(body.revoked_at, null); + assert.equal(body.last_used_at, null, "never used yet"); + + const row = tokens.get(body.id); + assert.ok(row, "the row exists"); + assert.equal(row.token_hash, await hashToken(body.token), "the digest of the whole token"); + assert.equal( + JSON.stringify(row).includes(body.token), + false, + "the plaintext is nowhere in the stored row", + ); +}); + +test("a mint needs a usable name", async () => { + for (const body of [{}, { name: "" }, { name: " " }, { name: "x".repeat(65) }]) { + const res = await worker.fetch( + req("POST", "/api/tokens", { headers: asPerson(), body }), + env, + ctx, + ); + assert.equal(res.status, 400, `${JSON.stringify(body)} is refused`); + assert.equal(tokens.size, 0, "and nothing was written"); + } +}); + +// ---- the credential actually works, without the broker ----------------------- + +test("a minted token authenticates, and never touches the broker", async () => { + const { token } = await mintViaRoute(); + brokerCalls = 0; + + const res = await worker.fetch( + req("GET", "/api/demos?scope=mine", { headers: asPerson(token) }), + env, + ctx, + ); + + assert.equal(res.status, 200, "an auth-gated route accepts the token"); + assert.equal(brokerCalls, 0, "verified locally — the credential never left this Worker"); + const body = await res.json(); + assert.equal(body.scope, "mine", "and it authenticated as somebody"); +}); + +test("a token acts as its creator, so ownership is unchanged", async () => { + const { token } = await mintViaRoute(); + // `?scope=mine` binds the identity's own address into the listing query, so a + // 200 here means the token resolved to an address — and the write log shows + // which one, without needing a seeded demo to come back. + await worker.fetch(req("GET", "/api/demos?scope=mine", { headers: asPerson(token) }), env, ctx); + const row = tokens.get(token.slice(PAT_PREFIX.length, PAT_PREFIX.length + 16)); + assert.equal(row.created_by, AUTHOR); +}); + +test("a malformed token of ours is refused on the anonymous routes too, not served", async () => { + // `/api/chat` admits anonymous callers, so the fence there reads the header + // rather than an identity. A truncated or corrupted `hot_pat_…` is therefore a + // 403 rather than an anonymous request: it is still a credential of ours being + // presented, and answering it as "no credential" would let a broken paste + // silently spend AI budget as an anonymous visitor. + for (const bearer of [`${PAT_PREFIX}truncated`, `${PAT_PREFIX}`]) { + const res = await worker.fetch( + req("POST", "/api/chat", { headers: asPerson(bearer), body: { question: "hi" } }), + env, + ctx, + ); + assert.equal(res.status, 403, `${bearer} on /api/chat`); + assert.equal((await res.json()).error, "token_forbidden"); + } +}); + +test("a row with no usable digest verifies as nothing, rather than throwing", async () => { + // A partial insert, a hand-edited row, or a future migration mid-backfill. The + // digest comparison is length-guarded, so this is a null and not a 500 — worth + // an assertion because it is the last unexercised branch in verifyToken. + const { id, token } = await mintViaRoute(); + for (const bad of ["", "not-a-digest", null]) { + tokens.get(id).token_hash = bad; + assert.equal( + await verifyToken(env, token, "2026-08-21T13:05:00.000Z"), + null, + `token_hash ${JSON.stringify(bad)} verifies as nothing`, + ); + } +}); + +test("a malformed token of ours is refused here, not forwarded to the broker", async () => { + // The security property in ADR-0037: falling through to the broker would post + // our own permanent credential to a third-party host, and the failure would + // look like a slow success. + for (const bearer of [ + `${PAT_PREFIX}nonsense`, + `${PAT_PREFIX}0123456789abcdef_short`, + PAT_PREFIX, + ]) { + brokerCalls = 0; + const res = await worker.fetch( + req("GET", "/api/demos", { headers: asPerson(bearer) }), + env, + ctx, + ); + assert.equal(res.status, 401, `${bearer} is refused`); + assert.equal(brokerCalls, 0, `${bearer} was never forwarded`); + } +}); + +test("an unknown id, and a right id with a wrong secret, are both refused", async () => { + const { token, id } = await mintViaRoute(); + const secret = token.slice(PAT_PREFIX.length + 16 + 1); + const otherSecret = mintToken().token.slice(PAT_PREFIX.length + 16 + 1); + + const unknown = `${PAT_PREFIX}${"0".repeat(16)}_${secret}`; + const tampered = `${PAT_PREFIX}${id}_${otherSecret}`; + + for (const [label, bearer] of [["an unknown id", unknown], ["a wrong secret", tampered]]) { + brokerCalls = 0; + const res = await worker.fetch(req("GET", "/api/demos", { headers: asPerson(bearer) }), env, ctx); + assert.equal(res.status, 401, `${label} is refused`); + assert.equal(brokerCalls, 0, `${label} did not reach the broker`); + } + + // The control: the real token still works, so the two refusals above are the + // digest comparison doing its job rather than the whole path being broken. + const ok = await worker.fetch(req("GET", "/api/demos", { headers: asPerson(token) }), env, ctx); + assert.equal(ok.status, 200); +}); + +test("extra whitespace after Bearer does not leak the token to the broker", async () => { + // RFC 7235 allows `1*SP` between the scheme and the credential, so + // `Bearer hot_pat_…` is a well-formed header — and slicing a fixed "Bearer " + // off the front used to leave the space attached, miss the prefix test, and + // forward our own permanent credential to the broker (Bugbot, #252). + const { token } = await mintViaRoute(); + + for (const header of [`Bearer ${token}`, `Bearer \t${token}`, `Bearer ${token} `]) { + brokerCalls = 0; + const res = await worker.fetch( + new Request(`${HOST}/api/demos`, { headers: { Authorization: header } }), + env, + ctx, + ); + assert.equal(res.status, 200, `${JSON.stringify(header)} still authenticates`); + assert.equal(brokerCalls, 0, `${JSON.stringify(header)} was not forwarded to the broker`); + } +}); + +test("extra whitespace after Bearer does not slip past the capability fence either", async () => { + // The other half of the same defect: `/api/chat` admits anonymous callers, so + // a token it failed to recognise would have been served as a visitor and + // allowed to spend AI budget. + const { token } = await mintViaRoute(); + + const res = await worker.fetch( + new Request(`${HOST}/api/chat`, { + method: "POST", + headers: { "Content-Type": "application/json", Authorization: `Bearer ${token}` }, + body: JSON.stringify({ question: "hi" }), + }), + env, + ctx, + ); + assert.equal(res.status, 403); + assert.equal((await res.json()).error, "token_forbidden"); +}); + +test("a lower-case auth scheme is still a token, not a fall-through", async () => { + // RFC 7235 makes `auth-scheme` case-insensitive. Returning null for + // `bearer hot_pat_…` would drop the request through to the DEV_AUTH_EMAIL + // bypass on a loopback host and grant a *person* identity with no `via`, so + // the capability fence would not engage locally — the exact failure the + // ordering in `authenticate()` exists to prevent. + const { token } = await mintViaRoute(); + + for (const scheme of ["bearer", "BEARER", "BeArEr"]) { + brokerCalls = 0; + const res = await worker.fetch( + new Request(`${HOST}/api/demos`, { headers: { Authorization: `${scheme} ${token}` } }), + env, + ctx, + ); + assert.equal(res.status, 200, `${scheme} authenticates`); + assert.equal(brokerCalls, 0, `${scheme} was not forwarded to the broker`); + } + + // And the fence sees it too, on a route that admits anonymous callers. + const chat = await worker.fetch( + new Request(`${HOST}/api/chat`, { + method: "POST", + headers: { "Content-Type": "application/json", Authorization: `bearer ${token}` }, + body: JSON.stringify({ question: "hi" }), + }), + env, + ctx, + ); + assert.equal(chat.status, 403); +}); + +test("the broker path still works, beside the new one", async () => { + const res = await worker.fetch(req("GET", "/api/demos", { headers: asPerson() }), env, ctx); + assert.equal(res.status, 200); + assert.ok(brokerCalls > 0, "a broker JWT is still validated by the broker"); +}); + +// ---- listing and revocation --------------------------------------------------- + +test("the listing shows every token in the organization and no secrets", async () => { + const mine = await mintViaRoute("mine", "test-token"); + const theirs = await mintViaRoute("theirs", "other-token"); + + const res = await worker.fetch(req("GET", "/api/tokens", { headers: asPerson() }), env, ctx); + assert.equal(res.status, 200); + const { tokens: listed } = await res.json(); + + assert.deepEqual( + listed.map((t) => t.id).sort(), + [mine.id, theirs.id].sort(), + "both people's tokens, to everybody", + ); + for (const row of listed) { + assert.equal("token_hash" in row, false, "no digest in the listing"); + assert.equal("token" in row, false, "no plaintext in the listing"); + assert.ok(row.created_by, "attribution is shown"); + } +}); + +test("anyone on the team can revoke anyone's token, and it stops working at once", async () => { + const { id, token } = await mintViaRoute("nightly e2e", "test-token"); + + const del = await worker.fetch( + req("DELETE", `/api/tokens/${id}`, { headers: asPerson("other-token") }), + env, + ctx, + ); + assert.equal(del.status, 204, "a different team member may revoke it (ADR-0037)"); + assert.equal(tokens.get(id).revoked_by, OTHER, "and is recorded as the one who did"); + + brokerCalls = 0; + const after = await worker.fetch( + req("GET", "/api/demos", { headers: asPerson(token) }), + env, + ctx, + ); + assert.equal(after.status, 401, "the next request is refused"); + assert.equal(brokerCalls, 0, "a revoked token is not retried against the broker"); +}); + +test("revoking twice keeps the first kill's attribution; revoking nothing is a 404", async () => { + const { id } = await mintViaRoute(); + + await worker.fetch(req("DELETE", `/api/tokens/${id}`, { headers: asPerson() }), env, ctx); + const firstAt = tokens.get(id).revoked_at; + assert.ok(firstAt); + + const second = await worker.fetch( + req("DELETE", `/api/tokens/${id}`, { headers: asPerson("other-token") }), + env, + ctx, + ); + assert.equal(second.status, 204, "idempotent"); + assert.equal(tokens.get(id).revoked_at, firstAt, "history is not rewritten"); + assert.equal(tokens.get(id).revoked_by, AUTHOR, "nor is the attribution"); + + const missing = await worker.fetch( + req("DELETE", "/api/tokens/deadbeefdeadbeef", { headers: asPerson() }), + env, + ctx, + ); + assert.equal(missing.status, 404); +}); + +test("every token route requires an identity", async () => { + for (const [method, path] of [ + ["GET", "/api/tokens"], + ["POST", "/api/tokens"], + ["DELETE", "/api/tokens/0123456789abcdef"], + ]) { + const res = await worker.fetch( + req(method, path, method === "POST" ? { body: { name: "x" } } : {}), + env, + ctx, + ); + assert.equal(res.status, 401, `${method} ${path} is gated`); + } +}); + +// ---- the capability fence ------------------------------------------------------ + +test("a token cannot read the token list either", async () => { + // Fenced along with the writes: no digests are exposed, but the listing names + // every credential in the organization and its owner (ADR-0037). + const { token } = await mintViaRoute(); + + const res = await worker.fetch(req("GET", "/api/tokens", { headers: asPerson(token) }), env, ctx); + assert.equal(res.status, 403); + const failure = await res.json(); + assert.equal(failure.error, "token_forbidden"); + assert.ok(failure.detail?.length > 0); +}); + +test("a token cannot mint or revoke a token", async () => { + // The fence that matters most: a leaked credential must not be able to mint + // itself a successor, nor revoke the token that would be used to kill it. + const first = await mintViaRoute("nightly e2e"); + const target = await mintViaRoute("the one it would kill"); + + const mint = await worker.fetch( + req("POST", "/api/tokens", { headers: asPerson(first.token), body: { name: "successor" } }), + env, + ctx, + ); + assert.equal(mint.status, 403); + assert.equal((await mint.json()).error, "token_forbidden"); + assert.equal(tokens.size, 2, "and nothing was minted"); + + const revoke = await worker.fetch( + req("DELETE", `/api/tokens/${target.id}`, { headers: asPerson(first.token) }), + env, + ctx, + ); + assert.equal(revoke.status, 403); + assert.equal(tokens.get(target.id).revoked_at, null, "the target is still live"); +}); + +test("a token cannot change the guardrail settings or kill a session", async () => { + const { token } = await mintViaRoute(); + + for (const [method, path, body] of [ + ["PUT", "/api/admin/settings", { limitUsd: 9999 }], + ["DELETE", "/api/admin/settings", undefined], + ["DELETE", "/api/admin/sessions/deadbeef", undefined], + ]) { + const res = await worker.fetch( + req(method, path, { headers: asPerson(token), body }), + env, + ctx, + ); + assert.equal(res.status, 403, `${method} ${path} is fenced`); + const failure = await res.json(); + assert.equal(failure.error, "token_forbidden"); + assert.ok(failure.detail?.length > 0, "and says why, so the client shows a sentence"); + } +}); + +test("a token cannot spend AI budget", async () => { + const { token } = await mintViaRoute(); + + for (const path of ["/api/chat", "/api/theme"]) { + const res = await worker.fetch( + req("POST", path, { headers: asPerson(token), body: { question: "hi" } }), + env, + ctx, + ); + assert.equal(res.status, 403, `${path} is fenced`); + assert.equal((await res.json()).error, "token_forbidden"); + } +}); + +test("a token may still read the admin listings the nightly canary needs", async () => { + // Deliberately NOT fenced, unlike the token listing: the session-leak spec + // reads this, and internal spend figures are internal rather than secret + // (admin.ts). + const { token } = await mintViaRoute(); + + const res = await worker.fetch( + req("GET", "/api/admin/sessions?awake=0&limit=5", { headers: asPerson(token) }), + env, + ctx, + ); + assert.equal(res.status, 200, "reading is allowed"); +}); + +// ---- last_used_at -------------------------------------------------------------- + +test("an authenticated request records use", async () => { + const { id, token } = await mintViaRoute(); + assert.equal(tokens.get(id).last_used_at, null); + + await worker.fetch(req("GET", "/api/demos", { headers: asPerson(token) }), env, ctx); + assert.ok(tokens.get(id).last_used_at, "the row now knows the token is in use"); +}); + +test("use is recorded at most once per clock hour", async () => { + // Driven through the store with injected stamps rather than through the route: + // the route reads the real clock, so a route-level version of this would + // depend on which minute the suite happened to run in. + const { id } = await mintViaRoute(); + + await touchToken(env, id, "2026-08-21T13:05:00.000Z"); + assert.equal(tokens.get(id).last_used_at, "2026-08-21T13:05:00.000Z", "the first use lands"); + + await touchToken(env, id, "2026-08-21T13:47:12.345Z"); + assert.equal( + tokens.get(id).last_used_at, + "2026-08-21T13:05:00.000Z", + "a second use in the same hour does not re-write", + ); + + await touchToken(env, id, "2026-08-21T14:00:00.000Z"); + assert.equal(tokens.get(id).last_used_at, "2026-08-21T14:00:00.000Z", "the next hour does"); +}); + +test("verifying a revoked token neither matches nor records use", async () => { + const { id, token } = await mintViaRoute(); + await worker.fetch(req("DELETE", `/api/tokens/${id}`, { headers: asPerson() }), env, ctx); + + const verified = await verifyToken(env, token, "2026-08-21T13:05:00.000Z"); + assert.equal(verified, null); + assert.equal(tokens.get(id).last_used_at, null, "a refused credential leaves no use stamp"); +}); diff --git a/runner/workers/api/migrations/0006_api_tokens.sql b/runner/workers/api/migrations/0006_api_tokens.sql new file mode 100644 index 000000000..3cc576b53 --- /dev/null +++ b/runner/workers/api/migrations/0006_api_tokens.sql @@ -0,0 +1,38 @@ +-- 0006_api_tokens.sql — persistent API tokens (DEV-2583, ADR-0037). +-- +-- The credential the nightly canary needs and the broker cannot mint. Keyed on +-- the token's own public id — the 16 hex characters that appear verbatim inside +-- `hot_pat__` — so verification is a primary-key hit rather than a +-- scan over digests. +-- +-- `token_hash` is a SHA-256 hex digest of the whole token string. The plaintext +-- exists only in the mint response; nothing here can reconstruct it. +-- +-- `created_by` is the verified @handsontable.com address the token *acts as*, +-- matching `demos.created_by` and `profiles.email` — there is no user table to +-- point a foreign key at (see 0005). A token is not an identity of its own, so +-- `sameOwner()`, `?scope=mine` and "My demos" need no changes. +-- +-- NULL semantics: `revoked_at IS NULL` means live, and it is the only liveness +-- test — rows are never deleted, so a revoked token's id can never be reissued +-- and the audit trail of who killed what survives. `last_used_at IS NULL` means +-- never used, and is coarsened to the hour by the atomic UPDATE in +-- token-store.ts rather than written on every request. +-- +-- Re-runnable (IF NOT EXISTS), like 0005. + +CREATE TABLE IF NOT EXISTS api_tokens ( + id TEXT PRIMARY KEY, + name TEXT NOT NULL, + token_hash TEXT NOT NULL, + created_by TEXT NOT NULL, + created_at TEXT NOT NULL, + last_used_at TEXT, + revoked_at TEXT, + revoked_by TEXT +); + +-- The listing is org-wide and ordered newest-first (ADR-0037: anyone on the team +-- may see and revoke any token), so the index that matters is the sort, not the +-- creator. `created_by` is carried for display and attribution only. +CREATE INDEX IF NOT EXISTS idx_api_tokens_created_at ON api_tokens(created_at DESC); diff --git a/runner/workers/api/src/auth.ts b/runner/workers/api/src/auth.ts index 328db4c58..098b154ba 100644 --- a/runner/workers/api/src/auth.ts +++ b/runner/workers/api/src/auth.ts @@ -1,12 +1,76 @@ // Internal-team auth via the Handsontable Google login broker (ADR-0007). // Write endpoints require a broker JWT; we re-validate it server-side and trust // only the returned email. No service account, no app-wide credential. +// +// Since ADR-0037 there is a second credential class: a persistent API token, +// minted by us and verified here rather than by the broker. The two are told +// apart by prefix *before* anything touches the network, and never fall through +// to one another — a token that reached `/broker/userinfo` because the local +// lookup missed would have shipped our own permanent credential to a +// third-party host, and the failure would look like a slow success. import type { Env } from "./env.js"; +import { constantTimeEquals } from "./constant-time.js"; +import { isTokenBearerValue } from "./token.js"; +import { verifyToken } from "./token-store.js"; export interface Identity { email: string; sub?: string; + /** + * The id of the persistent API token this request authenticated with, if it + * did (ADR-0037). Absent for a person signed in through the broker. + * + * A token acts as its creator's address, so `email` is all any existing + * caller needs and none of them changed. This field is what the capability + * fence reads — the four things a token may not do — and what an audit trail + * would follow back to a row in `api_tokens`. + */ + via?: string; +} + +/** Did this request authenticate with an API token rather than a broker login? */ +export function isTokenIdentity(identity: Identity | null): boolean { + return !!identity?.via; +} + +/** + * The bearer credential in an `Authorization` header, or null. + * + * The single parser for both callers below, deliberately: RFC 7235 spells the + * header `auth-scheme 1*SP token68`, so `Bearer hot_pat_...` — two spaces, a + * stray tab, a typo in a `curl -H` — is a well-formed header. Slicing a fixed + * `"Bearer "` off the front leaves that whitespace attached, the value then + * misses the `hot_pat_` prefix test, and the consequences are the two things + * this feature exists to prevent: the credential gets forwarded to the broker, + * and the fence on the anonymous-capable routes reads it as no credential at all + * (Bugbot, #252). Two parsers would be two chances to reintroduce that. + * + * The scheme match is case-insensitive, as RFC 7235 says it is, and that is + * load-bearing rather than pedantry: `bearer hot_pat_...` returning null here + * would fall through to the `DEV_AUTH_EMAIL` bypass on a loopback host and be + * granted a *person* identity with no `via`, so the capability fence would not + * engage — the very failure the ordering below exists to prevent. + */ +export function bearerFrom(request: Request): string | null { + const auth = request.headers.get("Authorization"); + if (!auth) return null; + const match = /^Bearer[ \t]+(.+)$/i.exec(auth.trim()); + const value = match?.[1]?.trim(); + return value ? value : null; +} + +/** + * Is the caller presenting an API token, whether or not it is a valid one? + * + * For the routes that admit anonymous callers — chat and the theme generator, + * which are budget-gated rather than sign-in-gated — where "no identity" and + * "a token identity" must be told apart before any work is done. Reads the + * header only: a malformed token of ours is still ours, and is refused rather + * than treated as an anonymous visitor. + */ +export function presentsToken(request: Request): boolean { + return isTokenBearerValue(bearerFrom(request)); } /** @@ -60,14 +124,6 @@ export function sameOwner(a: string | null | undefined, b: string | null | undef return left !== "" && left === normalizeEmail(b); } -/** Length-independent comparison, so a wrong secret cannot be found byte by byte. */ -function secretsMatch(a: string, b: string): boolean { - if (a.length !== b.length) return false; - let diff = 0; - for (let i = 0; i < a.length; i++) diff |= a.charCodeAt(i) ^ b.charCodeAt(i); - return diff === 0; -} - /** * Authenticate a **trusted service** (the Handsontable MCP) acting for a named team * member — the headless demo-creation path (DEV-2501, ADR-0033). @@ -85,7 +141,7 @@ export async function authenticateService(request: Request, env: Env): Promise { + const bearer = bearerFrom(request); + + // A persistent API token is ours to verify (ADR-0037). Discriminated on the + // prefix and answered locally either way: a malformed or revoked `hot_pat_…` + // is refused here rather than forwarded to the broker. + // + // Ahead of the DEV_AUTH_EMAIL bypass below, deliberately. Every developer is + // told to put that variable in `.dev.vars` (docs/run-and-deploy.md), so with + // the bypass first a presented token was accepted as a *person* — `via` never + // set, the capability fence never engaged, and `wrangler dev` therefore + // behaving as the exact opposite of production on the one thing this feature + // is careful about. Presenting a token now means being treated as one, wherever + // the Worker is running. + if (bearer !== null && isTokenBearerValue(bearer)) { + const verified = await verifyToken(env, bearer, new Date().toISOString()); + if (!verified) return null; + // The same team-only rule the broker path enforces. The address was already + // verified when the token was minted; re-asserting it means a row written + // before that rule, or by a future path that forgets it, still cannot widen + // who this Worker will act for. + if (!verified.createdBy.endsWith("@handsontable.com")) return null; + return { email: verified.createdBy, via: verified.id }; + } + const devEmail = (env as { DEV_AUTH_EMAIL?: string }).DEV_AUTH_EMAIL; if (devEmail && isLocalRequest(request)) return { email: devEmail }; - const auth = request.headers.get("Authorization"); - if (!auth?.startsWith("Bearer ")) return null; + if (bearer === null) return null; try { const res = await fetch(`${env.LOGIN_BROKER_URL}/broker/userinfo`, { - headers: { Authorization: auth }, + headers: { Authorization: `Bearer ${bearer}` }, }); if (!res.ok) return null; // Email and `sub` are all there is: the broker's authorize redirect asks for diff --git a/runner/workers/api/src/constant-time.ts b/runner/workers/api/src/constant-time.ts new file mode 100644 index 000000000..5c7064c0f --- /dev/null +++ b/runner/workers/api/src/constant-time.ts @@ -0,0 +1,18 @@ +// One length-independent string comparison, for the two credential paths that +// need it: the MCP shared secret (`auth.ts`) and a persistent API token's stored +// digest (`token-store.ts`). +// +// Its own module rather than an export of either caller: `auth.ts` imports +// `token-store.ts`, so `token-store.ts` importing `auth.ts` back would make the +// graph cyclic. Imports nothing itself, so it loads under bare +// `--experimental-strip-types` alongside the other leaves. +// +// Workers has no `crypto.timingSafeEqual`, which is why this is hand-rolled. + +/** Compare two strings without leaking where they first differ. */ +export function constantTimeEquals(a: string, b: string): boolean { + if (a.length !== b.length) return false; + let diff = 0; + for (let i = 0; i < a.length; i++) diff |= a.charCodeAt(i) ^ b.charCodeAt(i); + return diff === 0; +} diff --git a/runner/workers/api/src/index.ts b/runner/workers/api/src/index.ts index 89ba9153a..8e51c1dcb 100644 --- a/runner/workers/api/src/index.ts +++ b/runner/workers/api/src/index.ts @@ -20,7 +20,9 @@ import { import type { Env } from "./env.js"; import { FRAMEWORK_DEV, BUILD_CONFIG } from "./frameworks.generated.js"; import { dependencyMetadataFingerprint } from "./dependency-metadata.js"; -import { authenticate, authenticateService, sameOwner } from "./auth.js"; +import { authenticate, authenticateService, isTokenIdentity, presentsToken, sameOwner } from "./auth.js"; +import { hashToken, mintToken, normalizeTokenName } from "./token.js"; +import { createToken, listTokens, revokeToken } from "./token-store.js"; import { MAX_TITLE, isValidationError, validateDescription, validateTitle } from "./demo-info.js"; import { isMcpCreated, isMcpValidationError, validateMcpFiles } from "./mcp-create.js"; import { editorVersionRef, fetchVersionCatalog, resolveHandsontableVersion } from "./ht-version.js"; @@ -476,6 +478,22 @@ const json = (data: unknown, status = 200) => const nowIso = () => new Date().toISOString(); +/** + * The capability fence on a persistent API token (ADR-0037). + * + * A token may do what its creator may, minus four things: change the guardrail + * settings, kill somebody else's session, spend AI budget, and manage tokens. + * The last is the one that matters most — a leaked token must not be able to + * mint itself a successor or revoke the tokens that would be used to kill it. + * + * `token_forbidden` is a distinct wire code because the client's 403 branch + * otherwise renders an ownership sentence ("this demo belongs to someone + * else"), which is the wrong explanation and a reportable Sentry issue. The + * `detail` is the sentence a person sees; see `apiError.ts`. + */ +const tokenForbidden = (what: string) => + json({ error: "token_forbidden", detail: `An API token cannot ${what}.` }, 403); + /** Write out whatever the in-memory meters have accumulated (bytes, requests, * share views). Batched deliberately: one D1 write per asset served would * cost more than the asset does. */ @@ -1530,6 +1548,10 @@ export default Sentry.withSentry(sentryOptions, { // page hits and asks the model, holding the LiteLLM key server-side. // See src/chat.ts for why retrieval is split that way. if (request.method === "POST" && parts[0] === "api" && parts[1] === "chat" && parts.length === 2) { + // Answers cost money and this route admits anonymous callers, so the + // fence reads the header rather than an identity: "no credential" and + // "a token credential" are different answers here (ADR-0037). + if (presentsToken(request)) return tokenForbidden("use the AI features"); const ip = request.headers.get("cf-connecting-ip") ?? ""; const limit = await checkChatRateLimit(env, ip); if (!limit.ok) { @@ -1587,6 +1609,8 @@ export default Sentry.withSentry(sentryOptions, { // is the same gateway and the same money — but its own tool and its own // whitelist, so a styling request can never return file edits. if (request.method === "POST" && parts[0] === "api" && parts[1] === "theme" && parts.length === 2) { + // Same gateway, same money, same fence as /api/chat. + if (presentsToken(request)) return tokenForbidden("use the AI features"); const limit = await checkChatRateLimit(env, request.headers.get("cf-connecting-ip") ?? ""); if (!limit.ok) { ctx.waitUntil(recordUsageEvent(env, "chat_denied", "rate_limit")); @@ -1730,6 +1754,57 @@ export default Sentry.withSentry(sentryOptions, { return cors(await serveAvatar(env, parts[3]!)); } + // The persistent API tokens tab (DEV-2583, ADR-0037). The listing is + // org-wide and so is revocation: a permanent credential only its author + // can kill is worse than one anybody on the team can, because the author + // will eventually be on holiday and the token will not expire on their + // behalf. `token_hash` is never selected, so no response here can leak it. + + // GET /api/tokens (auth, people only) — every token in the organization. + if (request.method === "GET" && parts[0] === "api" && parts[1] === "tokens" && parts.length === 2) { + const identity = await authenticate(request, env); + if (!identity) return json({ error: "unauthorized" }, 401); + // Reading is fenced as well as writing, so the rule is simply "a token + // cannot touch token management". The listing holds no digests, but it + // does name every credential in the organization and who owns it — and + // that is reconnaissance a leaked token has no business doing. + if (isTokenIdentity(identity)) return tokenForbidden("read the token list"); + return json({ tokens: await listTokens(env) }); + } + + // POST /api/tokens (auth, people only) — mint one. The plaintext is in + // this response and nowhere else, ever again. + if (request.method === "POST" && parts[0] === "api" && parts[1] === "tokens" && parts.length === 2) { + const identity = await authenticate(request, env); + if (!identity) return json({ error: "unauthorized" }, 401); + if (isTokenIdentity(identity)) return tokenForbidden("mint another token"); + const parsed = normalizeTokenName(await request.json().catch(() => null)); + if (!parsed.ok) return json({ error: parsed.error }, 400); + const { id, token } = mintToken(); + const view = await createToken(env, { + id, + name: parsed.value, + tokenHash: await hashToken(token), + createdBy: identity.email, + now: nowIso(), + }); + console.log(`[tokens] ${identity.email} minted ${id}`); + return json({ ...view, token }, 201); + } + + // DELETE /api/tokens/:id (auth, people only) — revoke, from now on. + // Anyone on the team, not just the author. Idempotent: revoking a dead + // token keeps the first kill's timestamp and attribution. + if (request.method === "DELETE" && parts[0] === "api" && parts[1] === "tokens" && parts.length === 3) { + const identity = await authenticate(request, env); + if (!identity) return json({ error: "unauthorized" }, 401); + if (isTokenIdentity(identity)) return tokenForbidden("revoke a token"); + const existed = await revokeToken(env, { id: parts[2]!, revokedBy: identity.email, now: nowIso() }); + if (!existed) return json({ error: "not found" }, 404); + console.log(`[tokens] ${identity.email} revoked ${parts[2]}`); + return cors(new Response(null, { status: 204 })); + } + // GET /api/budget (public) — the degradation tier the client should // reflect. Dollar figures only for a signed-in Handsontable identity; // anonymous callers get the tier and the user-facing notice, which is @@ -1760,6 +1835,9 @@ export default Sentry.withSentry(sentryOptions, { && parts[0] === "api" && parts[1] === "admin" && parts[2] === "settings") { const identity = await authenticate(request, env); if (!identity) return json({ error: "unauthorized" }, 401); + // The spend ceiling and the enforcement switch are the reason the fence + // exists: this credential lives in a public repository's secrets. + if (isTokenIdentity(identity)) return tokenForbidden("change the guardrail settings"); if (request.method === "DELETE") { const defaults = await resetSettings(env); @@ -1848,6 +1926,9 @@ export default Sentry.withSentry(sentryOptions, { && parts[2] === "sessions" && parts.length === 4) { const identity = await authenticate(request, env); if (!identity) return json({ error: "unauthorized" }, 401); + // One person's click ending another person's session is not a thing to + // do on behalf of a credential in CI. + if (isTokenIdentity(identity)) return tokenForbidden("kill a session"); const ref = parts[3]!; const resolved = await lookupSessionRef(env, ref); if (!resolved.ok) { diff --git a/runner/workers/api/src/token-store.ts b/runner/workers/api/src/token-store.ts new file mode 100644 index 000000000..bdd9b6a73 --- /dev/null +++ b/runner/workers/api/src/token-store.ts @@ -0,0 +1,122 @@ +// D1 access for persistent API tokens (DEV-2583, ADR-0037). The decisions live +// in `token.ts` (pure, unit-tested); this file only moves bytes. +// +// Nothing here ever selects or returns `token_hash` to a caller: `verifyToken` +// compares it in place and hands back a row without it, so a listing cannot +// leak the digest by accident. + +import type { Env } from "./env.js"; +import { constantTimeEquals } from "./constant-time.js"; +import { hashToken, parseTokenId, touchThreshold } from "./token.js"; + +/** A token as the listing shows it — no digest, no plaintext, ever. */ +export interface TokenView { + id: string; + name: string; + created_by: string; + created_at: string; + last_used_at: string | null; + revoked_at: string | null; + revoked_by: string | null; +} + +const VIEW_COLUMNS = "id, name, created_by, created_at, last_used_at, revoked_at, revoked_by"; + +export async function createToken( + env: Env, + args: { id: string; name: string; tokenHash: string; createdBy: string; now: string }, +): Promise { + await env.DB.prepare( + `INSERT INTO api_tokens (id, name, token_hash, created_by, created_at) + VALUES (?, ?, ?, ?, ?)`, + ).bind(args.id, args.name, args.tokenHash, args.createdBy, args.now).run(); + return { + id: args.id, + name: args.name, + created_by: args.createdBy, + created_at: args.now, + last_used_at: null, + revoked_at: null, + revoked_by: null, + }; +} + +/** Every token in the organization, live and revoked, newest first (ADR-0037). */ +export async function listTokens(env: Env): Promise { + const rows = await env.DB.prepare( + `SELECT ${VIEW_COLUMNS} FROM api_tokens ORDER BY created_at DESC`, + ).all(); + return rows.results ?? []; +} + +/** + * Revoke a token. Idempotent: `revoked_at IS NULL` in the WHERE means a second + * revoke leaves the first one's timestamp and attribution alone rather than + * rewriting history. Returns whether such a token exists at all, so the route + * can tell 404 from "already dead". + */ +export async function revokeToken( + env: Env, + args: { id: string; revokedBy: string; now: string }, +): Promise { + await env.DB.prepare( + `UPDATE api_tokens SET revoked_at = ?, revoked_by = ? + WHERE id = ? AND revoked_at IS NULL`, + ).bind(args.now, args.revokedBy, args.id).run(); + const row = await env.DB.prepare("SELECT id FROM api_tokens WHERE id = ?") + .bind(args.id).first<{ id: string }>(); + return row !== null; +} + +/** + * Verify a presented token and return the address it acts as, or null. + * + * One primary-key read. The digest comparison is length-independent + * (`constantTimeEquals`, shared with the MCP secret path) even though both sides are hex: the cost is + * nothing and the alternative invites a future refactor to introduce a leak. + * Revocation is read from the same row, so it takes effect on the next request + * with no cache to invalidate. + */ +export async function verifyToken( + env: Env, + presented: string, + now: string, +): Promise<{ id: string; createdBy: string } | null> { + const id = parseTokenId(presented); + if (!id) return null; + + const row = await env.DB.prepare( + "SELECT id, token_hash, created_by, last_used_at, revoked_at FROM api_tokens WHERE id = ?", + ).bind(id).first<{ + id: string; + token_hash: string; + created_by: string; + last_used_at: string | null; + revoked_at: string | null; + }>(); + if (!row || row.revoked_at) return null; + // A row with no usable digest is not a token. `token_hash` is NOT NULL in the + // schema, so this only happens to a hand-edited row or one caught mid-backfill + // by a future migration — but a 500 out of the auth path would be a worse + // answer to that than a refusal, and it would be reported as an outage. + if (typeof row.token_hash !== "string" || row.token_hash === "") return null; + if (!constantTimeEquals(await hashToken(presented), row.token_hash)) return null; + + await touchToken(env, id, now); + return { id, createdBy: row.created_by }; +} + +/** + * Record use, at most once per clock hour per token. + * + * A single conditional UPDATE rather than a read followed by a write: there is + * no window for two concurrent requests to both decide they are the first this + * hour, and no `ctx` has to be threaded through the two dozen `authenticate()` + * call sites to defer the write. + */ +export async function touchToken(env: Env, id: string, now: string): Promise { + await env.DB.prepare( + `UPDATE api_tokens SET last_used_at = ? + WHERE id = ? AND (last_used_at IS NULL OR last_used_at < ?)`, + ).bind(now, id, touchThreshold(now)).run(); +} diff --git a/runner/workers/api/src/token.ts b/runner/workers/api/src/token.ts new file mode 100644 index 000000000..3011d778f --- /dev/null +++ b/runner/workers/api/src/token.ts @@ -0,0 +1,111 @@ +// Persistent API tokens — the credential's own rules (DEV-2583, ADR-0037). +// +// Imports nothing and touches no binding, so `pipeline/api-token.test.mjs` can +// load it directly under `node --experimental-strip-types`, where a sibling +// `./x.js` specifier does not resolve — the same rule `demos-list.ts` records. +// Every D1 access lives in `token-store.ts`. +// +// A token is `hot_pat__`: the id is public (it is the D1 primary +// key, the display form, and what DELETE /api/tokens/:id names) and the secret +// is never stored. Only a SHA-256 digest of the whole string is kept, which is +// sound because this is a 256-bit random secret rather than a password — there +// is no dictionary to run and no low-entropy guess space to grind, so a work +// factor would cost every authenticated request and defend nothing (ADR-0037). + +export const PAT_PREFIX = "hot_pat_"; + +/** Matches `normalizeProfileInput`'s cap on a display name — a label, not prose. */ +export const MAX_TOKEN_NAME = 64; + +const ID_CHARS = 16; +/** base64url over 32 bytes. Unpadded, so a fixed 43 characters. */ +const SECRET_CHARS = 43; + +/** + * The one true shape. Parsed positionally rather than by splitting on `_`, + * because base64url includes `_` and the secret may therefore contain one. + */ +const TOKEN_RE = new RegExp( + `^${PAT_PREFIX}([0-9a-f]{${ID_CHARS}})_([A-Za-z0-9_-]{${SECRET_CHARS}})$`, +); + +const hex = (bytes: Uint8Array): string => + [...bytes].map((b) => b.toString(16).padStart(2, "0")).join(""); + +/** Unpadded base64url — the alphabet a URL, a header and a shell all survive. */ +function base64url(bytes: Uint8Array): string { + let raw = ""; + for (const b of bytes) raw += String.fromCharCode(b); + return btoa(raw).replace(/\+/g, "-").replace(/\//g, "_").replace(/=+$/, ""); +} + +/** + * Mint a token. The plaintext is returned once, to be handed to the caller and + * then forgotten: only `hashToken(token)` is ever stored. + */ +export function mintToken(): { id: string; token: string } { + const id = hex(crypto.getRandomValues(new Uint8Array(ID_CHARS / 2))); + const secret = base64url(crypto.getRandomValues(new Uint8Array(32))); + return { id, token: `${PAT_PREFIX}${id}_${secret}` }; +} + +/** + * The public id inside a well-formed token, or null. + * + * Strict on purpose: a malformed `hot_pat_…` must be refused here rather than + * fall through to the broker path, which would post our own permanent + * credential to a third-party host (ADR-0037). + */ +export function parseTokenId(raw: string | null | undefined): string | null { + const match = TOKEN_RE.exec(raw ?? ""); + return match ? match[1]! : null; +} + +/** + * Is this bearer value ours to verify? + * + * The prefix and nothing else. This decides *which* path a credential takes, so + * it must answer without validating: a malformed token of ours is still ours, + * and is refused locally rather than forwarded. + */ +export function isTokenBearerValue(raw: string | null | undefined): boolean { + return (raw ?? "").startsWith(PAT_PREFIX); +} + +/** SHA-256 hex of the whole token string — the only form we store. */ +export async function hashToken(token: string): Promise { + const digest = await crypto.subtle.digest("SHA-256", new TextEncoder().encode(token)); + return hex(new Uint8Array(digest)); +} + +/** + * The `last_used_at` bound: the start of the hour containing `nowIso`. + * + * Used as `UPDATE … WHERE last_used_at IS NULL OR last_used_at < ?`, which + * bounds the hot auth path to one effective write per token per clock hour in a + * single atomic statement — no read-then-write, so nothing to race, and no + * `ctx` to thread through the two dozen `authenticate()` call sites. + */ +export function touchThreshold(nowIso: string): string { + const at = new Date(nowIso); + at.setUTCMinutes(0, 0, 0); + return at.toISOString(); +} + +/** + * The `{ name }` a mint request carries. Required, unlike a display name: a + * permanent credential nobody can identify in the list is one nobody dares + * revoke. + */ +export function normalizeTokenName( + body: unknown, +): { ok: true; value: string } | { ok: false; error: string } { + const raw = (body as { name?: unknown } | null)?.name; + if (typeof raw !== "string") return { ok: false, error: "name is required" }; + const value = raw.trim(); + if (!value) return { ok: false, error: "name is required" }; + if (value.length > MAX_TOKEN_NAME) { + return { ok: false, error: `name must be ${MAX_TOKEN_NAME} characters or fewer` }; + } + return { ok: true, value }; +}