From 37c9074f9b0d535109ddea9103483022dc1668af Mon Sep 17 00:00:00 2001 From: Dylan Boudro <121908331+starmorph@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:00:50 -0700 Subject: [PATCH 1/3] Restrict callbacks to operator-approved origins --- .env.example | 7 ++++ README.md | 1 + docs/0_QUICKSTART.md | 21 ++++++++++- docs/1_EXTENSION.md | 37 ++++++++++++++++-- docs/2_ARCHITECTURE.md | 10 ++++- docs/3_TAKEAWAYS.md | 2 + render.yaml | 8 +++- src/api.ts | 24 +++++++----- src/callback-policy.ts | 66 ++++++++++++++++++++++++++++++++ src/tasks.ts | 5 ++- src/workflow.ts | 2 + test/api.test.ts | 46 ++++++++++++++++++----- test/callback-policy.test.ts | 73 ++++++++++++++++++++++++++++++++++++ test/tasks.test.ts | 72 +++++++++++++++++++++++++++++++++-- 14 files changed, 345 insertions(+), 29 deletions(-) create mode 100644 src/callback-policy.ts create mode 100644 test/callback-policy.test.ts diff --git a/.env.example b/.env.example index f4827fb..eb9e88d 100644 --- a/.env.example +++ b/.env.example @@ -6,6 +6,11 @@ # ── API service ─────────────────────────────────────────────────────────── API_TOKEN= # WORKFLOW_SLUG=codebase-researcher-workflow +# Optional: set the same comma-separated origins on the API and workflow. Blank disables callbacks. +# HTTPS origins only; every path on each approved origin is trusted. Redirects are rejected. +CALLBACK_ALLOWED_ORIGINS= +# Local callback example (requires local Workflows dev mode on both services): +# CALLBACK_ALLOWED_ORIGINS=http://localhost:4000 # PORT=3000 # Points the API at `render workflows dev` instead of Render; RENDER_API_KEY is then optional. # RENDER_LOCAL_DEV_URL=http://localhost:8121 @@ -40,6 +45,8 @@ GITHUB_TOKEN= # ── Local development ─────────────────────────────────────────────────── # LOCAL_REPOS_ROOT=.scratch/repos +# render workflows dev sets RENDER_USE_LOCAL_DEV=true for the workflow process. +# The API uses its loopback RENDER_LOCAL_DEV_URL to verify local mode. HTTP callbacks stay disabled on Render. # Appended to the auto-bundled runner's command; --faux runs without a model. # LOCAL_RUNNER_ARGS=--faux diff --git a/README.md b/README.md index 942be49..f31e889 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,7 @@ A reader can follow each link to the exact lines and decide for themselves. - The model API key arrives as a one-time file that the runner deletes on read. The runner's Node binary is execute-only, so the agent's commands can't read the key from its memory. The snapshot build and every restored Sandbox must prove this, or the build fails and the Sandbox gets no work. - 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. +- Callbacks are disabled until the operator sets the same `CALLBACK_ALLOWED_ORIGINS` on the API and workflow. Both services enforce exact HTTPS origins, and delivery rejects redirects. Polling works without callback configuration. - Anyone who holds `API_TOKEN` can read the configured repositories through the researcher, so share it like a read token for those repositories. The architecture guide's [trust boundaries](docs/2_ARCHITECTURE.md#trust-boundaries) diff --git a/docs/0_QUICKSTART.md b/docs/0_QUICKSTART.md index 6a25ec0..9f60417 100644 --- a/docs/0_QUICKSTART.md +++ b/docs/0_QUICKSTART.md @@ -63,6 +63,17 @@ blank `sync: false` value counts as unset. The Blueprint generates `API_TOKEN` and wires `WORKFLOW_SLUG` from the workflow service into the API. +Callbacks are optional and disabled by default. To enable them, set the same +`CALLBACK_ALLOWED_ORIGINS` on both the API and workflow, for example +`https://support-hooks.example.com`. Use comma-separated HTTPS origins with +no path, query, fragment, or credentials. Every path on an approved origin is +trusted; choose a dedicated integration origin. Delivery rejects redirects. +Polling needs no callback configuration. + +Before upgrading an existing deployment that uses callbacks, configure these +origins on both services. Unapproved new requests and queued deliveries fail +closed after the upgrade; callers can retrieve replies by polling. + ### Build the first snapshot Open `codebase-researcher-snapshots` and click **Trigger Run**, or wait for its @@ -172,7 +183,15 @@ REPOS=render-oss/cli,render-oss/sdk LOCAL_RUNNER_ARGS=--faux \ API_TOKEN=dev-token RENDER_LOCAL_DEV_URL=http://localhost:8121 pnpm start:api ``` -Off Render, the workflow uses the local Sandbox provider by default. Then ask: +Off Render, the workflow uses the local Sandbox provider by default. Callbacks +remain disabled unless both processes receive the same explicit setting, for +example `CALLBACK_ALLOWED_ORIGINS=http://localhost:4000`. Loopback HTTP is +allowed only in local Workflows dev mode: the CLI sets `RENDER_USE_LOCAL_DEV` +for the workflow, and the API verifies its loopback `RENDER_LOCAL_DEV_URL`. +`localhost`, `127.0.0.1`, and `[::1]` are supported; ports must match exactly. +HTTP callbacks are never enabled when running on Render. + +Then ask: ```bash # Terminal 3 diff --git a/docs/1_EXTENSION.md b/docs/1_EXTENSION.md index 3d3419d..894ca75 100644 --- a/docs/1_EXTENSION.md +++ b/docs/1_EXTENSION.md @@ -195,7 +195,9 @@ The request body accepts: the agent's turn, then the new question. The earlier run must have finished (`409` otherwise), and a conversation holds at most 200 messages. - `callback`: optional `{ "url": "https://...", "secret": "..." }`. Secrets - must be 16–256 characters. Local development also permits localhost HTTP. + must be 16–256 characters. The URL must use an origin explicitly approved + by the operator in `CALLBACK_ALLOWED_ORIGINS`; callbacks are disabled when + that setting is missing or blank. - `metadata`: optional routing data with at most 16 string, number, or boolean values. It is returned unchanged in the callback. @@ -247,8 +249,35 @@ reading unrelated workflow runs in the workspace. ### Receive callbacks Polling suits scripts and short-lived callers. A long-lived integration, such -as a chat bot or support desk, should pass a callback instead. When one is -supplied, the `deliver` subtask sends: +as a chat bot or support desk, can use a callback after the operator enables it. +Set the same `CALLBACK_ALLOWED_ORIGINS` on the API and workflow, for example +`https://support-hooks.example.com,https://events.example.com:8443`. The API +checks before starting work, and the delivery subtask checks independently. +Missing or blank configuration disables callbacks and leaves polling available. +Malformed configuration prevents the service from starting its work. + +Each entry must be an origin only: scheme, host, and optional port, with no +credentials, path, query, or fragment. Matching uses normalized `URL.origin`; +subdomains and different ports need their own entries. Approved origins are +operator-trusted, including every path they serve. Choose a dedicated +integration origin and review its outbound behavior. All HTTP redirects are +rejected, even to another approved origin, so receivers must accept the POST +directly. + +Production accepts HTTPS only. Local development additionally accepts explicit +`http://localhost:`, `http://127.0.0.1:`, and +`http://[::1]:` entries. Local mode must come from operator environment: +`render workflows dev` sets `RENDER_USE_LOCAL_DEV` for the workflow, or a +loopback HTTP `RENDER_LOCAL_DEV_URL` verifies local mode for the API. Running +on Render disables the HTTP exception. Set the same local origins on both +processes; setting a request URL alone never enables callbacks. + +When upgrading a deployment that already uses callbacks, configure approved +origins on both services before deploying. New unapproved requests return 400; +unapproved queued deliveries also fail closed. Polling remains available for +retrieving the reply. + +For an approved callback, the `deliver` subtask sends: - `X-Researcher-Timestamp`: Unix seconds. - `X-Researcher-Signature`: @@ -318,6 +347,7 @@ string is treated as unset. | `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 | +| `CALLBACK_ALLOWED_ORIGINS` | Disabled | Comma-separated approved origins; must match the workflow | | `RENDER_LOCAL_DEV_URL` | None | Uses a local Workflows server instead of Render | ### Workflow service @@ -326,6 +356,7 @@ string is treated as unset. |---|---|---| | `RENDER_API_KEY` | None | Sandbox operations | | `RENDER_WORKSPACE_ID` | Resolved | Required when the key sees multiple workspaces | +| `CALLBACK_ALLOWED_ORIGINS` | Disabled | Comma-separated approved origins, checked again at delivery; must match the API | | `SANDBOX_PROVIDER` | Render on Render, local elsewhere | Sandbox implementation | | `MODEL_PROVIDER` | `anthropic` | Pi provider | | `MODEL` | `claude-opus-5` | Model ID | diff --git a/docs/2_ARCHITECTURE.md b/docs/2_ARCHITECTURE.md index f3d1ec2..37f4dae 100644 --- a/docs/2_ARCHITECTURE.md +++ b/docs/2_ARCHITECTURE.md @@ -146,7 +146,11 @@ answered, so a chain of follow-ups only ever reads the latest run. `turn_finished` outcome. - `investigate`: retryable infrastructure boundary; opens and runs a fresh Sandbox. -- `deliver`: independently retryable signed callback POST. +- `deliver`: independently retryable signed callback POST. Both the API and + delivery validate the operator-configured `CALLBACK_ALLOWED_ORIGINS`; no + configured origins means callbacks are disabled. Delivery rejects every + redirect. These requests originate from the host, outside Sandbox egress + controls. The parent does not retry. Infrastructure failures thrown by `investigate` retry on a fresh Sandbox; model and submission failures become user-safe @@ -312,7 +316,9 @@ adversarially. makes the process non-dumpable, and `verify.sh` must prove that property before the key is uploaded. - The **host workflow** treats Sandbox output as untrusted, repeats manifest - checks, constructs links, classifies failures, and owns cleanup. + checks, constructs links, classifies failures, and owns cleanup. Its callback + sender independently enforces approved origins and refuses redirects. Every + path on an approved origin is operator-trusted. `verify.sh` checks the actual builder and every restored Sandbox instead of assuming the build steps worked. Known residual risks remain: DNS resolves diff --git a/docs/3_TAKEAWAYS.md b/docs/3_TAKEAWAYS.md index 3f42213..0727bfc 100644 --- a/docs/3_TAKEAWAYS.md +++ b/docs/3_TAKEAWAYS.md @@ -239,6 +239,8 @@ the HTTP API. For any webhook-driven integration: - use the platform event ID as the idempotency key; - store the latest question ID per thread and send it as `follow_up_to`; - derive a per-event callback secret and verify callbacks against the raw body; +- configure the same exact approved callback origins on the API and workflow; + trust all paths on each approved origin and reject redirects at delivery; - make callback handling idempotent; - escape model text and suppress mentions. diff --git a/render.yaml b/render.yaml index dedc706..a2c792d 100644 --- a/render.yaml +++ b/render.yaml @@ -20,7 +20,7 @@ projects: startCommand: pnpm start:api healthCheckPath: /healthz buildFilter: - paths: [src/api.ts, src/question.ts, src/investigation.ts, src/callback.ts, src/log.ts, src/util.ts, package.json, pnpm-lock.yaml, pnpm-workspace.yaml, .node-version, tsconfig.json] + paths: [src/api.ts, src/question.ts, src/investigation.ts, src/callback.ts, src/callback-policy.ts, src/log.ts, src/util.ts, package.json, pnpm-lock.yaml, pnpm-workspace.yaml, .node-version, tsconfig.json] envVars: - key: RENDER_API_KEY sync: false @@ -31,6 +31,9 @@ projects: type: workflow name: codebase-researcher-workflow property: slug + # Optional: set identical HTTPS origins on the API and workflow. Blank disables callbacks. + - key: CALLBACK_ALLOWED_ORIGINS + sync: false - type: workflow name: codebase-researcher-workflow @@ -55,6 +58,9 @@ projects: sync: false - key: SANDBOX_NETWORK_POLICY value: allow-list + # Must match the API setting. The delivery subtask checks it independently. + - key: CALLBACK_ALLOWED_ORIGINS + sync: false - type: cron name: codebase-researcher-snapshots diff --git a/src/api.ts b/src/api.ts index 3804a14..c5a45d1 100644 --- a/src/api.ts +++ b/src/api.ts @@ -2,6 +2,7 @@ import { MESSAGE_MAX_CHARS, Transcript, TRANSCRIPT_MAX_MESSAGES } from "./invest import { QuestionMetadata, replyText, type Reply, type StartQuestion } from "./question.ts"; import { present } from "./util.ts"; import { safeEqual } from "./callback.ts"; +import { assertCallbackAllowed, loadCallbackPolicy, type CallbackPolicy } from "./callback-policy.ts"; import { createLog, type Log } from "./log.ts"; import { serve } from "@hono/node-server"; import { Hono } from "hono"; @@ -22,6 +23,7 @@ export type AppDeps = { startQuestion: StartQuestion; getTaskRun: GetTaskRun; isLocalDev: boolean; + callbackAllowedOrigins?: readonly string[]; log: Log; }; @@ -41,16 +43,19 @@ export function fromQuestionId(apiToken: string, questionId: string): string | n return safeEqual(questionId.slice(dot + 1), questionIdMac(apiToken, runId)) ? runId : null; } -const postBody = (isLocalDev: boolean) => +const postBody = (policy: CallbackPolicy) => z.object({ question: z.string().trim().min(1).max(MESSAGE_MAX_CHARS), follow_up_to: z.string().min(1).optional(), callback: z.object({ url: z.url().refine((raw) => { - const url = new URL(raw); - if (url.protocol === "https:") return true; - return isLocalDev && url.protocol === "http:" && (url.hostname === "localhost" || url.hostname === "127.0.0.1"); - }, "must be https:// (http://localhost allowed in local dev)"), + try { + assertCallbackAllowed(raw, policy); + return true; + } catch { + return false; + } + }, "callback URL must use an origin approved by CALLBACK_ALLOWED_ORIGINS (HTTPS, or explicit loopback HTTP in local dev; no credentials)"), secret: z.string().min(16).max(256), }).optional(), metadata: QuestionMetadata.optional(), @@ -66,7 +71,7 @@ function completedReply(run: TaskRun): { trace_id?: string; reply?: Reply } | un export function createApp(deps: AppDeps) { const app = new Hono(); - const PostBody = postBody(deps.isLocalDev); + const PostBody = postBody({ allowedOrigins: deps.callbackAllowedOrigins ?? [], isLocalDev: deps.isLocalDev }); async function readRun(questionId: string): Promise { const runId = fromQuestionId(deps.apiToken, questionId); @@ -164,10 +169,10 @@ const Env = z taskSlug: `${e.WORKFLOW_SLUG}/answer_question`, })); -export type ApiConfig = z.output; +export type ApiConfig = z.output & { callbackPolicy: CallbackPolicy }; export function loadEnv(env: NodeJS.ProcessEnv = process.env): ApiConfig { - return Env.parse(present(env)); + return { ...Env.parse(present(env)), callbackPolicy: loadCallbackPolicy(env) }; } // Starts the server only when run directly, not when tests import this module. @@ -195,7 +200,8 @@ if (process.argv[1] && import.meta.url === pathToFileURL(realpathSync(process.ar apiToken: env.apiToken, startQuestion, getTaskRun, - isLocalDev: !!env.localDevUrl, + isLocalDev: env.callbackPolicy.isLocalDev, + callbackAllowedOrigins: env.callbackPolicy.allowedOrigins, log, }); diff --git a/src/callback-policy.ts b/src/callback-policy.ts new file mode 100644 index 0000000..c86d265 --- /dev/null +++ b/src/callback-policy.ts @@ -0,0 +1,66 @@ +import { z } from "zod"; + +export type CallbackPolicy = { + allowedOrigins: readonly string[]; + isLocalDev: boolean; +}; + +const LOOPBACK = new Set(["localhost", "127.0.0.1", "[::1]"]); +const LOCAL_DEV_VALUES = new Set(["1", "t", "T", "true", "TRUE", "True"]); + +function isLocalDev(env: NodeJS.ProcessEnv): boolean { + if (env.RENDER === "true") return false; + if (LOCAL_DEV_VALUES.has(env.RENDER_USE_LOCAL_DEV ?? "")) return true; + if (!env.RENDER_LOCAL_DEV_URL) return false; + try { + const url = new URL(env.RENDER_LOCAL_DEV_URL); + return url.protocol === "http:" && LOOPBACK.has(url.hostname) && !url.username && !url.password; + } catch { + return false; + } +} + +function destination(raw: string, localDev: boolean): URL { + const url = new URL(raw); + if (url.username || url.password) throw new Error("callback URLs must not contain credentials"); + if (url.protocol !== "https:" && !(localDev && url.protocol === "http:" && LOOPBACK.has(url.hostname))) { + throw new Error("callbacks require HTTPS; explicit loopback HTTP origins are allowed only in local development"); + } + return url; +} + +const PolicyEnv = z.object({ CALLBACK_ALLOWED_ORIGINS: z.string().optional() }).transform((env, ctx) => { + if (!env.CALLBACK_ALLOWED_ORIGINS) return []; + const entries = env.CALLBACK_ALLOWED_ORIGINS.split(",").map((origin) => origin.trim()); + if (entries.some((origin) => !origin)) { + ctx.addIssue({ code: "custom", path: ["CALLBACK_ALLOWED_ORIGINS"], message: "must be a comma-separated list of origins without empty entries" }); + return z.NEVER; + } + return entries; +}); + +/** Missing or blank configuration disables callbacks. The environment is operator-controlled. */ +export function loadCallbackPolicy(env: NodeJS.ProcessEnv = process.env): CallbackPolicy { + const localDev = isLocalDev(env); + const entries = PolicyEnv.parse({ CALLBACK_ALLOWED_ORIGINS: env.CALLBACK_ALLOWED_ORIGINS?.trim() || undefined }); + const origins = entries.map((raw) => { + try { + const url = destination(raw, localDev); + if (!/^https?:\/\/[^\s/?#\\]+\/?$/i.test(raw) || url.pathname !== "/") { + throw new Error("entries must be origins only, without paths, query strings, or fragments"); + } + return url.origin; + } catch { + throw new Error("CALLBACK_ALLOWED_ORIGINS must contain valid HTTPS origins only (loopback HTTP requires local development)"); + } + }); + return { allowedOrigins: [...new Set(origins)], isLocalDev: localDev }; +} + +/** All paths on an approved origin are trusted; redirects must still be rejected by the sender. */ +export function assertCallbackAllowed(raw: string, policy: CallbackPolicy): void { + const url = destination(raw, policy.isLocalDev); + if (!policy.allowedOrigins.includes(url.origin)) { + throw new Error("callback origin is not approved by CALLBACK_ALLOWED_ORIGINS; omit callback to use polling"); + } +} diff --git a/src/tasks.ts b/src/tasks.ts index d02c212..1bfa159 100644 --- a/src/tasks.ts +++ b/src/tasks.ts @@ -13,6 +13,7 @@ import { RUNNER_EXIT, RunnerInput, type RunnerEvent, type Usage } from "./runner import { SnapshotUnavailableError, type OpenSandbox, type RunInvestigationResult, type Sandbox } from "./sandbox/index.ts"; import { getBuiltinModel } from "@earendil-works/pi-ai/providers/all"; import { signCallback } from "./callback.ts"; +import { assertCallbackAllowed, loadCallbackPolicy, type CallbackPolicy } from "./callback-policy.ts"; import type { Log } from "./log.ts"; import type { Config } from "./workflow.ts"; @@ -241,7 +242,8 @@ export async function answerQuestion( } /** POST the callback body with a signed request. Throws on non-2xx or network error. */ -export async function deliverCallback(input: DeliverInput): Promise { +export async function deliverCallback(input: DeliverInput, policy: CallbackPolicy = loadCallbackPolicy()): Promise { + assertCallbackAllowed(input.callback.url, policy); const { signature, timestamp } = signCallback(input.callback.secret, input.body); const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), 10_000); @@ -254,6 +256,7 @@ export async function deliverCallback(input: DeliverInput): Promise { "X-Researcher-Signature": signature, }, body: input.body, + redirect: "error", signal: controller.signal, }); if (!res.ok) throw new Error(`deliver got ${res.status}`); diff --git a/src/workflow.ts b/src/workflow.ts index 78a89da..bf71c00 100644 --- a/src/workflow.ts +++ b/src/workflow.ts @@ -7,6 +7,7 @@ import { task, type TaskContext } from "@renderinc/sdk/workflows"; import { z } from "zod"; import { answerQuestion, deliverCallback, investigateTask, type InvestigateInput, type InvestigateResult, type DeliverInput } from "./tasks.ts"; import { createLog } from "./log.ts"; +import { loadCallbackPolicy } from "./callback-policy.ts"; const MINUTE = 60_000; @@ -78,6 +79,7 @@ const Env = z modelApiKey: providerApiKey(e.MODEL_PROVIDER, env), agentTimeoutMs: e.AGENT_TIMEOUT_MS, sandboxProvisionTimeoutMs: e.SANDBOX_PROVISION_TIMEOUT_MS, + callbackPolicy: loadCallbackPolicy(env), }; }); export type Config = z.output; diff --git a/test/api.test.ts b/test/api.test.ts index 54b2ed4..01a0d3d 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -124,16 +124,44 @@ describe("HTTP API", () => { expect(long.started).toEqual([]); }); - it("requires an https callback URL, allowing http://localhost only in local dev", async () => { + it("disables callbacks until an operator approves an origin, while polling-only questions still start", async () => { + const { post, started } = harness(); + const rejected = await post({ question, callback: { url: "https://unapproved.example/hook", secret: "a".repeat(32) } }); + expect(rejected.status).toBe(400); + expect(started).toEqual([]); + expect((await post({ question })).status).toBe(202); + expect(started).toHaveLength(1); + }); + + it("accepts only approved callback origins, including the exact port, without URL credentials", async () => { + const secret = "a".repeat(32); + const { post, started } = harness({ callbackAllowedOrigins: ["https://receiver.example:8443"] }); + for (const url of [ + "https://receiver.example/hook", + "https://sub.receiver.example:8443/hook", + "https://receiver.example.attacker.example:8443/hook", + "https://127.0.0.1/hook", + "https://10.0.0.1/hook", + "https://user:password@receiver.example:8443/hook", + "http://receiver.example:8443/hook", + ]) { + expect((await post({ question, callback: { url, secret } })).status, url).toBe(400); + } + expect(started).toEqual([]); + expect((await post({ question, callback: { url: "https://RECEIVER.EXAMPLE:8443/hook?ticket=1", secret } })).status).toBe(202); + expect(started[0]!.input).toMatchObject({ callback: { url: "https://RECEIVER.EXAMPLE:8443/hook?ticket=1", secret } }); + }); + + it("allows explicit loopback HTTP origins only in local development", async () => { const secret = "a".repeat(32); - const prod = harness(); - expect((await prod.post({ question, callback: { url: "http://example.com/hook", secret } })).status).toBe(400); - expect((await prod.post({ question, callback: { url: "http://localhost:4000/hook", secret } })).status).toBe(400); - expect((await prod.post({ question, callback: { url: "https://example.com/hook", secret } })).status).toBe(202); - - const local = harness({ isLocalDev: true }); - expect((await local.post({ question, callback: { url: "http://localhost:4000/hook", secret } })).status).toBe(202); - expect((await local.post({ question, callback: { url: "http://example.com/hook", secret } })).status).toBe(400); + const url = "http://localhost:4000/hook"; + const origins = ["http://localhost:4000", "http://[::1]:4000"]; + expect((await harness({ callbackAllowedOrigins: origins }).post({ question, callback: { url, secret } })).status).toBe(400); + const local = harness({ isLocalDev: true, callbackAllowedOrigins: origins }); + expect((await local.post({ question, callback: { url, secret } })).status).toBe(202); + expect((await local.post({ question, callback: { url: "http://[::1]:4000/hook", secret } })).status).toBe(202); + expect((await local.post({ question, callback: { url: "http://localhost:4001/hook", secret } })).status).toBe(400); + expect((await harness({ isLocalDev: true }).post({ question, callback: { url, secret } })).status).toBe(400); }); it("maps the run status to running, completed (with the reply), or failed", async () => { diff --git a/test/callback-policy.test.ts b/test/callback-policy.test.ts new file mode 100644 index 0000000..598f4b0 --- /dev/null +++ b/test/callback-policy.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from "vitest"; +import { loadEnv } from "../src/api.ts"; +import { assertCallbackAllowed, loadCallbackPolicy } from "../src/callback-policy.ts"; +import { loadConfig } from "../src/workflow.ts"; + +describe("callback destination policy", () => { + it.each([undefined, "", " "])("disables callbacks when origins are missing or blank (%s)", (origins) => { + const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origins }); + expect(policy.allowedOrigins).toEqual([]); + expect(() => assertCallbackAllowed("https://receiver.example/hook", policy)).toThrow(/not approved/); + }); + + it("normalizes and deduplicates origins while approving all paths on the exact origin", () => { + const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "https://RECEIVER.example:443/, https://receiver.example" }); + expect(policy.allowedOrigins).toEqual(["https://receiver.example"]); + expect(() => assertCallbackAllowed("https://receiver.example/hooks/1?ticket=2", policy)).not.toThrow(); + for (const url of [ + "https://receiver.example:8443/hook", + "https://sub.receiver.example/hook", + "https://receiver.example.attacker.example/hook", + "https://user:password@receiver.example/hook", + "http://receiver.example/hook", + "https://10.0.0.1/hook", + "https://[::1]/hook", + ]) expect(() => assertCallbackAllowed(url, policy), url).toThrow(); + }); + + it.each([ + "not a URL", + "https:receiver.example", + "https://receiver.example/hook", + "https://receiver.example/hooks/..", + "https://receiver.example/./", + "https://receiver.example/%2e", + "https://receiver.example?ticket=1", + "https://receiver.example?", + "https://receiver.example#section", + "https://receiver.example#", + "https://receiver.example\\", + "https://user:password@receiver.example", + "ftp://receiver.example", + "http://receiver.example", + "http://localhost:4000", + "https://receiver.example,", + ])("rejects malformed, non-origin, or unsupported configuration: %s", (origin) => { + expect(() => loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin })).toThrow(/CALLBACK_ALLOWED_ORIGINS/); + }); + + it.each(["http://localhost:4000", "http://127.0.0.1:4000", "http://[::1]:4000"])( + "requires both local development and explicit approval for %s", (origin) => { + const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin, RENDER_LOCAL_DEV_URL: "http://localhost:8121" }); + expect(policy.isLocalDev).toBe(true); + expect(() => assertCallbackAllowed(`${origin}/hook`, policy)).not.toThrow(); + expect(() => assertCallbackAllowed(`${origin}/hook`, loadCallbackPolicy({ RENDER_USE_LOCAL_DEV: "true" }))).toThrow(/not approved/); + expect(() => loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin, RENDER: "true", RENDER_USE_LOCAL_DEV: "true" })).toThrow(); + }, + ); + + it.each(["http://remote.example:8121", "invalid", "https://localhost:8121"])( + "does not enable loopback HTTP from an unverified local-dev URL: %s", (localDevUrl) => { + expect(() => loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "http://localhost:4000", RENDER_LOCAL_DEV_URL: localDevUrl })).toThrow(); + }, + ); + + it("loads the same policy and validates bad settings at both service configuration boundaries", () => { + const env = { API_TOKEN: "test-token", QUESTION_ID_SECRET: "q".repeat(32), RENDER_API_KEY: "synthetic", CALLBACK_ALLOWED_ORIGINS: "https://receiver.example:8443" }; + expect(loadEnv(env).callbackPolicy).toEqual(loadConfig(env).callbackPolicy); + expect(loadEnv({ ...env, CALLBACK_ALLOWED_ORIGINS: " " }).callbackPolicy.allowedOrigins).toEqual([]); + expect(loadConfig({ CALLBACK_ALLOWED_ORIGINS: " " }).callbackPolicy.allowedOrigins).toEqual([]); + expect(() => loadEnv({ ...env, CALLBACK_ALLOWED_ORIGINS: "https://receiver.example/path" })).toThrow(); + expect(() => loadConfig({ CALLBACK_ALLOWED_ORIGINS: "https://receiver.example/path" })).toThrow(); + }); +}); diff --git a/test/tasks.test.ts b/test/tasks.test.ts index c15e84f..2069265 100644 --- a/test/tasks.test.ts +++ b/test/tasks.test.ts @@ -1,6 +1,7 @@ import { createServer } from "node:http"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { verifyCallback } from "../src/callback.ts"; +import { loadCallbackPolicy } from "../src/callback-policy.ts"; import type { Evidence, InvestigationResult } from "../src/investigation.ts"; import { createLog } from "../src/log.ts"; import type { AnswerQuestionInput } from "../src/question.ts"; @@ -269,6 +270,70 @@ describe("investigateTask", () => { }); describe("deliverCallback", () => { + it("fails closed when direct delivery has no configured origin", async () => { + const fetcher = vi.fn().mockResolvedValue(new Response(null, { status: 200 })); + vi.stubGlobal("fetch", fetcher); + vi.stubEnv("CALLBACK_ALLOWED_ORIGINS", ""); + try { + await expect(deliverCallback({ callback: CALLBACK, body: "{}" })).rejects.toThrow(/not approved/); + expect(fetcher).not.toHaveBeenCalled(); + } finally { + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); + } + }); + + it("sends HTTPS callbacks with redirect rejection and a bounded request", async () => { + const fetcher = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); + vi.stubGlobal("fetch", fetcher); + try { + await deliverCallback({ callback: CALLBACK, body: "{ \"trace_id\": \"raw-body\" }" }, loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "https://example.com" })); + expect(fetcher).toHaveBeenCalledWith("https://example.com/hook", expect.objectContaining({ + method: "POST", redirect: "error", body: "{ \"trace_id\": \"raw-body\" }", signal: expect.any(AbortSignal), + })); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("rejects an unapproved destination even when delivery is invoked directly", async () => { + const fetcher = vi.fn().mockResolvedValue(new Response(null, { status: 200 })); + vi.stubGlobal("fetch", fetcher); + try { + await expect(deliverCallback({ + callback: { url: "https://127.0.0.1/hook", secret: "a".repeat(32) }, + body: "{}", + }, loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "https://receiver.example" }))).rejects.toThrow(/not approved/); + expect(fetcher).not.toHaveBeenCalled(); + } finally { + vi.unstubAllGlobals(); + } + }); + + it.each([301, 302, 303, 307, 308])("rejects a %i redirect without forwarding the signed callback to another server", async (status) => { + let forwarded = 0; + const receiver = createServer((_req, res) => { forwarded++; res.end(); }); + await new Promise((resolve) => receiver.listen(0, "127.0.0.1", resolve)); + const target = `http://127.0.0.1:${(receiver.address() as { port: number }).port}`; + const redirector = createServer((_req, res) => { + res.writeHead(status, { location: `${target}/capture` }); + res.end(); + }); + await new Promise((resolve) => redirector.listen(0, "127.0.0.1", resolve)); + const origin = `http://127.0.0.1:${(redirector.address() as { port: number }).port}`; + try { + await expect(deliverCallback({ + callback: { url: `${origin}/hook`, secret: "a".repeat(32) }, + body: '{"trace_id":"redirect-test"}', + }, loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin, RENDER_USE_LOCAL_DEV: "true" }))).rejects.toThrow(); + expect(forwarded).toBe(0); + } finally { + await Promise.all([receiver, redirector].map((server) => new Promise((resolve, reject) => + server.close((err) => err ? reject(err) : resolve()), + ))); + } + }); + it("posts a verifiable signed body and rejects a non-2xx response so the deliver task retries", async () => { const secret = "test-secret-1234567890"; const received: { headers: Record; body: string }[] = []; @@ -285,8 +350,9 @@ describe("deliverCallback", () => { const base = `http://localhost:${(server.address() as { port: number }).port}`; const body = JSON.stringify({ trace_id: "t1", reply: { kind: "failure", traceId: "t1", reason: "test" } }); try { - await deliverCallback({ callback: { url: `${base}/hook`, secret }, body }); - await expect(deliverCallback({ callback: { url: `${base}/fail`, secret }, body })).rejects.toThrow("500"); + const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: base, RENDER_USE_LOCAL_DEV: "true" }); + await deliverCallback({ callback: { url: `${base}/hook`, secret }, body }, policy); + await expect(deliverCallback({ callback: { url: `${base}/fail`, secret }, body }, policy)).rejects.toThrow("500"); } finally { server.close(); } From e92790fe5c962f6d1db0388c8e3f23be0104c6cd Mon Sep 17 00:00:00 2001 From: Jacob Prall Date: Wed, 28 Oct 2026 13:03:09 -0700 Subject: [PATCH 2/3] Enforce callback origins at delivery only, with one workflow setting Delivery is the trust boundary: it runs on the workflow host, outside Sandbox egress rules. The workflow skips a callback whose origin isn't listed in CALLBACK_ALLOWED_ORIGINS (logged as deliver_skipped, so the parent doesn't wait on retries), deliverCallback re-checks the origin, and fetch refuses redirects. The API keeps its existing URL check, so deployers set one value on one service instead of matching values on two. Co-authored-by: Cursor --- .env.example | 9 +-- README.md | 2 +- docs/0_QUICKSTART.md | 28 ++----- docs/1_EXTENSION.md | 43 +++------- docs/2_ARCHITECTURE.md | 11 +-- docs/3_TAKEAWAYS.md | 2 - render.yaml | 8 +- src/api.ts | 24 +++--- src/callback-policy.ts | 66 ---------------- src/callback.ts | 22 ++++++ src/tasks.ts | 13 ++-- src/workflow.ts | 4 +- test/api.test.ts | 46 +++-------- test/callback-policy.test.ts | 73 ----------------- test/tasks.test.ts | 147 ++++++++++++++++++----------------- 15 files changed, 149 insertions(+), 349 deletions(-) delete mode 100644 src/callback-policy.ts delete mode 100644 test/callback-policy.test.ts diff --git a/.env.example b/.env.example index 370ece9..2d1643f 100644 --- a/.env.example +++ b/.env.example @@ -10,11 +10,6 @@ API_TOKEN= # Never give this signing secret to the workflow or CLI clients. Rotating it invalidates question IDs. QUESTION_ID_SECRET= # WORKFLOW_SLUG=codebase-researcher-workflow -# Optional: set the same comma-separated origins on the API and workflow. Blank disables callbacks. -# HTTPS origins only; every path on each approved origin is trusted. Redirects are rejected. -CALLBACK_ALLOWED_ORIGINS= -# Local callback example (requires local Workflows dev mode on both services): -# CALLBACK_ALLOWED_ORIGINS=http://localhost:4000 # PORT=3000 # Points the API at `render workflows dev` instead of Render; RENDER_API_KEY is then optional. # RENDER_LOCAL_DEV_URL=http://localhost:8121 @@ -43,14 +38,14 @@ SANDBOX_NETWORK_POLICY=allow-list # Deadline for opening a Render investigation Sandbox: restore, boot, verify.sh, and reading its manifest. # SANDBOX_PROVISION_TIMEOUT_MS=300000 # REPOS=render-oss/cli,render-oss/sdk +# Origins callbacks may be POSTed to; unset turns callbacks off. http://localhost: works locally. +# CALLBACK_ALLOWED_ORIGINS=https://hooks.example.com # ── Snapshot refresh cron ──────────────────────────────────────────────── GITHUB_TOKEN= # ── Local development ─────────────────────────────────────────────────── # LOCAL_REPOS_ROOT=.scratch/repos -# render workflows dev sets RENDER_USE_LOCAL_DEV=true for the workflow process. -# The API uses its loopback RENDER_LOCAL_DEV_URL to verify local mode. HTTP callbacks stay disabled on Render. # Appended to the auto-bundled runner's command; --faux runs without a model. # LOCAL_RUNNER_ARGS=--faux diff --git a/README.md b/README.md index 6e184b0..dab47f6 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ A reader can follow each link to the exact lines and decide for themselves. - The model API key arrives as a one-time file that the runner deletes on read. The runner's Node binary is execute-only and signal-triggered debugging is disabled, protecting its memory from the agent's commands. The snapshot build and every restored Sandbox must verify both protections, or the build fails and the Sandbox gets no work. - 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. -- Callbacks are disabled until the operator sets the same `CALLBACK_ALLOWED_ORIGINS` on the API and workflow. Both services enforce exact HTTPS origins, and delivery rejects redirects. Polling works without callback configuration. +- Callbacks are off until you list the allowed origins in the workflow's `CALLBACK_ALLOWED_ORIGINS`. Delivery never follows redirects. - 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). diff --git a/docs/0_QUICKSTART.md b/docs/0_QUICKSTART.md index ca44893..beba344 100644 --- a/docs/0_QUICKSTART.md +++ b/docs/0_QUICKSTART.md @@ -68,17 +68,6 @@ 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). -Callbacks are optional and disabled by default. To enable them, set the same -`CALLBACK_ALLOWED_ORIGINS` on both the API and workflow, for example -`https://support-hooks.example.com`. Use comma-separated HTTPS origins with -no path, query, fragment, or credentials. Every path on an approved origin is -trusted; choose a dedicated integration origin. Delivery rejects redirects. -Polling needs no callback configuration. - -Before upgrading an existing deployment that uses callbacks, configure these -origins on both services. Unapproved new requests and queued deliveries fail -closed after the upgrade; callers can retrieve replies by polling. - ### Build the first snapshot Open `codebase-researcher-snapshots` and click **Trigger Run**, or wait for its @@ -195,15 +184,7 @@ 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. Callbacks -remain disabled unless both processes receive the same explicit setting, for -example `CALLBACK_ALLOWED_ORIGINS=http://localhost:4000`. Loopback HTTP is -allowed only in local Workflows dev mode: the CLI sets `RENDER_USE_LOCAL_DEV` -for the workflow, and the API verifies its loopback `RENDER_LOCAL_DEV_URL`. -`localhost`, `127.0.0.1`, and `[::1]` are supported; ports must match exactly. -HTTP callbacks are never enabled when running on Render. - -Then ask: +Off Render, the workflow uses the local Sandbox provider by default. Then ask: ```bash # Terminal 3 @@ -243,6 +224,7 @@ All services emit one JSON object per line. The most useful events are: | `turn_finished` | Workflow | Final outcome, model, prompt version, commits, usage, cost, and duration | | `investigation_failed` | Workflow | The agent ran but did not submit an accepted result | | `workflow_misconfigured` | Workflow | Model or Sandbox configuration is invalid | +| `deliver_skipped` | Workflow | The callback's origin isn't in `CALLBACK_ALLOWED_ORIGINS` | | `deliver_failed` | Workflow | Callback retries were exhausted | | `snapshot_refresh` | Cron | Snapshot build result, repository count, and duration | @@ -275,8 +257,10 @@ events with `"ok": false`; a failed refresh leaves the previous snapshot availab - **Infrastructure retries exhausted:** inspect failed `investigate` task runs. Restore timeouts, Sandbox verification, or a rejected network policy are common causes. -- **Callback never arrives:** inspect `deliver_failed` and the `deliver` - subtask's attempts. The receiver must return 2xx within 10 seconds. +- **Callback never arrives:** a `deliver_skipped` event means the callback's + origin isn't in the workflow's `CALLBACK_ALLOWED_ORIGINS`. Otherwise, + inspect `deliver_failed` and the `deliver` subtask's attempts. The receiver + must return 2xx within 10 seconds and must not redirect. - **First request is slow:** the free API instance spun down after 15 idle minutes. Switch it to Starter in `render.yaml` to keep it warm. diff --git a/docs/1_EXTENSION.md b/docs/1_EXTENSION.md index 554a225..831b721 100644 --- a/docs/1_EXTENSION.md +++ b/docs/1_EXTENSION.md @@ -195,9 +195,7 @@ The request body accepts: the agent's turn, then the new question. The earlier run must have finished (`409` otherwise), and a conversation holds at most 200 messages. - `callback`: optional `{ "url": "https://...", "secret": "..." }`. Secrets - must be 16–256 characters. The URL must use an origin explicitly approved - by the operator in `CALLBACK_ALLOWED_ORIGINS`; callbacks are disabled when - that setting is missing or blank. + must be 16–256 characters. Local development also permits localhost HTTP. - `metadata`: optional routing data with at most 16 string, number, or boolean values. It is returned unchanged in the callback. @@ -271,33 +269,15 @@ questions. Legacy signatures are not accepted. ### Receive callbacks Polling suits scripts and short-lived callers. A long-lived integration, such -as a chat bot or support desk, can use a callback after the operator enables it. -Set the same `CALLBACK_ALLOWED_ORIGINS` on the API and workflow, for example -`https://support-hooks.example.com,https://events.example.com:8443`. The API -checks before starting work, and the delivery subtask checks independently. -Missing or blank configuration disables callbacks and leaves polling available. -Malformed configuration prevents the service from starting its work. - -Each entry must be an origin only: scheme, host, and optional port, with no -credentials, path, query, or fragment. Matching uses normalized `URL.origin`; -subdomains and different ports need their own entries. Approved origins are -operator-trusted, including every path they serve. Choose a dedicated -integration origin and review its outbound behavior. All HTTP redirects are -rejected, even to another approved origin, so receivers must accept the POST -directly. - -Production accepts HTTPS only. Local development additionally accepts explicit -`http://localhost:`, `http://127.0.0.1:`, and -`http://[::1]:` entries. Local mode must come from operator environment: -`render workflows dev` sets `RENDER_USE_LOCAL_DEV` for the workflow, or a -loopback HTTP `RENDER_LOCAL_DEV_URL` verifies local mode for the API. Running -on Render disables the HTTP exception. Set the same local origins on both -processes; setting a request URL alone never enables callbacks. - -When upgrading a deployment that already uses callbacks, configure approved -origins on both services before deploying. New unapproved requests return 400; -unapproved queued deliveries also fail closed. Polling remains available for -retrieving the reply. +as a chat bot or support desk, should pass a callback instead. + +Callbacks are off until you set `CALLBACK_ALLOWED_ORIGINS` on the workflow +service to a comma-separated list of origins, such as +`https://hooks.example.com`. Delivery runs on the workflow host, outside the +Sandbox's egress rules, so it only POSTs to an exact listed origin (any path) +and never follows redirects. A callback to any other origin is skipped and +logged as `deliver_skipped`; the reply is still available by polling. In local +development, `http://localhost:` origins are also accepted. For an approved callback, the `deliver` subtask sends: @@ -370,7 +350,6 @@ string is treated as unset. | `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 | -| `CALLBACK_ALLOWED_ORIGINS` | Disabled | Comma-separated approved origins; must match the workflow | | `RENDER_LOCAL_DEV_URL` | None | Uses a local Workflows server instead of Render | ### Workflow service @@ -379,7 +358,6 @@ string is treated as unset. |---|---|---| | `RENDER_API_KEY` | None | Sandbox operations | | `RENDER_WORKSPACE_ID` | Resolved | Required when the key sees multiple workspaces | -| `CALLBACK_ALLOWED_ORIGINS` | Disabled | Comma-separated approved origins, checked again at delivery; must match the API | | `SANDBOX_PROVIDER` | Render on Render, local elsewhere | Sandbox implementation | | `MODEL_PROVIDER` | `anthropic` | Pi provider | | `MODEL` | `claude-opus-5` | Model ID | @@ -390,6 +368,7 @@ string is treated as unset. | `SANDBOX_NETWORK_POLICY` | `allow-list` | `allow-list`, `allow-all`, or `deny-all` | | `SANDBOX_ALLOWED_DOMAINS` | Model host | Comma-separated HTTPS egress hosts; supports leftmost wildcards | | `SANDBOX_TIMEOUT_SECONDS` | `1800` | Provider-side termination backstop | +| `CALLBACK_ALLOWED_ORIGINS` | None (callbacks off) | Comma-separated origins callbacks may be delivered to | | `SNAPSHOT_NAME` | `config.ts` | Snapshot-name override | | `REPOS` | None | Repository-list override | diff --git a/docs/2_ARCHITECTURE.md b/docs/2_ARCHITECTURE.md index 3f4be7b..15f7f88 100644 --- a/docs/2_ARCHITECTURE.md +++ b/docs/2_ARCHITECTURE.md @@ -148,11 +148,8 @@ answered, so a chain of follow-ups only ever reads the latest run. `turn_finished` outcome. - `investigate`: retryable infrastructure boundary; opens and runs a fresh Sandbox. -- `deliver`: independently retryable signed callback POST. Both the API and - delivery validate the operator-configured `CALLBACK_ALLOWED_ORIGINS`; no - configured origins means callbacks are disabled. Delivery rejects every - redirect. These requests originate from the host, outside Sandbox egress - controls. +- `deliver`: independently retryable signed callback POST, only to an origin + listed in `CALLBACK_ALLOWED_ORIGINS`, with redirects refused. The parent does not retry. Infrastructure failures thrown by `investigate` retry on a fresh Sandbox; model and submission failures become user-safe @@ -321,9 +318,7 @@ adversarially. uploaded. These runtime changes require a successful snapshot refresh; existing snapshots retain their original runner and launch scripts. - The **host workflow** treats Sandbox output as untrusted, repeats manifest - checks, constructs links, classifies failures, and owns cleanup. Its callback - sender independently enforces approved origins and refuses redirects. Every - path on an approved origin is operator-trusted. + checks, constructs links, classifies failures, and owns cleanup. `verify.sh` checks the actual builder and every restored Sandbox instead of assuming the build steps worked. Known residual risks remain: DNS resolves diff --git a/docs/3_TAKEAWAYS.md b/docs/3_TAKEAWAYS.md index ffb0083..b831dbe 100644 --- a/docs/3_TAKEAWAYS.md +++ b/docs/3_TAKEAWAYS.md @@ -246,8 +246,6 @@ the HTTP API. For any webhook-driven integration: - use the platform event ID as the idempotency key; - store the latest question ID per thread and send it as `follow_up_to`; - derive a per-event callback secret and verify callbacks against the raw body; -- configure the same exact approved callback origins on the API and workflow; - trust all paths on each approved origin and reject redirects at delivery; - make callback handling idempotent; - escape model text and suppress mentions. diff --git a/render.yaml b/render.yaml index aa99157..a6aece0 100644 --- a/render.yaml +++ b/render.yaml @@ -20,7 +20,7 @@ projects: startCommand: pnpm start:api healthCheckPath: /healthz buildFilter: - paths: [src/api.ts, src/question.ts, src/investigation.ts, src/callback.ts, src/callback-policy.ts, src/log.ts, src/util.ts, package.json, pnpm-lock.yaml, pnpm-workspace.yaml, .node-version, tsconfig.json] + paths: [src/api.ts, src/question.ts, src/investigation.ts, src/callback.ts, src/log.ts, src/util.ts, package.json, pnpm-lock.yaml, pnpm-workspace.yaml, .node-version, tsconfig.json] envVars: - key: RENDER_API_KEY sync: false @@ -33,9 +33,6 @@ projects: type: workflow name: codebase-researcher-workflow property: slug - # Optional: set identical HTTPS origins on the API and workflow. Blank disables callbacks. - - key: CALLBACK_ALLOWED_ORIGINS - sync: false - type: workflow name: codebase-researcher-workflow @@ -60,9 +57,6 @@ projects: sync: false - key: SANDBOX_NETWORK_POLICY value: allow-list - # Must match the API setting. The delivery subtask checks it independently. - - key: CALLBACK_ALLOWED_ORIGINS - sync: false - type: cron name: codebase-researcher-snapshots diff --git a/src/api.ts b/src/api.ts index 7f0256d..dd361f9 100644 --- a/src/api.ts +++ b/src/api.ts @@ -2,7 +2,6 @@ import { MESSAGE_MAX_CHARS, Transcript, TRANSCRIPT_MAX_MESSAGES } from "./invest import { QuestionMetadata, replyText, type Reply, type StartQuestion } from "./question.ts"; import { present } from "./util.ts"; import { safeEqual } from "./callback.ts"; -import { assertCallbackAllowed, loadCallbackPolicy, type CallbackPolicy } from "./callback-policy.ts"; import { createLog, type Log } from "./log.ts"; import { serve } from "@hono/node-server"; import { Hono } from "hono"; @@ -24,7 +23,6 @@ export type AppDeps = { startQuestion: StartQuestion; getTaskRun: GetTaskRun; isLocalDev: boolean; - callbackAllowedOrigins?: readonly string[]; log: Log; }; @@ -44,19 +42,16 @@ export function fromQuestionId(questionIdSecret: string, questionId: string): st return safeEqual(questionId.slice(dot + 1), questionIdMac(questionIdSecret, runId)) ? runId : null; } -const postBody = (policy: CallbackPolicy) => +const postBody = (isLocalDev: boolean) => z.object({ question: z.string().trim().min(1).max(MESSAGE_MAX_CHARS), follow_up_to: z.string().min(1).optional(), callback: z.object({ url: z.url().refine((raw) => { - try { - assertCallbackAllowed(raw, policy); - return true; - } catch { - return false; - } - }, "callback URL must use an origin approved by CALLBACK_ALLOWED_ORIGINS (HTTPS, or explicit loopback HTTP in local dev; no credentials)"), + const url = new URL(raw); + if (url.protocol === "https:") return true; + return isLocalDev && url.protocol === "http:" && (url.hostname === "localhost" || url.hostname === "127.0.0.1"); + }, "must be https:// (http://localhost allowed in local dev)"), secret: z.string().min(16).max(256), }).optional(), metadata: QuestionMetadata.optional(), @@ -72,7 +67,7 @@ function completedReply(run: TaskRun): { trace_id?: string; reply?: Reply } | un export function createApp(deps: AppDeps) { const app = new Hono(); - const PostBody = postBody({ allowedOrigins: deps.callbackAllowedOrigins ?? [], isLocalDev: deps.isLocalDev }); + const PostBody = postBody(deps.isLocalDev); async function readRun(questionId: string): Promise { const runId = fromQuestionId(deps.questionIdSecret, questionId); @@ -177,10 +172,10 @@ const Env = z taskSlug: `${e.WORKFLOW_SLUG}/answer_question`, })); -export type ApiConfig = z.output & { callbackPolicy: CallbackPolicy }; +export type ApiConfig = z.output; export function loadEnv(env: NodeJS.ProcessEnv = process.env): ApiConfig { - return { ...Env.parse(present(env)), callbackPolicy: loadCallbackPolicy(env) }; + return Env.parse(present(env)); } // Starts the server only when run directly, not when tests import this module. @@ -209,8 +204,7 @@ if (process.argv[1] && import.meta.url === pathToFileURL(realpathSync(process.ar questionIdSecret: env.questionIdSecret, startQuestion, getTaskRun, - isLocalDev: env.callbackPolicy.isLocalDev, - callbackAllowedOrigins: env.callbackPolicy.allowedOrigins, + isLocalDev: !!env.localDevUrl, log, }); diff --git a/src/callback-policy.ts b/src/callback-policy.ts deleted file mode 100644 index c86d265..0000000 --- a/src/callback-policy.ts +++ /dev/null @@ -1,66 +0,0 @@ -import { z } from "zod"; - -export type CallbackPolicy = { - allowedOrigins: readonly string[]; - isLocalDev: boolean; -}; - -const LOOPBACK = new Set(["localhost", "127.0.0.1", "[::1]"]); -const LOCAL_DEV_VALUES = new Set(["1", "t", "T", "true", "TRUE", "True"]); - -function isLocalDev(env: NodeJS.ProcessEnv): boolean { - if (env.RENDER === "true") return false; - if (LOCAL_DEV_VALUES.has(env.RENDER_USE_LOCAL_DEV ?? "")) return true; - if (!env.RENDER_LOCAL_DEV_URL) return false; - try { - const url = new URL(env.RENDER_LOCAL_DEV_URL); - return url.protocol === "http:" && LOOPBACK.has(url.hostname) && !url.username && !url.password; - } catch { - return false; - } -} - -function destination(raw: string, localDev: boolean): URL { - const url = new URL(raw); - if (url.username || url.password) throw new Error("callback URLs must not contain credentials"); - if (url.protocol !== "https:" && !(localDev && url.protocol === "http:" && LOOPBACK.has(url.hostname))) { - throw new Error("callbacks require HTTPS; explicit loopback HTTP origins are allowed only in local development"); - } - return url; -} - -const PolicyEnv = z.object({ CALLBACK_ALLOWED_ORIGINS: z.string().optional() }).transform((env, ctx) => { - if (!env.CALLBACK_ALLOWED_ORIGINS) return []; - const entries = env.CALLBACK_ALLOWED_ORIGINS.split(",").map((origin) => origin.trim()); - if (entries.some((origin) => !origin)) { - ctx.addIssue({ code: "custom", path: ["CALLBACK_ALLOWED_ORIGINS"], message: "must be a comma-separated list of origins without empty entries" }); - return z.NEVER; - } - return entries; -}); - -/** Missing or blank configuration disables callbacks. The environment is operator-controlled. */ -export function loadCallbackPolicy(env: NodeJS.ProcessEnv = process.env): CallbackPolicy { - const localDev = isLocalDev(env); - const entries = PolicyEnv.parse({ CALLBACK_ALLOWED_ORIGINS: env.CALLBACK_ALLOWED_ORIGINS?.trim() || undefined }); - const origins = entries.map((raw) => { - try { - const url = destination(raw, localDev); - if (!/^https?:\/\/[^\s/?#\\]+\/?$/i.test(raw) || url.pathname !== "/") { - throw new Error("entries must be origins only, without paths, query strings, or fragments"); - } - return url.origin; - } catch { - throw new Error("CALLBACK_ALLOWED_ORIGINS must contain valid HTTPS origins only (loopback HTTP requires local development)"); - } - }); - return { allowedOrigins: [...new Set(origins)], isLocalDev: localDev }; -} - -/** All paths on an approved origin are trusted; redirects must still be rejected by the sender. */ -export function assertCallbackAllowed(raw: string, policy: CallbackPolicy): void { - const url = destination(raw, policy.isLocalDev); - if (!policy.allowedOrigins.includes(url.origin)) { - throw new Error("callback origin is not approved by CALLBACK_ALLOWED_ORIGINS; omit callback to use polling"); - } -} diff --git a/src/callback.ts b/src/callback.ts index c0dd97d..7ec4ee7 100644 --- a/src/callback.ts +++ b/src/callback.ts @@ -20,6 +20,28 @@ export function signCallback( return { signature: `v1=${sig}`, timestamp: ts }; } +const LOOPBACK = new Set(["localhost", "127.0.0.1", "[::1]"]); + +/** + * CALLBACK_ALLOWED_ORIGINS as normalized origins; unset or blank disables callbacks. Delivery runs on the + * workflow host, outside Sandbox egress rules, so only operator-approved origins are reachable. + */ +export function callbackOrigins(env: NodeJS.ProcessEnv = process.env): string[] { + const raw = env.CALLBACK_ALLOWED_ORIGINS?.trim(); + if (!raw) return []; + return raw.split(",").map((entry) => { + const url = URL.parse(entry.trim()); + const localHttp = url?.protocol === "http:" && LOOPBACK.has(url.hostname) && env.RENDER !== "true"; + if (!url || (url.protocol !== "https:" && !localHttp) || url.username || url.password || url.pathname !== "/" || url.search || url.hash) { + throw new Error(`CALLBACK_ALLOWED_ORIGINS: ${JSON.stringify(entry.trim())} must be an https:// origin with no path`); + } + return url.origin; + }); +} + +export const isApprovedCallback = (url: string, origins: readonly string[]): boolean => + origins.includes(URL.parse(url)?.origin ?? ""); + const CALLBACK_MAX_SKEW_SECONDS = 5 * 60; export function verifyCallback(opts: { diff --git a/src/tasks.ts b/src/tasks.ts index 1bfa159..74b2ffc 100644 --- a/src/tasks.ts +++ b/src/tasks.ts @@ -12,8 +12,7 @@ import type { RepoManifest } from "./manifest.ts"; import { RUNNER_EXIT, RunnerInput, type RunnerEvent, type Usage } from "./runner/protocol.ts"; import { SnapshotUnavailableError, type OpenSandbox, type RunInvestigationResult, type Sandbox } from "./sandbox/index.ts"; import { getBuiltinModel } from "@earendil-works/pi-ai/providers/all"; -import { signCallback } from "./callback.ts"; -import { assertCallbackAllowed, loadCallbackPolicy, type CallbackPolicy } from "./callback-policy.ts"; +import { callbackOrigins, isApprovedCallback, signCallback } from "./callback.ts"; import type { Log } from "./log.ts"; import type { Config } from "./workflow.ts"; @@ -211,7 +210,9 @@ export async function answerQuestion( reply = { kind: "failure", traceId, reason: failureReason(investigateResult.code, config.agentTimeoutMs) }; } - if (input.callback) { + if (input.callback && !isApprovedCallback(input.callback.url, config.callbackOrigins)) { + log.warn("callback origin not approved", { event: "deliver_skipped", trace_id: traceId, origin: URL.parse(input.callback.url)?.origin }); + } else if (input.callback) { const body = JSON.stringify({ trace_id: traceId, reply, @@ -241,9 +242,9 @@ export async function answerQuestion( return { trace_id: traceId, outcome, reply }; } -/** POST the callback body with a signed request. Throws on non-2xx or network error. */ -export async function deliverCallback(input: DeliverInput, policy: CallbackPolicy = loadCallbackPolicy()): Promise { - assertCallbackAllowed(input.callback.url, policy); +/** POST the callback body with a signed request. Throws on an unapproved origin, a redirect, non-2xx, or network error. */ +export async function deliverCallback(input: DeliverInput, origins: readonly string[] = callbackOrigins()): Promise { + if (!isApprovedCallback(input.callback.url, origins)) throw new Error("callback origin is not in CALLBACK_ALLOWED_ORIGINS"); const { signature, timestamp } = signCallback(input.callback.secret, input.body); const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), 10_000); diff --git a/src/workflow.ts b/src/workflow.ts index bf71c00..f41324f 100644 --- a/src/workflow.ts +++ b/src/workflow.ts @@ -7,7 +7,7 @@ import { task, type TaskContext } from "@renderinc/sdk/workflows"; import { z } from "zod"; import { answerQuestion, deliverCallback, investigateTask, type InvestigateInput, type InvestigateResult, type DeliverInput } from "./tasks.ts"; import { createLog } from "./log.ts"; -import { loadCallbackPolicy } from "./callback-policy.ts"; +import { callbackOrigins } from "./callback.ts"; const MINUTE = 60_000; @@ -79,7 +79,7 @@ const Env = z modelApiKey: providerApiKey(e.MODEL_PROVIDER, env), agentTimeoutMs: e.AGENT_TIMEOUT_MS, sandboxProvisionTimeoutMs: e.SANDBOX_PROVISION_TIMEOUT_MS, - callbackPolicy: loadCallbackPolicy(env), + callbackOrigins: callbackOrigins(env), }; }); export type Config = z.output; diff --git a/test/api.test.ts b/test/api.test.ts index 376c212..9fa00a0 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -127,44 +127,16 @@ describe("HTTP API", () => { expect(long.started).toEqual([]); }); - it("disables callbacks until an operator approves an origin, while polling-only questions still start", async () => { - const { post, started } = harness(); - const rejected = await post({ question, callback: { url: "https://unapproved.example/hook", secret: "a".repeat(32) } }); - expect(rejected.status).toBe(400); - expect(started).toEqual([]); - expect((await post({ question })).status).toBe(202); - expect(started).toHaveLength(1); - }); - - it("accepts only approved callback origins, including the exact port, without URL credentials", async () => { - const secret = "a".repeat(32); - const { post, started } = harness({ callbackAllowedOrigins: ["https://receiver.example:8443"] }); - for (const url of [ - "https://receiver.example/hook", - "https://sub.receiver.example:8443/hook", - "https://receiver.example.attacker.example:8443/hook", - "https://127.0.0.1/hook", - "https://10.0.0.1/hook", - "https://user:password@receiver.example:8443/hook", - "http://receiver.example:8443/hook", - ]) { - expect((await post({ question, callback: { url, secret } })).status, url).toBe(400); - } - expect(started).toEqual([]); - expect((await post({ question, callback: { url: "https://RECEIVER.EXAMPLE:8443/hook?ticket=1", secret } })).status).toBe(202); - expect(started[0]!.input).toMatchObject({ callback: { url: "https://RECEIVER.EXAMPLE:8443/hook?ticket=1", secret } }); - }); - - it("allows explicit loopback HTTP origins only in local development", async () => { + it("requires an https callback URL, allowing http://localhost only in local dev", async () => { const secret = "a".repeat(32); - const url = "http://localhost:4000/hook"; - const origins = ["http://localhost:4000", "http://[::1]:4000"]; - expect((await harness({ callbackAllowedOrigins: origins }).post({ question, callback: { url, secret } })).status).toBe(400); - const local = harness({ isLocalDev: true, callbackAllowedOrigins: origins }); - expect((await local.post({ question, callback: { url, secret } })).status).toBe(202); - expect((await local.post({ question, callback: { url: "http://[::1]:4000/hook", secret } })).status).toBe(202); - expect((await local.post({ question, callback: { url: "http://localhost:4001/hook", secret } })).status).toBe(400); - expect((await harness({ isLocalDev: true }).post({ question, callback: { url, secret } })).status).toBe(400); + const prod = harness(); + expect((await prod.post({ question, callback: { url: "http://example.com/hook", secret } })).status).toBe(400); + expect((await prod.post({ question, callback: { url: "http://localhost:4000/hook", secret } })).status).toBe(400); + expect((await prod.post({ question, callback: { url: "https://example.com/hook", secret } })).status).toBe(202); + + const local = harness({ isLocalDev: true }); + expect((await local.post({ question, callback: { url: "http://localhost:4000/hook", secret } })).status).toBe(202); + expect((await local.post({ question, callback: { url: "http://example.com/hook", secret } })).status).toBe(400); }); it("maps the run status to running, completed (with the reply), or failed", async () => { diff --git a/test/callback-policy.test.ts b/test/callback-policy.test.ts deleted file mode 100644 index 598f4b0..0000000 --- a/test/callback-policy.test.ts +++ /dev/null @@ -1,73 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { loadEnv } from "../src/api.ts"; -import { assertCallbackAllowed, loadCallbackPolicy } from "../src/callback-policy.ts"; -import { loadConfig } from "../src/workflow.ts"; - -describe("callback destination policy", () => { - it.each([undefined, "", " "])("disables callbacks when origins are missing or blank (%s)", (origins) => { - const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origins }); - expect(policy.allowedOrigins).toEqual([]); - expect(() => assertCallbackAllowed("https://receiver.example/hook", policy)).toThrow(/not approved/); - }); - - it("normalizes and deduplicates origins while approving all paths on the exact origin", () => { - const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "https://RECEIVER.example:443/, https://receiver.example" }); - expect(policy.allowedOrigins).toEqual(["https://receiver.example"]); - expect(() => assertCallbackAllowed("https://receiver.example/hooks/1?ticket=2", policy)).not.toThrow(); - for (const url of [ - "https://receiver.example:8443/hook", - "https://sub.receiver.example/hook", - "https://receiver.example.attacker.example/hook", - "https://user:password@receiver.example/hook", - "http://receiver.example/hook", - "https://10.0.0.1/hook", - "https://[::1]/hook", - ]) expect(() => assertCallbackAllowed(url, policy), url).toThrow(); - }); - - it.each([ - "not a URL", - "https:receiver.example", - "https://receiver.example/hook", - "https://receiver.example/hooks/..", - "https://receiver.example/./", - "https://receiver.example/%2e", - "https://receiver.example?ticket=1", - "https://receiver.example?", - "https://receiver.example#section", - "https://receiver.example#", - "https://receiver.example\\", - "https://user:password@receiver.example", - "ftp://receiver.example", - "http://receiver.example", - "http://localhost:4000", - "https://receiver.example,", - ])("rejects malformed, non-origin, or unsupported configuration: %s", (origin) => { - expect(() => loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin })).toThrow(/CALLBACK_ALLOWED_ORIGINS/); - }); - - it.each(["http://localhost:4000", "http://127.0.0.1:4000", "http://[::1]:4000"])( - "requires both local development and explicit approval for %s", (origin) => { - const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin, RENDER_LOCAL_DEV_URL: "http://localhost:8121" }); - expect(policy.isLocalDev).toBe(true); - expect(() => assertCallbackAllowed(`${origin}/hook`, policy)).not.toThrow(); - expect(() => assertCallbackAllowed(`${origin}/hook`, loadCallbackPolicy({ RENDER_USE_LOCAL_DEV: "true" }))).toThrow(/not approved/); - expect(() => loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin, RENDER: "true", RENDER_USE_LOCAL_DEV: "true" })).toThrow(); - }, - ); - - it.each(["http://remote.example:8121", "invalid", "https://localhost:8121"])( - "does not enable loopback HTTP from an unverified local-dev URL: %s", (localDevUrl) => { - expect(() => loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "http://localhost:4000", RENDER_LOCAL_DEV_URL: localDevUrl })).toThrow(); - }, - ); - - it("loads the same policy and validates bad settings at both service configuration boundaries", () => { - const env = { API_TOKEN: "test-token", QUESTION_ID_SECRET: "q".repeat(32), RENDER_API_KEY: "synthetic", CALLBACK_ALLOWED_ORIGINS: "https://receiver.example:8443" }; - expect(loadEnv(env).callbackPolicy).toEqual(loadConfig(env).callbackPolicy); - expect(loadEnv({ ...env, CALLBACK_ALLOWED_ORIGINS: " " }).callbackPolicy.allowedOrigins).toEqual([]); - expect(loadConfig({ CALLBACK_ALLOWED_ORIGINS: " " }).callbackPolicy.allowedOrigins).toEqual([]); - expect(() => loadEnv({ ...env, CALLBACK_ALLOWED_ORIGINS: "https://receiver.example/path" })).toThrow(); - expect(() => loadConfig({ CALLBACK_ALLOWED_ORIGINS: "https://receiver.example/path" })).toThrow(); - }); -}); diff --git a/test/tasks.test.ts b/test/tasks.test.ts index 2069265..dd1e50e 100644 --- a/test/tasks.test.ts +++ b/test/tasks.test.ts @@ -1,7 +1,6 @@ import { createServer } from "node:http"; -import { describe, expect, it, vi } from "vitest"; -import { verifyCallback } from "../src/callback.ts"; -import { loadCallbackPolicy } from "../src/callback-policy.ts"; +import { describe, expect, it } from "vitest"; +import { callbackOrigins, isApprovedCallback, verifyCallback } from "../src/callback.ts"; import type { Evidence, InvestigationResult } from "../src/investigation.ts"; import { createLog } from "../src/log.ts"; import type { AnswerQuestionInput } from "../src/question.ts"; @@ -61,7 +60,7 @@ const CALLBACK = { url: "https://example.com/hook", secret: "a".repeat(32) }; function harness(env: Record = {}) { const sandboxes = new FakeSandboxes(); const logs: Record[] = []; - const config = loadConfig(env); + const config = loadConfig({ CALLBACK_ALLOWED_ORIGINS: "https://example.com", ...env }); const log = createLog({}, (line: string) => void logs.push(JSON.parse(line))); const deliveries: DeliverInput[] = []; const h = { @@ -105,6 +104,17 @@ describe("answerQuestion", () => { expect(h.sandboxes.closed).toEqual(["sbx-1"]); }); + it("skips delivery to an unapproved callback origin but still returns the reply for polling", async () => { + const h = harness({ CALLBACK_ALLOWED_ORIGINS: "https://receiver.example" }); + h.sandboxes.script(success()); + + const result = await h.ask({ ...QUESTION, callback: CALLBACK }); + + expect(result.outcome).toBe("answer"); + expect(h.deliveries).toEqual([]); + expect(h.logs).toContainEqual(expect.objectContaining({ event: "deliver_skipped", origin: "https://example.com" })); + }); + it("returns a clarification when the agent needs one", async () => { const h = harness(); h.sandboxes.script(success(clarification)); @@ -270,70 +280,6 @@ describe("investigateTask", () => { }); describe("deliverCallback", () => { - it("fails closed when direct delivery has no configured origin", async () => { - const fetcher = vi.fn().mockResolvedValue(new Response(null, { status: 200 })); - vi.stubGlobal("fetch", fetcher); - vi.stubEnv("CALLBACK_ALLOWED_ORIGINS", ""); - try { - await expect(deliverCallback({ callback: CALLBACK, body: "{}" })).rejects.toThrow(/not approved/); - expect(fetcher).not.toHaveBeenCalled(); - } finally { - vi.unstubAllGlobals(); - vi.unstubAllEnvs(); - } - }); - - it("sends HTTPS callbacks with redirect rejection and a bounded request", async () => { - const fetcher = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); - vi.stubGlobal("fetch", fetcher); - try { - await deliverCallback({ callback: CALLBACK, body: "{ \"trace_id\": \"raw-body\" }" }, loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "https://example.com" })); - expect(fetcher).toHaveBeenCalledWith("https://example.com/hook", expect.objectContaining({ - method: "POST", redirect: "error", body: "{ \"trace_id\": \"raw-body\" }", signal: expect.any(AbortSignal), - })); - } finally { - vi.unstubAllGlobals(); - } - }); - - it("rejects an unapproved destination even when delivery is invoked directly", async () => { - const fetcher = vi.fn().mockResolvedValue(new Response(null, { status: 200 })); - vi.stubGlobal("fetch", fetcher); - try { - await expect(deliverCallback({ - callback: { url: "https://127.0.0.1/hook", secret: "a".repeat(32) }, - body: "{}", - }, loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: "https://receiver.example" }))).rejects.toThrow(/not approved/); - expect(fetcher).not.toHaveBeenCalled(); - } finally { - vi.unstubAllGlobals(); - } - }); - - it.each([301, 302, 303, 307, 308])("rejects a %i redirect without forwarding the signed callback to another server", async (status) => { - let forwarded = 0; - const receiver = createServer((_req, res) => { forwarded++; res.end(); }); - await new Promise((resolve) => receiver.listen(0, "127.0.0.1", resolve)); - const target = `http://127.0.0.1:${(receiver.address() as { port: number }).port}`; - const redirector = createServer((_req, res) => { - res.writeHead(status, { location: `${target}/capture` }); - res.end(); - }); - await new Promise((resolve) => redirector.listen(0, "127.0.0.1", resolve)); - const origin = `http://127.0.0.1:${(redirector.address() as { port: number }).port}`; - try { - await expect(deliverCallback({ - callback: { url: `${origin}/hook`, secret: "a".repeat(32) }, - body: '{"trace_id":"redirect-test"}', - }, loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: origin, RENDER_USE_LOCAL_DEV: "true" }))).rejects.toThrow(); - expect(forwarded).toBe(0); - } finally { - await Promise.all([receiver, redirector].map((server) => new Promise((resolve, reject) => - server.close((err) => err ? reject(err) : resolve()), - ))); - } - }); - it("posts a verifiable signed body and rejects a non-2xx response so the deliver task retries", async () => { const secret = "test-secret-1234567890"; const received: { headers: Record; body: string }[] = []; @@ -350,9 +296,8 @@ describe("deliverCallback", () => { const base = `http://localhost:${(server.address() as { port: number }).port}`; const body = JSON.stringify({ trace_id: "t1", reply: { kind: "failure", traceId: "t1", reason: "test" } }); try { - const policy = loadCallbackPolicy({ CALLBACK_ALLOWED_ORIGINS: base, RENDER_USE_LOCAL_DEV: "true" }); - await deliverCallback({ callback: { url: `${base}/hook`, secret }, body }, policy); - await expect(deliverCallback({ callback: { url: `${base}/fail`, secret }, body }, policy)).rejects.toThrow("500"); + await deliverCallback({ callback: { url: `${base}/hook`, secret }, body }, [base]); + await expect(deliverCallback({ callback: { url: `${base}/fail`, secret }, body }, [base])).rejects.toThrow("500"); } finally { server.close(); } @@ -367,6 +312,66 @@ describe("deliverCallback", () => { timestamp: headers["x-researcher-timestamp"] as string, })).toBe(true); }); + + it("refuses an unapproved origin without sending anything", async () => { + let received = 0; + const server = createServer((_req, res) => { received++; res.end(); }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const url = `http://127.0.0.1:${(server.address() as { port: number }).port}/hook`; + try { + await expect(deliverCallback({ callback: { url, secret: "a".repeat(32) }, body: "{}" }, ["https://receiver.example"])) + .rejects.toThrow("CALLBACK_ALLOWED_ORIGINS"); + expect(received).toBe(0); + } finally { + server.close(); + } + }); + + it("rejects a redirect without forwarding the signed body", async () => { + let forwarded = 0; + const target = createServer((_req, res) => { forwarded++; res.end(); }); + await new Promise((resolve) => target.listen(0, "127.0.0.1", resolve)); + const redirector = createServer((_req, res) => { + res.writeHead(307, { location: `http://127.0.0.1:${(target.address() as { port: number }).port}/capture` }); + res.end(); + }); + await new Promise((resolve) => redirector.listen(0, "127.0.0.1", resolve)); + const origin = `http://127.0.0.1:${(redirector.address() as { port: number }).port}`; + try { + await expect(deliverCallback({ callback: { url: `${origin}/hook`, secret: "a".repeat(32) }, body: "{}" }, [origin])).rejects.toThrow(); + expect(forwarded).toBe(0); + } finally { + target.close(); + redirector.close(); + } + }); +}); + +describe("callbackOrigins", () => { + it("is empty when unset or blank, which disables callbacks", () => { + expect(callbackOrigins({})).toEqual([]); + expect(callbackOrigins({ CALLBACK_ALLOWED_ORIGINS: " " })).toEqual([]); + }); + + it("normalizes origins and matches every path on an exact origin only", () => { + const origins = callbackOrigins({ CALLBACK_ALLOWED_ORIGINS: "https://Hooks.example:443/, https://events.example:8443" }); + expect(origins).toEqual(["https://hooks.example", "https://events.example:8443"]); + expect(isApprovedCallback("https://hooks.example/a/b?c=1", origins)).toBe(true); + for (const url of ["https://sub.hooks.example/", "https://hooks.example:8443/", "http://hooks.example/", "https://10.0.0.1/", "not a url"]) { + expect(isApprovedCallback(url, origins), url).toBe(false); + } + }); + + it.each(["https://hooks.example/path", "https://hooks.example?q=1", "https://user:pw@hooks.example", "http://hooks.example", "ftp://hooks.example", "nope"])( + "rejects %s", (origin) => { + expect(() => callbackOrigins({ CALLBACK_ALLOWED_ORIGINS: origin })).toThrow("CALLBACK_ALLOWED_ORIGINS"); + }, + ); + + it("allows loopback HTTP only off Render", () => { + expect(callbackOrigins({ CALLBACK_ALLOWED_ORIGINS: "http://localhost:4000" })).toEqual(["http://localhost:4000"]); + expect(() => callbackOrigins({ CALLBACK_ALLOWED_ORIGINS: "http://localhost:4000", RENDER: "true" })).toThrow(); + }); }); describe("task timeouts", () => { From 52f05c185d78928fc8a0dfbe7ba94a1a5924ea20 Mon Sep 17 00:00:00 2001 From: Jacob Prall Date: Wed, 28 Oct 2026 13:03:32 -0700 Subject: [PATCH 3/3] Enforce callback origins at delivery only, with one workflow setting Delivery is the trust boundary: it runs on the workflow host, outside Sandbox egress rules. The workflow skips a callback whose origin isn't listed in CALLBACK_ALLOWED_ORIGINS (logged as deliver_skipped, so the parent doesn't wait on retries), deliverCallback re-checks the origin, and fetch refuses redirects. The API keeps its existing URL check, so deployers set one value on one service instead of matching values on two. Co-authored-by: Cursor --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index dab47f6..6c306c8 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. - Callbacks are off until you list the allowed origins in the workflow's `CALLBACK_ALLOWED_ORIGINS`. Delivery never follows redirects. +- Callbacks are off until you list the allowed origins in the workflow's `CALLBACK_ALLOWED_ORIGINS`. Delivery never follows redirects. - 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).