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", () => {