From a9805d31bd7da601ad95e85c664c9e5526a16bfa Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 10:46:43 +0000 Subject: [PATCH 1/4] =?UTF-8?q?feat:=20cast=20github-app=20create/register?= =?UTF-8?q?=20=E2=80=94=20run=20the=20App=20Manifest=20flow=20instead=20of?= =?UTF-8?q?=20transcribing=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub App was the one piece of a Coolify instance cast could not reproduce. There is no REST endpoint that creates one — no POST /apps, no GraphQL mutation, no `gh app` subcommand, no PAT scope — so `create` runs the only programmatic path there is: GitHub's App Manifest flow, a one-shot page served on 127.0.0.1 whose form POST the operator's own browser session authenticates, followed by an unauthenticated code exchange. That exchange is the only moment GitHub yields the private key, the client secret and the webhook secret together; all three are persisted to /github-apps/ at 0600 under a .gitignore of `*`. `create` does not reimplement `register`: it obtains credentials and then calls exactly that path. Both verbs end at GET /github-apps/{id}/repositories, asserting the repo is actually reachable — the check that turns a silent misconfiguration into an error next to the thing that caused it. github_apps./ in environments.yaml (--name only seeds an absent entry, and is refused when it disagrees), the client secret is stdin-only, and --webhook-secret is optional. scripts/register-github-app.sh is deleted. No new dependencies: node:http for the callback, node:crypto's createSign("RSA-SHA256") for the App JWT that recovers the installation id from the App's own key rather than from a spoofable redirect parameter. Closes #7 Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 54 ++ README.md | 129 +++- scripts/register-github-app.sh | 36 - src/cli.ts | 172 +++++ src/github-app.ts | 1069 ++++++++++++++++++++++++++ test/github-app-register-cli.test.ts | 280 +++++++ test/github-app.test.ts | 931 ++++++++++++++++++++++ 7 files changed, 2633 insertions(+), 38 deletions(-) delete mode 100755 scripts/register-github-app.sh create mode 100644 src/github-app.ts create mode 100644 test/github-app-register-cli.test.ts create mode 100644 test/github-app.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f04eff..64a48ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,53 @@ actually cutting it, and this file starts there. what it was told to write. Basic-auth-only is the safe slice until then. +- **`cast github-app create` / `cast github-app register`** (#7, superseding + #5) — the GitHub App was the one piece of a Coolify instance cast could + not reproduce: made by hand in a browser, its four identifiers copied out + of the UI by eye, its private key downloaded to `~/Downloads`, its details + fed to `scripts/register-github-app.sh` as a six-variable env pile. + Nothing about that survived in state. There is no REST endpoint that + creates a GitHub App — no `POST /apps`, no GraphQL mutation, no `gh app` + subcommand, no PAT scope — so `create` runs the only programmatic path + there is, GitHub's App Manifest flow: a one-shot page served on + `127.0.0.1` whose form POST your own browser session authenticates, + followed by an unauthenticated code exchange. That exchange is the only + moment GitHub ever yields the private key, the client secret and the + webhook secret together, and cast now persists all three to + `/github-apps/` at 0600 under a `.gitignore` of `*`, so `git add + -A` in the state repo cannot commit them by accident. `register` adopts + credentials you already hold (an App made by hand, or a DR restore from a + stored PEM) and reads the client secret from **stdin only** — argv is + visible in `ps`. `create` does not reimplement it: it obtains credentials + and then calls exactly the `register` path. + + #5's three footguns are gone structurally rather than by validation. The + Coolify-facing name is resolved from `github_apps./` in + `environments.yaml` — the value every later `cast apply` resolves the App + by — and `--name` only *seeds* an absent entry (keyed by full slug, #6), + and is refused outright when it disagrees with one that exists. The state + file is written only after the App is registered *and* verified, because a + state file naming an App that does not work is worse than one naming none. + `--webhook-secret` is optional: a webhook-inactive App is the correct shape + for a tailnet-only Coolify, and nobody has to invent a placeholder. + + 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. + + No new dependencies — `node:http` serves the callback, `node:crypto`'s + `createSign("RSA-SHA256")` mints the App JWT that recovers the installation + id from the App's own key (never from the `installation_id` GitHub appends + to a redirect, which GitHub documents as a spoofable hint). + + **Unvalidated, and load-bearing**: that GitHub accepts a `redirect_url` on + `http://127.0.0.1:` at all. The manifest docs are silent on the + scheme, the precedent (Probot's setup flow) is strong, and validating it + requires a logged-in GitHub session — so the first real run is an + operator's, and README keeps the manual browser path documented until + `create` has succeeded once. + ### Changed - **`state:needs-human` no longer waits on the cron to become true** (#131) @@ -161,6 +208,13 @@ actually cutting it, and this file starts there. block, because the password cannot be read and a block a rebuild cannot honour is exactly the failure `UNCAPTURED.md` exists to prevent. +### Removed + +- **`scripts/register-github-app.sh`** — replaced by `cast github-app + register`. Kept as a thin wrapper it would have preserved exactly the + interface #5 catalogued as producing three live footguns, while adding a + second surface to keep in step with the CLI. + ### Fixed - **A PR that deletes a shipped release heading is now CI-red** (#133, diff --git a/README.md b/README.md index d81f5fb..a4d7048 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,9 @@ cast inventory / --env cast inventory --env [--emit-draft [--recipient age1…] [--no-secrets]] cast destroy / --env [--instance ] [--with-project] cast server add --ip --key --env [--user root] [--port 22] +cast github-app create / --env [--name ] [--port 8765] +cast github-app register / --env --app-id --installation-id \ + --client-id --client-secret-stdin --private-key cast smoke / --env [--project ] [--environment ] cast team [--env ] ``` @@ -166,6 +169,12 @@ cast team [--env ] environment's name at a plan that says, for every database, whether it is backed up and when the last backup landed. See *Tearing an environment down* below. - **`server add`** — uploads a server's private key and registers it with Coolify. +- **`github-app create`** — creates the GitHub App Coolify clones private repos + with, by running GitHub's App Manifest flow, then registers it. Two browser + clicks, zero transcription. See *The GitHub App* below. +- **`github-app register`** — adopts an App you already hold: one created by hand, + or a disaster-recovery restore from a stored private key. `create` ends by + running exactly this. - **`smoke`** — contract test against the project's `smoke_target`: proves Coolify's bulk env endpoint still *upserts* rather than replacing. Run it after every Coolify upgrade — `apply`'s never-delete guarantee rests on that behavior, @@ -208,6 +217,121 @@ no credentials at all it says so, and names the fix. The token is never put in the clone URL or in `http.extraheader` — both leak it into `ps`, and the latter persists it into the clone's git config. +## The GitHub App: `cast github-app` + +That is how *cast* clones. **Coolify** clones with a GitHub App, and the App used +to be the one piece of a Coolify instance cast could not reproduce: created by +hand in a browser, its four identifiers copied out of the UI by eye, its private +key downloaded to `~/Downloads`, its details fed to a shell script as a +six-variable env pile. Nothing about that survived in state. Rebuild the instance +and you redid the hoops from memory. + +```sh +cast github-app create heavy-duty/incubator --env prod --name hdb-coolify-prod +``` + +There is **no REST endpoint that creates a GitHub App** — no `POST /apps`, no +GraphQL mutation, no `gh app` subcommand, and no PAT scope that unlocks one. The +only programmatic path is GitHub's [App Manifest +flow](https://docs.github.com/en/apps/sharing-github-apps/registering-a-github-app-from-a-manifest): +a browser form POST whose authentication is your existing GitHub session, +followed by an unauthenticated code exchange. It is how Coolify's own *Create +GitHub App* button works, and it is why this command serves you a page instead of +calling an API. + +What `create` does: + +1. If `gh` is on `PATH` and authenticated, checks you are an **admin** of the org + — so you learn you cannot create Apps there *before* the browser dance, not + after. `gh` is never required; an absent one skips the check silently. +2. Resolves the App's Coolify-facing name from **`github_apps./` in + `environments.yaml`**, which is what every later `cast apply` resolves this + repo's App by. `--name` seeds that entry when it is absent and is **refused** + when it disagrees with one that exists. +3. Serves a one-shot page on `127.0.0.1` that submits an App manifest — + `contents: read` + `metadata: read`, webhook inactive, private. +4. You click *Create GitHub App*; GitHub redirects back to the loopback server, + 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 — + 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. + +`register` is the same command from step 8 onwards, for an App you already hold — +one made by hand, or a disaster-recovery restore from a stored PEM: + +```sh +pbpaste | cast github-app register heavy-duty/incubator --env prod \ + --app-id 12345 --installation-id 99887766 --client-id Iv23li… \ + --client-secret-stdin --private-key ~/Downloads/app.private-key.pem +``` + +The client secret is read from **stdin only** — argv is visible in `ps` and kept +in shell history. `--webhook-secret` is optional: a webhook-**inactive** App is +the right shape for a tailnet-only Coolify where deliveries can never arrive and +deploys are CI-triggered, and cast generates a value rather than making you +invent one. + +### Where the credentials land + +Into the state directory you point cast at — cast itself stores nothing: + +``` +/github-apps/ +├── .gitignore # `*` — written by cast +├── .pem # 0600, the private key +└── .json # 0600, app id, installation id, client id + secret, webhook secret +``` + +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/`: +that store holds per-repo-per-env *application* env vars, whose whole purpose is +to be decrypted and injected into the running container, which is the last place +an App private key belongs — and its age identity may not exist on the machine +doing the bootstrap at all. Encrypting the one credential that makes recovery +possible behind a key that might not be there is how DR fails at the moment it is +needed. + +So the guard is structural rather than cryptographic: the `.gitignore` means +`git add -A` in your state repo cannot commit these by accident. Committing them +stays possible and has to be deliberate — encrypt them yourself and commit the +ciphertext, or keep the directory out of the repo and back it up somewhere that +is not a git remote. + +### Until it has worked once + +`create`'s design rests on GitHub accepting a `redirect_url` on +`http://127.0.0.1:`. The manifest docs are silent on the scheme (loopback +HTTP is documented for *OAuth* redirect URIs), and the precedent is strong — +Probot's setup flow does exactly this — but it is unvalidated, because validating +it needs a logged-in GitHub session. **The manual path below stays supported +until `create` has succeeded against a real GitHub once.** If it fails, create +the App by hand in the browser and use `github-app register`, which does not +depend on the assumption at all. + +
+The manual path + +1. Org → Settings → Developer settings → GitHub Apps → **New GitHub App**. + Permissions: **Contents: Read-only**, **Metadata: Read-only**. Uncheck + *Active* under Webhook. Uncheck *Any account* (keep it private). +2. Note the **App ID** and **Client ID**; generate a **client secret**; generate + and download a **private key**. +3. **Install App** → pick the repository. The installation id is the last path + segment of the URL you land on (`…/settings/installations/`). +4. Feed all of it to `cast github-app register` (above), which validates the name + against state and verifies the repo is reachable. + +
+ ## Many Coolifys `--instance ` reads `/.coolify/.env` instead of @@ -1003,8 +1127,9 @@ the way back to zero from a half-applied first run. ## Scripts -Operational helpers, all argument-driven (`scripts/`): register a GitHub App with -Coolify, restore a database backup into a target container. +Operational helpers, all argument-driven (`scripts/`): restore a database backup +into a target container. (`register-github-app.sh` is gone — it is +`cast github-app register` now.) **They run where cast runs — off the box.** They drive the Coolify API, or reach a box over SSH; none of them expects to be executing *on* a server. Anything that diff --git a/scripts/register-github-app.sh b/scripts/register-github-app.sh deleted file mode 100755 index 59c0892..0000000 --- a/scripts/register-github-app.sh +++ /dev/null @@ -1,36 +0,0 @@ -#!/usr/bin/env bash -# Register a GitHub App (created via the manifest flow on your org) with a -# Coolify instance, so cast can create applications from private repos. -# -# usage: CAST_STATE=~/infra ./register-github-app.sh -# -# Takes everything as input — the app's identity is yours, not this tool's. -# APP_NAME must match the `github_apps.` value in your environments.yaml: -# that is the name cast resolves when it creates an application. -set -euo pipefail - -STATE="${CAST_STATE:-.}" -# shellcheck disable=SC1091 -source "${STATE}/.coolify.env" - -: "${APP_NAME:?the Coolify-facing GitHub App name (must match github_apps. in environments.yaml)}" -: "${ORG:?the GitHub org or user the App is installed on}" -: "${APP_ID:?}"; : "${INSTALLATION_ID:?}"; : "${CLIENT_ID:?}"; : "${CLIENT_SECRET:?}" -: "${WEBHOOK_SECRET:?}"; : "${PRIVATE_KEY_FILE:?path to the App private key PEM}" - -api() { curl -fsS -H "Authorization: Bearer ${COOLIFY_ACCESS_TOKEN}" -H "Content-Type: application/json" "$@"; } - -KEY_UUID=$(api -X POST "${COOLIFY_BASE_URL}/api/v1/security/keys" \ - -d "$(jq -n --arg name "${APP_NAME}-key" --rawfile pk "$PRIVATE_KEY_FILE" \ - '{name:$name, private_key:$pk}')" | jq -r .uuid) - -api -X POST "${COOLIFY_BASE_URL}/api/v1/github-apps" -d "$(jq -n \ - --arg name "$APP_NAME" --arg org "$ORG" \ - --arg app_id "$APP_ID" --arg inst "$INSTALLATION_ID" --arg cid "$CLIENT_ID" \ - --arg csec "$CLIENT_SECRET" --arg wh "$WEBHOOK_SECRET" --arg key "$KEY_UUID" \ - --arg api_url "https://api.github.com" --arg html_url "https://github.com" \ - '{name:$name, organization:$org, api_url:$api_url, html_url:$html_url, - app_id:($app_id|tonumber), installation_id:($inst|tonumber), client_id:$cid, client_secret:$csec, - webhook_secret:$wh, private_key_uuid:$key}')" - -echo "github app registered as ${APP_NAME}" diff --git a/src/cli.ts b/src/cli.ts index cd915b2..6eb1f02 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -84,6 +84,13 @@ import { renderFleetDiff, renderProjectHeading, } from "./fleet.js"; +import { + createGithubApp, + generateWebhookSecret, + registerGithubApp, + resolveAppName, + seedGithubAppBinding, +} from "./github-app.js"; import { type LiveResource, type SweepEnvironment, @@ -124,6 +131,10 @@ const USAGE = `usage: cast apply / --env [--path ] [-- cast inventory --env --emit-draft [--recipient age1…] [--no-secrets] cast destroy / --env [--instance ] [--path ] [--with-project] cast server add --ip --key --env [--user root] [--port 22] + cast github-app create / --env [--name ] [--port 8765] [--force] + cast github-app register / --env --app-id --installation-id + --client-id --client-secret-stdin --private-key + [--webhook-secret ] [--name ] [--force] cast smoke / --env [--project ] [--environment ] cast team [--env ] cast versions # list installed versions @@ -179,6 +190,25 @@ const USAGE = `usage: cast apply / --env [--path ] [-- coordinate: --path, --project, --environment, --resource, --hostname-overlay. +github-app (the credential Coolify clones private repos with): + create runs GitHub's App Manifest flow — the ONLY programmatic way to make a + GitHub App — then falls through into exactly what \`register\` does. It + 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. + 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 + webhook-INACTIVE App is the right shape for a tailnet-only Coolify. + --name seeds \`github_apps./\` in environments.yaml when it is + ABSENT, and is refused when it disagrees with an entry that exists. + The state file is the authority: its value is what every later + \`cast apply\` resolves this repo's App by. + --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. + capture (adopt a hand-built instance into the age secret store): --generated force NAME to the \`pending-coolify-generated\` placeholder, for a manifest that has not declared generated_secrets yet. @@ -1423,6 +1453,145 @@ function formatVersion(): string { return `cast ${typeof version === "string" ? version : "unknown"} (${dirname(pkgPath)})`; } +// `--client-secret-stdin`, mirroring `docker login --password-stdin`: argv is +// visible in `ps` and lands in shell history, and a GitHub App client secret is +// shown by GitHub exactly once. +async function readAllStdin(): Promise { + const chunks: Buffer[] = []; + for await (const chunk of process.stdin) chunks.push(Buffer.from(chunk)); + return Buffer.concat(chunks).toString("utf8").trim(); +} + +// `cast github-app register|create` — one command surface, ONE registration +// implementation (see src/github-app.ts). Everything up to the point where +// credentials exist differs between the two verbs; everything from there on is +// registerGithubApp, which `create` calls rather than reimplements. +async function githubAppCommand(rest: string[]): Promise { + const verb = rest[0]; + if (verb !== "register" && verb !== "create") { + console.error(USAGE); + return 2; + } + const { values, positionals } = parseArgs({ + args: rest.slice(1), + allowPositionals: true, + options: { + state: { type: "string" }, + env: { type: "string" }, + instance: { type: "string" }, + name: { type: "string" }, + force: { type: "boolean" }, + // create + port: { type: "string" }, + // register + "app-id": { type: "string" }, + "installation-id": { type: "string" }, + "client-id": { type: "string" }, + "client-secret-stdin": { type: "boolean" }, + "private-key": { type: "string" }, + "webhook-secret": { type: "string" }, + }, + }); + const orgRepo = positionals[0]; + // --env is required for the same reason `server add` requires it: this + // writes to a live Coolify, and every write first asserts that the token + // belongs to the environment's declared team. + if (!orgRepo || !orgRepo.includes("/") || !values.env) { + console.error(USAGE); + return 2; + } + const stateDir = stateDirFrom(values.state); + const bindingsPath = join(stateDir, "environments.yaml"); + const bindings = loadBindings(bindingsPath); + const binding = bindings.environments[values.env]; + if (!binding) { + console.error(`environment ${values.env} not in environments.yaml`); + return 2; + } + + // Step 2, before anything reaches a network: the Coolify-facing name comes + // from state. See resolveAppName — this is #5's footgun 1, dissolved. + const { name, seed } = resolveAppName({ + bindings, + orgRepo, + nameFlag: values.name, + }); + + const { instance, client } = openCoolify(stateDir, values.instance, binding); + assertWritable(instance, `github-app ${verb}`); + const team = await assertTeam(client, binding.team, values.env); + console.log(`team ${formatTeam(team)} ✓`); + console.log( + `github app name: ${name}${seed ? " (from --name, not yet in environments.yaml)" : " (from environments.yaml)"}`, + ); + + const org = orgRepo.split("/")[0] ?? orgRepo; + if (verb === "create") { + await createGithubApp({ + client, + orgRepo, + name, + stateDir, + force: values.force, + port: values.port ? Number(values.port) : undefined, + }); + } else { + const appId = values["app-id"]; + const installationId = values["installation-id"]; + const clientId = values["client-id"]; + const privateKey = values["private-key"]; + if (!appId || !installationId || !clientId || !privateKey) { + console.error(USAGE); + return 2; + } + if (!values["client-secret-stdin"]) { + console.error( + "--client-secret-stdin is required: the client secret is read from stdin,\nnever from argv (which `ps` shows and shell history keeps).", + ); + return 2; + } + const clientSecret = await readAllStdin(); + if (!clientSecret) { + console.error("no client secret on stdin"); + return 2; + } + // #5's footgun 3: a webhook-inactive App is the right configuration for a + // tailnet-only Coolify, and the old script still demanded a secret for it. + const webhookSecret = values["webhook-secret"] ?? generateWebhookSecret(); + if (!values["webhook-secret"]) { + console.log( + "no --webhook-secret: generated one (fine for a webhook-inactive App)", + ); + } + await registerGithubApp({ + client, + name, + org, + orgRepo, + stateDir, + force: values.force, + creds: { + appId: Number(appId), + installationId: Number(installationId), + clientId, + clientSecret, + webhookSecret, + privateKeyPem: readFileSync(privateKey, "utf8"), + }, + }); + } + + // Only after the App is registered AND verified: a state file that names an + // App which does not work is worse than one that names none. + if (seed) { + seedGithubAppBinding(bindingsPath, orgRepo, name); + console.log( + `environments.yaml: github_apps["${orgRepo}"] = ${name} (added)`, + ); + } + return 0; +} + async function main(): Promise { const [command, ...rest] = process.argv.slice(2); if (command === "-h" || command === "--help" || command === "help") { @@ -2298,6 +2467,9 @@ async function main(): Promise { }); return 0; } + if (command === "github-app") { + return await githubAppCommand(rest); + } if (command === "smoke") { const { values, positionals } = parseArgs({ args: rest, diff --git a/src/github-app.ts b/src/github-app.ts new file mode 100644 index 0000000..5809bb1 --- /dev/null +++ b/src/github-app.ts @@ -0,0 +1,1069 @@ +import { execFileSync } from "node:child_process"; +import { createSign, randomBytes } from "node:crypto"; +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 { parseDocument } from "yaml"; +import type { Bindings } from "./bindings.js"; +import type { CoolifyClient } from "./coolify.js"; + +// The GitHub App is the one piece of a Coolify instance cast could not +// reproduce (#7). Everything else derives from the manifest plus the state +// directory; the App was created by hand in a browser, its four identifiers +// read off a web page by eye, its private key downloaded to ~/Downloads, and +// its details fed to a script as a six-variable env pile (#5). Nothing about +// that survived in state. +// +// There is no REST endpoint that creates a GitHub App — no `POST /apps`, no +// GraphQL mutation, no `gh app` subcommand. A PAT cannot mint one at any +// scope. The ONLY programmatic path is the App Manifest flow: a browser form +// POST authenticated by the operator's existing GitHub session, followed by an +// unauthenticated code exchange. That is how Coolify's own "Create GitHub App" +// button works, and it is why this module serves an HTML page instead of +// calling an API. +// +// The flow gives us more than the manual path did, not less: its conversion +// response is the ONLY moment GitHub ever hands over the private key, the +// client secret and the webhook secret together. Today those are scattered +// across a download folder and a browser tab. Here they arrive in one JSON +// body that cast can persist deliberately (see persistCredentials). + +// The manifest schema requires `hook_attributes.url` even when the webhook is +// inactive, so it gets a deliberately dead value rather than a plausible one. +// `.invalid` is reserved by RFC 2606 and can never resolve. +const DEAD_HOOK_URL = "https://example.invalid/unused"; + +// The loopback literal, never `localhost`. GitHub's OAuth guidance explicitly +// prefers the IP: `localhost` resolves through the host's name resolution, +// which is a thing other software on the machine can change. +const LOOPBACK = "127.0.0.1"; + +export const DEFAULT_CALLBACK_PORT = 8765; + +// GitHub requires an App JWT's lifetime to be at most 10 minutes and tolerates +// clock skew badly, so `iat` is backdated a minute (their own documented +// advice) and `exp` is set well inside the ceiling rather than at it: 480 + 60 +// = 540s of span, 60s of headroom. A JWT rejected for being one second too +// long is indistinguishable, from the operator's side, from a bad key. +const JWT_BACKDATE_SECONDS = 60; +const JWT_AHEAD_SECONDS = 480; + +export type OwnerType = "Organization" | "User"; + +// What GitHub returns from the manifest conversion — the only body in the +// whole flow that carries secrets. +export type ManifestConversion = { + id: number; + slug: string; + clientId: string; + clientSecret: string; + webhookSecret: string | null; + pem: string; + ownerLogin: string; + ownerType: OwnerType; +}; + +// Everything Coolify needs to clone with. `register` is handed these by the +// operator; `create` obtains them from GitHub. The two paths converge here and +// nowhere later — registerGithubApp is the single implementation. +export type AppCredentials = { + appId: number; + installationId: number; + clientId: string; + clientSecret: string; + webhookSecret: string; + privateKeyPem: string; +}; + +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; + repositories: string[]; + pemPath: string; + secretsPath: string; +}; + +// --------------------------------------------------------------------------- +// Step 2 — the name comes from state, not from a flag +// --------------------------------------------------------------------------- + +// #5's footgun 1: `APP_NAME` was a free-form env var that had to equal +// `github_apps.` in environments.yaml, and the script could not know when +// it didn't. The operator passed the App's GitHub display name instead of the +// state value; nothing caught it, and every later `cast apply` failed to +// resolve the source. +// +// Dissolved structurally rather than validated after the fact: the state file +// is the authority. `--name` may SEED an absent entry, and is a hard refusal +// when it disagrees with one that exists — because at that point one of the two +// is wrong and cast cannot know which. +export function resolveAppName(opts: { + bindings: Bindings; + orgRepo: string; + nameFlag?: string; +}): { name: string; seed: boolean } { + const repoShort = opts.orgRepo.split("/")[1] ?? opts.orgRepo; + const bound = + opts.bindings.github_apps[opts.orgRepo] ?? + opts.bindings.github_apps[repoShort]; + if (bound !== undefined) { + if (opts.nameFlag !== undefined && opts.nameFlag !== bound) { + throw new Error( + [ + `--name ${opts.nameFlag} disagrees with environments.yaml`, + "", + ` state says: github_apps["${opts.orgRepo}"] = ${bound}`, + ` --name says: ${opts.nameFlag}`, + "", + "The state file is the authority: that value is what every later", + "`cast apply` resolves this repo's App by. Registering under a different", + "name produces an App that exists and is never found. Drop --name to use", + `${bound}, or change environments.yaml first if the state value is wrong.`, + ].join("\n"), + ); + } + return { name: bound, seed: false }; + } + if (opts.nameFlag === undefined) { + throw new Error( + [ + `no GitHub App name for ${opts.orgRepo}`, + "", + ` looked for: github_apps["${opts.orgRepo}"], then github_apps["${repoShort}"]`, + ` bound repos: ${Object.keys(opts.bindings.github_apps).join(", ") || "(none)"}`, + "", + "This name is the one every later `cast apply` resolves the App by, so it", + "has to be decided here. Either add it to environments.yaml:", + "", + " github_apps:", + ` ${opts.orgRepo}: `, + "", + "or pass --name and cast will write that line for you once the App", + "is registered.", + ].join("\n"), + ); + } + return { name: opts.nameFlag, seed: true }; +} + +// Write the binding cast just used into environments.yaml, keyed by the FULL +// slug (#6 — a bare `` key collides the day a second org shows up with +// the same repo short name, and this is a brand new entry, so it is written the +// way the resolver prefers). +// +// Comment-preserving: the yaml Document API edits the parsed tree in place, so +// an operator's comments, key order and blank lines survive. cast rewriting a +// hand-maintained file at all is a real intrusion, which is why it happens ONLY +// after a successful registration and only for a key that was absent. +export function seedGithubAppBinding( + bindingsPath: string, + orgRepo: string, + name: string, +): void { + const doc = parseDocument(readFileSync(bindingsPath, "utf8")); + doc.setIn(["github_apps", orgRepo], name); + writeFileSync(bindingsPath, doc.toString()); +} + +// --------------------------------------------------------------------------- +// Step 3 — the manifest, and the page that POSTs it +// --------------------------------------------------------------------------- + +export function buildManifest(opts: { + name: string; + orgRepo: string; + redirectUrl: string; +}): Record { + return { + name: opts.name, + url: `https://github.com/${opts.orgRepo}`, + // Required by the schema even inactive — see DEAD_HOOK_URL. + hook_attributes: { url: DEAD_HOOK_URL, active: false }, + redirect_url: opts.redirectUrl, + public: false, + default_events: [], + // Clone-only. Keys are snake_case here (`pull_requests`), NOT the + // hyphenated form the docs' reference page renders — a difference that + // costs an App you have to delete and recreate. + default_permissions: { contents: "read", metadata: "read" }, + }; +} + +export function newAppFormAction(owner: string, ownerType: OwnerType): string { + return ownerType === "User" + ? "https://github.com/settings/apps/new" + : `https://github.com/organizations/${encodeURIComponent(owner)}/settings/apps/new`; +} + +function escapeHtml(value: string): string { + return value + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); +} + +// The page the operator's browser loads. It carries the manifest as a single +// form field and submits itself; GitHub renders a confirmation screen, and the +// operator's existing session is the authentication. +export function manifestFormPage(opts: { + manifest: Record; + formAction: string; + csrf: string; + appName: string; +}): string { + const action = `${opts.formAction}?state=${encodeURIComponent(opts.csrf)}`; + return [ + "", + '', + `cast: create GitHub App ${escapeHtml(opts.appName)}`, + "", + `

