From 5c2ccc697c1ecfa744e53984c56e85320f16c67b Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Tue, 21 Jul 2026 12:43:04 +0000 Subject: [PATCH] fix: complete the basic-auth triple on username-only drift, at both guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three reviewers found the same hole, and it contradicted this PR's own documentation rather than merely being incomplete. `completeBasicAuth` keyed on `fields.is_http_basic_auth_enabled !== true` — the toggle being present IN THE PAYLOAD. But an update body is assembled from the field diffs, and the toggle is absent exactly when it MATCHES. So on the real drift case — basic auth already on at both ends, username edited in the UI — computeDiff emits `http_basic_auth_username` alone, the guard returned early, and the PATCH went out as a lone username. Coolify requires the whole triple on any write that enables basic auth, so that is a 422 mid-run: the precise failure the function exists to prevent, on the one path it was not looking at. The fix reads INTENT from the declared spec instead of from the payload, and completes whenever the payload touches basic auth at all. Two properties are kept deliberately: - it still never MANUFACTURES a write — a payload mentioning no basic-auth field is returned untouched, so the honest limit printed on every diff still holds; - a spec that does not enable basic auth completes nothing, so reading intent from the declaration does not trade one silent wrong write for another. The toggle is now completed alongside the credentials: Coolify's presence rule is about the write as a whole, and a credentials-only PATCH asks it to infer what cast can state. `applicationApiFields` shared the blind spot for the same reason — a lone username has no toggle to be true, so the belt never tightened either. It now refuses any partial basic-auth write, while still letting an explicit disable travel alone and ignoring payloads that do not mention basic auth. No documentation changed: docs/semantics.md:374 and the function's own comment already promised the triple is completed "whenever it sends one of them". The code simply did not do it. This makes them true. Tests: the existing "only the username drifted" case passed the toggle in its payload, so it never exercised the guard — which is why the hole survived review-by-suite. Added the real shape (lone username, lone password, no toggle), the spec-says-off case, three wire-level partial writes, and the two non-write cases. Verified by mutation: restoring the payload-keyed guard fails both new completion assertions. --- src/apply.ts | 36 ++++++++++++++++++++++++++++++++++-- src/cli.ts | 34 +++++++++++++++++++++++++++------- test/apply.test.ts | 43 +++++++++++++++++++++++++++++++++++++++++++ test/wire.test.ts | 33 ++++++++++++++++++++++++++++++++- 4 files changed, 136 insertions(+), 10 deletions(-) diff --git a/src/apply.ts b/src/apply.ts index 5302970..e083f2d 100644 --- a/src/apply.ts +++ b/src/apply.ts @@ -113,14 +113,46 @@ export function applyHostnameOverlay( // on every diff: rotating ONLY the password in the store produces no field diff, // therefore no PATCH, and `cast diff` says the password was not compared rather // than implying it matches. +const BASIC_AUTH_KEYS = [ + "is_http_basic_auth_enabled", + "http_basic_auth_username", + "http_basic_auth_password", +] as const; + export function completeBasicAuth( fields: Record, spec: Desired | undefined, ): Record { - if (fields.is_http_basic_auth_enabled !== true) return fields; const declared = spec?.fields ?? {}; + + // Only complete a payload that is ALREADY touching basic auth. This is what + // keeps the function from manufacturing a write, and it is the reason the + // honest limit above still holds. + if (!BASIC_AUTH_KEYS.some((k) => fields[k] !== undefined)) return fields; + + // Read the INTENT from the declared spec, not from the payload. Keying on + // `fields.is_http_basic_auth_enabled === true` was the bug (cast#76 review): + // the toggle is absent from an update body exactly when it already MATCHES, + // so on username-only drift — auth on at both ends, username edited in the + // UI — computeDiff emits `http_basic_auth_username` alone, the guard returned + // early, and the PATCH went out as a lone username. Coolify requires the + // whole triple on any write that enables basic auth, so that is a 422 + // mid-run: the failure this function exists to prevent, on the one path it + // was not looking at. + // + // A payload that explicitly DISABLES (toggle === false) is left alone — + // completing it with credentials would be manufacturing the opposite write. + const enabled = + fields.is_http_basic_auth_enabled === true || + (fields.is_http_basic_auth_enabled === undefined && + declared.is_http_basic_auth_enabled === true); + if (!enabled) return fields; + const completed = { ...fields }; - for (const k of ["http_basic_auth_username", "http_basic_auth_password"]) { + // The toggle is completed too, not just the credentials: Coolify's presence + // rule is about the write as a whole, and a username+password PATCH with no + // toggle asks it to infer what cast can simply state. + for (const k of BASIC_AUTH_KEYS) { if (completed[k] === undefined && declared[k] !== undefined) completed[k] = declared[k]; } diff --git a/src/cli.ts b/src/cli.ts index dfa9069..cd915b2 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -2833,22 +2833,42 @@ export function applicationApiFields( // any future caller assembling a payload by hand. Failing here costs one // exception; failing at Coolify costs a 422 in the middle of a run that has // already created a project, an environment and possibly a database. - if (rest.is_http_basic_auth_enabled === true) { + // Any write that TOUCHES basic auth without disabling it must carry the whole + // triple. Keying this on `=== true` alone shared apply's blind spot: a PATCH + // body of `{http_basic_auth_username}` — the username-only drift — has no + // toggle to be true, so the belt never tightened either. An explicit + // `false` is a legitimate disable and needs no credentials. + const touchesBasicAuth = ( + [ + "is_http_basic_auth_enabled", + "http_basic_auth_username", + "http_basic_auth_password", + ] as const + ).some((k) => rest[k] !== undefined); + if (touchesBasicAuth && rest.is_http_basic_auth_enabled !== false) { const missing = ( - ["http_basic_auth_username", "http_basic_auth_password"] as const + [ + "is_http_basic_auth_enabled", + "http_basic_auth_username", + "http_basic_auth_password", + ] as const ).filter((k) => { const v = rest[k]; + if (k === "is_http_basic_auth_enabled") return v !== true; return typeof v !== "string" || v === ""; }); if (missing.length > 0) { throw new Error( [ - `refusing to enable HTTP basic auth without ${missing.join(" and ")}`, + `refusing a partial HTTP basic auth write — missing ${missing.join(" and ")}`, "", - "Coolify requires a username AND a password whenever basic auth is enabled, and", - "would answer 422 mid-run. Half-configured basic auth protects nothing anyway:", - "declare both under the application's `basic_auth:` (the password as a ${REF}", - "held by the environment's age store), or set `basic_auth.enabled: false`.", + "Coolify requires the toggle, a username AND a password on any write that", + "enables basic auth, and would answer 422 mid-run. A write carrying only some", + "of the three is that 422 waiting to happen — including a lone username, which", + "is what a username-only drift produces if nothing completes it. Half-configured", + "basic auth protects nothing anyway: declare the pair under the application's", + "`basic_auth:` (the password as a ${REF} held by the environment's age store),", + "or set `basic_auth.enabled: false`.", ].join("\n"), ); } diff --git a/test/apply.test.ts b/test/apply.test.ts index 41a64cc..ff4d926 100644 --- a/test/apply.test.ts +++ b/test/apply.test.ts @@ -577,6 +577,49 @@ describe("completeBasicAuth", () => { ).toBe("s3cret"); }); + // The case the #76 review found, and the one the test above only LOOKED like + // it covered: that payload carries the toggle, so it never exercised the + // guard. When basic auth is already on at both ends and only the username is + // edited in the UI, the toggle MATCHES — so computeDiff emits no fieldDiff + // for it and the payload arrives as a lone username. The old guard keyed on + // the toggle being present and returned early, and the PATCH went out + // incomplete: a 422 mid-run, which is the exact failure this function exists + // to prevent. + it("completes the whole triple from a lone username — no toggle in the payload", () => { + expect(completeBasicAuth({ http_basic_auth_username: "ops" }, spec)).toEqual( + { + is_http_basic_auth_enabled: true, + http_basic_auth_username: "ops", + http_basic_auth_password: "s3cret", + }, + ); + }); + + // Same shape, other credential: a stored-password rotation riding along. + it("completes from a lone password too", () => { + expect( + completeBasicAuth({ http_basic_auth_password: "rotated" }, spec), + ).toEqual({ + is_http_basic_auth_enabled: true, + http_basic_auth_username: "ops", + http_basic_auth_password: "rotated", + }); + }); + + // Intent comes from the SPEC, so a spec that does not enable basic auth must + // not have credentials completed into its payload — otherwise reading intent + // from the declaration would trade one silent wrong write for another. + it("does not complete when the spec does not enable basic auth", () => { + const off: Desired = { + kind: "application", + name: "admin", + fields: { is_http_basic_auth_enabled: false }, + }; + expect(completeBasicAuth({ http_basic_auth_username: "ops" }, off)).toEqual({ + http_basic_auth_username: "ops", + }); + }); + it("leaves a payload that is not enabling basic auth completely alone", () => { // The load-bearing half: this must not MANUFACTURE a write. A run where // nothing about basic auth drifted sends nothing about basic auth. diff --git a/test/wire.test.ts b/test/wire.test.ts index 02deeeb..9512a59 100644 --- a/test/wire.test.ts +++ b/test/wire.test.ts @@ -86,9 +86,40 @@ describe("applicationApiFields — basic auth (#76)", () => { it(`refuses to enable basic auth with ${what}`, () => { expect(() => applicationApiFields({ is_http_basic_auth_enabled: true, ...fields }), - ).toThrow(/refusing to enable HTTP basic auth without/); + ).toThrow(/refusing a partial HTTP basic auth write/); }); } + + // The hole the #76 review found, at the wire: a username-only drift produces + // a PATCH with no toggle at all, so a guard keyed on `=== true` never looked + // at it. These are the shapes apply must never hand over uncompleted. + for (const [what, fields] of [ + ["a lone username", { http_basic_auth_username: "ops" }], + ["a lone password", { http_basic_auth_password: "s3cret" }], + [ + "credentials with no toggle", + { http_basic_auth_username: "ops", http_basic_auth_password: "s3cret" }, + ], + ] as const) { + it(`refuses ${what} — no toggle is not an exemption`, () => { + expect(() => applicationApiFields({ ...fields })).toThrow( + /refusing a partial HTTP basic auth write/, + ); + }); + } + + // A disable is a legitimate one-key write: it needs no credentials, and + // demanding them would make turning basic auth off impossible. + it("allows an explicit disable to travel alone", () => { + expect(() => + applicationApiFields({ is_http_basic_auth_enabled: false }), + ).not.toThrow(); + }); + + // And a payload that says nothing about basic auth is not a basic-auth write. + it("ignores a payload that does not mention basic auth at all", () => { + expect(() => applicationApiFields({ is_static: true })).not.toThrow(); + }); }); describe("databaseApiFields", () => {