diff --git a/.env.example b/.env.example index f4827fb..be7bbf3 100644 --- a/.env.example +++ b/.env.example @@ -1,10 +1,14 @@ -# Every environment variable the services read. Copy it to .env and pass it with Node's --env-file, for -# example `pnpm exec tsx --env-file=.env src/api.ts`. `render workflows dev` loads .env by default. On -# Render, set these on the services instead (see render.yaml and the Configuration reference in -# docs/1_EXTENSION.md). +# Every environment variable the services read. Copy only the variables a service needs to its own +# env file, for example `.env.api`, and pass it with `pnpm exec tsx --env-file=.env.api src/api.ts`. +# `render workflows dev` loads .env by default: keep API-only secrets out of that shared file. On +# Render, set variables on the services that need them (see render.yaml and docs/1_EXTENSION.md). # ── API service ─────────────────────────────────────────────────────────── +# Shared with API callers for authentication. API_TOKEN= +# API only: at least 32 characters, different from API_TOKEN. Keep stable across restarts/replicas. +# Never give this signing secret to the workflow or CLI clients. Rotating it invalidates question IDs. +QUESTION_ID_SECRET= # WORKFLOW_SLUG=codebase-researcher-workflow # PORT=3000 # Points the API at `render workflows dev` instead of Render; RENDER_API_KEY is then optional. diff --git a/README.md b/README.md index 942be49..bb48ffc 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,7 @@ A reader can follow each link to the exact lines and decide for themselves. - HTTPS egress defaults to the model provider's API host and can be replaced with an exact host and leftmost-wildcard allow-list. DNS still resolves arbitrary names. - Git tokens exist only in the builder Sandbox while it clones. They are scrubbed and verified absent before the snapshot is taken. - Anyone who holds `API_TOKEN` can read the configured repositories through the researcher, so share it like a read token for those repositories. +- The API signs question IDs with a separate, server-only `QUESTION_ID_SECRET`. The Blueprint generates it; keep it stable across API replicas and restarts, and never share it with callers or the workflow. See [question-ID rotation and migration](docs/1_EXTENSION.md#question-id-signing-and-rotation). The architecture guide's [trust boundaries](docs/2_ARCHITECTURE.md#trust-boundaries) section explains these properties and the residual risks. diff --git a/docs/0_QUICKSTART.md b/docs/0_QUICKSTART.md index 6a25ec0..5c0386d 100644 --- a/docs/0_QUICKSTART.md +++ b/docs/0_QUICKSTART.md @@ -60,8 +60,13 @@ blank `sync: false` value counts as unset. | Snapshot cron | `RENDER_WORKSPACE_ID` | The key can access multiple workspaces | | Snapshot cron | `GITHUB_TOKEN` | Reading private GitHub repositories | -The Blueprint generates `API_TOKEN` and wires `WORKFLOW_SLUG` from the -workflow service into the API. +The Blueprint generates separate `API_TOKEN` and `QUESTION_ID_SECRET` values +on the API service and wires `WORKFLOW_SLUG` from the workflow service into +the API. Share only `API_TOKEN` with callers. Keep `QUESTION_ID_SECRET` stable +across API replicas and restarts; it signs polling and follow-up IDs. Existing +deployments must set this new secret before upgrading. IDs issued by older +versions become invalid; start new questions after upgrading. See +[rotation and migration](1_EXTENSION.md#question-id-signing-and-rotation). ### Build the first snapshot @@ -168,8 +173,9 @@ Run each command in its own terminal: REPOS=render-oss/cli,render-oss/sdk LOCAL_RUNNER_ARGS=--faux \ render workflows dev --port 8121 -- pnpm start:workflow -# Terminal 2 -API_TOKEN=dev-token RENDER_LOCAL_DEV_URL=http://localhost:8121 pnpm start:api +# Terminal 2: these credential values are for local development only. +API_TOKEN=dev-token QUESTION_ID_SECRET=local-only-question-id-secret-1234567890 \ + RENDER_LOCAL_DEV_URL=http://localhost:8121 pnpm start:api ``` Off Render, the workflow uses the local Sandbox provider by default. Then ask: diff --git a/docs/1_EXTENSION.md b/docs/1_EXTENSION.md index 3d3419d..af44a60 100644 --- a/docs/1_EXTENSION.md +++ b/docs/1_EXTENSION.md @@ -240,9 +240,21 @@ answer as a new question with `follow_up_to` set to this ID. Failure reasons are safe to display. A workflow-level `failed` status means the parent task itself failed or was canceled. -Question IDs are `.`, signed with `API_TOKEN`. Rotating the -token invalidates outstanding IDs. The signature also prevents this API from -reading unrelated workflow runs in the workspace. +### Question ID signing and rotation + +Question IDs are `.`, signed with the server-only +`QUESTION_ID_SECRET`. This required secret must have at least 32 characters +and differ from `API_TOKEN`. Keep it on the API service only, stable across +replicas and restarts; callers, the workflow, and `pnpm ask` do not need it. +The signature prevents a caller who holds only `API_TOKEN` from authorizing +lookups of unrelated workflow runs. + +Rotating `QUESTION_ID_SECRET` invalidates outstanding polling and follow-up +IDs. Rotating only `API_TOKEN` preserves their signatures; callers must use +the new bearer token. When upgrading an existing deployment, set a new +independent `QUESTION_ID_SECRET` before starting the API. All IDs issued by +the old, bearer-token-signing implementation become invalid; start new +questions. Legacy signatures are not accepted. ### Receive callbacks @@ -314,7 +326,8 @@ string is treated as unset. | Variable | Default | Purpose | |---|---|---| -| `API_TOKEN` | Generated | Authenticates `/v1/*` and signs question IDs | +| `API_TOKEN` | Generated | Authenticates `/v1/*`; shared with callers | +| `QUESTION_ID_SECRET` | Generated by Blueprint; otherwise required | API-only question-ID signing key; at least 32 characters, different from `API_TOKEN` | | `PORT` | `3000` | HTTP port | | `RENDER_API_KEY` | None | Starts and reads workflow runs | | `WORKFLOW_SLUG` | `codebase-researcher-workflow` | Builds the `answer_question` task slug; wired by the Blueprint | diff --git a/docs/2_ARCHITECTURE.md b/docs/2_ARCHITECTURE.md index f3d1ec2..6239556 100644 --- a/docs/2_ARCHITECTURE.md +++ b/docs/2_ARCHITECTURE.md @@ -131,8 +131,10 @@ the underlying snapshot without coordinating with active workflow runs. question. 3. Starts `answer_question` with `AnswerQuestionInput`, forwarding an optional idempotency key. -4. Returns a signed question ID. `GET /v1/questions/:id` verifies that - signature before reading the workflow run. +4. Returns a question ID signed with the API-only `QUESTION_ID_SECRET`, + independently of the caller's `API_TOKEN`. `GET /v1/questions/:id` verifies + that signature before reading the workflow run. The same secret is shared + across API replicas and retained across restarts. The API has no database. Each workflow run stores the full conversation it answered, so a chain of follow-ups only ever reads the latest run. diff --git a/docs/3_TAKEAWAYS.md b/docs/3_TAKEAWAYS.md index 3f42213..7886a96 100644 --- a/docs/3_TAKEAWAYS.md +++ b/docs/3_TAKEAWAYS.md @@ -226,7 +226,8 @@ The API stores nothing. Each `answer_question` run's input holds the whole conversation it answered, and its result holds the reply, so the run is both the job and the database row: -- a signed question ID lets the API read back only the runs it started; +- a question ID signed with a separate server-only secret lets the API read + back only the runs it started; the caller's bearer token cannot mint IDs; - a follow-up reads the earlier run's transcript and reply, appends the new question, and starts a new run, so a chain of follow-ups never needs more than the latest ID; diff --git a/render.yaml b/render.yaml index dedc706..a6aece0 100644 --- a/render.yaml +++ b/render.yaml @@ -26,6 +26,8 @@ projects: sync: false - key: API_TOKEN generateValue: true + - key: QUESTION_ID_SECRET + generateValue: true - key: WORKFLOW_SLUG fromService: type: workflow diff --git a/src/api.ts b/src/api.ts index 3804a14..9575074 100644 --- a/src/api.ts +++ b/src/api.ts @@ -19,6 +19,7 @@ export type GetTaskRun = (id: string) => Promise; export type AppDeps = { apiToken: string; + questionIdSecret: string; startQuestion: StartQuestion; getTaskRun: GetTaskRun; isLocalDev: boolean; @@ -26,19 +27,19 @@ export type AppDeps = { }; /** - * A question id is `.`, so `GET` only reads runs this API started; the API token alone - * can't be used to read other runs in the workspace. Rotating API_TOKEN invalidates outstanding ids. + * A question id is `.`, signed with a server-only secret so the API token alone + * cannot authorize unrelated workflow lookups. Rotating QUESTION_ID_SECRET invalidates outstanding ids. */ -const questionIdMac = (apiToken: string, runId: string) => - createHmac("sha256", apiToken).update(`question-id:${runId}`).digest("base64url").slice(0, 22); +const questionIdMac = (questionIdSecret: string, runId: string) => + createHmac("sha256", questionIdSecret).update(`question-id:${runId}`).digest("base64url").slice(0, 22); -export const toQuestionId = (apiToken: string, runId: string) => `${runId}.${questionIdMac(apiToken, runId)}`; +export const toQuestionId = (questionIdSecret: string, runId: string) => `${runId}.${questionIdMac(questionIdSecret, runId)}`; -export function fromQuestionId(apiToken: string, questionId: string): string | null { +export function fromQuestionId(questionIdSecret: string, questionId: string): string | null { const dot = questionId.lastIndexOf("."); if (dot <= 0) return null; const runId = questionId.slice(0, dot); - return safeEqual(questionId.slice(dot + 1), questionIdMac(apiToken, runId)) ? runId : null; + return safeEqual(questionId.slice(dot + 1), questionIdMac(questionIdSecret, runId)) ? runId : null; } const postBody = (isLocalDev: boolean) => @@ -69,7 +70,7 @@ export function createApp(deps: AppDeps) { const PostBody = postBody(deps.isLocalDev); async function readRun(questionId: string): Promise { - const runId = fromQuestionId(deps.apiToken, questionId); + const runId = fromQuestionId(deps.questionIdSecret, questionId); if (!runId) throw httpError(404, "not found"); try { return await deps.getTaskRun(runId); @@ -123,7 +124,7 @@ export function createApp(deps: AppDeps) { signal: AbortSignal.timeout(5000), }); deps.log.info("task started", { event: "task_started", run_id: run.id, follow_up: !!follow_up_to }); - return c.json({ id: toQuestionId(deps.apiToken, run.id) }, 202); + return c.json({ id: toQuestionId(deps.questionIdSecret, run.id) }, 202); }, ); @@ -149,16 +150,22 @@ const Env = z .object({ PORT: z.coerce.number().int().positive().default(3000), API_TOKEN: z.string().min(1, "API_TOKEN is required"), + QUESTION_ID_SECRET: z.string({ error: "QUESTION_ID_SECRET is required" }).min(32, "QUESTION_ID_SECRET must be at least 32 characters"), RENDER_API_KEY: z.string().min(1).optional(), RENDER_LOCAL_DEV_URL: z.string().min(1).optional(), WORKFLOW_SLUG: z.string().min(1).default("codebase-researcher-workflow"), }) + .refine((e) => e.QUESTION_ID_SECRET !== e.API_TOKEN, { + message: "QUESTION_ID_SECRET must differ from API_TOKEN", + path: ["QUESTION_ID_SECRET"], + }) .refine((e) => e.RENDER_LOCAL_DEV_URL || e.RENDER_API_KEY, { message: "RENDER_API_KEY is required unless RENDER_LOCAL_DEV_URL is set", }) .transform((e) => ({ port: e.PORT, apiToken: e.API_TOKEN, + questionIdSecret: e.QUESTION_ID_SECRET, renderApiKey: e.RENDER_API_KEY, localDevUrl: e.RENDER_LOCAL_DEV_URL, taskSlug: `${e.WORKFLOW_SLUG}/answer_question`, @@ -193,6 +200,7 @@ if (process.argv[1] && import.meta.url === pathToFileURL(realpathSync(process.ar const app = createApp({ apiToken: env.apiToken, + questionIdSecret: env.questionIdSecret, startQuestion, getTaskRun, isLocalDev: !!env.localDevUrl, diff --git a/test/api.test.ts b/test/api.test.ts index 54b2ed4..1aa3def 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { createHmac } from "node:crypto"; import { ClientError } from "@renderinc/sdk"; import { createApp, loadEnv, toQuestionId, type AppDeps, type GetTaskRun } from "../src/api.ts"; import { signCallback, verifyCallback } from "../src/callback.ts"; @@ -6,9 +7,10 @@ import { createLog } from "../src/log.ts"; import type { StartQuestion } from "../src/question.ts"; const TOKEN = "test-api-token-1234"; +const QUESTION_ID_SECRET = "test-question-id-secret-1234567890"; const AUTH = { Authorization: `Bearer ${TOKEN}` }; const JSON_AUTH = { "content-type": "application/json", ...AUTH }; -const qid = (runId: string) => toQuestionId(TOKEN, runId); +const qid = (runId: string) => toQuestionId(QUESTION_ID_SECRET, runId); const question = "How does deploy retry?"; function harness(overrides: Partial = {}) { @@ -25,6 +27,7 @@ function harness(overrides: Partial = {}) { }; const app = createApp({ apiToken: TOKEN, + questionIdSecret: QUESTION_ID_SECRET, startQuestion, getTaskRun, isLocalDev: false, @@ -154,9 +157,67 @@ describe("HTTP API", () => { } }); + it("accepts issued question IDs for polling and follow-up without exposing the signing secret", async () => { + const { post, get, started, logs } = withRun({ + status: "completed", + input: [{ transcript: [{ role: "user", text: question }] }], + results: [{ reply: { kind: "failure", traceId: "t1", reason: "test reply" } }], + }); + const issued = await (await post({ question })).json() as { id: string }; + expect((await get(issued.id)).status).toBe(200); + expect((await post({ question: "Please explain", follow_up_to: issued.id })).status).toBe(202); + expect(JSON.stringify({ issued, started, logs })).not.toContain(QUESTION_ID_SECRET); + }); + + it("keeps issued IDs valid after bearer rotation when the signing secret stays fixed", async () => { + const issued = await (await harness().post({ question })).json() as { id: string }; + const rotatedToken = "rotated-api-token-5678"; + const rotated = harness({ apiToken: rotatedToken }); + expect((await rotated.app.request(`/v1/questions/${issued.id}`, { headers: AUTH })).status).toBe(401); + expect((await rotated.app.request(`/v1/questions/${issued.id}`, { + headers: { Authorization: `Bearer ${rotatedToken}` }, + })).status).toBe(200); + expect(rotated.lookups).toEqual(["run-123"]); + }); + + it("invalidates issued IDs after signing-secret rotation before any workflow access", async () => { + const issued = await (await harness().post({ question })).json() as { id: string }; + const rotated = harness({ questionIdSecret: "rotated-question-id-secret-12345678" }); + expect((await rotated.get(issued.id)).status).toBe(404); + expect((await rotated.post({ question, follow_up_to: issued.id })).status).toBe(404); + expect(rotated.lookups).toEqual([]); + expect(rotated.started).toEqual([]); + }); + + it("rejects question IDs forged with the caller's bearer token before any workflow access", async () => { + const lookups: string[] = []; + const { get, post, started } = harness({ + getTaskRun: async (id) => { + lookups.push(id); + return { + status: "completed", + input: [{ transcript: [{ role: "user", text: question }] }], + results: [{ reply: { kind: "failure", traceId: "t1", reason: "test reply" } }], + }; + }, + }); + const runId = "trn-unrelated"; + const mac = createHmac("sha256", TOKEN).update(`question-id:${runId}`).digest("base64url").slice(0, 22); + const forgedId = `${runId}.${mac}`; + + const statuses = [ + (await get(forgedId)).status, + (await post({ question, follow_up_to: forgedId })).status, + ]; + + expect(statuses).toEqual([404, 404]); + expect(lookups).toEqual([]); + expect(started).toEqual([]); + }); + it("only reads runs this API started: raw, forged, foreign-signed, or unknown ids are 404", async () => { const { get, lookups } = harness(); - for (const id of ["trn-other", "trn-other.AAAAAAAAAAAAAAAAAAAAAA", toQuestionId("a-different-api-token", "trn-other")]) { + for (const id of ["trn-other", "trn-other.AAAAAAAAAAAAAAAAAAAAAA", toQuestionId("a-different-signing-secret-12345678", "trn-other")]) { expect((await get(id)).status, id).toBe(404); } expect(lookups).toEqual([]); @@ -203,15 +264,24 @@ describe("signCallback / verifyCallback", () => { }); describe("loadEnv", () => { + it("requires a server-only question-ID secret of at least 32 characters, distinct from API_TOKEN", () => { + const base = { API_TOKEN: TOKEN, RENDER_API_KEY: "rk" }; + for (const secret of [undefined, "", " ", "s".repeat(31)]) { + expect(() => loadEnv({ ...base, QUESTION_ID_SECRET: secret })).toThrow(/QUESTION_ID_SECRET/); + } + expect(() => loadEnv({ ...base, API_TOKEN: QUESTION_ID_SECRET, QUESTION_ID_SECRET })).toThrow(/must differ from API_TOKEN/); + expect(loadEnv({ ...base, QUESTION_ID_SECRET }).questionIdSecret).toBe(QUESTION_ID_SECRET); + }); + it("requires API_TOKEN, and RENDER_API_KEY unless running against local dev", () => { - expect(() => loadEnv({ RENDER_API_KEY: "rk" })).toThrow(); - expect(() => loadEnv({ API_TOKEN: "tok" })).toThrow(/RENDER_API_KEY is required/); - expect(loadEnv({ API_TOKEN: "tok", RENDER_LOCAL_DEV_URL: "http://localhost:8121" }).localDevUrl).toBe("http://localhost:8121"); + expect(() => loadEnv({ QUESTION_ID_SECRET, RENDER_API_KEY: "rk" })).toThrow(); + expect(() => loadEnv({ QUESTION_ID_SECRET, API_TOKEN: "tok" })).toThrow(/RENDER_API_KEY is required/); + expect(loadEnv({ QUESTION_ID_SECRET, API_TOKEN: "tok", RENDER_LOCAL_DEV_URL: "http://localhost:8121" }).localDevUrl).toBe("http://localhost:8121"); }); it("treats blank Dashboard values as unset and derives the task slug from WORKFLOW_SLUG", () => { - expect(() => loadEnv({ API_TOKEN: "tok", RENDER_API_KEY: "", RENDER_LOCAL_DEV_URL: " " })).toThrow(/RENDER_API_KEY is required/); - expect(loadEnv({ API_TOKEN: "tok", RENDER_API_KEY: "rk" }).taskSlug).toBe("codebase-researcher-workflow/answer_question"); - expect(loadEnv({ API_TOKEN: "tok", RENDER_API_KEY: "rk", WORKFLOW_SLUG: "my-wf" }).taskSlug).toBe("my-wf/answer_question"); + expect(() => loadEnv({ QUESTION_ID_SECRET, API_TOKEN: "tok", RENDER_API_KEY: "", RENDER_LOCAL_DEV_URL: " " })).toThrow(/RENDER_API_KEY is required/); + expect(loadEnv({ QUESTION_ID_SECRET, API_TOKEN: "tok", RENDER_API_KEY: "rk" }).taskSlug).toBe("codebase-researcher-workflow/answer_question"); + expect(loadEnv({ QUESTION_ID_SECRET, API_TOKEN: "tok", RENDER_API_KEY: "rk", WORKFLOW_SLUG: "my-wf" }).taskSlug).toBe("my-wf/answer_question"); }); });