Submitting the App manifest for ${escapeHtml(opts.appName)} to GitHub…

`, + `
`, + ``, + '', + "
", + '', + "", + ].join("\n"); +} + +// --------------------------------------------------------------------------- +// Step 4 — the one-shot loopback server +// --------------------------------------------------------------------------- + +export type LoopbackServer = { + port: number; + // Where the operator points a browser to start the flow. + startUrl: string; + redirectUrl: string; + manifest: Record; + // Resolves with the manifest `code`; rejects if close() is called first. + code: Promise; + close: () => Promise; +}; + +export async function startManifestServer(opts: { + csrf: string; + port?: number; + formAction: string; + appName: string; + // The manifest cannot be built before the port is known (it contains the + // redirect_url), and with port 0 the port is only known after listen(). + manifestFor: (redirectUrl: string) => Record; +}): Promise { + let settle: ((code: string) => void) | undefined; + let fail: ((err: Error) => void) | undefined; + const code = new Promise((res, rej) => { + settle = res; + fail = rej; + }); + // The rejection is always handled by close(); without this an operator who + // Ctrl-Cs mid-flow gets an unhandled rejection warning on the way out. + code.catch(() => {}); + let captured = false; + + let manifest: Record = {}; + let page = ""; + + const server = createServer((req, res) => { + const url = new URL(req.url ?? "/", `http://${LOOPBACK}`); + if (url.pathname === "/") { + res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); + res.end(page); + return; + } + if (url.pathname === "/callback") { + const state = url.searchParams.get("state"); + const got = url.searchParams.get("code"); + // The CSRF check. A callback carrying someone else's `state` is not a + // slow request to retry — it is a page on the internet trying to hand + // this server a code, so it is refused and NOT resolved. The server + // keeps listening: refusing a forgery must not also cancel the real + // callback the operator is still on their way to producing. + if (state !== opts.csrf) { + res.writeHead(400, { "content-type": "text/plain; charset=utf-8" }); + res.end( + "state mismatch: this callback did not come from cast's flow\n", + ); + return; + } + if (!got) { + res.writeHead(400, { "content-type": "text/plain; charset=utf-8" }); + res.end("no code in callback\n"); + return; + } + res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); + res.end( + '

