Keep a URL config value out of the boot error that rejects it
CI / full-gate (push) Failing after 1m6s

This commit is contained in:
2026-10-31 13:48:43 +01:00
parent 03bcf216a7
commit 217a36252e
7 changed files with 67 additions and 27 deletions
+6
View File
@@ -6,6 +6,12 @@ at boot. Entries start at 0.3.0.
## Unreleased ## Unreleased
### A malformed URL setting'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,
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.
### A secret plugin setting's bad value stays out of the boot error ### A secret plugin setting's bad value stays out of the boot error
When a `secret` setting's value fails validation, the boot error no longer prints it, so a malformed When a `secret` setting's value fails validation, the boot error no longer prints it, so a malformed
+13
View File
@@ -291,3 +291,16 @@ Read most (opens and searches):
- `e2e-tests/full-flow.spec.ts` 1 - `e2e-tests/full-flow.spec.ts` 1
- `examples/plugins/admin/admin-users.test.ts` 1 - `examples/plugins/admin/admin-users.test.ts` 1
- `examples/plugins/admin/admin-users.ts` 1 - `examples/plugins/admin/admin-users.ts` 1
## Reading cost, chunk f0a49c300aac, finished 2026-10-31T12:47:27Z
63526 new input tokens before the first commit.
Read most (opens and searches):
- `README.md` 4
- `src/plugin-host/settings.test.ts` 3
- `todo.md` 3
- `CHANGELOG.md` 2
- `src/plugin-host/settings.ts` 2
- `AGENTS.md` 1
+10
View File
@@ -181,6 +181,16 @@ test("provisioning still runs with nothing to provision, so orphans are reported
assert.deepEqual(calls[0]?.pluginIds, []); 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 () => { test("a plugin declaring storage without a provisioning DSN fails loud, naming the plugin", async () => {
const { calls, provision } = recordingProvisioner(); const { calls, provision } = recordingProvisioner();
await assert.rejects( await assert.rejects(
+3 -2
View File
@@ -8,7 +8,7 @@
// Then prints a first-run banner; fails loud on any unexpected upstream error. // Then prints a first-run banner; fails loud on any unexpected upstream error.
import { existsSync, writeFileSync } from "node:fs"; import { existsSync, writeFileSync } from "node:fs";
import { fileURLToPath } from "node:url"; import { fileURLToPath } from "node:url";
import { resolvePluginDbConnectionLimit, resolvePluginDbSecret } from "../config.ts"; import { assertUrl, resolvePluginDbConnectionLimit, resolvePluginDbSecret } from "../config.ts";
import { discoverPlugins } from "../plugin-host/discovery.ts"; import { discoverPlugins } from "../plugin-host/discovery.ts";
import { declaredPermissions, isValidPermissionName, type Plugin } from "../plugin-host/plugin.ts"; import { declaredPermissions, isValidPermissionName, type Plugin } from "../plugin-host/plugin.ts";
import { provisionStorage } from "../plugin-host/storage-provisioning.ts"; import { provisionStorage } from "../plugin-host/storage-provisioning.ts";
@@ -179,6 +179,7 @@ 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 // 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". // 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. // 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"]); 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 }); if (mismatch) log.warn("PLUGIN_DB_ADMIN_URL and PLUGIN_DB_URL name different servers", { servers: mismatch });
const result = await provision({ const result = await provision({
@@ -199,7 +200,7 @@ export async function provisionPluginStorage(env: Env, plugins: Plugin[], log: L
export function serverMismatch(adminUrl: string, webUrl: string | undefined): string | null { export function serverMismatch(adminUrl: string, webUrl: string | undefined): string | null {
if (!webUrl) return null; if (!webUrl) return null;
const [admin, web] = [safeHostPort(adminUrl), safeHostPort(webUrl)]; const [admin, web] = [safeHostPort(adminUrl), safeHostPort(webUrl)];
if (admin === null || web === null || admin === web) return null; // a malformed URL fails in config.ts if (admin === null || web === null || admin === web) return null; // a malformed URL fails in assertUrl
return `${admin} vs ${web}`; return `${admin} vs ${web}`;
} }
+23
View File
@@ -39,6 +39,29 @@ test("PLUGIN_DB_URL carrying credentials is refused, not silently overwritten",
assert.equal(loadConfig({}).pluginDbUrl, undefined); // unset ⇒ storage off 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", () => {
const bad: Record<string, string> = {
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",
};
for (const [key, value] of Object.entries(bad)) {
assert.throws(
() => loadConfig({ [key]: value }),
(err: Error) => err.message.includes(key) && !err.message.includes("pa/ss") && !err.message.includes(value),
key,
);
}
});
test("the per-role connection ceiling defaults to 10 and rejects nonsense", () => { test("the per-role connection ceiling defaults to 10 and rejects nonsense", () => {
assert.equal(resolvePluginDbConnectionLimit({}), 10); assert.equal(resolvePluginDbConnectionLimit({}), 10);
assert.equal(resolvePluginDbConnectionLimit({ PLUGIN_DB_CONNECTION_LIMIT: "25" }), 25); assert.equal(resolvePluginDbConnectionLimit({ PLUGIN_DB_CONNECTION_LIMIT: "25" }), 25);
+12 -10
View File
@@ -98,26 +98,28 @@ function readEnum<T extends string>(env: Env, key: string, allowed: readonly T[]
throw new Error(`config: ${key} must be one of ${allowed.join(", ")}, got "${value}"`); throw new Error(`config: ${key} must be one of ${allowed.join(", ")}, got "${value}"`);
} }
// An optional absolute URL: unset/empty ⇒ undefined; a set-but-malformed value fails at boot. // Never echoes the value: a DSN whose password breaks URL parsing (a bare "/") would put the
function readOptionalUrl(env: Env, key: string): string | undefined { // password in the boot error and the log. bootstrap uses it too, before the postgres driver, which
const value = env[key] || undefined; // rethrows a malformed DSN with the whole string attached.
if (value === undefined) return undefined; export function assertUrl(key: string, value: string): void {
try { try {
new URL(value); new URL(value);
} catch { } catch {
throw new Error(`config: ${key} is not a valid URL: ${value}`); throw new Error(`config: ${key} must be an absolute URL (its value is not shown; percent-encode special characters in a password)`);
} }
}
// An optional absolute URL: unset/empty ⇒ undefined; a set-but-malformed value fails at boot.
function readOptionalUrl(env: Env, key: string): string | undefined {
const value = env[key] || undefined;
if (value !== undefined) assertUrl(key, value);
return value; return value;
} }
// An absolute URL: defaults to the Ory service; validated so a typo fails at boot. // An absolute URL: defaults to the Ory service; validated so a typo fails at boot.
function readUrl(env: Env, key: string, devDefault: string): string { function readUrl(env: Env, key: string, devDefault: string): string {
const value = env[key] ?? devDefault; const value = env[key] ?? devDefault;
try { assertUrl(key, value);
new URL(value);
} catch {
throw new Error(`config: ${key} is not a valid URL: ${value}`);
}
return value; return value;
} }
-15
View File
@@ -12,7 +12,6 @@
| ID | Release | Blocked by | Exempt | Item | L | E | I | R | S | Moves | Score | | ID | Release | Blocked by | Exempt | Item | L | E | I | R | S | Moves | Score |
|---|---|---|---|---|---|---|---|---|---|---|---| |---|---|---|---|---|---|---|---|---|---|---|---|
| 55 | 0.4.21 | | security | **Keep a URL config value out of the boot error that rejects it.** | 1 | 1 | 4 | 5 | 2 | | 0.0 |
| 19 | 0.5.0 | | question | **Run the documented Docker commands on macOS and fix whatever misbehaves.** | 2 | 2 | 1 | 3 | 3 | 1 by 3 | 3.3 | | 19 | 0.5.0 | | question | **Run the documented Docker commands on macOS and fix whatever misbehaves.** | 2 | 2 | 1 | 3 | 3 | 1 by 3 | 3.3 |
| 16 | 0.5.0 | | question | **Decide whether the single Keto `Resource` namespace splits into per-domain namespaces.** | 3 | 2 | 3 | 6 | 6 | 7 by 2 | 0.5 | | 16 | 0.5.0 | | question | **Decide whether the single Keto `Resource` namespace splits into per-domain namespaces.** | 3 | 2 | 3 | 6 | 6 | 7 by 2 | 0.5 |
| 48 | 0.5.0 | | | **Correct the Keto tuple in the `login.ts` and `bootstrap.ts` comments to the `granted` relation the code writes.** | 1 | 1 | 1 | 1 | 1 | code health by 4 | 28.7 | | 48 | 0.5.0 | | | **Correct the Keto tuple in the `login.ts` and `bootstrap.ts` comments to the `granted` relation the code writes.** | 1 | 1 | 1 | 1 | 1 | code health by 4 | 28.7 |
@@ -34,20 +33,6 @@
## Details ## Details
### 55. Keep a URL config value out of the boot error that rejects it.
`readOptionalUrl` and `readUrl` in `src/config.ts` throw `config: ${key} is not a valid URL: ${value}`,
so a malformed value reaches the boot error and the log. A DSN whose password breaks parsing, such as
`postgres://admin:pa/ss@db:5432/plugins`, leaks that password. The keys that can carry credentials:
`PLUGIN_DB_URL` (`readCredentiallessUrl` refuses userinfo only after `readOptionalUrl` has parsed it,
so a pasted admin DSN that fails to parse is echoed), and `OTLP_ENDPOINT`, whose collector URL may
embed basic-auth userinfo. The other URL keys, `APP_URL`, `HYDRA_ADMIN_URL`, `JWKS_URL`,
`KETO_READ_URL`, `KETO_WRITE_URL`, `KRATOS_ADMIN_URL` and `KRATOS_PUBLIC_URL`, pass through the same
two readers, so the fix covers them too. Name the variable and what it accepts, never the value, and
add a test per credential-bearing key. The other readers in `config.ts` echo only non-credential
values. `PLUGIN_DB_ADMIN_URL` isn't read in `config.ts`: `bootstrap.ts` hands it straight to
`postgres()`. Check whether the driver echoes a malformed one.
### 19. Run the documented Docker commands on macOS and fix whatever misbehaves. ### 19. Run the documented Docker commands on macOS and fix whatever misbehaves.
Needs a Mac. Two suspects, both from `--user "$(id -u):$(id -g)"`: a macOS `id -g` is `20`, which is Needs a Mac. Two suspects, both from `--user "$(id -u):$(id -g)"`: a macOS `id -g` is `20`, which is