diff --git a/CHANGELOG.md b/CHANGELOG.md index 64a48ac..e402893 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,10 +103,22 @@ actually cutting it, and this file starts there. `--webhook-secret` is optional: a webhook-inactive App is the correct shape for a tailnet-only Coolify, and nobody has to invent a placeholder. + The one-shot secrets are written the instant GitHub yields them, *before* + `create` waits ~5 minutes for you to install the App — so a timeout, a + dropped connection or a `Ctrl-C` during that wait cannot destroy a private + key GitHub will never re-show. Until the install lands the record carries + `"installation_id": null`, the single field GitHub will answer again, and + it is backfilled on success. A name collision is refused *before* the + browser flow starts, when no App exists yet and nothing can be lost. + Both verbs end at the step that matters most: `GET /github-apps/{id}/repositories`, asserting the repo is actually reachable. Until now a misconfigured App failed silently and surfaced hours later, in - a different command, as an unresolvable source at `cast apply` time. + a different command, as an unresolvable source at `cast apply` time. When + that check fails its advice is to re-run `register` — which now re-verifies + the existing Coolify Source instead of registering a second one, because + Coolify does not enforce unique Source names (`GithubController@create` + validates `name` without `unique` and calls a plain `GithubApp::create`). No new dependencies — `node:http` serves the callback, `node:crypto`'s `createSign("RSA-SHA256")` mints the App JWT that recovers the installation diff --git a/README.md b/README.md index a4d7048..ca12941 100644 --- a/README.md +++ b/README.md @@ -254,17 +254,30 @@ What `create` does: which checks the CSRF `state` and shuts down. 5. Exchanges the code. **This response is the only moment GitHub ever hands over the private key, the client secret and the webhook secret together.** -6. Prints (and tries to open) the install URL; you pick the repository. -7. Recovers the installation id by minting an RS256 JWT with the App's own key — +6. **Writes all three to disk immediately**, before waiting on anything — + see [Where the credentials land](#where-the-credentials-land). Everything + after this point can fail for ordinary reasons (a slow install screen, a + dropped network, `Ctrl-C`), and none of those may cost you a key GitHub will + not reissue. +7. Prints (and tries to open) the install URL; you pick the repository. +8. Recovers the installation id by minting an RS256 JWT with the App's own key — never from the `installation_id` GitHub appends to a redirect, which GitHub - documents as a spoofable hint. -8. Uploads the key to Coolify and creates the App record. -9. **Asks Coolify which repositories the App can actually see, and fails if - `/` is not among them.** This is the step that matters most: - without it a misconfigured App fails silently and surfaces hours later, in a - different command, as an unresolvable source at `cast apply` time. + documents as a spoofable hint — then fills it into the record from step 6. +9. Uploads the key to Coolify and creates the App record — unless a Source of + that name already exists, in which case it verifies that one rather than + registering a second (Coolify does not enforce unique Source names). +10. **Asks Coolify which repositories the App can actually see, and fails if + `/` is not among them.** This is the step that matters most: + without it a misconfigured App fails silently and surfaces hours later, in a + different command, as an unresolvable source at `cast apply` time. -`register` is the same command from step 8 onwards, for an App you already hold — +If the install never lands, `create` stops at step 8 and tells you the exact +`register` command that finishes the job against the files from step 6. Nothing +is lost and nothing has to be recreated — in particular, do **not** re-run +`create`, which would mint a second App. For that same reason `create` refuses +up front, before the browser flow, when `.pem` already exists. + +`register` is the same command from step 9 onwards, for an App you already hold — one made by hand, or a disaster-recovery restore from a stored PEM: ```sh @@ -290,6 +303,13 @@ Into the state directory you point cast at — cast itself stores nothing: └── .json # 0600, app id, installation id, client id + secret, webhook secret ``` +Both are written the instant GitHub yields them, which is *before* `create` +waits for you to install the App. Until the install lands, `.json` carries +`"installation_id": null` — that is the one field GitHub will answer again as +often as it is asked, and it is filled in on success. Re-running against an +existing file is idempotent on identical content and a **refusal** otherwise; +`--force` is the deliberate escape hatch for a stale half-run. + All three secrets, because GitHub shows them once and `register` needs the client secret to be re-runnable at all — a disaster-recovery restore that is missing it is not a restore. They are written **plaintext at 0600**, not into `secrets/`: diff --git a/src/cli.ts b/src/cli.ts index 6eb1f02..fd984ce 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -196,6 +196,11 @@ github-app (the credential Coolify clones private repos with): serves a one-shot page on 127.0.0.1, your browser session authenticates the form, and the conversion response hands over the private key, the client secret and the webhook secret in one body. Nothing is transcribed. + All three are written to disk the instant they arrive — BEFORE the wait + for you to install the App — so a timeout or a Ctrl-C during that wait + cannot lose a key GitHub shows exactly once. If the install never lands, + cast prints the \`register\` command that finishes the job; do not re-run + \`create\`, which would mint a second App. register adopts credentials you already hold: an App created by hand, or a disaster-recovery restore from a stored PEM. The client secret is read from STDIN (never argv); --webhook-secret is optional, because a @@ -207,7 +212,9 @@ github-app (the credential Coolify clones private repos with): --force overwrite an existing PEM/credentials file under /github-apps/. Both verbs end by asking Coolify which repositories the App can actually see and failing if / is not among them — the check that turns a silent - misconfiguration into an error next to the thing that caused it. + misconfiguration into an error next to the thing that caused it. Re-running + \`register\` after that failure RE-VERIFIES an existing Coolify Source of the + same name rather than registering a second one. capture (adopt a hand-built instance into the age secret store): --generated force NAME to the \`pending-coolify-generated\` placeholder, diff --git a/src/github-app.ts b/src/github-app.ts index 5809bb1..8b8e376 100644 --- a/src/github-app.ts +++ b/src/github-app.ts @@ -4,6 +4,7 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { createServer } from "node:http"; import type { AddressInfo } from "node:net"; import { join } from "node:path"; +import { fileURLToPath } from "node:url"; import { parseDocument } from "yaml"; import type { Bindings } from "./bindings.js"; import type { CoolifyClient } from "./coolify.js"; @@ -76,16 +77,58 @@ export type AppCredentials = { privateKeyPem: string; }; +// What `create` has in hand the instant the conversion returns, and what it +// persists BEFORE going anywhere near the install poll. It is an AppCredentials +// missing exactly one field, and that field is the only one recoverable later: +// GitHub will re-answer "which installation" forever, and will never re-show +// the private key or the client secret. +export type PendingAppCredentials = Omit & { + installationId?: number; +}; + export type RegisterResult = { // Coolify's OWN integer id for the App record, not GitHub's app id. It is // what GET /github-apps/{id}/repositories takes. coolifyAppId: number; - keyUuid: string; + // null when the App was already registered under this name and cast verified + // the existing record instead of creating a second one. + keyUuid: string | null; repositories: string[]; pemPath: string; secretsPath: string; }; +// grok #4: GitHub asks every client to identify itself, and an absent +// User-Agent is a documented cause of 403s that look like nothing else. +// Resolved from package.json the same way `cast --version` does, and never +// fatal — a User-Agent is not worth failing a bootstrap over. +let userAgentCache: string | undefined; +export function githubUserAgent(): string { + if (userAgentCache !== undefined) return userAgentCache; + let version = "unknown"; + try { + const pkgPath = fileURLToPath(new URL("../package.json", import.meta.url)); + const raw: unknown = JSON.parse(readFileSync(pkgPath, "utf8")).version; + if (typeof raw === "string") version = raw; + } catch { + // A missing or unreadable package.json means an odd install tree, not a + // reason to refuse to talk to GitHub. + } + userAgentCache = `cast/${version}`; + return userAgentCache; +} + +// Every GitHub request in this module goes out with these. +function githubHeaders( + extra: Record = {}, +): Record { + return { + Accept: "application/vnd.github+json", + "User-Agent": githubUserAgent(), + ...extra, + }; +} + // --------------------------------------------------------------------------- // Step 2 — the name comes from state, not from a flag // --------------------------------------------------------------------------- @@ -106,6 +149,7 @@ export function resolveAppName(opts: { nameFlag?: string; }): { name: string; seed: boolean } { const repoShort = opts.orgRepo.split("/")[1] ?? opts.orgRepo; + if (opts.nameFlag !== undefined) assertUsableAppName(opts.nameFlag); const bound = opts.bindings.github_apps[opts.orgRepo] ?? opts.bindings.github_apps[repoShort]; @@ -125,6 +169,9 @@ export function resolveAppName(opts: { ].join("\n"), ); } + // Applies to a value that came from state too: environments.yaml is + // hand-edited, and `github_apps` entries become filenames just the same. + assertUsableAppName(bound); return { name: bound, seed: false }; } if (opts.nameFlag === undefined) { @@ -364,10 +411,7 @@ export async function convertManifestCode( `https://api.github.com/app-manifests/${encodeURIComponent(code)}/conversions`, { method: "POST", - headers: { - Accept: "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - }, + headers: githubHeaders({ "X-GitHub-Api-Version": "2022-11-28" }), }, ); if (!res.ok) { @@ -509,11 +553,10 @@ export async function findInstallationId(opts: { ? `/users/${encodeURIComponent(opts.owner)}/installation` : `/orgs/${encodeURIComponent(opts.owner)}/installation`; const res = await (opts.fetchImpl ?? fetch)(`https://api.github.com${path}`, { - headers: { - Accept: "application/vnd.github+json", + headers: githubHeaders({ Authorization: `Bearer ${opts.jwt}`, "X-GitHub-Api-Version": "2022-11-28", - }, + }), }); if (res.status === 404) return undefined; if (!res.ok) { @@ -528,6 +571,15 @@ export async function findInstallationId(opts: { return raw.id; } +// Distinguishable so `create` can attach the remedy it alone can write — the +// real paths it just persisted to — without string-matching a message. +export class InstallationNeverArrivedError extends Error { + constructor(message: string) { + super(message); + this.name = "InstallationNeverArrivedError"; + } +} + // Poll while the operator clicks through the install screen in a browser. export async function awaitInstallationId(opts: { owner: string; @@ -565,18 +617,19 @@ export async function awaitInstallationId(opts: { } await sleep(intervalMs); } - throw new Error( + // Only the FACT belongs here. This function does not know where — or + // whether — anything was persisted, and the previous version of this message + // asserted that credentials were "already there" on exactly the path where + // they were not (all three reviewers, #124). The caller that owns the files + // owns the remedy: see createGithubApp. + throw new InstallationNeverArrivedError( [ `the App was never installed on ${opts.owner}`, "", `cast polled GET /${opts.ownerType === "User" ? "users" : "orgs"}/${opts.owner}/installation`, `for ${Math.round((attempts * intervalMs) / 1000)}s and it stayed 404.`, "", - "The App exists on GitHub — only the install step is missing. Open the", - "install URL printed above, choose the target repository, and then re-run", - "`cast github-app register` with the credentials cast saved under", - "/github-apps/ (the private key and client secret are already there;", - "nothing has to be recreated).", + "The App exists on GitHub — only the install step is missing.", ].join("\n"), ); } @@ -606,14 +659,88 @@ export function githubAppDir(stateDir: string): string { return join(stateDir, "github-apps"); } +// grok #3: `name` becomes a path segment (`.pem`, `.json`). It is +// operator-controlled and therefore not a security boundary — but a typo with a +// slash in it silently nests credentials under a subdirectory nobody will think +// to look in, and `..` walks out of the state directory entirely. Neither is +// worth diagnosing later, so both are refused here, at the one point where the +// name becomes a filename. +export function assertUsableAppName(name: string): void { + const bad = + name === "" + ? "is empty" + : name === "." || name === ".." + ? "is a directory reference" + : /[/\\]/.test(name) + ? "contains a path separator" + : name.includes("..") + ? "contains `..`" + : name.startsWith(".") + ? "starts with a dot" + : // biome-ignore lint/suspicious/noControlCharactersInRegex: rejecting them is the point + /[\x00-\x1f]/.test(name) + ? "contains a control character" + : undefined; + if (bad === undefined) return; + throw new Error( + [ + `github app name ${JSON.stringify(name)} ${bad}`, + "", + "The name is used verbatim as a filename under /github-apps/ and as", + "the Coolify Source label that environments.yaml binds. Use a plain name", + "like `hdb-coolify-prod`.", + ].join("\n"), + ); +} + +// Refuse a name collision BEFORE the browser flow, when nothing has been +// created and nothing can be lost (claude-bot, #124). +// +// On the `create` path a stale `.pem` is a trap: the freshly minted App's +// key differs from it by construction, so the post-conversion persist would hit +// writeExclusive's refusal holding the one and only copy of a key GitHub has +// already stopped showing — and that refusal's remedy ("pass --force and +// re-run") would mean minting a SECOND App. Checked here, the answer costs +// nothing: no App exists yet. +export function preflightCredentialSlot(opts: { + stateDir: string; + name: string; + force?: boolean; +}): void { + assertUsableAppName(opts.name); + if (opts.force === true) return; + const dir = githubAppDir(opts.stateDir); + const occupied = [`${opts.name}.pem`, `${opts.name}.json`] + .map((f) => join(dir, f)) + .filter((p) => existsSync(p)); + if (occupied.length === 0) return; + throw new Error( + [ + `${opts.name} already has credentials on disk`, + "", + ...occupied.map((p) => ` ${p}`), + "", + "`create` mints a NEW App, whose private key cannot match the one already", + "saved here — so this is checked now, before the browser flow, rather than", + "after GitHub has handed over a key that would have nowhere to go.", + "", + "If those files are the App you want, you do not need `create`: install it", + "on the repository and run `cast github-app register` against them.", + "If they are a stale half-run whose App you have since deleted, move them", + "aside or pass --force.", + ].join("\n"), + ); +} + export function persistCredentials(opts: { stateDir: string; name: string; - creds: AppCredentials; + creds: PendingAppCredentials; org: string; orgRepo: string; force?: boolean; }): { pemPath: string; secretsPath: string } { + assertUsableAppName(opts.name); const dir = githubAppDir(opts.stateDir); mkdirSync(dir, { recursive: true, mode: 0o700 }); const ignore = join(dir, ".gitignore"); @@ -639,49 +766,96 @@ export function persistCredentials(opts: { const pemPath = join(dir, `${opts.name}.pem`); const secretsPath = join(dir, `${opts.name}.json`); writeExclusive(pemPath, opts.creds.privateKeyPem, opts.force === true); - writeExclusive( - secretsPath, - `${JSON.stringify( - { - name: opts.name, - org: opts.org, - repo: opts.orgRepo, - app_id: opts.creds.appId, - installation_id: opts.creds.installationId, - client_id: opts.creds.clientId, - client_secret: opts.creds.clientSecret, - webhook_secret: opts.creds.webhookSecret, - private_key_file: `${opts.name}.pem`, - }, - null, - 2, - )}\n`, - opts.force === true, - ); + // `installation_id: null` is the honest representation of a record written + // between the conversion and the install: everything GitHub shows once is + // here, and the one missing field is the one GitHub will answer again. + const record = { + name: opts.name, + org: opts.org, + repo: opts.orgRepo, + app_id: opts.creds.appId, + installation_id: opts.creds.installationId ?? null, + client_id: opts.creds.clientId, + client_secret: opts.creds.clientSecret, + webhook_secret: opts.creds.webhookSecret, + private_key_file: `${opts.name}.pem`, + }; + writeCredentialsRecord(secretsPath, record, opts.force === true); return { pemPath, secretsPath }; } +// Same refusal as writeExclusive, with ONE transition carved out: a record +// whose only difference from the incoming one is that its `installation_id` was +// null. That is the backfill `create` performs once the install lands, and it +// is not the loss writeExclusive exists to prevent — nothing irreplaceable +// changes. Anything else still refuses. +function writeCredentialsRecord( + path: string, + record: Record, + force: boolean, +): void { + const next = `${JSON.stringify(record, null, 2)}\n`; + if (existsSync(path) && !force) { + const raw = readFileSync(path, "utf8"); + if (raw === next) return; + if (!isInstallationBackfill(raw, record)) { + throw refusalToOverwrite(path); + } + } + writeFileSync(path, next, { mode: 0o600 }); +} + +function isInstallationBackfill( + existing: string, + next: Record, +): boolean { + let prev: Record; + try { + prev = JSON.parse(existing) as Record; + } catch { + return false; + } + if (prev === null || typeof prev !== "object") return false; + // Only ever fills a null in; never overwrites an id with a different one. + if (prev.installation_id !== null) return false; + if (typeof next.installation_id !== "number") return false; + const keys = new Set([...Object.keys(prev), ...Object.keys(next)]); + for (const key of keys) { + if (key === "installation_id") continue; + if (prev[key] !== next[key]) return false; + } + return true; +} + // Idempotent when the content matches, a refusal when it does not. Overwriting // a DIFFERENT credential silently is how the one copy of a private key is lost. function writeExclusive(path: string, content: string, force: boolean): void { if (existsSync(path) && !force) { if (readFileSync(path, "utf8") === content) return; - throw new Error( - [ - `refusing to overwrite ${path}`, - "", - "It already holds different content. For a GitHub App private key that is", - "the only copy in existence — GitHub will not show it again — so cast will", - "not replace it without being told to.", - "", - "Move it aside, or pass --force if the existing file is stale (e.g. a", - "previous `create` attempt whose App you have since deleted).", - ].join("\n"), - ); + throw refusalToOverwrite(path); } writeFileSync(path, content, { mode: 0o600 }); } +// The remedy here is honest for `register`, where re-running is cheap and +// nothing is minted. `create` can no longer reach this refusal: it is +// pre-flighted before the browser flow (preflightCredentialSlot), so by the +// time a create-path persist runs, the slot is either clean or an exact match. +function refusalToOverwrite(path: string): Error { + return new Error( + [ + `refusing to overwrite ${path}`, + "", + "It already holds different content. For a GitHub App private key that is", + "the only copy in existence — GitHub will not show it again — so cast will", + "not replace it without being told to.", + "", + "Move it aside, or pass --force if the existing file is stale (e.g. a", + "previous `create` attempt whose App you have since deleted).", + ].join("\n"), + ); +} + // --------------------------------------------------------------------------- // Steps 8 + 9 — the register code path. `create` falls through into THIS. // --------------------------------------------------------------------------- @@ -722,6 +896,48 @@ export async function readAppRepositories( return names; } +// grok #2: the repo-visibility failure tells the operator to re-run `register` +// to re-check — and re-running used to re-POST the key and the App first. That +// is only harmless if Coolify de-dupes by name, and it does not. Coolify's own +// GithubController@create validates `'name' => 'required|string|max:255'` — +// no `unique` rule — and then calls a plain `GithubApp::create($payload)`. The +// vendored OpenAPI agrees by omission: the create response documents 201/400/ +// 401/422 and no conflict at all. So a second `register` under the same name +// yields a second Source, and the remedy printed by a failed post-condition +// quietly multiplies the thing it is asking the operator to fix. +// +// Fixed by looking first. This turns "re-run register to re-check" into what +// its own wording already promised — a re-check — and leaves the create path +// unchanged, since a clean instance has nothing to find. +export type ExistingApp = { id: number; appId: number | undefined }; + +// `undefined` means "cast could not read the list", which is NOT the same as +// "there is nothing there" — see the caller for why that distinction does not +// become a refusal here. +export async function findRegisteredApp( + client: CoolifyClient, + name: string, +): Promise { + let raw: unknown; + try { + raw = await client.get("/github-apps"); + } catch { + return undefined; + } + if (!Array.isArray(raw)) return undefined; + const found: ExistingApp[] = []; + for (const item of raw) { + if (typeof item !== "object" || item === null) continue; + const row = item as Record; + if (row.name !== name || typeof row.id !== "number") continue; + found.push({ + id: row.id, + appId: typeof row.app_id === "number" ? row.app_id : undefined, + }); + } + return found; +} + // Register the App with Coolify and PROVE it works. // // The two POSTs are the script's, unchanged in effect. The GET is the step that @@ -755,6 +971,121 @@ export async function registerGithubApp(opts: { log(`private key → ${pemPath}`); log(`credentials → ${secretsPath}`); + // Look before creating (grok #2). Both verbs do this, so `create`'s + // fall-through remains an identity of behaviour: the same call sequence, + // in the same order, whichever verb produced the credentials. + const existing = await findRegisteredApp(opts.client, opts.name); + if (existing === undefined) { + // Deliberately a warning and not a refusal. An unreadable list leaves cast + // exactly where it was before this check existed — it might create a + // duplicate — whereas refusing would block the bootstrap command outright + // on an instance whose list endpoint is restricted. The failure mode of + // proceeding is a spare Source the operator can delete; the failure mode of + // refusing is no Source at all. Said out loud rather than assumed away. + log( + "warning: could not list existing Coolify Sources; cannot check whether", + ); + log(` ${opts.name} is already registered. Proceeding to create.`); + } + if (existing !== undefined && existing.length > 1) { + throw new Error( + [ + `Coolify already has ${existing.length} GitHub App records named ${opts.name}`, + "", + ` coolify ids: ${existing.map((e) => e.id).join(", ")}`, + "", + "Coolify does not enforce unique Source names, so cast cannot tell which", + "of these `cast apply` would resolve. Delete the duplicates from Coolify's", + "Sources page, leaving the one that is correctly installed, then re-run.", + ].join("\n"), + ); + } + const reuse = existing?.[0]; + if ( + reuse !== undefined && + reuse.appId !== undefined && + reuse.appId !== opts.creds.appId + ) { + throw new Error( + [ + `Coolify already has a GitHub App named ${opts.name}, and it is a DIFFERENT App`, + "", + ` registered: github app id ${reuse.appId} (coolify id ${reuse.id})`, + ` supplied: github app id ${opts.creds.appId}`, + "", + "Registering these credentials would leave two Sources with one name and", + "no way for `cast apply` to tell them apart. Either delete the old record", + "from Coolify's Sources page, or bind this App to a different name in", + "environments.yaml.", + ].join("\n"), + ); + } + + let coolifyAppId: number; + let keyUuid: string | null = null; + if (reuse !== undefined) { + // The verify-only path. Nothing is POSTed, so a failed post-condition can + // be re-checked as many times as the operator needs. + coolifyAppId = reuse.id; + log( + `already registered as ${opts.name} (coolify id ${coolifyAppId}) — verifying, not re-creating`, + ); + } else { + ({ coolifyAppId, keyUuid } = await createCoolifyApp(opts, log)); + } + + const repositories = await readAppRepositories(opts.client, coolifyAppId); + if (repositories === undefined) { + throw new Error( + [ + `cannot verify that ${opts.name} can reach ${opts.orgRepo}`, + "", + `GET /github-apps/${coolifyAppId}/repositories did not return a repository`, + "list cast can read. The App IS registered — this is a failed check, not a", + "failed registration — but the check is the point: an App that cannot see", + "the repo fails at `cast apply` time instead, hours later and somewhere", + "else.", + "", + "Open Coolify's Sources page and confirm the App lists the repository, or", + "re-run this verification with `cast github-app register` once the install", + "is fixed. Re-running re-checks the existing record; it does not create a", + "second one.", + ].join("\n"), + ); + } + if (!repositories.includes(opts.orgRepo)) { + throw new Error( + [ + `${opts.name} is registered but cannot see ${opts.orgRepo}`, + "", + ` can see: ${repositories.join(", ") || "(no repositories at all)"}`, + "", + "The App exists on GitHub and in Coolify; it is INSTALLED on the wrong", + "repositories (or on none). Open", + " https://github.com/settings/installations", + "or the org's Settings → GitHub Apps, grant the App access to", + `${opts.orgRepo}, and re-run \`cast github-app register\` to re-check.`, + "That re-check reuses the record above rather than registering a second.", + "", + "Left unfixed this surfaces at `cast apply` time as an unresolvable source.", + ].join("\n"), + ); + } + log(`verified: ${opts.name} can clone ${opts.orgRepo} ✓`); + + return { coolifyAppId, keyUuid, repositories, pemPath, secretsPath }; +} + +// The two POSTs, unchanged in effect from the script's. +async function createCoolifyApp( + opts: { + client: CoolifyClient; + name: string; + org: string; + creds: AppCredentials; + }, + log: (line: string) => void, +): Promise<{ coolifyAppId: number; keyUuid: string }> { const key = (await opts.client.post("/security/keys", { name: `${opts.name}-key`, private_key: opts.creds.privateKeyPem, @@ -790,45 +1121,7 @@ export async function registerGithubApp(opts: { ); } log(`registered as ${opts.name} (coolify id ${coolifyAppId})`); - - const repositories = await readAppRepositories(opts.client, coolifyAppId); - if (repositories === undefined) { - throw new Error( - [ - `cannot verify that ${opts.name} can reach ${opts.orgRepo}`, - "", - `GET /github-apps/${coolifyAppId}/repositories did not return a repository`, - "list cast can read. The App IS registered — this is a failed check, not a", - "failed registration — but the check is the point: an App that cannot see", - "the repo fails at `cast apply` time instead, hours later and somewhere", - "else.", - "", - "Open Coolify's Sources page and confirm the App lists the repository, or", - "re-run this verification with `cast github-app register` once the install", - "is fixed.", - ].join("\n"), - ); - } - if (!repositories.includes(opts.orgRepo)) { - throw new Error( - [ - `${opts.name} is registered but cannot see ${opts.orgRepo}`, - "", - ` can see: ${repositories.join(", ") || "(no repositories at all)"}`, - "", - "The App exists on GitHub and in Coolify; it is INSTALLED on the wrong", - "repositories (or on none). Open", - " https://github.com/settings/installations", - "or the org's Settings → GitHub Apps, grant the App access to", - `${opts.orgRepo}, and re-run \`cast github-app register\` to re-check.`, - "", - "Left unfixed this surfaces at `cast apply` time as an unresolvable source.", - ].join("\n"), - ); - } - log(`verified: ${opts.name} can clone ${opts.orgRepo} ✓`); - - return { coolifyAppId, keyUuid, repositories, pemPath, secretsPath }; + return { coolifyAppId, keyUuid }; } // --------------------------------------------------------------------------- @@ -883,7 +1176,7 @@ export async function detectOwnerType( try { const res = await fetchImpl( `https://api.github.com/users/${encodeURIComponent(owner)}`, - { headers: { Accept: "application/vnd.github+json" } }, + { headers: githubHeaders() }, ); if (!res.ok) return undefined; const raw = (await res.json()) as Record; @@ -950,6 +1243,15 @@ export async function createGithubApp(opts: { const log = deps.log ?? ((line: string) => console.log(line)); const org = opts.orgRepo.split("/")[0] ?? opts.orgRepo; + // Before ANY of it — before the browser, before GitHub mints anything. A + // name collision discovered here costs nothing; discovered after the + // conversion it costs the private key of an App that now exists. + preflightCredentialSlot({ + stateDir: opts.stateDir, + name: opts.name, + force: opts.force, + }); + const ownerType = opts.ownerType ?? (await detectOwnerType(org, deps.fetchImpl)) ?? @@ -1017,6 +1319,44 @@ export async function createGithubApp(opts: { `created GitHub App: ${conversion.slug} (github app id ${conversion.id})`, ); + // PERSIST NOW — the blocker all three reviewers raised on #124. + // + // The conversion response is the only moment GitHub ever yields the private + // key, the client secret and the webhook secret. Everything after this line + // can fail for ordinary reasons and for a long time: the install poll runs + // ~5 minutes, the operator can wander off, the network can drop, Ctrl-C is + // one keystroke. Holding the one-shot payload in memory across all of that + // and only writing it inside registerGithubApp meant any of those events + // destroyed a credential GitHub will not reissue — and left the App itself + // orphaned on GitHub, needing manual deletion. + // + // So the record goes to disk here, complete but for the installation id, + // which is the ONE field GitHub will answer again as often as asked. It is + // backfilled below once the install lands. + const webhookSecret = conversion.webhookSecret ?? generateWebhookSecret(); + if (conversion.webhookSecret === null) { + log( + "github returned no webhook secret; generated one (webhook is inactive)", + ); + } + const pending: PendingAppCredentials = { + appId: conversion.id, + clientId: conversion.clientId, + clientSecret: conversion.clientSecret, + webhookSecret, + privateKeyPem: conversion.pem, + }; + const saved = persistCredentials({ + stateDir: opts.stateDir, + name: opts.name, + creds: pending, + org: conversion.ownerLogin, + orgRepo: opts.orgRepo, + force: opts.force, + }); + log(`private key → ${saved.pemPath}`); + log(`credentials → ${saved.secretsPath} (installation id pending)`); + // Step 6 — install it. Always print the URL; never assume an opener. const installUrl = `https://github.com/apps/${conversion.slug}/installations/new`; log(""); @@ -1027,28 +1367,58 @@ export async function createGithubApp(opts: { // Step 7 — recover the installation id from the App's own key, never from a // redirect parameter. - const installationId = await awaitInstallationId({ - owner: conversion.ownerLogin, - ownerType: conversion.ownerType, - privateKeyPem: conversion.pem, - clientId: conversion.clientId, - fetchImpl: deps.fetchImpl, - sleep: deps.sleep, - now: deps.now, - attempts: deps.installAttempts, - intervalMs: deps.installIntervalMs, - log, - }); + let installationId: number; + try { + installationId = await awaitInstallationId({ + owner: conversion.ownerLogin, + ownerType: conversion.ownerType, + privateKeyPem: conversion.pem, + clientId: conversion.clientId, + fetchImpl: deps.fetchImpl, + sleep: deps.sleep, + now: deps.now, + attempts: deps.installAttempts, + intervalMs: deps.installIntervalMs, + log, + }); + } catch (err) { + // Now the "nothing has to be recreated" claim is TRUE, and it can name the + // actual files rather than a directory shape. Attached here because this is + // the only scope that knows where the persist above landed. + if (err instanceof InstallationNeverArrivedError) { + throw new Error( + [ + err.message, + "", + "Nothing is lost. cast saved everything GitHub shows only once, before", + "it started waiting:", + "", + ` ${saved.pemPath}`, + ` ${saved.secretsPath}`, + "", + "Install the App from the URL above, then finish with:", + "", + ` cast github-app register ${opts.orgRepo} --env \\`, + ` --app-id ${conversion.id} --installation-id \\`, + ` --client-id ${conversion.clientId} \\`, + ` --private-key ${saved.pemPath} --client-secret-stdin`, + "", + `The client secret and webhook secret are in ${saved.secretsPath};`, + "the installation id is on the install's own URL, or read it from", + "https://github.com/settings/installations.", + "", + "Do NOT re-run `create`: the App already exists on GitHub, and creating", + "a second one is the thing this message exists to prevent.", + ].join("\n"), + ); + } + throw err; + } log(`installation id ${installationId} (recovered via the App JWT) ✓`); - const webhookSecret = conversion.webhookSecret ?? generateWebhookSecret(); - if (conversion.webhookSecret === null) { - log( - "github returned no webhook secret; generated one (webhook is inactive)", - ); - } - - // Steps 8 + 9 — the fall-through. Identical to what `register` calls. + // Steps 8 + 9 — the fall-through. Identical to what `register` calls; the + // persist inside it backfills the installation id onto the record written + // above rather than writing a second one. return registerGithubApp({ client: opts.client, name: opts.name, @@ -1057,13 +1427,6 @@ export async function createGithubApp(opts: { stateDir: opts.stateDir, force: opts.force, log, - creds: { - appId: conversion.id, - installationId, - clientId: conversion.clientId, - clientSecret: conversion.clientSecret, - webhookSecret, - privateKeyPem: conversion.pem, - }, + creds: { ...pending, installationId }, }); } diff --git a/test/github-app-register-cli.test.ts b/test/github-app-register-cli.test.ts index 133c90f..7dbc455 100644 --- a/test/github-app-register-cli.test.ts +++ b/test/github-app-register-cli.test.ts @@ -56,6 +56,10 @@ async function stubCoolify(opts: { repositories: unknown }): Promise { if (path === "/security/keys") return json({ uuid: "key-uuid-1" }); if (path === "/github-apps" && req.method === "POST") return json({ id: 7, uuid: "app-uuid" }); + // A clean instance: nothing registered under this name yet, so register + // goes on to create. (The list read is how it avoids a duplicate Source + // on a re-run — Coolify does not enforce unique names.) + if (path === "/github-apps" && req.method === "GET") return json([]); if (path === "/github-apps/7/repositories") return json({ repositories: opts.repositories }); res.writeHead(404); diff --git a/test/github-app.test.ts b/test/github-app.test.ts index c2b809d..fa6d1e5 100644 --- a/test/github-app.test.ts +++ b/test/github-app.test.ts @@ -13,16 +13,20 @@ import { loadBindings } from "../src/bindings.js"; import { CoolifyClient } from "../src/coolify.js"; import { type AppCredentials, + type PendingAppCredentials, awaitInstallationId, buildManifest, convertManifestCode, createGithubApp, detectOwnerType, findInstallationId, + findRegisteredApp, + githubUserAgent, manifestFormPage, mintAppJwt, newAppFormAction, persistCredentials, + preflightCredentialSlot, preflightOrgAdmin, readAppRepositories, registerGithubApp, @@ -658,6 +662,7 @@ describe("registering with Coolify, and the post-condition that matters", () => it("uploads the key, creates the App, and PROVES it can reach the repo", async () => { const c = coolify({ + "GET /github-apps": ok([{ id: 4, name: "something-else" }]), "POST /security/keys": ok({ uuid: "key-uuid-1" }), "POST /github-apps": ok({ id: 7, uuid: "app-uuid" }), "GET /github-apps/7/repositories": ok({ @@ -678,6 +683,7 @@ describe("registering with Coolify, and the post-condition that matters", () => }); expect(c.hits).toEqual([ + "GET /github-apps", "POST /security/keys", "POST /github-apps", "GET /github-apps/7/repositories", @@ -787,6 +793,214 @@ describe("registering with Coolify, and the post-condition that matters", () => }); }); +// grok #2, and the reason it is a real bug rather than a hypothetical: Coolify +// does not enforce unique Source names. Its GithubController@create validates +// `'name' => 'required|string|max:255'` — no `unique` — then calls a plain +// `GithubApp::create()`. So the old "re-run register to re-check" advice +// created a second Source every time it was followed. +describe("re-running `register` re-verifies instead of registering twice", () => { + const ok = (payload: unknown) => () => [200, payload] as [number, unknown]; + + it("reuses an existing Source of the same name and POSTs NOTHING", async () => { + const c = coolify({ + "GET /github-apps": ok([ + { id: 4, name: "other-app", app_id: 1 }, + { id: 7, name: "hdb-coolify-prod", app_id: 12345 }, + ]), + "GET /github-apps/7/repositories": ok([ + { full_name: "heavy-duty/incubator" }, + ]), + }); + const out = await registerGithubApp({ + client: c.client, + name: "hdb-coolify-prod", + org: "heavy-duty", + orgRepo: "heavy-duty/incubator", + creds: creds(), + stateDir: tmp("cast-state-"), + log: () => {}, + }); + // The verify-only path: the list, then the check. No key upload, no App + // create — following the error message's own advice is now free. + expect(c.hits).toEqual([ + "GET /github-apps", + "GET /github-apps/7/repositories", + ]); + expect(out.coolifyAppId).toBe(7); + expect(out.keyUuid).toBeNull(); + }); + + it("refuses when the name is taken by a DIFFERENT App rather than shadowing it", async () => { + const c = coolify({ + "GET /github-apps": ok([ + { id: 7, name: "hdb-coolify-prod", app_id: 999999 }, + ]), + }); + await expect( + registerGithubApp({ + client: c.client, + name: "hdb-coolify-prod", + org: "heavy-duty", + orgRepo: "heavy-duty/incubator", + creds: creds(), + stateDir: tmp("cast-state-"), + log: () => {}, + }), + ).rejects.toThrow(/DIFFERENT App[\s\S]*github app id 999999/); + expect(c.hits).toEqual(["GET /github-apps"]); + }); + + it("refuses when duplicates ALREADY exist, because cast cannot pick one", async () => { + const c = coolify({ + "GET /github-apps": ok([ + { id: 7, name: "dup", app_id: 12345 }, + { id: 8, name: "dup", app_id: 12345 }, + ]), + }); + await expect( + registerGithubApp({ + client: c.client, + name: "dup", + org: "o", + orgRepo: "o/r", + creds: creds(), + stateDir: tmp("cast-state-"), + log: () => {}, + }), + ).rejects.toThrow(/2 GitHub App records named dup[\s\S]*coolify ids: 7, 8/); + }); + + it("warns and proceeds when the list is unreadable — a bootstrap must not be blocked by a check", async () => { + const lines: string[] = []; + const c = coolify({ + // No "GET /github-apps" route: the list 404s. + "POST /security/keys": ok({ uuid: "k" }), + "POST /github-apps": ok({ id: 3 }), + "GET /github-apps/3/repositories": ok([{ full_name: "o/r" }]), + }); + const out = await registerGithubApp({ + client: c.client, + name: "n", + org: "o", + orgRepo: "o/r", + creds: creds(), + stateDir: tmp("cast-state-"), + log: (l) => lines.push(l), + }); + expect(out.coolifyAppId).toBe(3); + // Unreadable is not "empty" — it is said out loud, not assumed away. + expect(lines.join("\n")).toContain("could not list existing Coolify"); + }); + + it("reads names off the list and ignores everything else", async () => { + const c = coolify({ + "GET /github-apps": ok([ + { id: 1, name: "a" }, + "not an object", + { name: "wanted-but-no-id" }, + { id: 2, name: "wanted", app_id: 5 }, + ]), + }); + expect(await findRegisteredApp(c.client, "wanted")).toEqual([ + { id: 2, appId: 5 }, + ]); + expect(await findRegisteredApp(c.client, "absent")).toEqual([]); + }); +}); + +// grok #3. Not a security boundary — the name is the operator's own — but a +// slash in it silently nests the credentials somewhere nobody will look, and +// `..` walks clean out of the state directory. +describe("the App name has to be usable as a filename", () => { + it("rejects separators, dot-references and empties before they become paths", () => { + for (const bad of ["", "a/b", "a\\b", "..", ".", "../escape", ".hidden"]) { + expect(() => + persistCredentials({ + stateDir: tmp("cast-state-"), + name: bad, + creds: creds(), + org: "o", + orgRepo: "o/r", + }), + ).toThrow(/github app name/); + } + expect(() => + preflightCredentialSlot({ stateDir: tmp("cast-state-"), name: "a/b" }), + ).toThrow(/contains a path separator/); + // Ordinary names stay ordinary. + expect(() => + persistCredentials({ + stateDir: tmp("cast-state-"), + name: "hdb-coolify-prod", + creds: creds(), + org: "o", + orgRepo: "o/r", + }), + ).not.toThrow(); + }); + + it("catches it at name resolution too, so a bad --name never reaches a network", () => { + const empty = loadBindings("", { + overrideText: [ + "environments:", + " prod:", + " server: box", + " team: { id: 0, name: Root Team }", + "github_apps: {}", + "", + ].join("\n"), + }); + expect(() => + resolveAppName({ + bindings: empty, + orgRepo: "heavy-duty/incubator", + nameFlag: "../oops", + }), + ).toThrow(/github app name/); + // And a hand-edited environments.yaml entry gets the same treatment: it + // becomes a filename by exactly the same route. + const bad = loadBindings("", { + overrideText: [ + "environments:", + " prod:", + " server: box", + " team: { id: 0, name: Root Team }", + 'github_apps: { "heavy-duty/incubator": "../oops" }', + "", + ].join("\n"), + }); + expect(() => + resolveAppName({ bindings: bad, orgRepo: "heavy-duty/incubator" }), + ).toThrow(/github app name/); + }); +}); + +// grok #4. +describe("cast identifies itself to GitHub", () => { + it("sends a User-Agent, because GitHub asks for one and 403s look like nothing else", async () => { + expect(githubUserAgent()).toMatch(/^cast\//); + const seen: Record[] = []; + const fetchImpl = vi.fn(async (_url: string | URL, init?: RequestInit) => { + seen.push((init?.headers ?? {}) as Record); + return new Response(JSON.stringify({ type: "Organization" }), { + status: 200, + }); + }) as unknown as typeof fetch; + await detectOwnerType("heavy-duty", fetchImpl); + await findInstallationId({ + owner: "heavy-duty", + ownerType: "Organization", + jwt: "jwt", + fetchImpl, + }).catch(() => {}); + await convertManifestCode("code", fetchImpl).catch(() => {}); + expect(seen.length).toBe(3); + for (const headers of seen) { + expect(headers["User-Agent"]).toBe(githubUserAgent()); + } + }); +}); + describe("the optional org-admin preflight", () => { it("passes on admin and refuses on anything else", () => { expect(preflightOrgAdmin("heavy-duty", () => '{"role":"admin"}')).toEqual({ @@ -845,6 +1059,7 @@ describe("`create` falls through into `register` — one implementation, not two }) as unknown as typeof fetch; const c = coolify({ + "GET /github-apps": () => [200, []], "POST /security/keys": () => [200, { uuid: "key-uuid-1" }], "POST /github-apps": () => [200, { id: 11 }], "GET /github-apps/11/repositories": () => [ @@ -887,9 +1102,10 @@ describe("`create` falls through into `register` — one implementation, not two }); const out = await flow; - // The fall-through, asserted as an identity of behaviour: the same three + // The fall-through, asserted as an identity of behaviour: the same four // calls, in the same order, that the `register`-only test above pins. expect(c.hits).toEqual([ + "GET /github-apps", "POST /security/keys", "POST /github-apps", "GET /github-apps/11/repositories", @@ -908,6 +1124,181 @@ describe("`create` falls through into `register` — one implementation, not two ).toBe(privateKeyPem); }); + // The blocker all three reviewers raised on #124, pinned. Conversion + // SUCCEEDS — GitHub has minted the App and shown the private key for the only + // time it ever will — and then the install poll fails for every attempt. The + // old order held that payload in memory across the whole poll and wrote it + // only inside registerGithubApp, so this scenario destroyed it. + it("keeps the one-shot PEM and client secret when the install NEVER lands", async () => { + const conversion = { + id: 424242, + slug: "hdb-coolify-prod", + client_id: "Iv23liXYZ", + client_secret: "github-issued-secret", + webhook_secret: "github-issued-webhook", + pem: privateKeyPem, + owner: { login: "heavy-duty", type: "Organization" }, + }; + const githubFetch = vi.fn(async (url: string | URL) => { + const u = String(url); + if (u.endsWith("/conversions")) + return new Response(JSON.stringify(conversion), { status: 200 }); + // Never installed. 404 on every single attempt, which is the state the + // poll is designed to wait out and eventually give up on. + if (u.includes("/installation")) + return new Response("{}", { status: 404 }); + return new Response(JSON.stringify({ type: "Organization" }), { + status: 200, + }); + }) as unknown as typeof fetch; + + const c = coolify({}); + const state = tmp("cast-state-"); + const err = await createGithubApp({ + client: c.client, + orgRepo: "heavy-duty/incubator", + name: "hdb-coolify-prod", + stateDir: state, + port: 0, + deps: { + fetchImpl: githubFetch, + openUrl: (url) => { + if (url.startsWith("http://127.0.0.1")) { + const u = new URL(url); + fetch(url) + .then((r) => r.text()) + .then((page) => { + const s = /state=([^"&]+)/.exec(page)?.[1] ?? ""; + return fetch(`${u.origin}/callback?code=the-code&state=${s}`); + }); + } + return false; + }, + sleep: async () => {}, + runGh: () => '{"role":"admin"}', + log: () => {}, + installAttempts: 3, + installIntervalMs: 1, + }, + }).then( + () => undefined, + (e: Error) => e, + ); + + expect(err).toBeDefined(); + // 1. The secrets GitHub shows exactly once are ON DISK. + const pem = join(state, "github-apps", "hdb-coolify-prod.pem"); + const json = join(state, "github-apps", "hdb-coolify-prod.json"); + expect(readFileSync(pem, "utf8")).toBe(privateKeyPem); + const saved = JSON.parse(readFileSync(json, "utf8")); + expect(saved.client_secret).toBe("github-issued-secret"); + expect(saved.webhook_secret).toBe("github-issued-webhook"); + expect(saved.app_id).toBe(424242); + // The one field that is legitimately unknown, and the only one GitHub will + // answer again as many times as it is asked. + expect(saved.installation_id).toBeNull(); + + // 2. The remedy MATCHES REALITY — it names the files that exist and the + // command that finishes the job, and it does not claim credentials are + // saved somewhere they are not. + const message = (err as Error).message; + expect(message).toContain("Nothing is lost"); + expect(message).toContain(pem); + expect(message).toContain(json); + expect(message).toContain("cast github-app register"); + expect(message).toContain("--app-id 424242"); + expect(message).toContain("Do NOT re-run `create`"); + + // 3. Nothing was registered with Coolify, so there is no half-record to + // reconcile — only an App on GitHub awaiting its install. + expect(c.hits).toEqual([]); + }); + + it("backfills the installation id onto the pending record rather than refusing itself", async () => { + const state = tmp("cast-state-"); + const pending: PendingAppCredentials = { + appId: 12345, + clientId: "Iv23liABCDEF", + clientSecret: "cs-secret", + webhookSecret: "wh-secret", + privateKeyPem: privateKeyPem ?? "PEM", + }; + const args = { stateDir: state, name: "app", org: "o", orgRepo: "o/r" }; + const { secretsPath } = persistCredentials({ ...args, creds: pending }); + expect(JSON.parse(readFileSync(secretsPath, "utf8")).installation_id).toBe( + null, + ); + + // The completion `create` performs once the install lands. This is the ONE + // transition allowed without --force, because nothing irreplaceable moves. + persistCredentials({ + ...args, + creds: { ...pending, installationId: 5150 }, + }); + expect(JSON.parse(readFileSync(secretsPath, "utf8")).installation_id).toBe( + 5150, + ); + + // And it really is only that one field: a different client secret arriving + // alongside a filled-in installation id is still a refusal. + expect(() => + persistCredentials({ + ...args, + creds: { ...pending, installationId: 5150, clientSecret: "other" }, + }), + ).toThrow(/refusing to overwrite/); + // Nor does a KNOWN installation id get quietly replaced by a different one. + expect(() => + persistCredentials({ + ...args, + creds: { ...pending, installationId: 6000 }, + }), + ).toThrow(/refusing to overwrite/); + }); + + // claude-bot's addition: the post-conversion persist must never be the thing + // that throws, because at that moment it is holding the only copy of the key. + it("refuses a name collision BEFORE the browser flow, when nothing can be lost", async () => { + const state = tmp("cast-state-"); + persistCredentials({ + stateDir: state, + name: "hdb-coolify-prod", + creds: creds({ privateKeyPem: "an older App's key" }), + org: "heavy-duty", + orgRepo: "heavy-duty/incubator", + }); + + const c = coolify({}); + const githubFetch = vi.fn(async () => { + throw new Error("GitHub must not be reached"); + }) as unknown as typeof fetch; + + await expect( + createGithubApp({ + client: c.client, + orgRepo: "heavy-duty/incubator", + name: "hdb-coolify-prod", + stateDir: state, + port: 0, + ownerType: "Organization", + deps: { + fetchImpl: githubFetch, + runGh: () => '{"role":"admin"}', + log: () => {}, + openUrl: () => false, + }, + }), + ).rejects.toThrow(/already has credentials on disk[\s\S]*register/); + + // No browser flow, no App minted, no Coolify call — the whole point of + // checking now instead of after the conversion. + expect(c.hits).toEqual([]); + // And the older key is untouched. + expect( + readFileSync(join(state, "github-apps", "hdb-coolify-prod.pem"), "utf8"), + ).toBe("an older App's key"); + }); + it("refuses before the browser dance when gh says you are not an org admin", async () => { const c = coolify({}); await expect(