From 8ffbb0b70dd2ed6d01e0b2158664f65e04aba7f3 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 31 Oct 2026 13:56:46 +0100 Subject: [PATCH] =?UTF-8?q?Answer=20stability=20round=201:=200=20?= =?UTF-8?q?=F0=9F=94=B4,=204=20=F0=9F=9F=A1=20(0=20misread);=20the=20drive?= =?UTF-8?q?r=20decides=20what=20parses=20so=20a=20multi-host=20DSN=20still?= =?UTF-8?q?=20works,=20and=20the=20nine-key=20test=20shrinks=20to=20four?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 4 ++-- src/auth/bootstrap.test.ts | 10 ---------- src/auth/bootstrap.ts | 5 ++--- src/config.test.ts | 8 +------- src/config.ts | 5 ++--- src/plugin-host/storage-provisioning.ts | 12 +++++++++++- src/plugin-host/storage.test.ts | 7 +++++++ 7 files changed, 25 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74d3b46..2281811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,9 +6,9 @@ at boot. Entries start at 0.3.0. ## Unreleased -### A malformed URL setting's boot error no longer prints the value +### A malformed URL variable's boot error no longer prints the value -When a URL setting such as `PLUGIN_DB_URL`, `PLUGIN_DB_ADMIN_URL` or `OTLP_ENDPOINT` can't be parsed, +When a URL variable such as `PLUGIN_DB_URL`, `PLUGIN_DB_ADMIN_URL` or `OTLP_ENDPOINT` can't be parsed, the boot error names the variable and not its value, so a DSN whose password breaks parsing doesn't put the password in the log. diff --git a/src/auth/bootstrap.test.ts b/src/auth/bootstrap.test.ts index 5040804..f57655b 100644 --- a/src/auth/bootstrap.test.ts +++ b/src/auth/bootstrap.test.ts @@ -181,16 +181,6 @@ test("provisioning still runs with nothing to provision, so orphans are reported assert.deepEqual(calls[0]?.pluginIds, []); }); -// The postgres driver rethrows a malformed DSN with the whole string on the error (`input`), so -// the URL is checked here, before the driver sees it. -test("a malformed PLUGIN_DB_ADMIN_URL is refused by name, never echoing the password", async () => { - const env = { PLUGIN_DB_ADMIN_URL: "postgres://admin:pa/ss@db:5432/plugins" }; - await assert.rejects( - provisionPluginStorage(env, [storagePlugin("things")], SILENT, async () => ({ orphans: [], provisioned: [] })), - (err: Error) => err.message.includes("PLUGIN_DB_ADMIN_URL") && !err.message.includes("pa/ss"), - ); -}); - test("a plugin declaring storage without a provisioning DSN fails loud, naming the plugin", async () => { const { calls, provision } = recordingProvisioner(); await assert.rejects( diff --git a/src/auth/bootstrap.ts b/src/auth/bootstrap.ts index 2e3c732..137ef59 100644 --- a/src/auth/bootstrap.ts +++ b/src/auth/bootstrap.ts @@ -8,7 +8,7 @@ // Then prints a first-run banner; fails loud on any unexpected upstream error. import { existsSync, writeFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; -import { assertUrl, resolvePluginDbConnectionLimit, resolvePluginDbSecret } from "../config.ts"; +import { resolvePluginDbConnectionLimit, resolvePluginDbSecret } from "../config.ts"; import { discoverPlugins } from "../plugin-host/discovery.ts"; import { declaredPermissions, isValidPermissionName, type Plugin } from "../plugin-host/plugin.ts"; import { provisionStorage } from "../plugin-host/storage-provisioning.ts"; @@ -179,7 +179,6 @@ export async function provisionPluginStorage(env: Env, plugins: Plugin[], log: L // Provisioned here, connected to from web: a different server means the role is created in one // place and looked for in another, surfacing inside a plugin as "password authentication failed". // Warned, not refused — web reaching a pooler that cannot run CREATE DATABASE is a legitimate split. - assertUrl("PLUGIN_DB_ADMIN_URL", adminUrl); const mismatch = serverMismatch(adminUrl, env["PLUGIN_DB_URL"]); if (mismatch) log.warn("PLUGIN_DB_ADMIN_URL and PLUGIN_DB_URL name different servers", { servers: mismatch }); const result = await provision({ @@ -200,7 +199,7 @@ export async function provisionPluginStorage(env: Env, plugins: Plugin[], log: L export function serverMismatch(adminUrl: string, webUrl: string | undefined): string | null { if (!webUrl) return null; const [admin, web] = [safeHostPort(adminUrl), safeHostPort(webUrl)]; - if (admin === null || web === null || admin === web) return null; // a malformed URL fails in assertUrl + if (admin === null || web === null || admin === web) return null; // a malformed URL fails in config.ts return `${admin} vs ${web}`; } diff --git a/src/config.test.ts b/src/config.test.ts index dbd9b11..87a591a 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -39,16 +39,10 @@ test("PLUGIN_DB_URL carrying credentials is refused, not silently overwritten", assert.equal(loadConfig({}).pluginDbUrl, undefined); // unset ⇒ storage off }); -// A DSN whose password breaks URL parsing (a bare "/") must not reach the boot error or the log. -// The credential-bearing keys get their own value; the rest share the two readers, so one loop covers them. test("a malformed URL value is refused by naming the variable, never echoing the value", () => { + // PLUGIN_DB_URL and OTLP_ENDPOINT can carry credentials; APP_URL and KRATOS_PUBLIC_URL stand for the other keys, one per reader. const bad: Record = { APP_URL: "https://acme:pa/ss@[host", - HYDRA_ADMIN_URL: "http://u:pa/ss@[hydra:4445", - JWKS_URL: "http://u:pa/ss@[jwks", - KETO_READ_URL: "http://u:pa/ss@[keto:4466", - KETO_WRITE_URL: "http://u:pa/ss@[keto:4467", - KRATOS_ADMIN_URL: "http://u:pa/ss@[kratos:4434", KRATOS_PUBLIC_URL: "http://u:pa/ss@[kratos:4433", OTLP_ENDPOINT: "http://otel:pa/ss@[collector:4318", PLUGIN_DB_URL: "postgres://admin:pa/ss@db:5432/plugins", diff --git a/src/config.ts b/src/config.ts index bfb81a9..b14dde7 100644 --- a/src/config.ts +++ b/src/config.ts @@ -99,9 +99,8 @@ function readEnum(env: Env, key: string, allowed: readonly T[] } // Never echoes the value: a DSN whose password breaks URL parsing (a bare "/") would put the -// password in the boot error and the log. bootstrap uses it too, before the postgres driver, which -// rethrows a malformed DSN with the whole string attached. -export function assertUrl(key: string, value: string): void { +// password in the boot error and the log. +function assertUrl(key: string, value: string): void { try { new URL(value); } catch { diff --git a/src/plugin-host/storage-provisioning.ts b/src/plugin-host/storage-provisioning.ts index c58d422..4c89b52 100644 --- a/src/plugin-host/storage-provisioning.ts +++ b/src/plugin-host/storage-provisioning.ts @@ -16,10 +16,20 @@ export interface ProvisionResult { provisioned: string[]; } +// The driver's error for a DSN it can't parse carries the whole string (`input`) — password included — +// so it is swallowed here. The driver, not `new URL`, decides what parses: it takes a multi-host DSN. +function connect(adminUrl: string): ReturnType { + try { + return postgres(adminUrl, { connect_timeout: 10, max: 1 }); + } catch { + throw new Error("PLUGIN_DB_ADMIN_URL must be a Postgres URL (its value is not shown; percent-encode special characters in a password)"); + } +} + export async function provisionStorage(options: ProvisionOptions): Promise { // Notices are left to surface: a REVOKE the account cannot perform only *warns*, and silencing // that would mean reporting a locked-down database that is still open to PUBLIC. - const sql = postgres(options.adminUrl, { connect_timeout: 10, max: 1 }); + const sql = connect(options.adminUrl); try { const provisioned: string[] = []; for (const pluginId of options.pluginIds) { diff --git a/src/plugin-host/storage.test.ts b/src/plugin-host/storage.test.ts index 3bc49c1..e34714d 100644 --- a/src/plugin-host/storage.test.ts +++ b/src/plugin-host/storage.test.ts @@ -108,6 +108,13 @@ test("a connection limit that is not a positive integer is refused, not interpol } }); +test("a malformed admin DSN is refused by name, without echoing its password", async () => { + await assert.rejects( + provisionStorage({ adminUrl: "postgres://admin:pa/ss@db:5432/plugins", connectionLimit: 10, pluginIds: [], secret: SECRET }), + (err: Error) => err.message.includes("PLUGIN_DB_ADMIN_URL") && !err.message.includes("pa/ss"), + ); +}); + // --- Integration: the statements above, against a real Postgres ----------------------- // Opt-in via PLUGIN_DB_ADMIN_URL (a superuser DSN); the unit gate runs no Postgres. What the unit // tests cannot prove lives here: the owner may create tables, and a peer role is locked out.