diff --git a/secrets/README.md b/secrets/README.md index f767e2dba..95af3b74c 100644 --- a/secrets/README.md +++ b/secrets/README.md @@ -123,7 +123,12 @@ of those too; add a matching `secrets:` entry in `docker-compose.yml` (or a Everything in this directory except this README is gitignored. `scripts/selfhost-init-secrets.sh` generates a real random value for each self-generatable file (so `docker compose build`/`up` never fails on a missing file, and boots without any manual `openssl` step) and creates only an -**empty** placeholder for the four externally-issued ones it can't generate a usable value for. Either +**empty** placeholder for the four externally-issued ones it can't generate a usable value for. Those +four placeholders are safe to leave empty: the app treats an empty file for `GITHUB_APP_PRIVATE_KEY_FILE`, +`ORB_ENROLLMENT_SECRET_FILE`, `PAGERDUTY_ROUTING_KEY_FILE`, or `CLAUDE_CODE_OAUTH_TOKEN_FILE` as "not +configured", skips it, and logs a `selfhost_secret_file_empty_optional` warning naming the variable. An empty +file for any OTHER secret is still a hard boot failure -- there it can only mean a truncated write, which +would otherwise leave you running with a credential that silently reads as unset. Either way, it only ever touches the *permissions* of a file that is still empty, never its content — the moment a real value lands in one (written by the script or by you), both the content and whatever mode you set are left alone on every future run. Always safe to re-run. diff --git a/src/selfhost/load-file-secrets.ts b/src/selfhost/load-file-secrets.ts index 43180c6d0..902726642 100644 --- a/src/selfhost/load-file-secrets.ts +++ b/src/selfhost/load-file-secrets.ts @@ -15,6 +15,32 @@ import { readFileSync } from "node:fs"; // named exactly one of these. const COMPOSE_RESERVED_FILE_VARS = new Set(["COMPOSE_FILE", "COMPOSE_ENV_FILE"]); +/** + * The secrets `scripts/selfhost-init-secrets.sh` deliberately leaves EMPTY, for which empty therefore means + * "not configured yet" rather than "truncated". + * + * WHY THIS EXISTS: #9487 made an empty secret file fatal at boot, which is right for a secret the init + * script fills with a real random value -- an empty one there can only mean a truncated write, and the bug + * it fixed (a truncated GITHUB_WEBHOOK_SECRET booting an instance that silently rejected every webhook) is + * exactly that. But these four come from an EXTERNAL party, so the init script cannot generate them and + * creates a zero-byte placeholder instead (secrets/README.md says so explicitly). Compose also requires the + * file to exist before the stack will start. The result was that running the documented setup and starting + * the container crash-looped it -- observed on the ORB, where an unused GitHub App key did precisely that. + * + * So for these four ONLY, an empty file is skipped rather than fatal, and loudly logged. That is strictly + * better than the pre-#9487 behavior it superficially resembles: back then an empty file silently became an + * empty env var that every `nonBlank()` downstream read as unconfigured, with no signal at all. Here the + * target var is left genuinely unset and the operator gets a named warning at boot. + * + * Every other secret keeps #9487's fail-closed behavior unchanged. + */ +const OPTIONAL_WHEN_EMPTY_FILE_VARS = new Set([ + "GITHUB_APP_PRIVATE_KEY_FILE", + "ORB_ENROLLMENT_SECRET_FILE", + "PAGERDUTY_ROUTING_KEY_FILE", + "CLAUDE_CODE_OAUTH_TOKEN_FILE", +]); + /** `env` and `readFile` are injectable purely for testability -- every real caller uses the defaults * (`process.env`, `node:fs`'s `readFileSync`), so this is byte-identical to a hardcoded version at * runtime while letting tests pass a plain object and a mock reader instead of mutating global state. */ @@ -59,6 +85,19 @@ export function loadFileSecrets( // re-reported as "unreadable", collapsing two genuinely different operator problems (a bad path/permission // vs a truncated write) into one misleading message and the wrong log event. if (value === "") { + // An externally-issued secret the init script could only stub out: empty is "not configured", so skip + // it and leave the target var genuinely unset -- but say so, loudly and by name. + if (OPTIONAL_WHEN_EMPTY_FILE_VARS.has(key)) { + console.warn( + JSON.stringify({ + level: "warn", + event: "selfhost_secret_file_empty_optional", + var: key, + message: `${key} points at an empty file (${path}); treating it as not configured. Write the issued value if you need this capability.`, + }), + ); + continue; + } console.error( JSON.stringify({ level: "error", diff --git a/test/unit/selfhost-load-file-secrets.test.ts b/test/unit/selfhost-load-file-secrets.test.ts index 5cacf69f5..f5d897dfe 100644 --- a/test/unit/selfhost-load-file-secrets.test.ts +++ b/test/unit/selfhost-load-file-secrets.test.ts @@ -180,3 +180,62 @@ describe("loadFileSecrets return value (#9543 — which vars actually came from expect(loadFileSecrets(env, () => "unused")).toEqual([]); }); }); + +describe("empty placeholders for externally-issued secrets (#9487 follow-up)", () => { + // scripts/selfhost-init-secrets.sh creates a ZERO-BYTE placeholder for the four secrets it cannot + // generate, and compose refuses to start unless the file exists. #9487 then made an empty file fatal, so + // running the documented setup and starting the container crash-looped it. Observed live on the ORB: an + // unused GitHub App key took the whole instance down on upgrade. + const EXTERNALLY_ISSUED = [ + "GITHUB_APP_PRIVATE_KEY_FILE", + "ORB_ENROLLMENT_SECRET_FILE", + "PAGERDUTY_ROUTING_KEY_FILE", + "CLAUDE_CODE_OAUTH_TOKEN_FILE", + ]; + + it.each(EXTERNALLY_ISSUED)("REGRESSION: an empty %s is skipped, not fatal", (key) => { + const env: Record = { [key]: "/run/secrets/placeholder" }; + expect(() => loadFileSecrets(env, () => "")).not.toThrow(); + // Left genuinely UNSET rather than set to "" -- the pre-#9487 bug was an empty string that read as + // configured-but-blank to anything checking presence rather than truthiness. + expect(env[key.slice(0, -"_FILE".length)]).toBeUndefined(); + }); + + it("REGRESSION: reproduces the exact ORB boot crash and shows it is fixed", () => { + const env: Record = { + GITHUB_APP_PRIVATE_KEY_FILE: "/run/secrets/github_app_private_key", + GITHUB_WEBHOOK_SECRET_FILE: "/run/secrets/github_webhook_secret", + }; + const files: Record = { + "/run/secrets/github_app_private_key": "", + "/run/secrets/github_webhook_secret": "a-real-webhook-secret", + }; + expect(() => loadFileSecrets(env, (path) => files[path] ?? "")).not.toThrow(); + expect(env.GITHUB_APP_PRIVATE_KEY).toBeUndefined(); + expect(env.GITHUB_WEBHOOK_SECRET).toBe("a-real-webhook-secret"); + }); + + it("INVARIANT: #9487's fail-closed behavior is UNCHANGED for a self-generated secret", () => { + // The bug #9487 actually fixed: a truncated webhook secret booting an instance that silently rejected + // every webhook. The init script writes a real random value here, so empty can only mean truncation. + const env: Record = { GITHUB_WEBHOOK_SECRET_FILE: "/run/secrets/github_webhook_secret" }; + expect(() => loadFileSecrets(env, () => "")).toThrow(/is empty/); + }); + + it("whitespace-only is treated the same as empty for an optional secret", () => { + const env: Record = { PAGERDUTY_ROUTING_KEY_FILE: "/run/secrets/pagerduty_routing_key" }; + expect(() => loadFileSecrets(env, () => " \n\t ")).not.toThrow(); + expect(env.PAGERDUTY_ROUTING_KEY).toBeUndefined(); + }); + + it("a REAL value in an optional secret still loads normally", () => { + const env: Record = { CLAUDE_CODE_OAUTH_TOKEN_FILE: "/run/secrets/claude_code_oauth_token" }; + expect(loadFileSecrets(env, () => " real-token ")).toEqual(["CLAUDE_CODE_OAUTH_TOKEN"]); + expect(env.CLAUDE_CODE_OAUTH_TOKEN).toBe("real-token"); + }); + + it("an UNREADABLE optional secret is still fatal — missing is not the same as deliberately empty", () => { + const env: Record = { GITHUB_APP_PRIVATE_KEY_FILE: "/run/secrets/gone" }; + expect(() => loadFileSecrets(env, () => { throw new Error("ENOENT"); })).toThrow(/Failed to read secret file/); + }); +});