App created. You can close this tab and return to the terminal.

', + ); + captured = true; + settle?.(got); + return; + } + res.writeHead(404, { "content-type": "text/plain; charset=utf-8" }); + res.end("not found\n"); + }); + + await new Promise((res, rej) => { + server.once("error", rej); + server.listen(opts.port ?? DEFAULT_CALLBACK_PORT, LOOPBACK, () => res()); + }); + + const port = (server.address() as AddressInfo).port; + const redirectUrl = `http://${LOOPBACK}:${port}/callback`; + manifest = opts.manifestFor(redirectUrl); + page = manifestFormPage({ + manifest, + formAction: opts.formAction, + csrf: opts.csrf, + appName: opts.appName, + }); + + return { + port, + startUrl: `http://${LOOPBACK}:${port}/`, + redirectUrl, + manifest, + code, + close: () => + new Promise((res) => { + if (!captured) { + fail?.(new Error("the manifest callback never arrived")); + } + server.close(() => res()); + server.closeAllConnections?.(); + }), + }; +} + +export function newCsrfToken(): string { + return randomBytes(24).toString("hex"); +} + +// --------------------------------------------------------------------------- +// Step 5 — the code exchange +// --------------------------------------------------------------------------- + +function readOwnerType(raw: unknown): OwnerType { + return raw === "User" ? "User" : "Organization"; +} + +// POST /app-manifests/{code}/conversions, deliberately with NO Authorization +// header: the code itself is the credential. It is valid for one hour and is +// treated as single-use. +export async function convertManifestCode( + code: string, + fetchImpl: typeof fetch = fetch, +): Promise { + const res = await fetchImpl( + `https://api.github.com/app-manifests/${encodeURIComponent(code)}/conversions`, + { + method: "POST", + headers: { + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }, + }, + ); + if (!res.ok) { + const body = await res.text(); + // GitHub's own wording for both of these says nothing about the remedy, + // and the remedy is not guessable: the code is spent, so the fix is to run + // the whole flow again, not to retry the exchange. + if (res.status === 404) { + throw new Error( + [ + "GitHub rejected the manifest code (404).", + "", + "The code is valid for one hour and can be exchanged once. This one is", + "expired, already spent, or was never issued.", + "", + "Remedy: run `cast github-app create` again and click through the browser", + "form once more. Nothing was registered, and no App was created on GitHub", + "by the exchange — if the browser step DID create one, delete it first", + "(GitHub Apps can be deleted from the org's Settings → Developer settings).", + ].join("\n"), + ); + } + if (res.status === 422) { + throw new Error( + [ + "GitHub refused the manifest conversion (422).", + "", + "This is GitHub's rate-limit/abuse response on this endpoint, not a bad", + `manifest. Body: ${body}`, + "", + "Remedy: wait a few minutes, then run `cast github-app create` again. If", + "earlier attempts left half-created Apps on the org, delete them first —", + "repeated create attempts are what trips this.", + ].join("\n"), + ); + } + throw new Error( + `POST /app-manifests/{code}/conversions → ${res.status}: ${body}`, + ); + } + const raw = (await res.json()) as Record; + const owner = (raw.owner ?? {}) as Record; + const id = raw.id; + const slug = raw.slug; + const clientId = raw.client_id; + const pem = raw.pem; + const clientSecret = raw.client_secret; + const ownerLogin = owner.login; + // Strict: this body is the only copy of these values that will ever exist. + // A field cast cannot read is not defaulted — half-persisted credentials are + // worse than a loud failure, because the App exists on GitHub either way and + // only the loud failure says so. + if ( + typeof id !== "number" || + typeof slug !== "string" || + typeof clientId !== "string" || + typeof clientSecret !== "string" || + typeof pem !== "string" || + typeof ownerLogin !== "string" + ) { + throw new Error( + [ + "the manifest conversion response is missing fields cast needs.", + "", + ` got: ${JSON.stringify(Object.keys(raw))}`, + "", + "The App may well have been created on GitHub — check the org's Developer", + "settings, delete it if so, and re-run. This body is the only time GitHub", + "hands over the private key, so cast refuses to persist a partial copy.", + ].join("\n"), + ); + } + return { + id, + slug, + clientId, + clientSecret, + webhookSecret: + typeof raw.webhook_secret === "string" ? raw.webhook_secret : null, + pem, + ownerLogin, + ownerType: readOwnerType(owner.type), + }; +} + +// --------------------------------------------------------------------------- +// Step 7 — recover the installation id from the App's own key +// --------------------------------------------------------------------------- + +function b64url(value: string | Buffer): string { + return Buffer.from(value).toString("base64url"); +} + +// An RS256 App JWT, signed with the PEM the conversion just handed over. No +// dependency: node's createSign("RSA-SHA256") IS the RS256 signature, and a +// JWT is two base64url JSON segments and that signature over them. +// +// `iss` is the CLIENT ID, not the app id: GitHub now recommends it, and both +// are accepted, so the recommended one is what cast sends. +export function mintAppJwt(opts: { + privateKeyPem: string; + clientId: string; + now?: number; +}): string { + const nowSeconds = Math.floor((opts.now ?? Date.now()) / 1000); + const header = { alg: "RS256", typ: "JWT" }; + const payload = { + iat: nowSeconds - JWT_BACKDATE_SECONDS, + exp: nowSeconds + JWT_AHEAD_SECONDS, + iss: opts.clientId, + }; + const signingInput = `${b64url(JSON.stringify(header))}.${b64url(JSON.stringify(payload))}`; + const signature = createSign("RSA-SHA256") + .update(signingInput) + .sign(opts.privateKeyPem); + return `${signingInput}.${b64url(signature)}`; +} + +// The installation id, from the App's own credential. Nothing else is needed — +// no PAT, no operator token. +// +// Deliberately NOT read from the `installation_id` GitHub appends to a +// setup_url redirect: GitHub documents that value as a hint and warns it can be +// spoofed. An installation id is the thing Coolify clones through; a wrong one +// is a silent wrong-repo grant. +// +// `undefined` means "read cleanly, not installed yet" — the state the poll +// waits out. A transport or auth failure throws instead, because "not installed +// yet" and "cast could not ask" are different facts and only one of them is +// worth waiting on. +export async function findInstallationId(opts: { + owner: string; + ownerType: OwnerType; + jwt: string; + fetchImpl?: typeof fetch; +}): Promise { + const path = + opts.ownerType === "User" + ? `/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", + Authorization: `Bearer ${opts.jwt}`, + "X-GitHub-Api-Version": "2022-11-28", + }, + }); + if (res.status === 404) return undefined; + if (!res.ok) { + throw new Error(`GET ${path} → ${res.status}: ${await res.text()}`); + } + const raw = (await res.json()) as Record; + if (typeof raw.id !== "number") { + throw new Error( + `GET ${path} returned no usable installation id: ${JSON.stringify(raw)}`, + ); + } + return raw.id; +} + +// Poll while the operator clicks through the install screen in a browser. +export async function awaitInstallationId(opts: { + owner: string; + ownerType: OwnerType; + privateKeyPem: string; + clientId: string; + attempts?: number; + intervalMs?: number; + fetchImpl?: typeof fetch; + sleep?: (ms: number) => Promise; + now?: () => number; + log?: (line: string) => void; +}): Promise { + const attempts = opts.attempts ?? 60; + const intervalMs = opts.intervalMs ?? 5000; + const sleep = + opts.sleep ?? ((ms: number) => new Promise((r) => setTimeout(r, ms))); + for (let attempt = 0; attempt < attempts; attempt++) { + // Minted per attempt, not once: a JWT lives 8 minutes and this loop can + // outlast that while an operator reads the install screen. + const jwt = mintAppJwt({ + privateKeyPem: opts.privateKeyPem, + clientId: opts.clientId, + now: opts.now?.(), + }); + const id = await findInstallationId({ + owner: opts.owner, + ownerType: opts.ownerType, + jwt, + fetchImpl: opts.fetchImpl, + }); + if (id !== undefined) return id; + if (attempt === 0) { + opts.log?.("waiting for the App to be installed…"); + } + await sleep(intervalMs); + } + throw new Error( + [ + `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).", + ].join("\n"), + ); +} + +// --------------------------------------------------------------------------- +// Where the secrets land +// --------------------------------------------------------------------------- + +// The conversion response is the only time GitHub yields the PEM, the client +// secret and the webhook secret, so cast persists all three — in the state +// directory the operator points it at, never anywhere of its own (public tool, +// private state). +// +// Why plaintext, 0600, and not the age store: `secrets/` is per-repo-per-env +// APPLICATION env vars (secrets.ts) — its contents are decrypted and injected +// into the deployed container, which is the last place an App private key +// should be. And `keyFileFor` throws outright where no age key exists, which is +// the incubator deployment's actual state; encrypting the one credential that +// makes disaster recovery possible behind a key that may not exist is how DR +// fails at the moment it is needed. +// +// So the guard is structural instead of cryptographic: the directory carries a +// `.gitignore` of `*`, which means an unthinking `git add -A` in the state repo +// cannot commit these, and committing them stays possible but has to be +// deliberate. See the PR body for the argument and the follow-up. +export function githubAppDir(stateDir: string): string { + return join(stateDir, "github-apps"); +} + +export function persistCredentials(opts: { + stateDir: string; + name: string; + creds: AppCredentials; + org: string; + orgRepo: string; + force?: boolean; +}): { pemPath: string; secretsPath: string } { + const dir = githubAppDir(opts.stateDir); + mkdirSync(dir, { recursive: true, mode: 0o700 }); + const ignore = join(dir, ".gitignore"); + if (!existsSync(ignore)) { + writeFileSync( + ignore, + [ + "# Written by `cast github-app`. These files are the ONLY copy of a GitHub", + "# App's private key, client secret and webhook secret — GitHub shows them", + "# once and never again.", + "#", + "# Ignored by default so that `git add -A` in this state repo cannot commit", + "# plaintext credentials by accident. Committing them is still possible and", + "# is a deliberate act: encrypt them first (age, sops, …) and commit the", + "# ciphertext, or keep this directory out of the repo and back it up", + "# somewhere that is not a git remote.", + "*", + "", + ].join("\n"), + ); + } + + 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, + ); + return { pemPath, secretsPath }; +} + +// 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"), + ); + } + writeFileSync(path, content, { mode: 0o600 }); +} + +// --------------------------------------------------------------------------- +// Steps 8 + 9 — the register code path. `create` falls through into THIS. +// --------------------------------------------------------------------------- + +// Read the repo list back off a Coolify GitHub App record. Coolify proxies +// GitHub with the installation token, so this answers the only question that +// matters: can this App, as registered, actually see the repo cast will ask it +// to clone? +export async function readAppRepositories( + client: CoolifyClient, + coolifyAppId: number, +): Promise { + const raw = (await client.get( + `/github-apps/${coolifyAppId}/repositories`, + )) as unknown; + const list = Array.isArray(raw) + ? raw + : ((raw as Record | null)?.repositories ?? null); + if (!Array.isArray(list)) return undefined; + const names: string[] = []; + for (const item of list) { + if (typeof item !== "object" || item === null) return undefined; + const row = item as Record; + if (typeof row.full_name === "string") { + names.push(row.full_name); + continue; + } + const owner = (row.owner ?? {}) as Record; + if (typeof owner.login === "string" && typeof row.name === "string") { + names.push(`${owner.login}/${row.name}`); + continue; + } + // One unreadable row makes the whole read unreadable: a partial list is + // indistinguishable from a complete one, and this list is the evidence for + // a claim ("the App can clone this repo") that must not be made loosely. + return undefined; + } + return names; +} + +// 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 +// matters most: today a misconfigured App fails silently and surfaces hours +// later as an unresolvable source at `cast apply` time, in a different command +// on a different day. Here it is a hard error at creation, next to the thing +// that caused it. +export async function registerGithubApp(opts: { + client: CoolifyClient; + name: string; + org: string; + orgRepo: string; + creds: AppCredentials; + stateDir: string; + force?: boolean; + log?: (line: string) => void; +}): Promise { + const log = opts.log ?? ((line: string) => console.log(line)); + + // Persist BEFORE the Coolify calls. If Coolify refuses, the credentials are + // still on disk and `register` can be re-run against them; the reverse order + // loses the private key to a failed HTTP call. + const { pemPath, secretsPath } = persistCredentials({ + stateDir: opts.stateDir, + name: opts.name, + creds: opts.creds, + org: opts.org, + orgRepo: opts.orgRepo, + force: opts.force, + }); + log(`private key → ${pemPath}`); + log(`credentials → ${secretsPath}`); + + const key = (await opts.client.post("/security/keys", { + name: `${opts.name}-key`, + private_key: opts.creds.privateKeyPem, + })) as { uuid: string } | null; + const keyUuid = key?.uuid; + if (typeof keyUuid !== "string") { + throw new Error( + `POST /security/keys returned no key uuid: ${JSON.stringify(key)}`, + ); + } + + const created = (await opts.client.post("/github-apps", { + name: opts.name, + organization: opts.org, + api_url: "https://api.github.com", + html_url: "https://github.com", + 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_uuid: keyUuid, + })) as Record | null; + const coolifyAppId = created?.id; + if (typeof coolifyAppId !== "number") { + throw new Error( + [ + `POST /github-apps returned no usable id: ${JSON.stringify(created)}`, + "", + "The App record may exist in Coolify, but cast cannot verify that it can", + "reach the repo without that id. Check Coolify's Sources page.", + ].join("\n"), + ); + } + 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 }; +} + +// --------------------------------------------------------------------------- +// Step 1 — the optional preflight +// --------------------------------------------------------------------------- + +export type PreflightResult = + | { kind: "admin" } + | { kind: "not-admin"; role: string } + | { kind: "skipped"; why: string }; + +// Cheap and worth it: without this the operator completes the entire browser +// dance and only THEN learns they cannot create Apps on that org. +// +// `gh` is never required — an absent or unauthenticated `gh` skips silently. +// Making a nice-to-have check into a hard dependency is how a bootstrap command +// stops working on the machine that most needs it. +export function preflightOrgAdmin( + org: string, + run: (file: string, args: string[]) => string = (file, args) => + execFileSync(file, args, { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }), +): PreflightResult { + let out: string; + try { + out = run("gh", ["api", `/user/memberships/orgs/${org}`]); + } catch { + return { + kind: "skipped", + why: "gh is absent, unauthenticated, or errored", + }; + } + try { + const role = (JSON.parse(out) as { role?: unknown }).role; + if (role === "admin") return { kind: "admin" }; + return { kind: "not-admin", role: typeof role === "string" ? role : "?" }; + } catch { + return { kind: "skipped", why: "gh returned a body cast could not read" }; + } +} + +// Whether the owner is an org or a personal account — which decides the form +// URL and the installation endpoint. Read from GitHub's public, unauthenticated +// user endpoint; on any failure cast assumes an org (the overwhelmingly common +// case here) and says so rather than refusing. +export async function detectOwnerType( + owner: string, + fetchImpl: typeof fetch = fetch, +): Promise { + try { + const res = await fetchImpl( + `https://api.github.com/users/${encodeURIComponent(owner)}`, + { headers: { Accept: "application/vnd.github+json" } }, + ); + if (!res.ok) return undefined; + const raw = (await res.json()) as Record; + return raw.type === "User" ? "User" : "Organization"; + } catch { + return undefined; + } +} + +export function openBrowser( + url: string, + run: (file: string, args: string[]) => void = (file, args) => { + execFileSync(file, args, { stdio: "ignore" }); + }, +): boolean { + const opener = process.platform === "darwin" ? "open" : "xdg-open"; + try { + run(opener, [url]); + return true; + } catch { + return false; + } +} + +// --------------------------------------------------------------------------- +// `create` — the manifest flow, falling through into registerGithubApp +// --------------------------------------------------------------------------- + +// #5's footgun 3: the script required WEBHOOK_SECRET even for an App whose +// webhook is inactive — the correct configuration for a tailnet-only Coolify +// where deliveries can never arrive and deploys are CI-triggered — so operators +// invented a placeholder by hand. `create` gets a real one from GitHub; +// `register` generates one and says so. +export function generateWebhookSecret(): string { + return randomBytes(16).toString("hex"); +} + +export type CreateDeps = { + fetchImpl?: typeof fetch; + sleep?: (ms: number) => Promise; + now?: () => number; + openUrl?: (url: string) => boolean; + log?: (line: string) => void; + runGh?: (file: string, args: string[]) => string; + installAttempts?: number; + installIntervalMs?: number; +}; + +// The whole browser flow, ending in EXACTLY the code path `register` runs. +// There is no second registration implementation: everything above this line +// exists to produce an AppCredentials, and everything below the call to +// registerGithubApp is registerGithubApp's. +export async function createGithubApp(opts: { + client: CoolifyClient; + orgRepo: string; + name: string; + stateDir: string; + port?: number; + force?: boolean; + ownerType?: OwnerType; + deps?: CreateDeps; +}): Promise { + const deps = opts.deps ?? {}; + const log = deps.log ?? ((line: string) => console.log(line)); + const org = opts.orgRepo.split("/")[0] ?? opts.orgRepo; + + const ownerType = + opts.ownerType ?? + (await detectOwnerType(org, deps.fetchImpl)) ?? + "Organization"; + + // Step 1 — optional, cheap, and it saves the operator the entire browser + // dance when the answer is no. + if (ownerType === "Organization") { + const pre = + deps.runGh === undefined + ? preflightOrgAdmin(org) + : preflightOrgAdmin(org, deps.runGh); + if (pre.kind === "not-admin") { + throw new Error( + [ + `you are not an admin of ${org} (gh reports role: ${pre.role})`, + "", + "Only org admins can create GitHub Apps on an organization. Ask an admin", + "to run this, or create the App on a personal account instead.", + ].join("\n"), + ); + } + log( + pre.kind === "admin" + ? `preflight: admin of ${org} ✓` + : `preflight: skipped (${pre.why})`, + ); + } + + // Steps 3 + 4 — serve the form, wait for the redirect. + const csrf = newCsrfToken(); + const server = await startManifestServer({ + csrf, + port: opts.port, + appName: opts.name, + formAction: newAppFormAction(org, ownerType), + manifestFor: (redirectUrl) => + buildManifest({ name: opts.name, orgRepo: opts.orgRepo, redirectUrl }), + }); + + let conversion: ManifestConversion; + try { + log(""); + log( + "open this to create the App (a browser session is the authentication):", + ); + log(` ${server.startUrl}`); + if ((deps.openUrl ?? openBrowser)(server.startUrl)) { + log(" (opened in your browser)"); + } + log(""); + const code = await server.code; + // Step 5 — the exchange. Unauthenticated by design; the code IS the + // credential, it lives an hour, and it is single-use. + conversion = await convertManifestCode(code, deps.fetchImpl); + } finally { + await server.close(); + } + + // GitHub may have created the App under a suffixed name if ours was taken — + // which is fine and worth saying out loud, because the name in the GitHub UI + // and the name in Coolify are then different things. Coolify's name is a + // local label, and it is the one environments.yaml binds. + log( + `created GitHub App: ${conversion.slug} (github app id ${conversion.id})`, + ); + + // Step 6 — install it. Always print the URL; never assume an opener. + const installUrl = `https://github.com/apps/${conversion.slug}/installations/new`; + log(""); + log("now install it on the repository:"); + log(` ${installUrl}`); + (deps.openUrl ?? openBrowser)(installUrl); + log(""); + + // 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, + }); + 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. + return registerGithubApp({ + client: opts.client, + name: opts.name, + org: conversion.ownerLogin, + orgRepo: opts.orgRepo, + stateDir: opts.stateDir, + force: opts.force, + log, + creds: { + appId: conversion.id, + installationId, + clientId: conversion.clientId, + clientSecret: conversion.clientSecret, + webhookSecret, + privateKeyPem: conversion.pem, + }, + }); +} diff --git a/test/github-app-register-cli.test.ts b/test/github-app-register-cli.test.ts new file mode 100644 index 0000000..133c90f --- /dev/null +++ b/test/github-app-register-cli.test.ts @@ -0,0 +1,280 @@ +import { spawn } from "node:child_process"; +import { generateKeyPairSync } from "node:crypto"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { createServer } from "node:http"; +import type { AddressInfo } from "node:net"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeAll, describe, expect, it } from "vitest"; + +// `cast github-app register` through the real CLI: argv parsing, the stdin-only +// client secret, the team assert, the name resolved from state, the +// post-condition check, and WHEN environments.yaml is written. +// +// The Coolify here is a stub. Registration against a live instance is +// operator-only territory (#7's testability boundary) and nothing in this file +// pretends otherwise — what it proves is that cast sends the right things and +// reacts correctly to each answer. + +let privateKeyPem: string; + +beforeAll(() => { + privateKeyPem = generateKeyPairSync("rsa", { modulusLength: 2048 }) + .privateKey.export({ type: "pkcs8", format: "pem" }) + .toString(); +}); + +type Stub = { + url: string; + hits: string[]; + bodies: Record>; + close: () => Promise; +}; +const stubs: Stub[] = []; + +async function stubCoolify(opts: { repositories: unknown }): Promise { + const hits: string[] = []; + const bodies: Record> = {}; + const server = createServer((req, res) => { + const path = new URL(req.url ?? "", "http://x").pathname.replace( + "/api/v1", + "", + ); + const key = `${req.method} ${path}`; + hits.push(key); + let raw = ""; + req.on("data", (d) => { + raw += String(d); + }); + req.on("end", () => { + if (raw) bodies[key] = JSON.parse(raw); + const json = (body: unknown) => { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify(body)); + }; + if (path === "/teams/current") return json({ id: 0, name: "Root Team" }); + if (path === "/security/keys") return json({ uuid: "key-uuid-1" }); + if (path === "/github-apps" && req.method === "POST") + return json({ id: 7, uuid: "app-uuid" }); + if (path === "/github-apps/7/repositories") + return json({ repositories: opts.repositories }); + res.writeHead(404); + res.end("{}"); + }); + }); + await new Promise((r) => { + server.listen(0, "127.0.0.1", r); + }); + const stub: Stub = { + url: `http://127.0.0.1:${(server.address() as AddressInfo).port}`, + hits, + bodies, + close: () => + new Promise((r) => { + server.close(() => r()); + }), + }; + stubs.push(stub); + return stub; +} + +afterEach(async () => { + await Promise.all(stubs.splice(0).map((s) => s.close())); +}); + +function fixture( + url: string, + githubApps: string, +): { state: string; pem: string } { + const state = mkdtempSync(join(tmpdir(), "cast-state-")); + writeFileSync( + join(state, ".coolify.env"), + `COOLIFY_BASE_URL="${url}"\nCOOLIFY_ACCESS_TOKEN="t"\n`, + ); + writeFileSync( + join(state, "environments.yaml"), + [ + "# hand-maintained", + "environments:", + " prod:", + " server: prod-box", + " team: { id: 0, name: Root Team }", + githubApps, + "", + ].join("\n"), + ); + const pem = join(state, "downloaded.pem"); + writeFileSync(pem, privateKeyPem); + return { state, pem }; +} + +function run( + args: string[], + stdin: string | null, +): Promise<{ code: number; output: string }> { + return new Promise((resolve) => { + const child = spawn("node", ["dist/cli.js", ...args], { + stdio: [stdin === null ? "ignore" : "pipe", "pipe", "pipe"], + }); + if (stdin !== null) { + child.stdin?.end(stdin); + } + let output = ""; + child.stdout.on("data", (d) => { + output += String(d); + }); + child.stderr.on("data", (d) => { + output += String(d); + }); + child.on("close", (code) => resolve({ code: code ?? 0, output })); + }); +} + +const REGISTER = (state: string, pem: string) => [ + "github-app", + "register", + "heavy-duty/incubator", + "--env", + "prod", + "--state", + state, + "--app-id", + "12345", + "--installation-id", + "99887766", + "--client-id", + "Iv23liABCDEF", + "--client-secret-stdin", + "--private-key", + pem, +]; + +describe("cast github-app register", () => { + it("registers against the name in state, verifies the repo, and never takes the secret from argv", 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 r = await run(REGISTER(f.state, f.pem), "the-client-secret\n"); + expect(r.code).toBe(0); + expect(r.output).toContain('team id=0 name="Root Team" ✓'); + expect(r.output).toContain("(from environments.yaml)"); + expect(r.output).toContain( + "verified: hdb-coolify-prod can clone heavy-duty/incubator ✓", + ); + // The secret reached Coolify, and it came off stdin — it is nowhere in + // argv, which `ps` shows and shell history keeps. + expect(stub.bodies["POST /github-apps"].client_secret).toBe( + "the-client-secret", + ); + expect(stub.bodies["POST /security/keys"].name).toBe( + "hdb-coolify-prod-key", + ); + // A webhook-INACTIVE App is the right shape for a tailnet-only Coolify, so + // no operator has to invent a placeholder any more (#5 footgun 3). + expect(r.output).toContain("generated one"); + expect( + String(stub.bodies["POST /github-apps"].webhook_secret).length, + ).toBeGreaterThan(0); + // The credentials landed in the state dir, under a git-ignored directory. + expect( + readFileSync( + join(f.state, "github-apps", "hdb-coolify-prod.pem"), + "utf8", + ), + ).toBe(privateKeyPem); + expect( + readFileSync(join(f.state, "github-apps", ".gitignore"), "utf8"), + ).toContain("*"); + }); + + it("seeds an ABSENT binding from --name, keyed by the full slug, comments intact", async () => { + const stub = await stubCoolify({ + repositories: [{ full_name: "heavy-duty/incubator" }], + }); + const f = fixture(stub.url, "github_apps: {}"); + const r = await run( + [...REGISTER(f.state, f.pem), "--name", "hdb-coolify-prod"], + "s\n", + ); + expect(r.code).toBe(0); + const after = readFileSync(join(f.state, "environments.yaml"), "utf8"); + expect(after).toContain("heavy-duty/incubator: hdb-coolify-prod"); + expect(after).toContain("# hand-maintained"); + }); + + it("REFUSES a --name that disagrees with the state file", async () => { + const stub = await stubCoolify({ repositories: [] }); + const f = fixture( + stub.url, + "github_apps:\n heavy-duty/incubator: hdb-coolify-prod", + ); + const r = await run( + [...REGISTER(f.state, f.pem), "--name", "My Cool App"], + "s\n", + ); + expect(r.code).toBe(1); + expect(r.output).toContain("disagrees with environments.yaml"); + // Refused before it touched Coolify at all — not even the team assert. + expect(stub.hits).toEqual([]); + }); + + it("refuses a client secret passed any way other than stdin", async () => { + const stub = await stubCoolify({ repositories: [] }); + const f = fixture( + stub.url, + "github_apps:\n heavy-duty/incubator: hdb-coolify-prod", + ); + const withoutFlag = REGISTER(f.state, f.pem).filter( + (a) => a !== "--client-secret-stdin", + ); + const r = await run(withoutFlag, null); + expect(r.code).toBe(2); + expect(r.output).toContain("--client-secret-stdin is required"); + }); + + it("fails, and does NOT seed state, when the App cannot see the repo", async () => { + // A state file naming an App that does not work is worse than one naming + // none: the next `cast apply` resolves it, uses it, and fails at clone time. + const stub = await stubCoolify({ + repositories: [{ full_name: "heavy-duty/something-else" }], + }); + const f = fixture(stub.url, "github_apps: {}"); + const r = await run( + [...REGISTER(f.state, f.pem), "--name", "hdb-coolify-prod"], + "s\n", + ); + expect(r.code).toBe(1); + expect(r.output).toContain("cannot see heavy-duty/incubator"); + expect(r.output).toContain("can see: heavy-duty/something-else"); + expect(readFileSync(join(f.state, "environments.yaml"), "utf8")).toContain( + "github_apps: {}", + ); + }); + + it("refuses a read-only instance before any write", async () => { + const stub = await stubCoolify({ repositories: [] }); + const f = fixture( + stub.url, + "github_apps:\n heavy-duty/incubator: hdb-coolify-prod", + ); + writeFileSync( + join(f.state, ".coolify.env"), + `COOLIFY_BASE_URL="${stub.url}"\nCOOLIFY_ACCESS_TOKEN="t"\nCOOLIFY_READ_ONLY=true\n`, + ); + const r = await run(REGISTER(f.state, f.pem), "s\n"); + expect(r.code).toBe(1); + expect(r.output).toContain("refusing to github-app register"); + expect(stub.hits).toEqual([]); + }); + + it("prints usage for an unknown subcommand", async () => { + const r = await run(["github-app", "wat"], null); + expect(r.code).toBe(2); + expect(r.output).toContain("cast github-app create"); + expect(r.output).toContain("cast github-app register"); + }); +}); diff --git a/test/github-app.test.ts b/test/github-app.test.ts new file mode 100644 index 0000000..c2b809d --- /dev/null +++ b/test/github-app.test.ts @@ -0,0 +1,931 @@ +import { createVerify, generateKeyPairSync } from "node:crypto"; +import { + existsSync, + mkdtempSync, + readFileSync, + statSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { beforeAll, describe, expect, it, vi } from "vitest"; +import { loadBindings } from "../src/bindings.js"; +import { CoolifyClient } from "../src/coolify.js"; +import { + type AppCredentials, + awaitInstallationId, + buildManifest, + convertManifestCode, + createGithubApp, + detectOwnerType, + findInstallationId, + manifestFormPage, + mintAppJwt, + newAppFormAction, + persistCredentials, + preflightOrgAdmin, + readAppRepositories, + registerGithubApp, + resolveAppName, + seedGithubAppBinding, + startManifestServer, +} from "../src/github-app.js"; + +// WHAT THIS FILE DOES NOT TEST, said out loud because the issue asks for it +// (#7, "Testability boundary"): +// +// - The browser form POST. It is authenticated by the operator's logged-in +// GitHub session and there is no headless path to it. Nothing here proves +// that GitHub accepts a `redirect_url` on http://127.0.0.1: — that +// assumption is the load-bearing one, it is unvalidated, and the first real +// run is an operator's. +// - Registration against a live Coolify. Every Coolify call below is mocked. +// +// What IS proven here is everything on cast's side of that line: the JSON it +// builds, the server it serves, the JWT it signs, the requests it makes, and +// what it does with each answer. + +let privateKeyPem: string; +let publicKeyPem: string; + +beforeAll(() => { + const pair = generateKeyPairSync("rsa", { modulusLength: 2048 }); + privateKeyPem = pair.privateKey.export({ + type: "pkcs8", + format: "pem", + }) as string; + publicKeyPem = pair.publicKey.export({ + type: "spki", + format: "pem", + }) as string; +}); + +function tmp(prefix: string): string { + return mkdtempSync(join(tmpdir(), prefix)); +} + +function creds(over: Partial = {}): AppCredentials { + return { + appId: 12345, + installationId: 99887766, + clientId: "Iv23liABCDEF", + clientSecret: "cs-secret", + webhookSecret: "wh-secret", + privateKeyPem: privateKeyPem ?? "PEM", + ...over, + }; +} + +// A Coolify whose every route is declared by the test, and which records what +// it was asked — so a test can assert on the ABSENCE of a call as easily as on +// its presence. +function coolify( + routes: Record [number, unknown]>, +): { client: CoolifyClient; hits: string[]; bodies: Record } { + const hits: string[] = []; + const bodies: Record = {}; + const fetchImpl = vi.fn(async (url: string | URL, init?: RequestInit) => { + const path = new URL(String(url)).pathname.replace("/api/v1", ""); + const key = `${init?.method ?? "GET"} ${path}`; + hits.push(key); + const body = init?.body ? JSON.parse(String(init.body)) : undefined; + if (body !== undefined) bodies[key] = body; + const route = routes[key]; + if (!route) return new Response("no such route", { status: 404 }); + const [status, payload] = route(body); + return new Response(JSON.stringify(payload), { status }); + }) as unknown as typeof fetch; + return { + client: new CoolifyClient("https://coolify.test", "tok", fetchImpl), + hits, + bodies, + }; +} + +describe("the manifest cast POSTs to GitHub", () => { + const manifest = buildManifest({ + name: "hdb-coolify-prod", + orgRepo: "heavy-duty/incubator", + redirectUrl: "http://127.0.0.1:8765/callback", + }); + + it("declares clone-only permissions in snake_case", () => { + // Hyphenated keys (`pull-requests`, as the docs' reference page renders + // them) are silently wrong and cost an App you have to delete. + expect(manifest.default_permissions).toEqual({ + contents: "read", + metadata: "read", + }); + for (const key of Object.keys( + manifest.default_permissions as Record, + )) { + expect(key).not.toContain("-"); + } + }); + + it("subscribes to no events and keeps the webhook inactive on a dead url", () => { + expect(manifest.default_events).toEqual([]); + expect(manifest.hook_attributes).toEqual({ + // Required by the schema even when inactive, so it points at a name that + // can never resolve (RFC 2606 reserves `.invalid`). + url: "https://example.invalid/unused", + active: false, + }); + }); + + it("is private, points at the repo, and redirects to the loopback LITERAL", () => { + expect(manifest.public).toBe(false); + expect(manifest.url).toBe("https://github.com/heavy-duty/incubator"); + expect(manifest.redirect_url).toBe("http://127.0.0.1:8765/callback"); + // Never `localhost`: it resolves through the host's name resolution, which + // other software on the machine can change. + expect(String(manifest.redirect_url)).not.toContain("localhost"); + }); + + it("targets the org form for an org and the personal form for a user", () => { + expect(newAppFormAction("heavy-duty", "Organization")).toBe( + "https://github.com/organizations/heavy-duty/settings/apps/new", + ); + expect(newAppFormAction("danmt", "User")).toBe( + "https://github.com/settings/apps/new", + ); + }); + + it("escapes the manifest into the form field rather than breaking out of it", () => { + const page = manifestFormPage({ + manifest: { name: 'a">' }, + formAction: "https://github.com/settings/apps/new", + csrf: "tok/en", + appName: "x", + }); + expect(page).not.toContain(""); + expect(page).toContain("""); + // The csrf token rides the action as `state`, url-encoded. + expect(page).toContain("state=tok%2Fen"); + }); +}); + +describe("the loopback callback server", () => { + it("serves the auto-submitting form and captures the code from a real request", async () => { + // Driven with a real HTTP request against a real ephemeral server — the + // behaviour under test is an HTTP handshake, so nothing here is stubbed. + const server = await startManifestServer({ + csrf: "csrf-value", + port: 0, + appName: "hdb-coolify-prod", + formAction: + "https://github.com/organizations/heavy-duty/settings/apps/new", + manifestFor: (redirectUrl) => + buildManifest({ + name: "hdb-coolify-prod", + orgRepo: "heavy-duty/incubator", + redirectUrl, + }), + }); + try { + // The manifest could not have been built before listen(): with port 0 the + // port is only known afterwards, and it is inside redirect_url. + expect(server.manifest.redirect_url).toBe( + `http://127.0.0.1:${server.port}/callback`, + ); + + const page = await (await fetch(server.startUrl)).text(); + expect(page).toContain('name="manifest"'); + expect(page).toContain("hdb-coolify-prod"); + expect(page).toContain("state=csrf-value"); + + const res = await fetch( + `${server.startUrl}callback?code=abc123&state=csrf-value`, + ); + expect(res.status).toBe(200); + expect(await server.code).toBe("abc123"); + } finally { + await server.close(); + } + }); + + it("refuses a callback carrying the wrong state, and keeps serving the right one", async () => { + const server = await startManifestServer({ + csrf: "the-real-token", + port: 0, + appName: "app", + formAction: "https://github.com/settings/apps/new", + manifestFor: (redirectUrl) => + buildManifest({ name: "app", orgRepo: "o/r", redirectUrl }), + }); + try { + const forged = await fetch( + `${server.startUrl}callback?code=attacker&state=guessed`, + ); + expect(forged.status).toBe(400); + expect(await forged.text()).toContain("state mismatch"); + + // The load-bearing half: refusing a forgery must not also cancel the + // real callback the operator is still on their way to producing. + const real = await fetch( + `${server.startUrl}callback?code=genuine&state=the-real-token`, + ); + expect(real.status).toBe(200); + expect(await server.code).toBe("genuine"); + } finally { + await server.close(); + } + }); + + it("400s a callback with no code at all", async () => { + const server = await startManifestServer({ + csrf: "t", + port: 0, + appName: "app", + formAction: "https://github.com/settings/apps/new", + manifestFor: (redirectUrl) => + buildManifest({ name: "app", orgRepo: "o/r", redirectUrl }), + }); + try { + const res = await fetch(`${server.startUrl}callback?state=t`); + expect(res.status).toBe(400); + } finally { + await server.close(); + } + }); + + it("rejects the pending code when it is closed without a callback", async () => { + const server = await startManifestServer({ + csrf: "t", + port: 0, + appName: "app", + formAction: "https://github.com/settings/apps/new", + manifestFor: (redirectUrl) => + buildManifest({ name: "app", orgRepo: "o/r", redirectUrl }), + }); + const pending = server.code; + await server.close(); + await expect(pending).rejects.toThrow(/callback never arrived/); + }); +}); + +describe("the App JWT", () => { + // Verified independently: this test does not call cast's own code to check + // cast's signature. It re-derives the segments and verifies with the PUBLIC + // key, which is what GitHub does. + function decode(jwt: string) { + const [h, p, s] = jwt.split("."); + return { + header: JSON.parse(Buffer.from(h, "base64url").toString("utf8")), + payload: JSON.parse(Buffer.from(p, "base64url").toString("utf8")), + signingInput: `${h}.${p}`, + signature: Buffer.from(s, "base64url"), + }; + } + + it("is an RS256 signature over the two segments, verifiable with the public key", () => { + const jwt = mintAppJwt({ privateKeyPem, clientId: "Iv23liABCDEF" }); + const { header, signingInput, signature } = decode(jwt); + expect(header).toEqual({ alg: "RS256", typ: "JWT" }); + expect( + createVerify("RSA-SHA256") + .update(signingInput) + .verify(publicKeyPem, signature), + ).toBe(true); + }); + + it("does not verify against a different key", () => { + const other = generateKeyPairSync("rsa", { modulusLength: 2048 }); + const jwt = mintAppJwt({ privateKeyPem, clientId: "x" }); + const { signingInput, signature } = decode(jwt); + expect( + createVerify("RSA-SHA256") + .update(signingInput) + .verify( + other.publicKey.export({ type: "spki", format: "pem" }) as string, + signature, + ), + ).toBe(false); + }); + + it("backdates iat, stays inside GitHub's 10-minute ceiling, and issues as the CLIENT id", () => { + const now = 1_770_000_000_000; + const nowSeconds = Math.floor(now / 1000); + const { payload } = decode( + mintAppJwt({ privateKeyPem, clientId: "Iv23liABCDEF", now }), + ); + // Backdated against clock skew — GitHub's own documented advice. + expect(payload.iat).toBe(nowSeconds - 60); + expect(payload.iat).toBeLessThan(nowSeconds); + // "no more than 10 minutes into the future", measured from iat. Cast sits + // inside the ceiling rather than on it: a JWT rejected for being one second + // too long looks exactly like a bad key from the operator's side. + expect(payload.exp - payload.iat).toBeLessThanOrEqual(600); + expect(payload.exp).toBeGreaterThan(nowSeconds); + // `iss` is the client id, which GitHub now recommends over the app id. + expect(payload.iss).toBe("Iv23liABCDEF"); + }); +}); + +describe("the manifest code exchange", () => { + const conversionBody = { + id: 424242, + slug: "hdb-coolify-prod", + client_id: "Iv23liABCDEF", + client_secret: "cs", + webhook_secret: "wh", + pem: "-----BEGIN RSA PRIVATE KEY-----\nx\n-----END RSA PRIVATE KEY-----\n", + owner: { login: "heavy-duty", type: "Organization" }, + }; + + it("POSTs to the conversions endpoint with NO Authorization header", async () => { + const fetchImpl = vi.fn( + async () => new Response(JSON.stringify(conversionBody), { status: 200 }), + ) as unknown as typeof fetch; + const out = await convertManifestCode("the-code", fetchImpl); + const [url, init] = (fetchImpl as unknown as ReturnType).mock + .calls[0]; + expect(String(url)).toBe( + "https://api.github.com/app-manifests/the-code/conversions", + ); + expect(init.method).toBe("POST"); + // The code IS the credential. Sending a token here is not merely + // unnecessary — the endpoint is documented as unauthenticated. + expect(Object.keys(init.headers)).not.toContain("Authorization"); + expect(out.clientSecret).toBe("cs"); + expect(out.ownerLogin).toBe("heavy-duty"); + expect(out.ownerType).toBe("Organization"); + }); + + it("turns a 404 into the remedy, because the code is spent and retrying the exchange cannot help", async () => { + const fetchImpl = vi.fn( + async () => new Response("Not Found", { status: 404 }), + ) as unknown as typeof fetch; + await expect(convertManifestCode("c", fetchImpl)).rejects.toThrow( + /valid for one hour[\s\S]*run `cast github-app create` again/, + ); + }); + + it("turns a 422 into the rate-limit remedy rather than 'bad manifest'", async () => { + const fetchImpl = vi.fn( + async () => new Response("Unprocessable", { status: 422 }), + ) as unknown as typeof fetch; + await expect(convertManifestCode("c", fetchImpl)).rejects.toThrow( + /rate-limit[\s\S]*wait a few minutes/, + ); + }); + + it("refuses a partial body instead of persisting half a credential", async () => { + const fetchImpl = vi.fn( + async () => + new Response(JSON.stringify({ ...conversionBody, pem: undefined }), { + status: 200, + }), + ) as unknown as typeof fetch; + await expect(convertManifestCode("c", fetchImpl)).rejects.toThrow( + /missing fields cast needs/, + ); + }); + + it("tolerates an absent webhook secret by reporting it as absent, not empty", async () => { + const fetchImpl = vi.fn( + async () => + new Response( + JSON.stringify({ ...conversionBody, webhook_secret: null }), + { status: 200 }, + ), + ) as unknown as typeof fetch; + expect( + (await convertManifestCode("c", fetchImpl)).webhookSecret, + ).toBeNull(); + }); +}); + +describe("recovering the installation id from the App's own key", () => { + function githubFetch( + handler: (path: string, init?: RequestInit) => Response, + ): { impl: typeof fetch; paths: string[] } { + const paths: string[] = []; + const impl = vi.fn(async (url: string | URL, init?: RequestInit) => { + const path = new URL(String(url)).pathname; + paths.push(path); + return handler(path, init); + }) as unknown as typeof fetch; + return { impl, paths }; + } + + it("asks the org endpoint with the JWT as a bearer token", async () => { + const { impl, paths } = githubFetch( + () => new Response(JSON.stringify({ id: 5150 }), { status: 200 }), + ); + const id = await findInstallationId({ + owner: "heavy-duty", + ownerType: "Organization", + jwt: "the.jwt.here", + fetchImpl: impl, + }); + expect(id).toBe(5150); + expect(paths[0]).toBe("/orgs/heavy-duty/installation"); + const [, init] = (impl as unknown as ReturnType).mock + .calls[0]; + expect(init.headers.Authorization).toBe("Bearer the.jwt.here"); + }); + + it("asks the user endpoint for a personal account", async () => { + const { impl, paths } = githubFetch( + () => new Response(JSON.stringify({ id: 1 }), { status: 200 }), + ); + await findInstallationId({ + owner: "danmt", + ownerType: "User", + jwt: "j", + fetchImpl: impl, + }); + expect(paths[0]).toBe("/users/danmt/installation"); + }); + + it("reads a 404 as 'not installed yet' and a 500 as an error — they are different facts", async () => { + const notInstalled = githubFetch(() => new Response("", { status: 404 })); + expect( + await findInstallationId({ + owner: "o", + ownerType: "Organization", + jwt: "j", + fetchImpl: notInstalled.impl, + }), + ).toBeUndefined(); + + const broken = githubFetch(() => new Response("boom", { status: 500 })); + await expect( + findInstallationId({ + owner: "o", + ownerType: "Organization", + jwt: "j", + fetchImpl: broken.impl, + }), + ).rejects.toThrow(/→ 500/); + }); + + it("polls while the operator clicks through the install screen", async () => { + let call = 0; + const impl = vi.fn(async () => { + call++; + return call < 3 + ? new Response("", { status: 404 }) + : new Response(JSON.stringify({ id: 777 }), { status: 200 }); + }) as unknown as typeof fetch; + const slept: number[] = []; + const id = await awaitInstallationId({ + owner: "heavy-duty", + ownerType: "Organization", + privateKeyPem, + clientId: "Iv1", + fetchImpl: impl, + intervalMs: 5000, + sleep: async (ms) => { + slept.push(ms); + }, + }); + expect(id).toBe(777); + expect(call).toBe(3); + expect(slept).toEqual([5000, 5000]); + }); + + it("gives up with an error that says the App exists and only the install is missing", async () => { + const impl = vi.fn( + async () => new Response("", { status: 404 }), + ) as unknown as typeof fetch; + await expect( + awaitInstallationId({ + owner: "heavy-duty", + ownerType: "Organization", + privateKeyPem, + clientId: "Iv1", + fetchImpl: impl, + attempts: 2, + intervalMs: 1, + sleep: async () => {}, + }), + ).rejects.toThrow( + /never installed on heavy-duty[\s\S]*The App exists on GitHub/, + ); + }); +}); + +describe("the Coolify-facing name is resolved from state, not from a flag (#5 footgun 1)", () => { + const bindings = (apps: Record) => + loadBindings("", { + overrideText: [ + "environments:", + " prod:", + " server: box", + " team: { id: 0, name: Root Team }", + `github_apps: ${JSON.stringify(apps)}`, + "", + ].join("\n"), + }); + + it("uses the full-slug entry and ignores a matching --name", () => { + const b = bindings({ "heavy-duty/incubator": "hdb-coolify-prod" }); + expect( + resolveAppName({ bindings: b, orgRepo: "heavy-duty/incubator" }), + ).toEqual({ name: "hdb-coolify-prod", seed: false }); + expect( + resolveAppName({ + bindings: b, + orgRepo: "heavy-duty/incubator", + nameFlag: "hdb-coolify-prod", + }).name, + ).toBe("hdb-coolify-prod"); + }); + + it("still honours a legacy bare-repo key (#6's compatibility fallback)", () => { + expect( + resolveAppName({ + bindings: bindings({ incubator: "legacy-name" }), + orgRepo: "heavy-duty/incubator", + }), + ).toEqual({ name: "legacy-name", seed: false }); + }); + + it("REFUSES a --name that disagrees with state — the footgun, dissolved", () => { + expect(() => + resolveAppName({ + bindings: bindings({ "heavy-duty/incubator": "hdb-coolify-prod" }), + orgRepo: "heavy-duty/incubator", + nameFlag: "My Cool App", + }), + ).toThrow( + /disagrees with environments.yaml[\s\S]*state file is the authority/, + ); + }); + + it("seeds from --name only when the entry is absent, and refuses when neither exists", () => { + expect( + resolveAppName({ + bindings: bindings({}), + orgRepo: "heavy-duty/incubator", + nameFlag: "hdb-coolify-prod", + }), + ).toEqual({ name: "hdb-coolify-prod", seed: true }); + expect(() => + resolveAppName({ + bindings: bindings({}), + orgRepo: "heavy-duty/incubator", + }), + ).toThrow(/no GitHub App name for heavy-duty\/incubator/); + }); + + it("writes the seeded entry by FULL slug, preserving the operator's comments", () => { + const dir = tmp("cast-bind-"); + const path = join(dir, "environments.yaml"); + const original = [ + "# the control plane's bindings — hand maintained", + "environments:", + " prod:", + " server: box # the tailnet one", + " team: { id: 0, name: Root Team }", + "", + "github_apps: {}", + "", + ].join("\n"); + writeFileSync(path, original); + seedGithubAppBinding(path, "heavy-duty/incubator", "hdb-coolify-prod"); + const after = readFileSync(path, "utf8"); + expect(after).toContain("# the control plane's bindings — hand maintained"); + expect(after).toContain("# the tailnet one"); + expect(after).toContain("heavy-duty/incubator: hdb-coolify-prod"); + // And it round-trips through the real schema. + expect(loadBindings(path).github_apps["heavy-duty/incubator"]).toBe( + "hdb-coolify-prod", + ); + }); +}); + +describe("where the secrets land", () => { + it("writes the PEM and the other two secrets 0600, under a directory git ignores by default", () => { + const state = tmp("cast-state-"); + const { pemPath, secretsPath } = persistCredentials({ + stateDir: state, + name: "hdb-coolify-prod", + creds: creds(), + org: "heavy-duty", + orgRepo: "heavy-duty/incubator", + }); + expect(readFileSync(pemPath, "utf8")).toBe(creds().privateKeyPem); + const saved = JSON.parse(readFileSync(secretsPath, "utf8")); + // All three, because the conversion response is the ONLY time GitHub yields + // them and `register` needs the client secret to be re-runnable at all. + expect(saved.client_secret).toBe("cs-secret"); + expect(saved.webhook_secret).toBe("wh-secret"); + expect(saved.app_id).toBe(12345); + expect(saved.installation_id).toBe(99887766); + for (const p of [pemPath, secretsPath]) { + expect(statSync(p).mode & 0o777).toBe(0o600); + } + // The structural half of the "loud note": `git add -A` in the state repo + // cannot commit plaintext credentials by accident. + const ignore = join(state, "github-apps", ".gitignore"); + expect(existsSync(ignore)).toBe(true); + expect(readFileSync(ignore, "utf8")).toContain("*"); + }); + + it("is idempotent on identical content and REFUSES to overwrite different content", () => { + const state = tmp("cast-state-"); + const args = { + stateDir: state, + name: "app", + creds: creds(), + org: "o", + orgRepo: "o/r", + }; + persistCredentials(args); + expect(() => persistCredentials(args)).not.toThrow(); + expect(() => + persistCredentials({ + ...args, + creds: creds({ privateKeyPem: "a different key" }), + }), + ).toThrow(/refusing to overwrite[\s\S]*only copy in existence/); + // --force is the deliberate escape hatch for a stale half-run. + expect(() => + persistCredentials({ + ...args, + creds: creds({ privateKeyPem: "a different key" }), + force: true, + }), + ).not.toThrow(); + }); +}); + +describe("registering with Coolify, and the post-condition that matters", () => { + const ok = (payload: unknown) => () => [200, payload] as [number, unknown]; + + it("uploads the key, creates the App, and PROVES it can reach the repo", async () => { + const c = coolify({ + "POST /security/keys": ok({ uuid: "key-uuid-1" }), + "POST /github-apps": ok({ id: 7, uuid: "app-uuid" }), + "GET /github-apps/7/repositories": ok({ + repositories: [ + { full_name: "heavy-duty/other" }, + { 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: () => {}, + }); + + expect(c.hits).toEqual([ + "POST /security/keys", + "POST /github-apps", + "GET /github-apps/7/repositories", + ]); + expect(c.bodies["POST /security/keys"]).toEqual({ + name: "hdb-coolify-prod-key", + private_key: creds().privateKeyPem, + }); + expect(c.bodies["POST /github-apps"]).toEqual({ + name: "hdb-coolify-prod", + organization: "heavy-duty", + api_url: "https://api.github.com", + html_url: "https://github.com", + app_id: 12345, + installation_id: 99887766, + client_id: "Iv23liABCDEF", + client_secret: "cs-secret", + // No invented placeholder any more (#5 footgun 3). + webhook_secret: "wh-secret", + private_key_uuid: "key-uuid-1", + }); + expect(out.coolifyAppId).toBe(7); + expect(out.repositories).toContain("heavy-duty/incubator"); + }); + + it("fails HARD when the App cannot see the repo, naming what it can see", async () => { + // The whole point of step 9. Without it this misconfiguration surfaces + // hours later, in a different command, as an unresolvable source. + const c = coolify({ + "POST /security/keys": ok({ uuid: "k" }), + "POST /github-apps": ok({ id: 9 }), + "GET /github-apps/9/repositories": ok({ + repositories: [{ full_name: "heavy-duty/something-else" }], + }), + }); + 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( + /cannot see heavy-duty\/incubator[\s\S]*can see: heavy-duty\/something-else/, + ); + }); + + it("fails when the repo list is UNREADABLE, rather than reporting it as empty", async () => { + const c = coolify({ + "POST /security/keys": ok({ uuid: "k" }), + "POST /github-apps": ok({ id: 9 }), + "GET /github-apps/9/repositories": ok({ repositories: "not a list" }), + }); + await expect( + registerGithubApp({ + client: c.client, + name: "n", + org: "o", + orgRepo: "o/r", + creds: creds(), + stateDir: tmp("cast-state-"), + log: () => {}, + }), + ).rejects.toThrow(/cannot verify that n can reach o\/r/); + }); + + it("persists the credentials BEFORE the Coolify calls, so a Coolify failure does not lose the key", async () => { + const c = coolify({}); // every route 404s + const state = tmp("cast-state-"); + await expect( + registerGithubApp({ + client: c.client, + name: "app", + org: "o", + orgRepo: "o/r", + creds: creds(), + stateDir: state, + log: () => {}, + }), + ).rejects.toThrow(); + // GitHub shows the private key once. Losing it to a failed HTTP call would + // mean deleting the App and starting over. + expect(existsSync(join(state, "github-apps", "app.pem"))).toBe(true); + }); + + it("reads a bare array and an owner/name pair as well as full_name", async () => { + const c = coolify({ + "GET /github-apps/3/repositories": ok([ + { owner: { login: "heavy-duty" }, name: "incubator" }, + ]), + }); + expect(await readAppRepositories(c.client, 3)).toEqual([ + "heavy-duty/incubator", + ]); + }); + + it("treats ONE unreadable row as an unreadable list — a partial list reads exactly like a complete one", async () => { + const c = coolify({ + "GET /github-apps/3/repositories": ok({ + repositories: [{ full_name: "a/b" }, { nothing: "usable" }], + }), + }); + expect(await readAppRepositories(c.client, 3)).toBeUndefined(); + }); +}); + +describe("the optional org-admin preflight", () => { + it("passes on admin and refuses on anything else", () => { + expect(preflightOrgAdmin("heavy-duty", () => '{"role":"admin"}')).toEqual({ + kind: "admin", + }); + expect(preflightOrgAdmin("heavy-duty", () => '{"role":"member"}')).toEqual({ + kind: "not-admin", + role: "member", + }); + }); + + it("skips silently when gh is absent — a nice-to-have must never become a dependency", () => { + const result = preflightOrgAdmin("heavy-duty", () => { + throw new Error("ENOENT"); + }); + expect(result.kind).toBe("skipped"); + }); +}); + +describe("detecting whether the owner is an org or a personal account", () => { + it("reads the type, and falls back to undefined rather than guessing on failure", async () => { + const okFetch = vi.fn( + async () => + new Response(JSON.stringify({ type: "User" }), { status: 200 }), + ) as unknown as typeof fetch; + expect(await detectOwnerType("danmt", okFetch)).toBe("User"); + + const badFetch = vi.fn(async () => { + throw new Error("offline"); + }) as unknown as typeof fetch; + expect(await detectOwnerType("danmt", badFetch)).toBeUndefined(); + }); +}); + +describe("`create` falls through into `register` — one implementation, not two", () => { + it("ends in exactly the Coolify calls `register` makes, with GitHub's own secrets", 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" }, + }; + // GitHub, mocked: owner type, the conversion, then the installation. + const githubFetch = vi.fn(async (url: string | URL) => { + const u = String(url); + if (u.endsWith("/conversions")) + return new Response(JSON.stringify(conversion), { status: 200 }); + if (u.includes("/installation")) + return new Response(JSON.stringify({ id: 5150 }), { status: 200 }); + return new Response(JSON.stringify({ type: "Organization" }), { + status: 200, + }); + }) as unknown as typeof fetch; + + const c = coolify({ + "POST /security/keys": () => [200, { uuid: "key-uuid-1" }], + "POST /github-apps": () => [200, { id: 11 }], + "GET /github-apps/11/repositories": () => [ + 200, + { repositories: [{ full_name: "heavy-duty/incubator" }] }, + ], + }); + + const state = tmp("cast-state-"); + // Drive the browser step: as soon as cast prints its start url, fetch the + // callback the way GitHub's redirect would. + const flow = 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); + // The state parameter is not knowable from outside: read it off the + // page cast is serving, exactly as a browser would. + fetch(url) + .then((r) => r.text()) + .then((page) => { + const state = /state=([^"&]+)/.exec(page)?.[1] ?? ""; + return fetch( + `${u.origin}/callback?code=the-code&state=${state}`, + ); + }); + } + return false; + }, + sleep: async () => {}, + runGh: () => '{"role":"admin"}', + log: () => {}, + }, + }); + + const out = await flow; + // The fall-through, asserted as an identity of behaviour: the same three + // calls, in the same order, that the `register`-only test above pins. + expect(c.hits).toEqual([ + "POST /security/keys", + "POST /github-apps", + "GET /github-apps/11/repositories", + ]); + const body = c.bodies["POST /github-apps"] as Record; + expect(body.app_id).toBe(424242); + // Recovered via the JWT path, NOT read off a setup_url redirect parameter + // (GitHub documents that one as a spoofable hint). + expect(body.installation_id).toBe(5150); + expect(body.client_secret).toBe("github-issued-secret"); + expect(body.webhook_secret).toBe("github-issued-webhook"); + expect(out.coolifyAppId).toBe(11); + // And the secrets landed, all three of them. + expect( + readFileSync(join(state, "github-apps", "hdb-coolify-prod.pem"), "utf8"), + ).toBe(privateKeyPem); + }); + + it("refuses before the browser dance when gh says you are not an org admin", async () => { + const c = coolify({}); + await expect( + createGithubApp({ + client: c.client, + orgRepo: "heavy-duty/incubator", + name: "n", + stateDir: tmp("cast-state-"), + port: 0, + ownerType: "Organization", + deps: { + runGh: () => '{"role":"member"}', + log: () => {}, + openUrl: () => false, + }, + }), + ).rejects.toThrow(/not an admin of heavy-duty/); + // Nothing was served, nothing was registered — the point of a preflight. + expect(c.hits).toEqual([]); + }); +}); -- 2.45.2 From 5a1ec74e04cde8188df75c1b8d6e9fb7a9f82b1b Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 11:38:03 +0000 Subject: [PATCH 2/4] fix: persist the manifest conversion before the install poll can lose it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three reviewers, independently: `createGithubApp` held the one-shot conversion payload in memory across `awaitInstallationId` — a ~5 minute poll — and `persistCredentials` ran only inside `registerGithubApp`. A timeout, a dropped network or a Ctrl-C during that wait destroyed a private key and client secret GitHub never re-shows, and left the App orphaned on GitHub. The timeout message then claimed the credentials were "already there" under `/github-apps/`, which was false on exactly the path that printed it. The payload now goes to disk the instant the exchange returns, complete but for the installation id — the one field GitHub will answer again as often as it is asked. It is written as `installation_id: null` and backfilled on success; `writeCredentialsRecord` allows precisely that one transition and refuses every other difference, so nothing irreplaceable is ever overwritten silently. The timeout path now names the two files it wrote and prints the `register` command that finishes the job, and says not to re-run `create`. claude-bot's addition: persisting post-conversion could still throw in `writeExclusive` against a stale `.pem`, losing the fresh key just the same — and that refusal's remedy ("pass --force and re-run") would mean minting a second App. So the collision is pre-flighted before the browser flow starts, when nothing exists and nothing can be lost. The post-conversion persist now only ever meets a clean slot or an exact match, and `writeExclusive`'s wording stays honest for `register`. grok #2: re-running `register` to re-check a failed repo-visibility assertion used to re-POST the key and the App first. Coolify does not de-dupe by name — `GithubController@create` validates `'name' => 'required|string|max:255'` with no `unique` rule and calls a plain `GithubApp::create()`, and the vendored OpenAPI documents no conflict response — so following that advice created a second Source every time. Both verbs now read `GET /github-apps` first and verify an existing record of that name instead of creating another; a name held by a different App, or already duplicated, is a hard error. An unreadable list warns and proceeds rather than blocking a bootstrap command. grok #3: `name` becomes `.pem`/`.json`, so separators, dot references, empties and control characters are rejected where the name is resolved and again where it becomes a filename. grok #4: every GitHub request now sends `User-Agent: cast/`. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 14 +- README.md | 38 +- src/cli.ts | 9 +- src/github-app.ts | 591 +++++++++++++++++++++------ test/github-app-register-cli.test.ts | 4 + test/github-app.test.ts | 393 +++++++++++++++++- 6 files changed, 923 insertions(+), 126 deletions(-) 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( -- 2.45.2 From 9c0e6c830cc4b5719f5ffe8c8374069aabc9c627 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Tue, 21 Jul 2026 12:54:37 +0000 Subject: [PATCH 3/4] 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); -- 2.45.2 From d442b8cc1b09e8f35c5b755337e2069e85f5a769 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Tue, 21 Jul 2026 13:05:36 +0000 Subject: [PATCH 4/4] =?UTF-8?q?fix:=20--port=20is=20argv=20too=20=E2=80=94?= =?UTF-8?q?=20reject=20it=20before=20the=20preflight,=20not=20at=20listen(?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @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. --- src/cli.ts | 20 ++++++++++++ test/github-app-register-cli.test.ts | 46 ++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/src/cli.ts b/src/cli.ts index e7f23b0..446f331 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1535,6 +1535,26 @@ async function githubAppCommand(rest: string[]): Promise { } } } + // `--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 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 ccd9f3f..d93fa9b 100644 --- a/test/github-app-register-cli.test.ts +++ b/test/github-app-register-cli.test.ts @@ -341,6 +341,52 @@ describe("cast github-app register", () => { 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 () => { const r = await run(["github-app", "wat"], null); expect(r.code).toBe(2); -- 2.45.2