fix(apply): refuse to write the generated-secret placeholder over a live value (#47) #55

Merged

dan-claude-bot merged 1 commit from fix/generated-secret-guard into main

2026-07-14 23:04:25 +00:00

dan-claude-bot commented

2026-07-14 22:24:50 +00:00

(Migrated from github.com)

The problem

The bootstrap is two-pass, and only the first pass was ever safe to repeat.

A manifest declares generated_secrets: [DATABASE_URL_PROD, REDIS_URL_PROD], so the store holds them as the literal pending-coolify-generated. The first apply sends it, Coolify creates the Postgres/Redis and replaces it with the real URL. From that moment the store is known-wrong, and nothing knew it: GENERATED_PLACEHOLDER was referenced only by capture.ts and draft.ts — the write-side-of-the-store verbs. diff and apply, the only two that could act on it, had never heard of the one literal cast invented to mean "this value is not real yet".

So diff printed secret DATABASE_URL differs — word for word what a legitimate rotation of the same secret prints — and apply stood ready to PATCH the placeholder back over the live, working URL and redeploy migrate, api and worker onto it.

Nothing on the far side stopped it either. Confirmed against the real source rather than the (provably incomplete) vendored OpenAPI spec — coollabsio/coolify@v4.1.2, app/Http/Controllers/Api/ApplicationsController.php::create_bulk_envs: for a key that already exists, the handler does $env->value = $item->get('value'); … $env->save();. A plain overwrite. cli.ts:syncEnv bulk-upserts every var from the store verbatim, so the placeholder lands.

heavy-duty/incubator's prod box is in exactly this state today, held off by a hand-written "run NO cast apply against prod" line in an operator punch list.

What changed

diff.ts — diffEnv gives the placeholder its own EnvDiff state, placeholder-conflict, when the store holds the placeholder for a secret var and the live resource holds anything else. Live value also-placeholder → no diff at all. Var absent live → plain add. Create path untouched — the placeholder is correct there (Coolify replaces it), and diffEnv only ever runs against a live resource, so a create can't reach the new state by construction.
diff.ts — renderDiff says it in words no rotation prints: secret DATABASE_URL: store holds the generated-secret PLACEHOLDER, live holds a real value — apply would OVERWRITE it, plus a count in the summary line (the line an operator actually reads before typing apply).
apply.ts — applyPlan REFUSES, it does not warn. Before any resource is touched, same fail-closed shape as the not-updatable refusal above it. The message names the key and the resource — never the live value (capture.ts's rule) — and points at the remedy.
placeholderConflicts(report) is the one reading of the report that both the warning and the refusal use, so they cannot disagree about what counts as one.
docs/semantics.md documents the two-pass bootstrap and the full store×live disposition table.

$ cast diff …
update database postgres
  secret DATABASE_URL: store holds the generated-secret PLACEHOLDER, live holds a real value — apply would OVERWRITE it
1 change(s), 0 orphan(s), 1 generated-secret PLACEHOLDER conflict(s) — apply will REFUSE

$ cast apply …
refusing apply: the store still holds the pending-coolify-generated placeholder for secret(s) whose live value Coolify has already generated:
  DATABASE_URL on database postgres

Writing the store's value would overwrite the real one and break every consumer.
Fill the store from the live resource first (`cast capture --generated-only`, #48),
or, if the name is no longer provider-generated, drop it from the manifest's
`generated_secrets:` and capture its real value.

Where the issue was wrong, and what I did instead

The issue prescribes that computeDiff learn the environment's generated_secrets list and match env vars against it. That check would have sailed straight past the case that motivated the issue.

generated_secrets: names store refs; an EnvDiff is keyed by env var key; the template maps one to the other and they routinely differ. The repo's own fixtures prove it — test/capture.test.ts declares generated_secrets: [DATABASE_URL_PROD] over a template line DATABASE_URL=${DATABASE_URL_PROD}, and requiredSecrets returns { ref: "DATABASE_URL_PROD", key: "DATABASE_URL" }. Matching the declared names against env-var keys finds nothing. Threading a ref→key map through instead would mean widening ResolvedEnv (envtemplate.ts), outside this PR's blast radius and a rebase conflict for the siblings.

So the guard is keyed on the store's value, which carries the same fact to where it is needed: resolveTemplate copies the store's value in verbatim, and secret is true exactly when the RHS was a single ${REF}. v.secret && v.value === GENERATED_PLACEHOLDER is "this var's store ref currently holds the placeholder".

It is also the stricter rule, and deliberately so. A name dropped from generated_secrets: while the store still holds the placeholder — the first half of the remedy the issue itself suggests — would pass the declaration-keyed check and write pending-coolify-generated over a live database URL. The placeholder is a promise, never a value; writing it over anything real is never right, whatever the manifest currently says. The refusal therefore tells the operator to drop the name and capture its real value, not just the former.

Net effect: no cli.ts change was needed at all. The diff is diff.ts + apply.ts + tests + docs, which should also make the rebase against #45/#50/#51 mechanical.

Tests

312 passing, up from 294 — 18 new, and the refusal path is tested hard, since it is the entire product of this PR.

test/diff.test.ts (8): conflict is raised, and is not a change; live-also-placeholder is clean; absent-live is a plain add; the create path is a plain add; an ordinary rotation stays a plain change; a non-secret template literal that happens to read pending-coolify-generated is not flagged; a structural diff raises nothing; renderDiff prints the loud line, drops the old secret X differs line, counts it in the summary, and leaks neither the URL nor its password.

test/apply.test.ts (10, as a nested describe): refuses before any mutation (recorder sees zero calls — no syncEnv, no redeploy); names every conflicted key and its resource; never prints the live value or its password; points at cast capture --generated-only and #48; refuses even when the conflict rides along with legitimate updatable drift (the refusal is not a filter — nothing goes out); refuses on a conflict carried by a second resource behind a first one that carries non-updatable drift, so the data-loss refusal is the one an operator is told about first; still sends the placeholder on a create; proceeds when live is also the placeholder; proceeds when the var is absent live; and still applies an ordinary secret rotation — a guard that turns every rotation into a refusal is a guard that gets disabled.

Also verified end-to-end against the compiled dist/, not just the unit imports: the refusal fires and syncEnv is never reached.

Noticed, not fixed (out of blast radius)

The guard only covers the store holding the placeholder. A store holding a stale real value for a generated secret (e.g. hand-edited, or captured from a source box) still overwrites the live one with no complaint. Same failure family, different literal — worth its own issue.
applyPlan's needsEnv sends the entire resolved env on any env drift, so one changed var rewrites all of them. Harmless today (upsert of identical values), but it means the blast radius of any env write is always the whole resource. Adjacent to #45's loop work.

Closes #47.

🤖 Generated with Claude Code

## The problem The bootstrap is **two-pass**, and only the first pass was ever safe to repeat. A manifest declares `generated_secrets: [DATABASE_URL_PROD, REDIS_URL_PROD]`, so the store holds them as the literal `pending-coolify-generated`. The first `apply` sends it, Coolify creates the Postgres/Redis and replaces it with the real URL. **From that moment the store is known-wrong, and nothing knew it**: `GENERATED_PLACEHOLDER` was referenced only by `capture.ts` and `draft.ts` — the write-side-of-the-store verbs. `diff` and `apply`, the only two that could act on it, had never heard of the one literal cast invented to mean *"this value is not real yet"*. So `diff` printed `secret DATABASE_URL differs` — word for word what a legitimate **rotation** of the same secret prints — and `apply` stood ready to PATCH the placeholder back over the live, working URL and redeploy `migrate`, `api` and `worker` onto it. Nothing on the far side stopped it either. Confirmed against the real source rather than the (provably incomplete) vendored OpenAPI spec — `coollabsio/coolify@v4.1.2`, `app/Http/Controllers/Api/ApplicationsController.php::create_bulk_envs`: for a key that already exists, the handler does `$env->value = $item->get('value'); … $env->save();`. A plain overwrite. `cli.ts:syncEnv` bulk-upserts every var from the store verbatim, so the placeholder lands. `heavy-duty/incubator`'s prod box is in exactly this state today, held off by a hand-written *"run NO `cast apply` against prod"* line in an operator punch list. ## What changed - **`diff.ts` — `diffEnv`** gives the placeholder its own `EnvDiff` state, `placeholder-conflict`, when the store holds the placeholder for a secret var and the live resource holds *anything else*. Live value also-placeholder → no diff at all. Var absent live → plain `add`. **Create path untouched** — the placeholder is *correct* there (Coolify replaces it), and `diffEnv` only ever runs against a live resource, so a create can't reach the new state by construction. - **`diff.ts` — `renderDiff`** says it in words no rotation prints: `secret DATABASE_URL: store holds the generated-secret PLACEHOLDER, live holds a real value — apply would OVERWRITE it`, plus a count in the summary line (the line an operator actually reads before typing `apply`). - **`apply.ts` — `applyPlan` REFUSES**, it does not warn. Before any resource is touched, same fail-closed shape as the not-updatable refusal above it. The message names the key and the resource — **never the live value** (capture.ts's rule) — and points at the remedy. - **`placeholderConflicts(report)`** is the one reading of the report that both the warning and the refusal use, so they cannot disagree about what counts as one. - **`docs/semantics.md`** documents the two-pass bootstrap and the full store×live disposition table. ``` $ cast diff … update database postgres secret DATABASE_URL: store holds the generated-secret PLACEHOLDER, live holds a real value — apply would OVERWRITE it 1 change(s), 0 orphan(s), 1 generated-secret PLACEHOLDER conflict(s) — apply will REFUSE $ cast apply … refusing apply: the store still holds the pending-coolify-generated placeholder for secret(s) whose live value Coolify has already generated: DATABASE_URL on database postgres Writing the store's value would overwrite the real one and break every consumer. Fill the store from the live resource first (`cast capture --generated-only`, #48), or, if the name is no longer provider-generated, drop it from the manifest's `generated_secrets:` and capture its real value. ``` ## Where the issue was wrong, and what I did instead The issue prescribes that `computeDiff` learn the environment's **`generated_secrets`** list and match env vars against it. **That check would have sailed straight past the case that motivated the issue.** `generated_secrets:` names **store refs**; an `EnvDiff` is keyed by **env var key**; the template maps one to the other and they routinely differ. The repo's own fixtures prove it — `test/capture.test.ts` declares `generated_secrets: [DATABASE_URL_PROD]` over a template line `DATABASE_URL=${DATABASE_URL_PROD}`, and `requiredSecrets` returns `{ ref: "DATABASE_URL_PROD", key: "DATABASE_URL" }`. Matching the declared names against env-var keys finds nothing. Threading a ref→key map through instead would mean widening `ResolvedEnv` (`envtemplate.ts`), outside this PR's blast radius and a rebase conflict for the siblings. So the guard is keyed on the **store's value**, which carries the same fact to where it is needed: `resolveTemplate` copies the store's value in verbatim, and `secret` is true exactly when the RHS was a single `${REF}`. `v.secret && v.value === GENERATED_PLACEHOLDER` **is** "this var's store ref currently holds the placeholder". It is also the **stricter** rule, and deliberately so. A name dropped from `generated_secrets:` while the store still holds the placeholder — the *first half* of the remedy the issue itself suggests — would pass the declaration-keyed check and write `pending-coolify-generated` over a live database URL. The placeholder is a promise, never a value; writing it over anything real is never right, whatever the manifest currently says. The refusal therefore tells the operator to drop the name **and capture its real value**, not just the former. Net effect: no `cli.ts` change was needed at all. The diff is `diff.ts` + `apply.ts` + tests + docs, which should also make the rebase against #45/#50/#51 mechanical. ## Tests **312 passing, up from 294** — 18 new, and the refusal path is tested hard, since it is the entire product of this PR. `test/diff.test.ts` (8): conflict is raised, and is *not* a `change`; live-also-placeholder is **clean**; absent-live is a plain `add`; the create path is a plain `add`; an ordinary rotation stays a plain `change`; a *non-secret* template literal that happens to read `pending-coolify-generated` is not flagged; a structural diff raises nothing; `renderDiff` prints the loud line, drops the old `secret X differs` line, counts it in the summary, and **leaks neither the URL nor its password**. `test/apply.test.ts` (10, as a nested `describe`): refuses before any mutation (**recorder sees zero calls** — no `syncEnv`, no `redeploy`); names every conflicted key and its resource; **never prints the live value or its password**; points at `cast capture --generated-only` and #48; refuses even when the conflict rides along with legitimate updatable drift (the refusal is not a filter — *nothing* goes out); refuses on a conflict carried by a *second* resource behind a first one that carries non-updatable drift, so the data-loss refusal is the one an operator is told about first; still sends the placeholder on a **create**; proceeds when live is also the placeholder; proceeds when the var is absent live; and still applies an **ordinary secret rotation** — a guard that turns every rotation into a refusal is a guard that gets disabled. Also verified end-to-end against the compiled `dist/`, not just the unit imports: the refusal fires and `syncEnv` is never reached. ## Noticed, not fixed (out of blast radius) - The guard only covers the store holding the **placeholder**. A store holding a **stale real value** for a generated secret (e.g. hand-edited, or captured from a source box) still overwrites the live one with no complaint. Same failure family, different literal — worth its own issue. - `applyPlan`'s `needsEnv` sends the **entire** resolved env on any env drift, so one changed var rewrites all of them. Harmless today (upsert of identical values), but it means the blast radius of any env write is always the whole resource. Adjacent to #45's loop work. Closes #47. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: heavy-duty/cast#55

No description provided.