fix: --port is argv too — reject it before the preflight, not at listen()
@claude-bot-andresmgsl's outstanding item from the prior round, which my last reply passed over in silence. That silence read as an oversight because it was one. `--port` on the create path was still bare `Number()`, so `--port abc` became NaN, reached `server.listen(NaN)` in github-app.ts, and died as an uncaught ERR_SOCKET_BAD_PORT stack trace — after `detectOwnerType` and the org-admin preflight had already gone out. It is the same missing argv validation this round fixed for the two ids, in a command whose stated rule is reject before any write or network call. Nothing is destroyed when it fails: no App and no client secret exist at that point. So this is not about damage, it is about the command honouring its own rule, and about failing with a sentence rather than a stack trace. Range-checked as well as digits-only: `--port 99999` passes every test the ids need and still cannot be listened on. Scope, stated rather than assumed: `server add --port` (src/cli.ts:2430) has the identical shape but predates this branch and is not in its diff. It is a real instance of the same bug and belongs in its own change, not smuggled into this one. Four CLI cases — non-numeric, out-of-range, zero, decimal — asserting exit 2, no stub hits and an unchanged state dir, driven through `create` because that is the path that reads the flag. Verified by mutation: disabling the check fails all four.
This commit is contained in:
parent
9c0e6c830c
commit
d442b8cc1b
2 changed files with 66 additions and 0 deletions
20
src/cli.ts
20
src/cli.ts
|
|
@ -1535,6 +1535,26 @@ async function githubAppCommand(rest: string[]): Promise<number> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// `--port` on the create path has the same defect the ids had, and the same
|
||||||
|
// rule applies: `Number("abc")` is NaN, which reaches `server.listen(NaN)` in
|
||||||
|
// github-app.ts and dies as an uncaught ERR_SOCKET_BAD_PORT stack trace —
|
||||||
|
// after `detectOwnerType` and the org-admin preflight have already gone out.
|
||||||
|
// Nothing is lost when it fails (no App and no secret exist yet), so this is
|
||||||
|
// about the command honouring its own stated rule rather than about damage:
|
||||||
|
// reject before any write or network call, and fail with a sentence instead
|
||||||
|
// of a stack trace.
|
||||||
|
//
|
||||||
|
// Range-checked as well as digits-only, because `--port 99999` is accepted by
|
||||||
|
// every check the ids need and still cannot be listened on.
|
||||||
|
if (values.port !== undefined) {
|
||||||
|
const p = Number(values.port);
|
||||||
|
if (!/^\d+$/.test(values.port) || p < 1 || p > 65535) {
|
||||||
|
console.error(
|
||||||
|
`--port must be a port number between 1 and 65535 (got ${JSON.stringify(values.port)})`,
|
||||||
|
);
|
||||||
|
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);
|
||||||
|
|
|
||||||
|
|
@ -341,6 +341,52 @@ describe("cast github-app register", () => {
|
||||||
expect(readdirSync(f.state).sort()).toEqual(before);
|
expect(readdirSync(f.state).sort()).toEqual(before);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// `--port` belongs to the CREATE path, and had the same defect the ids did:
|
||||||
|
// `Number("abc")` is NaN, which reaches server.listen(NaN) and dies as an
|
||||||
|
// uncaught ERR_SOCKET_BAD_PORT stack trace — after detectOwnerType and the
|
||||||
|
// org-admin preflight have already gone out. Nothing is lost when it fails
|
||||||
|
// (no App and no secret exist yet), so this is about the command honouring
|
||||||
|
// its own rule — reject before any write or network call — and failing with
|
||||||
|
// a sentence rather than a stack trace.
|
||||||
|
//
|
||||||
|
// Driven through `create` because that is the path that reads the flag. The
|
||||||
|
// validation sits in the shared preamble, above openCoolify, so the run ends
|
||||||
|
// before the browser flow this command would otherwise need.
|
||||||
|
for (const [what, port] of [
|
||||||
|
["a non-numeric --port", "abc"],
|
||||||
|
["an out-of-range --port", "99999"],
|
||||||
|
["a zero --port", "0"],
|
||||||
|
["a decimal --port", "80.5"],
|
||||||
|
] as const) {
|
||||||
|
it(`refuses ${what} before touching disk or Coolify`, async () => {
|
||||||
|
const stub = await stubCoolify({ repositories: [] });
|
||||||
|
const f = fixture(
|
||||||
|
stub.url,
|
||||||
|
"github_apps:\n heavy-duty/incubator: hdb-coolify-prod",
|
||||||
|
);
|
||||||
|
const before = readdirSync(f.state).sort();
|
||||||
|
|
||||||
|
const r = await run(
|
||||||
|
[
|
||||||
|
"github-app",
|
||||||
|
"create",
|
||||||
|
"heavy-duty/incubator",
|
||||||
|
"--env",
|
||||||
|
"prod",
|
||||||
|
"--state",
|
||||||
|
f.state,
|
||||||
|
"--port",
|
||||||
|
port,
|
||||||
|
],
|
||||||
|
null,
|
||||||
|
);
|
||||||
|
expect(r.code).toBe(2);
|
||||||
|
expect(r.output).toContain("--port must be a port number");
|
||||||
|
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);
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue