From 9c0e6c830cc4b5719f5ffe8c8374069aabc9c627 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Tue, 21 Jul 2026 12:54:37 +0000 Subject: [PATCH] fix: reject a non-integer --app-id/--installation-id before anything happens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers' blocker. `githubAppCommand` checked the two ids for truthiness only, then handed them to `Number()`. `--app-id nope` becomes NaN, and `JSON.stringify(NaN)` is `null` — so on a path that deliberately persists BEFORE calling Coolify, a typo wrote a credential record with a null app_id and could upload the security key before `POST /github-apps` rejected it. A half-run leaving a corrupt record on disk and a stray key on the server. Validated with the other ARGV checks, ABOVE openCoolify/assertTeam rather than in the register branch where I first put it. The first placement still let `GET /teams/current` go out before the refusal — the new test caught that, which is the argument for asserting "no stub hits" rather than "no writes". A typo should cost nothing, not one request. Digits-only rather than Number.isInteger: `1e3` and `0x10` are integers to JavaScript but are not how a GitHub App id is written, and quietly storing 1000 for `1e3` is the same class of wrong answer as storing null for `nope`. Coverage asserts both halves the review asked for — no stub hit AND an unchanged state directory — across non-numeric (both flags), zero, decimal, exponent and hex. A negative id gets its own case rather than joining the loop: parseArgs reads the leading dash as an option and rejects `-5` as unknown, exiting 1 rather than 2. The property that matters still holds — refused before any write or request — but it is a different path with a different exit code, and a loosened shared assertion would have hidden that rather than recorded it. Verified by mutation: disabling the check fails all six loop cases. --- src/cli.ts | 28 ++++++++++++ test/github-app-register-cli.test.ts | 68 +++++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/src/cli.ts b/src/cli.ts index fd984ce..e7f23b0 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1507,6 +1507,34 @@ async function githubAppCommand(rest: string[]): Promise { console.error(USAGE); return 2; } + // `register`'s two ids reach `Number()` far below, and a non-numeric string + // becomes NaN silently. That matters more here than it usually would, because + // `register` deliberately persists BEFORE it talks to Coolify: + // `JSON.stringify(NaN)` is `null`, so `--app-id nope` would write a credential + // record whose app_id is null and could upload the security key before + // `POST /github-apps` rejects it — a half-run leaving a corrupt record on disk + // and a stray key on the server (cast#7 review). + // + // This sits with the other ARGV checks, above openCoolify/assertTeam, because + // "reject before any write or network call" has to mean the team read too. A + // typo should cost nothing, not one request. + // + // Digits-only rather than Number.isInteger: `1e3` and `0x10` are integers to + // JavaScript but are not how a GitHub App id is written, and quietly storing + // 1000 for `1e3` is the same class of wrong answer this check exists to stop. + if (verb === "register") { + for (const [flag, raw] of [ + ["--app-id", values["app-id"]], + ["--installation-id", values["installation-id"]], + ] as const) { + if (raw !== undefined && (!/^\d+$/.test(raw) || Number(raw) <= 0)) { + console.error( + `${flag} must be a positive integer (got ${JSON.stringify(raw)})`, + ); + return 2; + } + } + } const stateDir = stateDirFrom(values.state); const bindingsPath = join(stateDir, "environments.yaml"); const bindings = loadBindings(bindingsPath); diff --git a/test/github-app-register-cli.test.ts b/test/github-app-register-cli.test.ts index 7dbc455..ccd9f3f 100644 --- a/test/github-app-register-cli.test.ts +++ b/test/github-app-register-cli.test.ts @@ -1,6 +1,6 @@ import { spawn } from "node:child_process"; import { generateKeyPairSync } from "node:crypto"; -import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; import { createServer } from "node:http"; import type { AddressInfo } from "node:net"; import { tmpdir } from "node:os"; @@ -275,6 +275,72 @@ describe("cast github-app register", () => { expect(stub.hits).toEqual([]); }); + // Invalid ids must be refused before ANYTHING happens (cast#7 review). + // `register` persists the credential record before it calls Coolify, and + // `Number("nope")` is NaN which `JSON.stringify` writes as `null` — so + // without this gate a typo produces a credential file with a null app_id AND + // a security key uploaded to a live Coolify, from a run that then fails. + // Both halves are asserted: no stub hit, and no file written. + for (const [what, argv] of [ + ["a non-numeric --app-id", ["--app-id", "nope"]], + ["a non-numeric --installation-id", ["--installation-id", "nope"]], + ["a zero --app-id", ["--app-id", "0"]], + ["a decimal --app-id", ["--app-id", "12.5"]], + // Integers to JavaScript, but not how an id is written — and silently + // storing 1000 for "1e3" is the quiet wrong answer, not a convenience. + ["an exponent --app-id", ["--app-id", "1e3"]], + ["a hex --app-id", ["--app-id", "0x10"]], + ] as const) { + it(`refuses ${what} before touching disk or Coolify`, async () => { + const stub = await stubCoolify({ + repositories: [{ full_name: "heavy-duty/incubator" }], + }); + const f = fixture( + stub.url, + "github_apps:\n heavy-duty/incubator: hdb-coolify-prod", + ); + const before = readdirSync(f.state).sort(); + + const base = REGISTER(f.state, f.pem); + const i = base.indexOf(argv[0]); + const args = [...base]; + args[i + 1] = argv[1]; + + const r = await run(args, "s\n"); + expect(r.code).toBe(2); + expect(r.output).toContain("must be a positive integer"); + // Nothing reached the network... + expect(stub.hits).toEqual([]); + // ...and nothing was created or rewritten in the state dir. + expect(readdirSync(f.state).sort()).toEqual(before); + }); + } + + // A NEGATIVE id never reaches the check above: parseArgs reads a leading dash + // as an option and rejects `-5` as unknown, exiting 1 rather than 2. That is + // still a refusal before any write or request, which is the property that + // matters — but it is a different code path with a different exit code, so it + // gets its own case rather than a loosened assertion hiding the difference. + it("refuses a negative --app-id before touching disk or Coolify", async () => { + const stub = await stubCoolify({ + repositories: [{ full_name: "heavy-duty/incubator" }], + }); + const f = fixture( + stub.url, + "github_apps:\n heavy-duty/incubator: hdb-coolify-prod", + ); + const before = readdirSync(f.state).sort(); + + const base = REGISTER(f.state, f.pem); + const args = [...base]; + args[base.indexOf("--app-id") + 1] = "-5"; + + const r = await run(args, "s\n"); + expect(r.code).not.toBe(0); + expect(stub.hits).toEqual([]); + expect(readdirSync(f.state).sort()).toEqual(before); + }); + it("prints usage for an unknown subcommand", async () => { const r = await run(["github-app", "wat"], null); expect(r.code).toBe(2);