fix: reject a non-integer --app-id/--installation-id before anything happens

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.
This commit is contained in:
dan-claude-bot 2026-07-21 12:54:37 +00:00
parent 5a1ec74e04
commit 9c0e6c830c
2 changed files with 95 additions and 1 deletions

View file

@ -1507,6 +1507,34 @@ async function githubAppCommand(rest: string[]): Promise<number> {
console.error(USAGE); console.error(USAGE);
return 2; 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 stateDir = stateDirFrom(values.state);
const bindingsPath = join(stateDir, "environments.yaml"); const bindingsPath = join(stateDir, "environments.yaml");
const bindings = loadBindings(bindingsPath); const bindings = loadBindings(bindingsPath);

View file

@ -1,6 +1,6 @@
import { spawn } from "node:child_process"; import { spawn } from "node:child_process";
import { generateKeyPairSync } from "node:crypto"; 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 { createServer } from "node:http";
import type { AddressInfo } from "node:net"; import type { AddressInfo } from "node:net";
import { tmpdir } from "node:os"; import { tmpdir } from "node:os";
@ -275,6 +275,72 @@ describe("cast github-app register", () => {
expect(stub.hits).toEqual([]); 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 () => { it("prints usage for an unknown subcommand", async () => {
const r = await run(["github-app", "wat"], null); const r = await run(["github-app", "wat"], null);
expect(r.code).toBe(2); expect(r.code).toBe(2);