fix: complete the basic-auth triple on username-only drift, at both guards
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.
This commit is contained in:
parent
4de89c360b
commit
5c2ccc697c
4 changed files with 136 additions and 10 deletions
36
src/apply.ts
36
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<string, unknown>,
|
||||
spec: Desired | undefined,
|
||||
): Record<string, unknown> {
|
||||
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];
|
||||
}
|
||||
|
|
|
|||
34
src/cli.ts
34
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"),
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue