From 54931ca37c3b17d1fe64e137400773bcead43af2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Fri, 21 Aug 2026 10:11:11 +0200 Subject: [PATCH 1/5] docs(runner): ADR-0037 persistent API tokens (DEV-2583) The nightly live canary's authed step is gated on a hand-copied broker session JWT that expires in about an hour, so the one test that drives builder + R2 + D1 against production is skipped every night behind a green run. Records the decision: a first-party `hot_pat__` credential, prefix-discriminated before any network call, stored as a SHA-256 digest, verified inside the Worker, acting as its creator's address, fenced off admin writes / chat / theme / token management, and revocable by anyone on the team. Co-Authored-By: Claude Opus 5 --- runner/docs/adr/0037-persistent-api-tokens.md | 145 ++++++++++++++++++ runner/docs/adr/README.md | 1 + 2 files changed, 146 insertions(+) create mode 100644 runner/docs/adr/0037-persistent-api-tokens.md 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..282f2c60a --- /dev/null +++ b/runner/docs/adr/0037-persistent-api-tokens.md @@ -0,0 +1,145 @@ +# 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. 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/admin/*` stays 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) | From f2eec7d982801a1feb09c85e0c3526de32a76007 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Fri, 21 Aug 2026 10:41:12 +0200 Subject: [PATCH 2/5] feat(runner): persistent API tokens, verified in the Worker (DEV-2583) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nightly canary's authed step was gated on a hand-copied broker session JWT that expires in about an hour, so the one test that drives builder + R2 + D1 against production was skipped every night behind a green run. There was no mint path to automate: `authenticate()` forwards the bearer to the broker and trusts the address it returns, and nothing in the system could issue or validate a credential of its own. Adds `hot_pat__` (ADR-0037): minted from a new API tokens page, stored as a SHA-256 digest in `api_tokens`, prefix-discriminated before any network call so our own permanent credential is never posted to the broker's third-party host, and verified with one primary-key read plus the existing constant-time compare. A token acts as its creator's address, so `sameOwner()`, `created_by` and `?scope=mine` are untouched. Capability fence, because this credential lives in a public repository's secrets and any team identity is currently a full admin: no admin writes, no session kills, no `/api/chat` or `/api/theme`, and no token management at all — reads included, so a leaked token can neither mint a successor nor enumerate the credentials it would need to revoke. Admin *reads* stay open, which the session-leak spec needs. Every token is visible to and revocable by the whole team: the author of a permanent credential will eventually be on holiday, and it will not expire on their behalf. Also fixes the vite proxy's `/api` key, which was a bare prefix and so swallowed `/api-tokens` — wider than the `demos.handsontable.com/api/*` route it stands in for, the same bug `/d` was already regex'd for. Co-Authored-By: Claude Opus 5 --- .claude/skills/runner-playwright-e2e/SKILL.md | 4 +- .github/workflows/e2e-live.yml | 29 +- runner/AGENTS.md | 15 +- runner/apps/authoring/src/ApiTokens.tsx | 569 ++++++++++++++++++ runner/apps/authoring/src/App.tsx | 64 +- runner/apps/authoring/src/Guide.tsx | 1 + runner/apps/authoring/src/MyDemos.tsx | 1 + runner/apps/authoring/src/Settings.tsx | 1 + runner/apps/authoring/src/apiError.ts | 16 +- runner/apps/authoring/src/auth.ts | 31 +- runner/apps/authoring/src/tokens.ts | 63 ++ runner/apps/authoring/vite.config.ts | 14 +- runner/docs/TESTING.md | 2 +- runner/docs/adr/0037-persistent-api-tokens.md | 17 +- runner/docs/run-and-deploy.md | 2 + runner/e2e/api-tokens.spec.ts | 311 ++++++++++ runner/e2e/session-abandoned-create.spec.ts | 4 +- runner/e2e/share-create-live.spec.ts | 26 +- .../packages/editor-shell/src/AccountMenu.tsx | 18 +- .../packages/editor-shell/src/EditorShell.tsx | 3 + runner/packages/editor-shell/src/SideNav.tsx | 8 +- runner/packages/editor-shell/src/TopBar.tsx | 5 + runner/packages/editor-shell/src/icons/ui.tsx | 8 + runner/pipeline/api-error.test.mjs | 32 + runner/pipeline/api-token.test.mjs | 138 +++++ runner/pipeline/fixtures/worker-harness.mjs | 94 ++- runner/pipeline/mcp-create.test.mjs | 19 +- runner/pipeline/token-routes.test.mjs | 434 +++++++++++++ .../api/migrations/0006_api_tokens.sql | 38 ++ runner/workers/api/src/auth.ts | 79 ++- runner/workers/api/src/constant-time.ts | 18 + runner/workers/api/src/index.ts | 83 ++- runner/workers/api/src/token-store.ts | 122 ++++ runner/workers/api/src/token.ts | 111 ++++ 34 files changed, 2302 insertions(+), 78 deletions(-) create mode 100644 runner/apps/authoring/src/ApiTokens.tsx create mode 100644 runner/apps/authoring/src/tokens.ts create mode 100644 runner/e2e/api-tokens.spec.ts create mode 100644 runner/pipeline/api-token.test.mjs create mode 100644 runner/pipeline/token-routes.test.mjs create mode 100644 runner/workers/api/migrations/0006_api_tokens.sql create mode 100644 runner/workers/api/src/constant-time.ts create mode 100644 runner/workers/api/src/token-store.ts create mode 100644 runner/workers/api/src/token.ts 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..31eca8f08 --- /dev/null +++ b/runner/apps/authoring/src/ApiTokens.tsx @@ -0,0 +1,569 @@ +// 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, logout, type User } from "./auth.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); + 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; + fail(e, "tokens-list"); + setTokens([]); + }); + 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) { + 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) { + 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={() => { 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 &&

{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. */} + +
+
+ )} + + {tokenSession + ? null + : tokens === null + ?

Loading tokens…

+ : tokens.length === 0 + ?

No tokens yet.

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

+ {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. +

+
+ + {/* Focus lands here, not on Revoke — the same reason My Demos orders + its delete confirmation this way. */} + +
+ + )} + + ); +} + +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, +}; + +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.border}`, + 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..a2bf57d9f 100644 --- a/runner/apps/authoring/src/Guide.tsx +++ b/runner/apps/authoring/src/Guide.tsx @@ -67,6 +67,7 @@ export function GuidePage({ apiBase, user }: GuidePageProps) { accountAvatarUrl={profile?.avatar_url} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} + onApiTokens={() => { 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..7e7bf046c 100644 --- a/runner/apps/authoring/src/MyDemos.tsx +++ b/runner/apps/authoring/src/MyDemos.tsx @@ -270,6 +270,7 @@ export function MyDemosPage({ apiBase, user, scope = "mine" }: MyDemosPageProps) accountAvatarUrl={ownerAvatar} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} + onApiTokens={() => { 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..5c33c240e 100644 --- a/runner/apps/authoring/src/Settings.tsx +++ b/runner/apps/authoring/src/Settings.tsx @@ -185,6 +185,7 @@ export function SettingsPage({ apiBase, user }: SettingsPageProps) { accountAvatarUrl={profile?.avatar_url} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} + onApiTokens={() => { 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 index 282f2c60a..97ca438a8 100644 --- a/runner/docs/adr/0037-persistent-api-tokens.md +++ b/runner/docs/adr/0037-persistent-api-tokens.md @@ -73,12 +73,17 @@ hash, verified inside the Worker, and revocable by anyone on the team.** 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. 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/admin/*` stays open, because the - session-leak spec reads `/api/admin/sessions` and reading internal spend figures is - what `admin.ts` already says it is. + `/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 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..8d034b25c --- /dev/null +++ b/runner/e2e/api-tokens.spec.ts @@ -0,0 +1,311 @@ +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("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..7364e980a 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,7 +52,7 @@ 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 @@ -72,7 +78,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..ba012eb36 --- /dev/null +++ b/runner/pipeline/token-routes.test.mjs @@ -0,0 +1,434 @@ +// 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("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..6a5b2875e 100644 --- a/runner/workers/api/src/auth.ts +++ b/runner/workers/api/src/auth.ts @@ -1,12 +1,51 @@ // 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; +} + +/** + * 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 { + const auth = request.headers.get("Authorization"); + return !!auth?.startsWith("Bearer ") && isTokenBearerValue(auth.slice("Bearer ".length)); } /** @@ -60,14 +99,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 +116,7 @@ export async function authenticateService(request: Request, env: Env): Promise { + const auth = request.headers.get("Authorization"); + const bearer = auth?.startsWith("Bearer ") ? auth.slice("Bearer ".length) : null; + + // 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 }; +} From 1a8918dd756566b7b5cd6059eaa7d95bd8b82427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Fri, 21 Aug 2026 12:00:42 +0200 Subject: [PATCH 3/5] fix(runner): one bearer parser, dark-safe reveal field, fenced menu row (DEV-2583) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from Bugbot on #252, all reproduced before fixing. RFC 7235 spells the header `auth-scheme 1*SP token68`, so `Bearer ` — two spaces, a stray tab, a typo in a `curl -H` — is well-formed. Slicing a fixed "Bearer " off the front left that whitespace attached, the value then missed the `hot_pat_` prefix test, and both of the things ADR-0037 exists to prevent followed: the credential was forwarded to the broker, and `presentsToken()` read it as no credential at all, so a token could have spent AI budget as an anonymous visitor. `bearerFrom()` is now the single parser for both callers, and two route-level tests fail against the old parsing. The one-time reveal field outlined itself with `border` on `surfaceSunken` — #222222 against #353535 in dark, i.e. no visible edge on the one field that must stay obvious after a mint. `controlBorder`, which the shared `fieldInput` already uses for this reason. `onApiTokens` was gated on a token session only in the editor, so the account-menu row stayed live on Settings, My Demos, Guide and the tokens page itself — an enabled control leading to a page that explains it cannot be used. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/ApiTokens.tsx | 9 ++++-- runner/apps/authoring/src/Guide.tsx | 7 +++-- runner/apps/authoring/src/MyDemos.tsx | 7 +++-- runner/apps/authoring/src/Settings.tsx | 7 +++-- runner/pipeline/token-routes.test.mjs | 38 +++++++++++++++++++++++++ runner/workers/api/src/auth.ts | 26 ++++++++++++++--- 6 files changed, 82 insertions(+), 12 deletions(-) diff --git a/runner/apps/authoring/src/ApiTokens.tsx b/runner/apps/authoring/src/ApiTokens.tsx index 31eca8f08..5f9e04dbb 100644 --- a/runner/apps/authoring/src/ApiTokens.tsx +++ b/runner/apps/authoring/src/ApiTokens.tsx @@ -166,7 +166,7 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { accountAvatarUrl={profile?.avatar_url} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} - onApiTokens={() => { location.href = "/api-tokens"; }} + onApiTokens={tokenSession ? undefined : () => { location.href = "/api-tokens"; }} onGuide={() => { location.href = "/guide"; }} onLogout={() => logout("/")} /> @@ -430,12 +430,17 @@ const calloutBody: CSSProperties = { 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.border}`, + border: `1px solid ${theme.color.controlBorder}`, borderRadius: theme.radius.md, background: theme.color.surfaceSunken, }; diff --git a/runner/apps/authoring/src/Guide.tsx b/runner/apps/authoring/src/Guide.tsx index a2bf57d9f..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,7 +67,10 @@ export function GuidePage({ apiBase, user }: GuidePageProps) { accountAvatarUrl={profile?.avatar_url} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} - onApiTokens={() => { location.href = "/api-tokens"; }} + // 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 7e7bf046c..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,7 +270,10 @@ export function MyDemosPage({ apiBase, user, scope = "mine" }: MyDemosPageProps) accountAvatarUrl={ownerAvatar} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} - onApiTokens={() => { location.href = "/api-tokens"; }} + // 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 5c33c240e..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,7 +185,10 @@ export function SettingsPage({ apiBase, user }: SettingsPageProps) { accountAvatarUrl={profile?.avatar_url} onMyDemos={() => { location.href = "/my-demos"; }} onSettings={() => { location.href = "/settings"; }} - onApiTokens={() => { location.href = "/api-tokens"; }} + // 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/pipeline/token-routes.test.mjs b/runner/pipeline/token-routes.test.mjs index ba012eb36..a31c08b64 100644 --- a/runner/pipeline/token-routes.test.mjs +++ b/runner/pipeline/token-routes.test.mjs @@ -220,6 +220,44 @@ test("an unknown id, and a right id with a wrong secret, are both refused", asyn 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("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); diff --git a/runner/workers/api/src/auth.ts b/runner/workers/api/src/auth.ts index 6a5b2875e..d9bce29a4 100644 --- a/runner/workers/api/src/auth.ts +++ b/runner/workers/api/src/auth.ts @@ -34,6 +34,26 @@ 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. + */ +export function bearerFrom(request: Request): string | null { + const auth = request.headers.get("Authorization"); + if (!auth) return null; + const match = /^Bearer[ \t]+(.+)$/.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? * @@ -44,8 +64,7 @@ export function isTokenIdentity(identity: Identity | null): boolean { * than treated as an anonymous visitor. */ export function presentsToken(request: Request): boolean { - const auth = request.headers.get("Authorization"); - return !!auth?.startsWith("Bearer ") && isTokenBearerValue(auth.slice("Bearer ".length)); + return isTokenBearerValue(bearerFrom(request)); } /** @@ -125,8 +144,7 @@ export async function authenticateService(request: Request, env: Env): Promise { - const auth = request.headers.get("Authorization"); - const bearer = auth?.startsWith("Bearer ") ? auth.slice("Bearer ".length) : null; + 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_…` From 98d00b131b3f381051f8faafdc9978e3b0168cfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Fri, 21 Aug 2026 12:25:48 +0200 Subject: [PATCH 4/5] fix(runner): revoke confirm opens on Cancel, and four smaller review fixes (DEV-2583) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second high-effort review pass on #252. The revoke confirmation used React's `autoFocus` on Cancel, which loses to `Dialog`'s own focus effect — that runs after the layout phase and lands on the content's first focusable, i.e. **Revoke**. Opening the dialog and pressing Enter therefore revoked a live credential without the question having been answered. `data-autofocus` is the hatch `Dialog` documents for exactly this and what every other destructive confirm in the app uses. An e2e case now asserts focus and that Enter revokes nothing; it fails against the old markup. `bearerFrom()` matched the auth scheme case-sensitively. RFC 7235 makes it case-insensitive, and the difference is load-bearing rather than pedantic: `bearer hot_pat_…` returned null, fell through to the `DEV_AUTH_EMAIL` bypass on a loopback host, and was granted a *person* identity with no `via` — so the capability fence did not engage locally, which is the exact failure the branch ordering exists to prevent. A failed listing rendered "No tokens yet.", telling the reader of a page whose only job is enumerating live credentials that there are none to worry about. Failure is now its own state, distinct from "still loading". Neither mint nor revoke had the `isSessionExpired` branch that `MyDemos` uses, so an expired session left an enabled form that 401'd on every click; and a failed revoke painted its error behind the modal scrim, where the dialog hid it. Both fixed. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/ApiTokens.tsx | 33 ++++++++++++++++++----- runner/e2e/api-tokens.spec.ts | 36 +++++++++++++++++++++++++ runner/e2e/share-create-live.spec.ts | 4 ++- runner/pipeline/token-routes.test.mjs | 32 ++++++++++++++++++++++ runner/workers/api/src/auth.ts | 8 +++++- 5 files changed, 104 insertions(+), 9 deletions(-) diff --git a/runner/apps/authoring/src/ApiTokens.tsx b/runner/apps/authoring/src/ApiTokens.tsx index 5f9e04dbb..ea6fa2acd 100644 --- a/runner/apps/authoring/src/ApiTokens.tsx +++ b/runner/apps/authoring/src/ApiTokens.tsx @@ -33,7 +33,8 @@ import { shellStyles, theme, } from "@handsontable/demo-editor-shell"; -import { isTokenSession, logout, type User } from "./auth.js"; +import { isTokenSession, login, logout, type User } from "./auth.js"; +import { isSessionExpired } from "./apiError.js"; import { fetchTokens, mintApiToken, @@ -75,6 +76,10 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { 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); @@ -87,8 +92,9 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { .then((list) => { if (live) setTokens(list); }) .catch((e) => { if (!live) return; + if (isSessionExpired(e)) return login(); fail(e, "tokens-list"); - setTokens([]); + setLoadFailed(true); }); return () => { live = false; }; }, [apiBase, tokenSession]); @@ -115,6 +121,7 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { const { token: _plaintext, ...row } = created; setTokens((current) => [row, ...(current ?? [])]); } catch (e) { + if (isSessionExpired(e)) return login(); fail(e, "tokens-mint"); } finally { setBusy(null); @@ -136,6 +143,10 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { 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); @@ -191,7 +202,7 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) {

    )} - {error &&

    {error}

    } + {error && !confirming &&

    {error}

    } {minted && (
    @@ -245,7 +256,7 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { @@ -255,6 +266,8 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { {tokenSession ? null + : loadFailed + ?

    The token list could not be loaded. Reload to try again.

    : tokens === null ?

    Loading tokens…

    : tokens.length === 0 @@ -279,6 +292,7 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { title="Revoke this token?" onClose={() => { 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 @@ -293,14 +307,19 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { > {busy === "revoke" ? "Revoking…" : "Revoke"} - {/* Focus lands here, not on Revoke — the same reason My Demos orders - its delete confirmation this way. */} + {/* `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. */} diff --git a/runner/e2e/api-tokens.spec.ts b/runner/e2e/api-tokens.spec.ts index 8d034b25c..56a69fefc 100644 --- a/runner/e2e/api-tokens.spec.ts +++ b/runner/e2e/api-tokens.spec.ts @@ -197,6 +197,42 @@ test.describe("/api-tokens", () => { 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("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. diff --git a/runner/e2e/share-create-live.spec.ts b/runner/e2e/share-create-live.spec.ts index 7364e980a..c234c2996 100644 --- a/runner/e2e/share-create-live.spec.ts +++ b/runner/e2e/share-create-live.spec.ts @@ -59,7 +59,9 @@ test("a demo shared today is a page a client can open — until it is revoked", // 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 diff --git a/runner/pipeline/token-routes.test.mjs b/runner/pipeline/token-routes.test.mjs index a31c08b64..dffcecd5f 100644 --- a/runner/pipeline/token-routes.test.mjs +++ b/runner/pipeline/token-routes.test.mjs @@ -258,6 +258,38 @@ test("extra whitespace after Bearer does not slip past the capability fence eith 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); diff --git a/runner/workers/api/src/auth.ts b/runner/workers/api/src/auth.ts index d9bce29a4..098b154ba 100644 --- a/runner/workers/api/src/auth.ts +++ b/runner/workers/api/src/auth.ts @@ -45,11 +45,17 @@ export function isTokenIdentity(identity: Identity | null): boolean { * 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]+(.+)$/.exec(auth.trim()); + const match = /^Bearer[ \t]+(.+)$/i.exec(auth.trim()); const value = match?.[1]?.trim(); return value ? value : null; } From 3b227f37846e1a964886ddf9e0e48300003dd519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Fri, 21 Aug 2026 12:36:34 +0200 Subject: [PATCH 5/5] fix(runner): show a token minted after a failed listing (DEV-2583) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A regression from the previous commit. Making failure its own render state put `loadFailed` ahead of `tokens` in the branch, while Create stayed live after a failed read — so a mint wrote its row into state and the page kept showing the failure message instead. The plaintext callout appeared, the row did not, and the credential that had just been created could not be revoked from the page. That is the worst state this feature has. The notice is now a line beside the list rather than instead of it, so the minted row and its Revoke are reachable. The notice stays up either way: one row known locally is not the list, and the page must not imply it is. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/ApiTokens.tsx | 19 +++++++++++--- runner/e2e/api-tokens.spec.ts | 35 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/runner/apps/authoring/src/ApiTokens.tsx b/runner/apps/authoring/src/ApiTokens.tsx index ea6fa2acd..ac47c9f77 100644 --- a/runner/apps/authoring/src/ApiTokens.tsx +++ b/runner/apps/authoring/src/ApiTokens.tsx @@ -264,14 +264,25 @@ export function ApiTokensPage({ apiBase, user }: ApiTokensPageProps) { )} + {/* 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 - : loadFailed - ?

    The token list could not be loaded. Reload to try again.

    : tokens === null - ?

    Loading tokens…

    + ? (loadFailed ? null :

    Loading tokens…

    ) : tokens.length === 0 - ?

    No tokens yet.

    + ? (loadFailed ? null :

    No tokens yet.

    ) : (
      {tokens.map((token) => ( diff --git a/runner/e2e/api-tokens.spec.ts b/runner/e2e/api-tokens.spec.ts index 56a69fefc..deef14575 100644 --- a/runner/e2e/api-tokens.spec.ts +++ b/runner/e2e/api-tokens.spec.ts @@ -233,6 +233,41 @@ test.describe("/api-tokens", () => { 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.