diff --git a/src/diff.ts b/src/diff.ts index 4dc524c..1bad5e4 100644 --- a/src/diff.ts +++ b/src/diff.ts @@ -1,18 +1,34 @@ import { GENERATED_PLACEHOLDER } from "./capture.js"; import type { ResolvedEnv } from "./envtemplate.js"; -import { isReservedEnvName, reservedConsequence } from "./reserved.js"; +import { + isPlatformOwnedEnvName, + 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). +// used to happen the moment it was read. +// +// `value` is the raw stored value — `trim(decrypt(...))` — and is MASKED for a +// secret to a token without read:sensitive. `realValue` is an APPENDED ACCESSOR: +// recomputed from `value` on every read and then shell-escaped — single-quoted +// when the var is `is_literal`/`is_multiline`, otherwise run through +// escapeEnvVariables (EnvironmentVariable.php:81,171-207 @ v4.1.2). For a secret +// it is the only readable plaintext; for a NON-secret it is a RENDERING of the +// value, not the value — so comparing a manifest literal against it is wrong on +// its face (`'true'` is not `true`). // // 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` +// fetch time) is what lets the comparison pick the right side per var: `value` // for non-secrets, `realValue ?? value` for secrets. See fetchEnv, diffEnv. +// +// #79 landed this split citing a "stale `real_value`, a stored column Coolify +// does not refresh on an in-place PATCH". That was false — an accessor cannot go +// stale, and `real_value` tracks `value` on every row of a real box. The drift it +// was chasing came from a duplicate PREVIEW row shadowing the production one +// (#85, fixed in #86). The split is still correct, for the escaping reason above; +// only its stated motivation was wrong. export type LiveEnvVar = { value: string; realValue?: string }; export type Desired = { kind: ResourceKind; @@ -174,6 +190,10 @@ function liveValueFor(secret: boolean, live: LiveEnvVar): string { function diffEnv( desired: ResolvedEnv, live: Record, + // Only the live-only pass reads it, and only to choose how wide "the platform + // owns this name" runs — narrow for an application cast models completely, + // wide for a service, which is a vendored bundle it does not (#87). + kind: ResourceKind, ): EnvDiff[] { const diffs: EnvDiff[] = []; for (const [key, v] of Object.entries(desired.vars)) { @@ -221,13 +241,25 @@ function diffEnv( } } for (const key of Object.keys(live)) { + if (key in desired.vars) continue; // A reserved name is deliberately NOT a remove-candidate: it is collected // separately, as a finding (see ReservedVar). Leaving it here as well would // report the same var twice under two headings, one of which says it is // harmless. It also cannot be an `add`/`change`: the manifest side can never // declare one — resolve.ts refuses the run first. - if (!(key in desired.vars) && !isReservedEnvName(key)) - diffs.push({ key, state: "remove-candidate", secret: false }); + if (isReservedEnvName(key)) continue; + // Nor is a name Coolify MINTED (#87). `remove-candidate` means "a live-only + // var the manifest does not declare; apply never removes it; read it by eye" + // — and for SERVICE_FQDN_API or a one-click service's POSTGRES_PASSWORD that + // is a category error twice over: cast did not put it there, and there is no + // vocabulary it could ever be declared in, so it is not a candidate for + // anything. Sixteen such lines on a correct box held prod permanently in + // `change` and left no way to see that it was in fact clean — the state in + // which a real orphan arrives unread. See isPlatformOwnedEnvName for why the + // width differs by kind, and why widening it for applications would hide the + // one orphan most worth printing. + if (isPlatformOwnedEnvName(key, kind)) continue; + diffs.push({ key, state: "remove-candidate", secret: false }); } return diffs; } @@ -348,7 +380,7 @@ export function computeDiff( updatable: !NON_UPDATABLE[d.kind].includes(field), })); const envDiffs = - mode === "full" && d.env ? diffEnv(d.env, l.env ?? {}) : []; + mode === "full" && d.env ? diffEnv(d.env, l.env ?? {}, d.kind) : []; if (fieldDiffs.length > 0 || envDiffs.length > 0) { changes.push({ kind: d.kind, diff --git a/src/draft.ts b/src/draft.ts index 44237b3..d548974 100644 --- a/src/draft.ts +++ b/src/draft.ts @@ -2,7 +2,11 @@ import { existsSync, mkdirSync, readdirSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { stringify } from "yaml"; import { GENERATED_PLACEHOLDER } from "./capture.js"; -import { isReservedEnvName, reservedConsequence } from "./reserved.js"; +import { + isProviderGeneratedEnvName, + isReservedEnvName, + reservedConsequence, +} from "./reserved.js"; import { encryptSecrets } from "./secrets.js"; // `inventory` can already SEE a whole instance (#22). This is it writing down @@ -78,47 +82,14 @@ import { encryptSecrets } from "./secrets.js"; // A name-pattern rule is not a promise, and the docs say so: a var that points // at the source box under a name cast does not recognize WILL be copied. That is // what the disposition table printed at the end of a run is for — read it. -const COOLIFY_MAGIC = - /^SERVICE_(FQDN|URL|USER|PASSWORD|BASE64|REALBASE64)(_|$)/; - -const DATASTORE_WORDS = new Set([ - "DATABASE", - "DB", - "POSTGRES", - "POSTGRESQL", - "PG", - "MYSQL", - "MARIADB", - "MONGO", - "MONGODB", - "REDIS", - "VALKEY", - "KEYDB", - "DRAGONFLY", - "CLICKHOUSE", -]); - -const CONNECTION_WORDS = new Set([ - "URL", - "URI", - "DSN", - "HOST", - "HOSTNAME", - "PORT", - "PASSWORD", - "PASS", - "USER", - "USERNAME", -]); - -export function isProviderGenerated(key: string): boolean { - if (COOLIFY_MAGIC.test(key)) return true; - const words = key.split("_"); - return ( - words.some((w) => DATASTORE_WORDS.has(w)) && - words.some((w) => CONNECTION_WORDS.has(w)) - ); -} +// +// The rule itself lives in reserved.ts (#87) — one home for the names the +// PLATFORM owns, shared with the diff, which asks the same vocabulary for a +// NARROWER width. The width is the whole difference between the two callers and +// reserved.ts explains why: here, over-matching withholds a value for review +// (loud, recoverable); in a diff it would hide a live-only var (silent). This is +// the wide one, deliberately. +export { isProviderGeneratedEnvName as isProviderGenerated }; // --- Names ------------------------------------------------------------------- @@ -689,7 +660,7 @@ function planSecrets( for (const [key, sites] of byKey) { const provenance: Provenance = isReservedEnvName(key) ? "suppressed" - : isProviderGenerated(key) + : isProviderGeneratedEnvName(key) ? "generated" : "captured"; if (provenance === "suppressed") { diff --git a/src/reserved.ts b/src/reserved.ts index 5aceaee..47f9a13 100644 --- a/src/reserved.ts +++ b/src/reserved.ts @@ -57,6 +57,128 @@ export function isReservedEnvName(key: string): boolean { return RESERVED_EXACT.includes(key) || RESERVED_PREFIX.test(key); } +// --- Generated: the names Coolify MINTS --------------------------------------- +// +// The second family of platform-owned names, and the distinction from the +// reserved ones above is why this is a separate export rather than more entries +// in them: +// +// RESERVED declaring one SUPPRESSES the platform's value, so cast refuses +// the manifest outright (assertNoReservedEnvNames). The danger runs +// manifest -> box. +// GENERATED Coolify MINTS these per instance — a compose app's per-container +// domains (SERVICE_FQDN_API), a one-click service's bundled +// datastore credentials (SERVICE_PASSWORD_POSTGRES). The danger runs +// the other way, box -> report: a LIVE one is not an orphan var cast +// should offer to remove, it is the platform's own. Calling it a +// `remove-candidate` is a category error, and sixteen such lines on +// a correct box is how an operator learns to stop reading the diff +// (#87 — the argument #78's own Impact section already made: +// "an operator who learns these always show change stops trusting +// the diff"). +// +// TWO WIDTHS, ON PURPOSE. The asymmetry is load-bearing, and a future reader who +// "unifies" these will silently blind the diff: +// +// isCoolifyGeneratedEnvName NARROW — a prefix rule, nothing heuristic. +// isProviderGeneratedEnvName WIDE — the prefix, OR a name carrying both a +// datastore word and a connection word +// (DATABASE_URL, DB_HOST, POSTGRES_PASSWORD). +// +// Over-matching is SAFE in a draft and UNSAFE in a diff: +// +// - draft (WIDE): over-matching withholds a VALUE and lists it for disposition +// — noisy, recoverable, loud. Under-matching copies the source box's +// DATABASE_URL into a new box that comes up working against the OLD box's +// database, and nobody finds out until the old box is deleted. Silent and +// unrecoverable, so it errs wide (see draft.ts). +// - diff (NARROW): over-matching HIDES a live-only var. A hand-left +// DATABASE_URL still pointing at a box nobody declares any more is the single +// orphan most worth printing — and it matches the wide rule. Not theoretical: +// probed against prod, the wide bucket on a real application held exactly +// DATABASE_URL and REDIS_URL, both of them cast's OWN declared vars (#87). +export const COOLIFY_GENERATED = + /^SERVICE_(FQDN|URL|USER|PASSWORD|BASE64|REALBASE64)(_|$)/; + +// A name that carries both a datastore word and a connection word is a +// connection coordinate for a datastore the PROVIDER creates. Kept here rather +// than in draft.ts so the two callers share one vocabulary and differ only in +// the width they ask for. +const DATASTORE_WORDS = new Set([ + "DATABASE", + "DB", + "POSTGRES", + "POSTGRESQL", + "PG", + "MYSQL", + "MARIADB", + "MONGO", + "MONGODB", + "REDIS", + "VALKEY", + "KEYDB", + "DRAGONFLY", + "CLICKHOUSE", +]); + +// The db NAME is a connection coordinate like any other — you cannot connect +// without it — so `DB` sits in BOTH sets, and that is not a mistake: it is a +// datastore word in `DB_HOST` and a connection word in `POSTGRES_DB`. Without it +// the pair-rule missed `POSTGRES_DB` outright ([POSTGRES, DB] is datastore + +// datastore, no connection word), which is exactly the var a one-click service +// mints for its bundled Postgres. Found by the #87 tests, on the real umami. +const CONNECTION_WORDS = new Set([ + "URL", + "URI", + "DSN", + "HOST", + "HOSTNAME", + "PORT", + "PASSWORD", + "PASS", + "USER", + "USERNAME", + "DB", +]); + +export function isCoolifyGeneratedEnvName(key: string): boolean { + return COOLIFY_GENERATED.test(key); +} + +export function isProviderGeneratedEnvName(key: string): boolean { + if (isCoolifyGeneratedEnvName(key)) return true; + const words = key.split("_"); + return ( + words.some((w) => DATASTORE_WORDS.has(w)) && + words.some((w) => CONNECTION_WORDS.has(w)) + ); +} + +// The live-only names a DIFF must not offer to remove (#87), and the one place +// the width is chosen. +// +// NARROW for an application: cast models an application's env completely — every +// var it should carry is in an env template — so an undeclared datastore var +// there is a hand-left one, and printing it is the whole point. +// +// WIDE for a service: a Coolify service is a VENDORED BUNDLE whose internals cast +// does not model at all. Its manifest entry is `type` + `service_domains` + an +// env_template; everything else on it (POSTGRES_USER, POSTGRES_DB, the one-click +// template's own wiring) belongs to the bundle. cast cannot meaningfully call +// those orphans — it did not put them there, it will not remove them, and it has +// no vocabulary to declare them in. +// +// `kind` is spelled structurally rather than imported as ResourceKind: diff.ts +// imports this module, so importing its type back would be a cycle. +export function isPlatformOwnedEnvName( + key: string, + kind: "application" | "database" | "service", +): boolean { + return kind === "service" + ? isProviderGeneratedEnvName(key) + : isCoolifyGeneratedEnvName(key); +} + // One sentence, wherever a reserved name has to be reported rather than refused // (`diff` on a live box, `inventory --emit-draft`'s UNCAPTURED.md). Whatever the // verb, the consequence is the same sentence — a reader who has met it once in a diff --git a/test/draft.test.ts b/test/draft.test.ts index 36933b5..09452ee 100644 --- a/test/draft.test.ts +++ b/test/draft.test.ts @@ -147,6 +147,11 @@ describe("isProviderGenerated — the one judgment that must not be wrong", () = "UMAMI_DATABASE_URL", "REDIS_URL", "POSTGRES_PASSWORD", + // The db NAME is a connection coordinate too — [POSTGRES, DB] is datastore + // + datastore, so the pair-rule missed it until `DB` joined the connection + // words. It is exactly what a one-click service mints for its bundled + // Postgres (#87). + "POSTGRES_DB", "DB_HOST", "MONGO_URI", ]) { diff --git a/test/reserved.test.ts b/test/reserved.test.ts index f5d174e..799de8b 100644 --- a/test/reserved.test.ts +++ b/test/reserved.test.ts @@ -347,6 +347,103 @@ describe("diff — a reserved name on a live box is a finding", () => { }); }); +// --- diff: a name Coolify MINTED is not an orphan either (#87) ----------------- +// +// The other half of the same rule. A reserved name is not a remove-candidate +// because it is consequential; a generated one is not a remove-candidate because +// it is not cast's at all. The width differs by kind, and that is the part worth +// pinning: an application that swallowed a live-only DATABASE_URL would hide the +// single orphan most worth printing. + +const liveService = (env: Record) => ({ + kind: "service" as const, + name: "umami", + uuid: "s1", + fields: { type: "umami" }, + env: liveEnv(env), +}); +const desiredService = { + kind: "service" as const, + name: "umami", + fields: { type: "umami" }, + env: { vars: {} }, +}; + +describe("diff — Coolify's own generated vars are not orphans", () => { + // The exact prod box: six magic vars minted for core's per-container domains, + // and nothing else live-only. It held the report in `change` forever. + it("reads CLEAN on an application carrying only SERVICE_* magic vars", () => { + const r = computeDiff( + [desiredApp], + [ + liveApp({ + PORT: "3000", + SERVICE_URL_API: "https://api.example.com", + SERVICE_FQDN_API: "https://api.example.com", + SERVICE_URL_ADMIN: "https://admin.example.com", + SERVICE_FQDN_ADMIN: "https://admin.example.com", + SERVICE_URL_INTAKE: "https://apply.example.com", + SERVICE_FQDN_INTAKE: "https://apply.example.com", + }), + ], + "full", + ); + expect(r.changes).toHaveLength(0); + expect(r.clean).toBe(true); + }); + + // The hazard the NARROW rule exists to preserve. A hand-left DATABASE_URL is a + // connection string still pointing at a box nobody declares any more — the + // exact poison draft.ts refuses to copy. The wide rule matches it; an + // application must NOT use the wide rule. + it("still reports a hand-left DATABASE_URL on an application", () => { + const r = computeDiff( + [desiredApp], + [liveApp({ PORT: "3000", DATABASE_URL: "postgres://old-box/app" })], + "full", + ); + expect(r.changes[0].envDiffs).toEqual([ + { key: "DATABASE_URL", state: "remove-candidate", secret: false }, + ]); + expect(r.clean).toBe(false); + }); + + // A service is a vendored bundle cast does not model: the one-click template's + // own POSTGRES_* wiring is the bundle's, not an orphan. Only the WIDE rule + // catches these — they carry no SERVICE_ prefix. + it("reads CLEAN on a service carrying the one-click template's own wiring", () => { + const r = computeDiff( + [desiredService], + [ + liveService({ + SERVICE_FQDN_UMAMI_3000: "https://analytics.example.com:3000", + SERVICE_PASSWORD_POSTGRES: "generated", + SERVICE_PASSWORD_64_UMAMI: "generated", + POSTGRES_USER: "umami", + POSTGRES_PASSWORD: "generated", + POSTGRES_DB: "umami", + }), + ], + "full", + ); + expect(r.changes).toHaveLength(0); + expect(r.clean).toBe(true); + }); + + // …but the width must not become "a service reports nothing". A name that is + // neither Coolify-minted nor a datastore coordinate is still somebody's doing. + it("still reports a non-generated live-only var on a service", () => { + const r = computeDiff( + [desiredService], + [liveService({ POSTGRES_DB: "umami", LEGACY_FLAG: "on" })], + "full", + ); + expect(r.changes[0].envDiffs).toEqual([ + { key: "LEGACY_FLAG", state: "remove-candidate", secret: false }, + ]); + }); +}); + // --- smoke: it writes an env var too ------------------------------------------- describe("smoke — the probe it writes can never be a reserved name", () => {