From 614f0d1e93e1725c246ca76bf394cccfa5916b76 Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Thu, 16 Jul 2026 14:53:00 +0000 Subject: [PATCH] fix(diff): compare non-secret env vars against fresh `value`, not stale `real_value` (#78) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cast diff` re-proposed an env var that was updated in place and is correct on the box: a flag flipped false→true, applied, and redeployed still showed `env … : change` on every subsequent diff, while created-once vars did not. A false drift that never clears also masks real drift. Root cause: `fetchEnv` collapsed each live var to `real_value ?? value`, and Coolify leaves `real_value` at the pre-update value after an in-place PATCH of `value` (a redeploy does not refresh it either). So the diff read the stale `real_value` and compared "false" against the manifest's "true". The `real_value ?? value` choice is deliberate for SECRETS — `value` is masked to a plain token, so `real_value` is the only plaintext to compare — so the fix is per-var, not a blanket switch. `fetchEnv` now carries both forms through as `LiveEnvVar {value, realValue}` and `diffEnv` picks per the desired side's `secret` flag it already knows: `value` for non-secrets (always fresh), `real_value ?? value` for secrets (unchanged). Capture and draft, which want the decrypted plaintext and compare against no manifest literal, keep the old flattening via `flattenEnv`. Tests: a non-secret flipped in place with stale `realValue` reads clean; a masked secret still diffs via `realValue` so a genuine rotation is caught. Co-Authored-By: Claude Opus 4.8 --- src/cli.ts | 30 ++++++++++--- src/diff.ts | 30 +++++++++++-- test/apply.test.ts | 19 ++++++--- test/diff.test.ts | 98 +++++++++++++++++++++++++++++++++++++++---- test/reserved.test.ts | 8 +++- 5 files changed, 162 insertions(+), 23 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index a752788..6657b3f 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -51,6 +51,7 @@ import { import { type Change, type Live, + type LiveEnvVar, type ResourceKind, computeDiff, renderDiff, @@ -822,8 +823,11 @@ export function aliasLive( }); } -// A live resource's env vars, by key. `real_value` is the decrypted one and -// needs a token with read:sensitive; `value` is what a lesser token sees. +// A live resource's env vars, by key, each carrying both forms Coolify returns: +// `value` (fresh after every write, but masked for a secret to a plain token) +// and `realValue` (the decrypted plaintext, needs a token with read:sensitive). +// The diff picks between them per var (see LiveEnvVar, diffEnv); callers that +// only need one flattened string use flattenEnv below. // // A 404 (a resource we just listed no longer having an envs endpoint — not // expected in practice, but consistent with treating "gone" as "no env vars") @@ -834,13 +838,25 @@ export function aliasLive( async function fetchEnv( client: CoolifyClient, l: Pick, -): Promise> { +): Promise> { const base = l.kind === "database" ? "databases" : `${l.kind}s`; const envs = (await client.get(`/${base}/${l.uuid}/envs`).catch((err) => { if (err instanceof HttpError && err.status === 404) return []; throw err; })) as Array<{ key: string; real_value?: string; value: string }>; - return Object.fromEntries(envs.map((e) => [e.key, e.real_value ?? e.value])); + return Object.fromEntries( + envs.map((e) => [e.key, { value: e.value, realValue: e.real_value }]), + ); +} + +// Collapse a live env map to one string per key, preferring the decrypted +// `realValue` — what capture writes into a store and what draft scaffolds from, +// where the plaintext of a secret is the point and the diff's stale-`realValue` +// hazard (#78) does not apply (nothing is compared against a manifest literal). +function flattenEnv(env: Record): Record { + return Object.fromEntries( + Object.entries(env).map(([k, e]) => [k, e.realValue ?? e.value]), + ); } // The databases inside ONE project+environment, each carrying the value it @@ -1643,7 +1659,7 @@ async function main(): Promise { } const liveEnvs: LiveEnvs = {}; for (const l of envBearing) { - liveEnvs[l.name] = await fetchEnv(client, l); + liveEnvs[l.name] = flattenEnv(await fetchEnv(client, l)); } const classification = classify( required, @@ -1857,7 +1873,9 @@ async function main(): Promise { // Databases hold no manifest-templated env of their own — their URL is // what the APPS reference, and that name is generated, not captured. if (r.kind === "database") continue; - r.env = await fetchEnv(client, { kind: r.kind, uuid: r.uuid }); + r.env = flattenEnv( + await fetchEnv(client, { kind: r.kind, uuid: r.uuid }), + ); } draftProjects.push({ name: p.name, diff --git a/src/diff.ts b/src/diff.ts index 9d2e937..4dc524c 100644 --- a/src/diff.ts +++ b/src/diff.ts @@ -3,6 +3,17 @@ import type { ResolvedEnv } from "./envtemplate.js"; import { isReservedEnvName, reservedConsequence } from "./reserved.js"; export type ResourceKind = "application" | "database" | "service"; +// One live env var as Coolify returns it, before the per-secret flattening that +// used to happen the moment it was read. `value` is what a plain token sees: +// fresh after every write, but MASKED for a secret. `realValue` is the decrypted +// plaintext (a `read:sensitive` token); it is the only readable form of a secret, +// but for a NON-secret var it is a stored column Coolify does NOT recompute on an +// in-place PATCH — so it goes stale and reads back the pre-update value (#78). +// +// Carrying both to diffEnv (rather than collapsing to `realValue ?? value` at +// fetch time) is what lets the comparison pick the fresh side per var: `value` +// for non-secrets, `realValue ?? value` for secrets. See fetchEnv, diffEnv. +export type LiveEnvVar = { value: string; realValue?: string }; export type Desired = { kind: ResourceKind; name: string; @@ -14,7 +25,7 @@ export type Live = { name: string; uuid: string; fields: Record; - env?: Record; + env?: Record; // The destination (Docker network) Coolify reports this resource on. // // NOT in `fields`, because `fields` is the desired-vs-live comparison @@ -147,9 +158,22 @@ function eq(a: unknown, b: unknown): boolean { return JSON.stringify(a) === JSON.stringify(b); } +// The live value to compare a desired var against, chosen by whether the desired +// side declares it secret (#78). For a NON-secret, `value` is authoritative and +// always fresh; `realValue` is a stored column Coolify leaves stale after an +// in-place PATCH, so trusting it re-proposes an already-correct var on every +// diff. For a SECRET, `value` is masked to a plain token, so `realValue` (the +// decrypted plaintext) is the only comparable form — kept as it always was, at +// the cost of the same theoretical staleness, which a masked `value` cannot +// stand in for. `realValue` is optional (a plain token omits it); fall back to +// `value` so a secret still compares against SOMETHING rather than `undefined`. +function liveValueFor(secret: boolean, live: LiveEnvVar): string { + return secret ? (live.realValue ?? live.value) : live.value; +} + function diffEnv( desired: ResolvedEnv, - live: Record, + live: Record, ): EnvDiff[] { const diffs: EnvDiff[] = []; for (const [key, v] of Object.entries(desired.vars)) { @@ -157,7 +181,7 @@ function diffEnv( v.derived !== undefined ? { derived: v.derived.resource } : {}; if (!(key in live)) diffs.push({ key, state: "add", secret: v.secret, ...derived }); - else if (live[key] !== v.value) { + else if (liveValueFor(v.secret, live[key]) !== v.value) { // The second pass of the two-pass bootstrap, which for years only ever // ran once. The store's value for a provider-generated secret is the // literal `pending-coolify-generated` (capture.ts); the first apply sends diff --git a/test/apply.test.ts b/test/apply.test.ts index 4aac5ce..fca2def 100644 --- a/test/apply.test.ts +++ b/test/apply.test.ts @@ -8,6 +8,13 @@ import { import { GENERATED_PLACEHOLDER } from "../src/capture.js"; import { type Desired, type Live, computeDiff } from "../src/diff.js"; +// Wrap plain live values as Coolify's {value, realValue} pairs (here the two +// agree); computeDiff reads them per LiveEnvVar. See diffEnv / #78. +const liveEnv = ( + m: Record, +): Record => + Object.fromEntries(Object.entries(m).map(([k, v]) => [k, { value: v }])); + const desired: Desired[] = [ { kind: "application", @@ -63,7 +70,7 @@ describe("applyPlan", () => { name: "core-api", uuid: "u1", fields: { build_pack: "static", domains: ["https://api.example.com"] }, - env: { PORT: "3000" }, + env: liveEnv({ PORT: "3000" }), }, ]; await expect( @@ -98,7 +105,7 @@ describe("applyPlan", () => { name: "core-api", uuid: "u1", fields: { build_pack: "nixpacks" }, - env, + env: liveEnv(env), }, ]; @@ -201,7 +208,7 @@ describe("applyPlan", () => { name: "web", uuid: "u0", fields: { build_pack: "static" }, - env: { PORT: "3000" }, + env: liveEnv({ PORT: "3000" }), }, ...liveApp({ DATABASE_URL: REAL_URL, REDIS_URL: REAL_REDIS }), ]; @@ -291,7 +298,7 @@ describe("applyPlan", () => { build_pack: "nixpacks", domains: ["https://api.example.com"], }, - env: { PORT: "3000" }, + env: liveEnv({ PORT: "3000" }), }, ]; const r = await applyPlan( @@ -313,7 +320,7 @@ describe("applyPlan", () => { build_pack: "nixpacks", domains: ["https://api.example.com"], }, - env: { PORT: "3000", LEGACY_VAR: "keep-me" }, + env: liveEnv({ PORT: "3000", LEGACY_VAR: "keep-me" }), }, ]; const report = computeDiff(desired, live, "full"); @@ -429,7 +436,7 @@ describe("applyPlan ordering (#45)", () => { name: "core", uuid: "u-core", fields: { build_pack: "nixpacks" }, // NON_UPDATABLE drift - env: { DATABASE_URL: "postgres://x" }, + env: liveEnv({ DATABASE_URL: "postgres://x" }), }, ]; await expect( diff --git a/test/diff.test.ts b/test/diff.test.ts index 821d461..4dc871b 100644 --- a/test/diff.test.ts +++ b/test/diff.test.ts @@ -2,6 +2,15 @@ import { describe, expect, it } from "vitest"; import { GENERATED_PLACEHOLDER } from "../src/capture.js"; import { computeDiff, placeholderConflicts, renderDiff } from "../src/diff.js"; +// Wrap plain live values as Coolify's {value, realValue} pairs. Here the two +// agree — the ordinary case, where nothing was updated in place — so only +// `value` is set; diffEnv reads it per LiveEnvVar. The stale-`realValue` case +// (#78) is exercised with explicit pairs in its own test below. +const liveEnv = ( + m: Record, +): Record => + Object.fromEntries(Object.entries(m).map(([k, v]) => [k, { value: v }])); + const desiredApp = { kind: "application" as const, name: "core-api", @@ -30,7 +39,7 @@ describe("computeDiff", () => { name: "core-api", uuid: "u1", fields: { ...desiredApp.fields }, - env: { PORT: "3000", MAILGUN_KEY: "mk-123" }, + env: liveEnv({ PORT: "3000", MAILGUN_KEY: "mk-123" }), }, ], "full", @@ -46,7 +55,7 @@ describe("computeDiff", () => { name: "core-api", uuid: "u1", fields: { build_pack: "static", domains: desiredApp.fields.domains }, - env: { PORT: "3000", MAILGUN_KEY: "mk-123" }, + env: liveEnv({ PORT: "3000", MAILGUN_KEY: "mk-123" }), }, ], "full", @@ -67,7 +76,7 @@ describe("computeDiff", () => { name: "core-api", uuid: "u1", fields: { ...desiredApp.fields }, - env: { PORT: "3000", MAILGUN_KEY: "OLD", EXTRA: "x" }, + env: liveEnv({ PORT: "3000", MAILGUN_KEY: "OLD", EXTRA: "x" }), }, ]; const full = computeDiff([desiredApp], live, "full"); @@ -89,6 +98,81 @@ describe("computeDiff", () => { ]); expect(r.clean).toBe(false); }); + // #78. Coolify leaves a non-secret var's `realValue` at the pre-update value + // after an in-place PATCH, while `value` is fresh. A flip that landed and + // redeployed must read clean, not re-propose forever — compare non-secrets + // against `value`. + it("does not re-propose a non-secret var flipped in place (stale realValue)", () => { + const r = computeDiff( + [desiredApp], + [ + { + kind: "application", + name: "core-api", + uuid: "u1", + fields: { ...desiredApp.fields }, + env: { + PORT: { value: "3000", realValue: "3000" }, + // Manifest wants MAILGUN_KEY = mk-123 (a secret); realValue carries + // the plaintext and agrees, so the secret stays clean too. + MAILGUN_KEY: { value: "mk-123", realValue: "mk-123" }, + }, + }, + ], + "full", + ); + expect(r.changes).toHaveLength(0); + expect(r.clean).toBe(true); + }); + // The same reproduction with the flip the incubator prod cutover hit: manifest + // "true", live value already "true", but realValue still the stale "false". + it("reads a flipped-in-place flag clean even when realValue is stale", () => { + const flag = { + kind: "application" as const, + name: "core-api", + fields: {}, + env: { vars: { REPORTING_ENABLED: { value: "true", secret: false } } }, + }; + const r = computeDiff( + [flag], + [ + { + kind: "application", + name: "core-api", + uuid: "u1", + fields: {}, + env: { REPORTING_ENABLED: { value: "true", realValue: "false" } }, + }, + ], + "full", + ); + expect(r.changes).toHaveLength(0); + expect(r.clean).toBe(true); + }); + // The regression guard the fix must not trip: a SECRET whose `value` is masked + // to a plain token still compares via `realValue`, so a genuine rotation is + // still caught (and a masked value is never mistaken for the desired plaintext). + it("still diffs a secret via realValue when value is masked", () => { + const r = computeDiff( + [desiredApp], + [ + { + kind: "application", + name: "core-api", + uuid: "u1", + fields: { ...desiredApp.fields }, + env: { + PORT: { value: "3000" }, + MAILGUN_KEY: { value: "**********", realValue: "mk-OLD" }, + }, + }, + ], + "full", + ); + expect(r.changes[0].envDiffs).toEqual([ + { key: "MAILGUN_KEY", state: "change", secret: true }, + ]); + }); }); // The second pass of the two-pass bootstrap (#47). The store holds the literal @@ -115,7 +199,7 @@ const liveGenerated = (env: Record) => [ name: "core-api", uuid: "u1", fields: {}, - env, + env: liveEnv(env), }, ]; @@ -173,7 +257,7 @@ describe("computeDiff generated-secret placeholder", () => { name: "core-api", uuid: "u1", fields: { ...desiredApp.fields }, - env: { PORT: "3000", MAILGUN_KEY: "mk-OLD" }, + env: liveEnv({ PORT: "3000", MAILGUN_KEY: "mk-OLD" }), }, ], "full", @@ -239,7 +323,7 @@ describe("derived resource refs (#60)", () => { name: "core-api", uuid: "u1", fields: { ...derivedApp.fields }, - env, + env: liveEnv(env), }, ]; @@ -460,7 +544,7 @@ describe("renderDiff", () => { name: "core-api", uuid: "u1", fields: { ...desiredApp.fields }, - env: { PORT: "3000", MAILGUN_KEY: "OLD-SECRET" }, + env: liveEnv({ PORT: "3000", MAILGUN_KEY: "OLD-SECRET" }), }, ]; const out = renderDiff(computeDiff([desiredApp], live, "full")); diff --git a/test/reserved.test.ts b/test/reserved.test.ts index 18bb21c..f5d174e 100644 --- a/test/reserved.test.ts +++ b/test/reserved.test.ts @@ -259,12 +259,18 @@ const desiredApp = { fields: { build_pack: "nixpacks" }, env: { vars: { PORT: { value: "3000", secret: false } } }, }; +// Wrap plain live values as Coolify's {value, realValue} pairs (here the two +// agree); computeDiff reads them per LiveEnvVar. See diffEnv / #78. +const liveEnv = ( + m: Record, +): Record => + Object.fromEntries(Object.entries(m).map(([k, v]) => [k, { value: v }])); const liveApp = (env: Record) => ({ kind: "application" as const, name: "core", uuid: "u1", fields: { build_pack: "nixpacks" }, - env, + env: liveEnv(env), }); describe("diff — a reserved name on a live box is a finding", () => {