From 7cb7d75348beea4c1a68f25c974b5ac8f4aac3de Mon Sep 17 00:00:00 2001 From: Baki Date: Fri, 21 Aug 2026 09:00:04 +0300 Subject: [PATCH] fix(compose): keep env behind an embedded variable the re-parse cannot resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `${VAR}` expression embedded in a larger string lost its provenance. `resolveComposeValue` carries `variable`/`required` only when the expression is the entire value, so `postgres://u:${PW}@db` resolved to the non-empty `postgres://u:@db` and named no variable at all. `keepUnresolvedEnv` exists to stop a push-deploy re-parse from overwriting a value the user configured in the wizard — the repo's `.env` holds the secret and is not committed, so the re-parse cannot resolve it. Both of its guards (`value === ""`, `source === "missing"`) miss the embedded shape, so a working DATABASE_URL was replaced with the broken one on the next push that touched the compose file. Interpolation now reports the variables that contributed nothing, carried on the meta as `unresolvedVariables` (names only, never values) plus `required` for the mandatory operators. A satisfied `${VAR:-default}`, or a `${VAR:+alt}` that correctly yielded "", is not reported — those resolved as authored. `maskEnvironmentMeta` is an allowlist, so the new field is named there or it never reaches the client. Refs #673 --- apps/api/src/lib/compose-parser.ts | 58 +++++++++++++-- apps/api/src/lib/secret-env.ts | 14 +++- .../src/modules/deployments/build.service.ts | 27 ++++++- apps/api/test/lib/compose-parser.test.ts | 72 ++++++++++++++++++ .../modules/deployments/build.service.test.ts | 74 +++++++++++++++++++ 5 files changed, 230 insertions(+), 15 deletions(-) diff --git a/apps/api/src/lib/compose-parser.ts b/apps/api/src/lib/compose-parser.ts index 705de7fc6..96124fa4e 100644 --- a/apps/api/src/lib/compose-parser.ts +++ b/apps/api/src/lib/compose-parser.ts @@ -112,8 +112,22 @@ export interface ComposeEnvironmentMeta { defaultValue?: string; resolvedValue: string; expression?: string; - /** The file declares this one mandatory (`:?` / `?`). Only set when it also - * came back unresolved, i.e. alongside `source: "missing"`. */ + /** + * Variables an EMBEDDED expression (`source: "interpolated"`) referenced that + * resolved to nothing — a `${VAR}` with no value, or an unsatisfied + * `${VAR:?msg}`. NAMES ONLY, never values. + * + * A whole-value expression names its single variable in `variable`. An + * embedded one has no single variable to name, and before this it named none + * at all — so `postgres://user:${PASS}@db`, which resolves to the non-empty + * `postgres://user:@db`, was indistinguishable from a literal the author + * typed (#673). + */ + unresolvedVariables?: string[]; + /** The file declares this one mandatory (`:?` / `?`) and nothing satisfied it. + * Alongside `source: "missing"` for a whole-value expression, or + * `source: "interpolated"` when the mandatory variable sits inside a larger + * string. */ required?: boolean; } @@ -833,6 +847,16 @@ function findClosingQuote(value: string, quote: '"' | "'"): number { const BARE_VARIABLE_RE = /^[A-Za-z_][A-Za-z0-9_]*/; +/** + * Reports a referenced variable that contributed nothing to the result. + * `required` marks the mandatory operators (`:?` / `?`). + * + * Only genuine non-resolutions are reported. A `${VAR:-fallback}` that used its + * default HAS resolved, and a `${VAR:+alt}` that yielded "" resolved exactly as + * the author asked — neither is a hole the caller needs to know about. + */ +type UnresolvedSink = (variable: string, required: boolean) => void; + /** * Reads the `${...}` expression opening at `start`, counting nested `${` so the * matching close brace is found. Returns null when the expression is never closed. @@ -853,7 +877,11 @@ function readBracedExpression( return null; } -function interpolateComposeString(input: string, env: Record): string { +function interpolateComposeString( + input: string, + env: Record, + onUnresolved?: UnresolvedSink, +): string { const escapedDollar = "\0COMPOSE_ESCAPED_DOLLAR\0"; const protectedInput = input.replace(/\$\$/g, escapedDollar); @@ -868,15 +896,17 @@ function interpolateComposeString(input: string, env: Record): s if (protectedInput[dollar + 1] === "{") { const braced = readBracedExpression(protectedInput, dollar); if (braced && braced.expression) { - out += resolveInterpolationExpression(braced.expression, env).value; + out += resolveInterpolationExpression(braced.expression, env, onUnresolved).value; cursor = braced.end; continue; } } else { const bare = protectedInput.slice(dollar + 1).match(BARE_VARIABLE_RE); if (bare) { - out += env[bare[0]] ?? ""; - cursor = dollar + 1 + bare[0].length; + const key = bare[0]; + if (!Object.prototype.hasOwnProperty.call(env, key)) onUnresolved?.(key, false); + out += env[key] ?? ""; + cursor = dollar + 1 + key.length; continue; } } @@ -923,7 +953,13 @@ function resolveComposeValue( }; } - const value = interpolateComposeString(input, env); + // An embedded expression has no single variable to name, so collect the holes + // instead: without them a half-interpolated value is just a string, and every + // consumer downstream reads it as one the author typed (#673). + const unresolved = new Map(); + const value = interpolateComposeString(input, env, (variable, required) => { + unresolved.set(variable, (unresolved.get(variable) ?? false) || required); + }); if (!input.includes("$")) return { value }; return { @@ -932,6 +968,8 @@ function resolveComposeValue( source: "interpolated", resolvedValue: value, expression: input, + ...(unresolved.size > 0 && { unresolvedVariables: [...unresolved.keys()] }), + ...([...unresolved.values()].some(Boolean) && { required: true }), }, }; } @@ -956,6 +994,7 @@ function resolveBareEnvironmentKey( function resolveInterpolationExpression( expression: string, env: Record, + onUnresolved?: UnresolvedSink, ): { value: string; source: ComposeEnvironmentMeta["source"]; @@ -970,10 +1009,11 @@ function resolveInterpolationExpression( const hasValue = Object.prototype.hasOwnProperty.call(env, key); const value = env[key] ?? ""; const isNonEmpty = hasValue && value !== ""; - const word = () => interpolateComposeString(rawWord, env); + const word = () => interpolateComposeString(rawWord, env, onUnresolved); switch (operator) { case undefined: + if (!hasValue) onUnresolved?.(key, false); return { value: hasValue ? value : "", source: hasValue ? "env-file" : "missing", variable: key }; case ":-": if (isNonEmpty) return { value, source: "env-file", variable: key }; @@ -989,9 +1029,11 @@ function resolveInterpolationExpression( } case ":?": if (isNonEmpty) return { value, source: "env-file", variable: key }; + onUnresolved?.(key, true); return reportMissingRequired(key, rawWord, env); case "?": if (hasValue) return { value, source: "env-file", variable: key }; + onUnresolved?.(key, true); return reportMissingRequired(key, rawWord, env); case ":+": if (!isNonEmpty) return { value: "", source: "missing", variable: key }; diff --git a/apps/api/src/lib/secret-env.ts b/apps/api/src/lib/secret-env.ts index 3565af642..bb00f5334 100644 --- a/apps/api/src/lib/secret-env.ts +++ b/apps/api/src/lib/secret-env.ts @@ -166,14 +166,19 @@ interface EnvMetaLike { resolvedValue?: string; expression?: string; required?: boolean; + unresolvedVariables?: string[]; } /** * Mask a compose `environmentMeta` map. Keeps the structural fields (`source`, - * `variable` — the variable NAME, not its value, and `required`) so the scan UI - * can still show where a value resolved from, but strips every value-bearing - * field (`resolvedValue`, `defaultValue`, `expression` — a `${VAR:-secret}` - * default embeds the value) so a secret can't leak through the metadata. + * `variable` and `unresolvedVariables` — variable NAMES, not their values, and + * `required`) so the scan UI can still show where a value resolved from, but + * strips every value-bearing field (`resolvedValue`, `defaultValue`, + * `expression` — a `${VAR:-secret}` default embeds the value) so a secret can't + * leak through the metadata. + * + * This is an ALLOWLIST: a field added to `ComposeEnvironmentMeta` and not named + * here is dropped on the way out, so the client never sees it. */ export function maskEnvironmentMeta( meta: Record | null | undefined, @@ -185,6 +190,7 @@ export function maskEnvironmentMeta( ...(m.source !== undefined && { source: m.source }), ...(m.variable !== undefined && { variable: m.variable }), ...(m.required !== undefined && { required: m.required }), + ...(m.unresolvedVariables !== undefined && { unresolvedVariables: m.unresolvedVariables }), ...(m.resolvedValue !== undefined && { resolvedValue: maskValue(m.resolvedValue) }), ...(m.defaultValue !== undefined && { defaultValue: maskValue(m.defaultValue) }), }; diff --git a/apps/api/src/modules/deployments/build.service.ts b/apps/api/src/modules/deployments/build.service.ts index 7dd2d84e6..791aea74c 100644 --- a/apps/api/src/modules/deployments/build.service.ts +++ b/apps/api/src/modules/deployments/build.service.ts @@ -598,6 +598,27 @@ async function reconcileComposeDrift( } } +/** The `environmentMeta` fields {@link isUnresolvedParse} decides on. */ +type UnresolvedParseMeta = { source?: string; unresolvedVariables?: string[] }; + +/** + * Whether a re-parsed value is a hole the repo's own files could not fill, + * rather than something the author actually wrote. + * + * Two shapes, because a compose variable can be the whole value or sit inside + * one: + * - `DB_PASSWORD: ${DB_PASSWORD}` resolves to `""` — the emptiness is the tell. + * - `DATABASE_URL: postgres://u:${DB_PASSWORD}@db` resolves to the NON-empty + * `postgres://u:@db`, so emptiness says nothing and only the variables the + * parser could not resolve do (#673). Before they were reported, this value + * read as an ordinary upstream edit and overwrote the working URL the user + * had typed in the wizard. + */ +function isUnresolvedParse(value: string, meta: UnresolvedParseMeta | undefined): boolean { + if (value === "" && meta?.source === "missing") return true; + return (meta?.unresolvedVariables?.length ?? 0) > 0; +} + /** * A re-parse of the repo's compose resolves `${DB_PASSWORD}` against the repo's * own `.env` — which for a secret is exactly the file that ISN'T committed, so it @@ -610,11 +631,11 @@ async function reconcileComposeDrift( * the variable from the container instead), and a real upstream edit — a new key, * a changed literal, a different `${VAR:-default}` — still drifts normally. */ -function keepUnresolvedEnv< +export function keepUnresolvedEnv< T extends { name: string; environment?: Record; - environmentMeta?: Record; + environmentMeta?: Record; }, >(parsed: T[], stored: { name: string; environment?: unknown }[]): T[] { const storedByName = new Map( @@ -627,7 +648,7 @@ function keepUnresolvedEnv< if (!storedEnv) return svc; // new upstream service — nothing to preserve let patched: Record | undefined; for (const [key, value] of Object.entries(svc.environment)) { - if (value !== "" || meta[key]?.source !== "missing") continue; + if (!isUnresolvedParse(value, meta[key])) continue; const kept = storedEnv[key]; if (!kept) continue; patched ??= { ...svc.environment }; diff --git a/apps/api/test/lib/compose-parser.test.ts b/apps/api/test/lib/compose-parser.test.ts index 71e9c47c7..15cbcefaf 100644 --- a/apps/api/test/lib/compose-parser.test.ts +++ b/apps/api/test/lib/compose-parser.test.ts @@ -732,6 +732,78 @@ services: expect(parsed.missingRequired.map((m) => m.variable)).toEqual(["DB_PASSWORD", "API_KEY"]); }); + // #673: the same operator EMBEDDED in a larger string. It resolves to a + // non-empty value with a hole in it (`postgresql://username:@postgres:5432`), + // which used to be indistinguishable from a literal the author typed — the + // meta said only `source: "interpolated"` and named no variable at all. + it("names the unresolved variable when the expression is embedded in a string", () => { + const parsed = parseComposeFile(` +services: + api: + image: api:latest + environment: + DATABASE_URL: postgresql://username:\${POSTGRES_PASSWORD:?set POSTGRES_PASSWORD in .env}@postgres:5432 +`); + expect(parsed.services[0]?.environment.DATABASE_URL).toBe( + "postgresql://username:@postgres:5432", + ); + expect(parsed.services[0]?.environmentMeta?.DATABASE_URL).toMatchObject({ + source: "interpolated", + unresolvedVariables: ["POSTGRES_PASSWORD"], + required: true, + }); + expect(parsed.missingRequired).toEqual([ + { variable: "POSTGRES_PASSWORD", message: "set POSTGRES_PASSWORD in .env" }, + ]); + }); + + it("names an embedded variable that is merely unset, without marking it required", () => { + const parsed = parseComposeFile(` +services: + api: + image: api:latest + environment: + DATABASE_URL: postgresql://username:\${POSTGRES_PASSWORD}@postgres:5432 +`); + expect(parsed.services[0]?.environmentMeta?.DATABASE_URL).toMatchObject({ + source: "interpolated", + unresolvedVariables: ["POSTGRES_PASSWORD"], + }); + expect(parsed.services[0]?.environmentMeta?.DATABASE_URL.required).toBeUndefined(); + }); + + it("names nothing once the variable resolves", () => { + const parsed = parseComposeFile( + ` +services: + api: + image: api:latest + environment: + DATABASE_URL: postgresql://username:\${POSTGRES_PASSWORD:?required}@postgres:5432 +`, + { env: { POSTGRES_PASSWORD: "s3cret" } }, + ); + expect(parsed.services[0]?.environment.DATABASE_URL).toBe( + "postgresql://username:s3cret@postgres:5432", + ); + expect(parsed.services[0]?.environmentMeta?.DATABASE_URL.unresolvedVariables).toBeUndefined(); + expect(parsed.missingRequired).toEqual([]); + }); + + it("does not treat a satisfied default as unresolved", () => { + const parsed = parseComposeFile(` +services: + api: + image: api:latest + environment: + DATABASE_URL: postgresql://username:\${PW:-fallback}@postgres:5432 +`); + expect(parsed.services[0]?.environment.DATABASE_URL).toBe( + "postgresql://username:fallback@postgres:5432", + ); + expect(parsed.services[0]?.environmentMeta?.DATABASE_URL.unresolvedVariables).toBeUndefined(); + }); + it("satisfies a required variable from the caller-supplied env (#383)", () => { const parsed = parseComposeFile(compose("NODE_VERSION:?required"), { env: { NODE_VERSION: "24" }, diff --git a/apps/api/test/modules/deployments/build.service.test.ts b/apps/api/test/modules/deployments/build.service.test.ts index 22f8adb3c..c1a21649f 100644 --- a/apps/api/test/modules/deployments/build.service.test.ts +++ b/apps/api/test/modules/deployments/build.service.test.ts @@ -89,6 +89,7 @@ vi.mock("../../../src/modules/deployments/smart-route", () => ({ import { requestBuildAccess, resolveSnapshotTarget, + keepUnresolvedEnv, triggerDeployment, type DeploymentConfigSnapshot, } from "../../../src/modules/deployments/build.service"; @@ -609,3 +610,76 @@ describe("requestBuildAccess — folder-upload compose services", () => { ).rejects.toThrow(/Upload session not found/); }); }); + +/** + * Issue #673: `keepUnresolvedEnv` protects a value the user typed in the wizard + * from being overwritten by a push-deploy re-parse that could not resolve the + * variable behind it — the repo's `.env` holds the secret and is not committed. + * + * It recognised the whole-value spelling only. An EMBEDDED variable resolves to + * a non-empty string with a hole in it, so neither of the old guards + * (`value === ""`, `source === "missing"`) fired and the working URL was wiped. + */ +describe("keepUnresolvedEnv - embedded compose variables (#673)", () => { + const stored = [ + { + name: "api", + environment: { + DATABASE_URL: "postgresql://username:s3cret@postgres:5432", + DB_PASSWORD: "s3cret", + }, + }, + ]; + + it("keeps the stored value when an embedded variable did not resolve", () => { + const [svc] = keepUnresolvedEnv( + [ + { + name: "api", + environment: { DATABASE_URL: "postgresql://username:@postgres:5432" }, + environmentMeta: { + DATABASE_URL: { + source: "interpolated", + unresolvedVariables: ["POSTGRES_PASSWORD"], + }, + }, + }, + ], + stored, + ); + + expect(svc?.environment.DATABASE_URL).toBe("postgresql://username:s3cret@postgres:5432"); + }); + + it("still keeps the stored value for a whole-value variable", () => { + const [svc] = keepUnresolvedEnv( + [ + { + name: "api", + environment: { DB_PASSWORD: "" }, + environmentMeta: { DB_PASSWORD: { source: "missing" } }, + }, + ], + stored, + ); + + expect(svc?.environment.DB_PASSWORD).toBe("s3cret"); + }); + + it("lets a real upstream edit through - a resolved value is not a hole", () => { + const [svc] = keepUnresolvedEnv( + [ + { + name: "api", + environment: { DATABASE_URL: "postgresql://username:s3cret@postgres:6543" }, + environmentMeta: { + DATABASE_URL: { source: "interpolated" }, + }, + }, + ], + stored, + ); + + expect(svc?.environment.DATABASE_URL).toBe("postgresql://username:s3cret@postgres:6543"); + }); +});