From b7792ad33502311d2802763b6584320f647b8dcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iwo=20Strzebo=C5=84ski?= Date: Sun, 18 Oct 2026 19:52:22 +0000 Subject: [PATCH] Refuse to boot without R2 credentials `?? ""` on the access key and secret meant a deployment with no object storage configured started happily, and `POST /jobs` handed clients presigned URLs signed with an empty key. The first sign of trouble was a 403 from R2 - either in the client's uploader or, worse, inside a gateway that had downloaded R2's XML error document in place of a print. Nothing in that chain said "R2 is not configured". Reads both credentials through a `required()` that throws, naming the variable, mirroring auth/verifier.ts and auth/edge_secret.ts. The endpoint is read with `||` rather than `??`, because the committed template ships `R2_ENDPOINT=` empty and `??` only catches null - so an empty value was taken as a real endpoint and the account-id derivation never ran. That is the trap auth/edge_secret.ts documents, and it caught this line too, in the first draft of it. A test covers the derivation. Each boot-failure test fills every other variable so the one under test is the only thing missing; otherwise whichever variable is read first wins and the assertion passes for the wrong reason. e2e-edge.sh gains dummy values, since it boots the backend. It presigns nothing, so they only have to exist. --- .env.example | 9 +++++- scripts/e2e-edge.sh | 12 +++++++ src/s3/index.ts | 57 ++++++++++++++++++++++++++++---- tests/authz.test.ts | 79 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 7 deletions(-) diff --git a/.env.example b/.env.example index 030b521..b07f628 100644 --- a/.env.example +++ b/.env.example @@ -11,7 +11,14 @@ DATABASE_POOL_MAX=10 # understands a `valkey://` scheme. VALKEY_URL=redis://localhost:6379 -# Cloudflare R2 (S3-compatible) +# Cloudflare R2 (S3-compatible). The two credentials are required and the +# process will not start without them: an empty key signs presigned URLs that +# R2 answers with a 403, which surfaced as a gateway downloading an XML error +# document in place of a print rather than as "R2 is not configured". +# +# Either R2_ENDPOINT or R2_ACCOUNT_ID is needed too - the endpoint is derived +# from the account id the way Cloudflare's own URLs are formed. R2_BUCKET is +# not a credential and defaults to "continuum". R2_ACCOUNT_ID= R2_BUCKET=continuum R2_ENDPOINT= diff --git a/scripts/e2e-edge.sh b/scripts/e2e-edge.sh index 4cebebc..265185c 100755 --- a/scripts/e2e-edge.sh +++ b/scripts/e2e-edge.sh @@ -39,7 +39,14 @@ BASE="http://localhost:$PORT" DATABASE_URL="${DATABASE_URL:-postgres://continuum:continuum@localhost:5432/continuum}" VALKEY_URL="${VALKEY_URL:-redis://localhost:6379}" EDGE_JWT_SECRET="${EDGE_JWT_SECRET:-e2e-edge-secret}" +# The backend refuses to boot without object-storage credentials, which is +# deliberate - see src/s3/index.ts. Nothing in this test presigns or uploads +# anything, so these only have to exist. +R2_ACCESS_KEY_ID="${R2_ACCESS_KEY_ID:-e2e}" +R2_SECRET_ACCESS_KEY="${R2_SECRET_ACCESS_KEY:-e2e}" +R2_ENDPOINT="${R2_ENDPOINT:-http://localhost:9000}" export DATABASE_URL VALKEY_URL EDGE_JWT_SECRET PORT +export R2_ACCESS_KEY_ID R2_SECRET_ACCESS_KEY R2_ENDPOINT WORK="$(mktemp -d)" BACKEND_PID="" @@ -196,6 +203,11 @@ env -C "$PROXY_DIR" \ CONTINUUM_FARM_ID="$FARM_ID" \ CONTINUUM_GATEWAY_TOKEN="$TOKEN" \ CONTINUUM_UPLINK_URL="ws://localhost:$PORT/ws/edge/v1" \ + `# Deliberately absent, not an oversight: what is under test is the config` \ + `# the gateway writes and the camera_stream it advertises, both of which` \ + `# happen without go2rtc. Spawning the real one would bind :1984 and dial` \ + `# the printers on the LAN, so the test would depend on hardware it does` \ + `# not need. The supervisor logs a spawn failure every 5s; that is expected.` \ CONTINUUM_GO2RTC_BIN=/nonexistent-go2rtc \ CONTINUUM_GO2RTC_CONFIG="$WORK/go2rtc.yaml" \ CONTINUUM_STREAM_BASE_URL="$STREAM_BASE_URL" \ diff --git a/src/s3/index.ts b/src/s3/index.ts index 6b75739..91273ab 100644 --- a/src/s3/index.ts +++ b/src/s3/index.ts @@ -1,16 +1,61 @@ import { S3Client, PutObjectCommand, GetObjectCommand, HeadObjectCommand } from "@aws-sdk/client-s3"; import { getSignedUrl } from "@aws-sdk/s3-request-presigner"; +/** + * Object storage for job files, configured once at module load so a + * misconfigured deployment fails at boot rather than on the first upload - + * the same shape as `auth/verifier.ts` and `auth/edge_secret.ts`, and for + * the same reason. + * + * It replaces `?? ""` on the access key and secret. An empty credential is + * not a usable one, so that default bought nothing except moving the failure + * somewhere much worse: the process started happily, `POST /jobs` handed a + * client a presigned URL signed with an empty key, and the first sign that + * anything was wrong was a 403 from R2 - either in the client's uploader or, + * worse, inside a gateway that had downloaded R2's XML error document + * instead of a print. Nothing in the chain said "R2 is not configured". + * + * `R2_BUCKET` keeps a default, because a bucket name is not a credential and + * "continuum" is a reasonable one. `R2_ENDPOINT` keeps its derivation from + * the account id, which is how Cloudflare's own URLs are formed. + */ +function required(name: string): string { + const value = process.env[name]; + if (!value) { + throw new Error( + `${name} is not set - continuum-backend cannot store or serve job files without it. ` + + "See .env.example; an empty value is not a usable credential.", + ); + } + return value; +} + const accountId = process.env.R2_ACCOUNT_ID ?? ""; -const bucket = process.env.R2_BUCKET ?? "continuum"; +// `||`, so an empty value in the template falls back rather than becoming a +// bucket named "". +const bucket = process.env.R2_BUCKET || "continuum"; + +// Read before the client is built, so the error names the missing variable +// rather than surfacing later as a signing failure. +const accessKeyId = required("R2_ACCESS_KEY_ID"); +const secretAccessKey = required("R2_SECRET_ACCESS_KEY"); + +// `||` and not `??`: the committed template ships `R2_ENDPOINT=` empty, and +// `??` only catches null and undefined - so an empty value would be taken as +// a real endpoint and the derivation below would never run. This is the same +// trap `auth/edge_secret.ts` documents, and it caught this line too. +const endpoint = + process.env.R2_ENDPOINT || (accountId ? `https://${accountId}.r2.cloudflarestorage.com` : ""); +if (!endpoint) { + throw new Error( + "Neither R2_ENDPOINT nor R2_ACCOUNT_ID is set - there is no object storage URL to sign against.", + ); +} export const s3 = new S3Client({ region: "auto", - endpoint: process.env.R2_ENDPOINT ?? `https://${accountId}.r2.cloudflarestorage.com`, - credentials: { - accessKeyId: process.env.R2_ACCESS_KEY_ID ?? "", - secretAccessKey: process.env.R2_SECRET_ACCESS_KEY ?? "", - }, + endpoint, + credentials: { accessKeyId, secretAccessKey }, }); /** Presigned PUT for clients uploading a G-code/3MF file directly to R2. */ diff --git a/tests/authz.test.ts b/tests/authz.test.ts index 011cef2..3e755ef 100644 --- a/tests/authz.test.ts +++ b/tests/authz.test.ts @@ -170,3 +170,82 @@ describe("configuration", () => { expect((await grep("process\\.env\\.EDGE_JWT_SECRET")).trim().split("\n")).toHaveLength(1); }); }); + +/** + * Object storage is a credential like any other, and used to be the one that + * failed quietly. + * + * `R2_ACCESS_KEY_ID` and `R2_SECRET_ACCESS_KEY` defaulted to `""`, so a + * deployment that copied `.env.example` and filled in everything else + * started happily. `POST /jobs` then handed a client a presigned URL signed + * with an empty key, and the first symptom was a 403 from R2 - in the + * client's uploader, or inside a gateway that had downloaded R2's XML error + * document instead of a print. Nothing in that chain said "R2 is not + * configured". + */ +describe("object storage configuration", () => { + const importS3 = (env: Record) => + Bun.spawn(["bun", "-e", "await import('./src/s3/index.ts')"], { + cwd: import.meta.dir + "/..", + env: { ...process.env, ...env }, + stderr: "pipe", + stdout: "pipe", + }); + + // Every other variable is filled in, so the one under test is the only + // thing missing and the error has to name it. Without this the first + // missing variable in read order wins and the assertion passes for the + // wrong reason. + const complete = { + R2_ACCESS_KEY_ID: "key", + R2_SECRET_ACCESS_KEY: "secret", + R2_ACCOUNT_ID: "acct", + }; + + test("fails at boot when a credential is empty, naming the variable", async () => { + for (const name of ["R2_ACCESS_KEY_ID", "R2_SECRET_ACCESS_KEY"]) { + const proc = importS3({ ...complete, [name]: "" }); + expect(await proc.exited).not.toBe(0); + expect(await new Response(proc.stderr).text()).toContain(name); + } + }); + + /** + * An empty string, not just an absent variable - which is the case that + * actually happens, because the committed template ships these keys with + * no value. `??` would have missed every one of them. + */ + test("treats an empty credential as missing, not as a value", async () => { + const proc = importS3({ ...complete, R2_ACCESS_KEY_ID: "" }); + expect(await proc.exited).not.toBe(0); + expect(await new Response(proc.stderr).text()).toContain("R2_ACCESS_KEY_ID"); + }); + + test("refuses to start with no endpoint and no account id to derive one from", async () => { + const proc = importS3({ ...complete, R2_ENDPOINT: "", R2_ACCOUNT_ID: "" }); + expect(await proc.exited).not.toBe(0); + expect(await new Response(proc.stderr).text()).toContain("R2_ENDPOINT"); + }); + + test("starts with a complete configuration", async () => { + // R2_ENDPOINT empty on purpose: it must be derived from the account id, + // which is the `||`-not-`??` case that caught this module out. + const proc = importS3({ ...complete, R2_ENDPOINT: "" }); + expect(await proc.exited).toBe(0); + }); + + test("reads each credential in exactly one place", async () => { + // So there is only ever one line to get right, the same rule + // `EDGE_JWT_SECRET` is held to above. + for (const name of ["R2_ACCESS_KEY_ID", "R2_SECRET_ACCESS_KEY"]) { + const proc = Bun.spawn(["grep", "-rn", `process.env.${name}`, "src"], { + cwd: import.meta.dir + "/..", + stdout: "pipe", + }); + const hits = (await new Response(proc.stdout).text()).trim(); + // Read through the `required()` helper, so the literal name appears + // only where it is passed in - not as a `process.env.X` at all. + expect(hits).toBe(""); + } + }); +});