Keep a URL config value out of the boot error that rejects it #202

Merged
lilleman merged 6 commits from item-55 into main 2026-10-31 14:13:48 +01:00
8 changed files with 144 additions and 25 deletions
+6
View File
@@ -6,6 +6,12 @@ at boot. Entries start at 0.3.0.
## Unreleased
### A malformed URL variable's boot error no longer prints the value
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.
### 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
+68
View File
@@ -222,3 +222,71 @@ Inherited architect, would restructure first:
- Locality, Overall: `src/http/app.ts:185` `handleRequest`: the most concerns and the highest complexity, already grandfathered, and every request-wide concern lands in it
- Shape, Navigation: `src/ui/stylesheet-floor.ts:15` `FLOOR`: the 3am rule's home is a test-only module under `ui/`
- Shape: `src/auth/routes.ts:275` `buildAuthRoutes`: the 19-module `src/auth/` mixes six concerns, and this file with `flow-view.ts` is where the next flow lands
## 2026-10-31T13:06:16Z, PR #202 at 8e73d16
| Seat | Navigation | Locality | Shape | Self-sufficiency | Overall |
|---|---|---|---|---|---|
| Junior A | 7 | 6 | 6.5 | 6.5 | 6.5 |
| Mid A | 7 | 6 | 6 | 6.5 | 6.5 |
| Maintainability senior | 7 | 6 | 6 | 6.5 | 6.5 |
| Inherited architect | 6.5 | 6 | 6 | 6.5 | 6.5 |
| Mean | 6.88 | 6.00 | 6.13 | 6.50 | 6.50 |
Junior A, hardest first:
- Locality, Overall: `src/http/app.ts:185` `handleRequest`: about 165 lines in a fixed order that only a top-to-bottom read shows
- Self-sufficiency, Shape: `src/auth/login.ts:40` `readPermissions`: the comment says `#members` where the code uses `granted`
- Self-sufficiency: `src/plugin-host/settings.ts:35` `SettingsOf`: nested conditional types that need the charter's rule to decode
Mid A, hardest first:
- Locality, Shape, Overall: `src/http/app.ts:185` `handleRequest`: complexity 43 inside the 318-line `createApp` closure, and `switchBase` reads `builtinRoutes` declared 60 lines later
- Locality, Shape: `src/ui/stylesheet-floor.ts:119` `positive`: a regex CSS walker whose how stayed opaque
- Self-sufficiency: `src/auth/bootstrap.ts:31` `permissionTuple`: the comment says `#members` where the code uses `granted`
Maintainability senior, hardest first:
- Locality, Shape, Overall: `src/http/app.ts:185` `handleRequest`: 167 lines and complexity 43 mixing twelve concerns whose order no type expresses
- Locality: `src/ui/stylesheet-floor.ts:119` `positive`: a hand-rolled regex CSS scanner traced by hand
- Self-sufficiency: `src/auth/gate.ts:13` `allows`: an empty gate is allowed, and the reason lives only in AGENTS.md
Inherited architect, would restructure first:
- Locality, Shape, Overall: `src/http/app.ts:185` `handleRequest`: the largest hub, where every request-wide concern lands
- Shape, Navigation: `src/auth/routes.ts:275` `buildAuthRoutes`: the 19-module `src/auth/` mixes four concerns with no sub-folder
- Shape, Navigation: `src/ui/stylesheet-floor.ts:30` `floorViolations`: test tooling sits among the runtime UI modules
## 2026-10-31T13:06:21Z, PR #202 rerun at 8e73d16
| Seat | Navigation | Locality | Shape | Self-sufficiency | Overall |
|---|---|---|---|---|---|
| Junior A | 7 | 5.5 | 6 | 6 | 6 |
| Mid A | 7 | 6 | 6 | 6 | 6.5 |
| Maintainability senior | 7 | 6 | 6 | 6 | 6.5 |
| Inherited architect | 7 | 5.5 | 6 | 7 | 6.5 |
| Mean | 7.00 | 5.75 | 6.00 | 6.25 | 6.38 |
Junior A, hardest first:
- Locality, Shape, Overall: `src/http/app.ts:185` `handleRequest`: closures over request state in a 318-line `createApp`, and it stayed opaque
- Self-sufficiency: `src/auth/routes.ts:58` `flowFailure`: the Kratos status semantics live outside the repo
- Self-sufficiency, Shape: `src/auth/login.ts:45` `readPermissions`: the comment says `#members` where the code uses `granted`
Mid A, hardest first:
- Locality, Shape, Overall: `src/http/app.ts:185` `handleRequest`: one pipeline whose steps mutate what later steps read
- Locality: `src/ui/stylesheet-floor.ts:119` `positive`: traced by hand
- Self-sufficiency, Shape: `src/auth/login.ts:45` `readPermissions`: the comment says `#members` where the code uses `granted`
Maintainability senior, hardest first:
- Locality, Shape, Overall: `src/http/app.ts:185` `handleRequest`: an unstated, load-bearing step order in shared closure state
- Locality: `src/ui/stylesheet-floor.ts:119` `positive`: a hand-rolled CSS scanner built on lookbehind regexes
- Self-sufficiency, Shape: `src/auth/bootstrap.ts:31` `permissionTuple`: the comment is false on the security-relevant path
Inherited architect, would restructure first:
- Locality, Shape, Overall: `src/http/app.ts:185` `handleRequest`: stage order lives only in line order, in a grandfathered complexity-43 function
- Shape, Navigation: `src/auth/routes.ts:275` `buildAuthRoutes`: 19 flat modules in `src/auth/`, and the next flow lands here
- Shape, Navigation: `src/ui/stylesheet-floor.ts:15` `FLOOR`: the 3am rule's home is a test-only module under `ui/`
+13
View File
@@ -291,3 +291,16 @@ Read most (opens and searches):
- `e2e-tests/full-flow.spec.ts` 1
- `examples/plugins/admin/admin-users.test.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
+17
View File
@@ -39,6 +39,23 @@ test("PLUGIN_DB_URL carrying credentials is refused, not silently overwritten",
assert.equal(loadConfig({}).pluginDbUrl, undefined); // unset ⇒ storage off
});
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<string, string> = {
APP_URL: "https://acme:pa/ss@[host",
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", () => {
assert.equal(resolvePluginDbConnectionLimit({}), 10);
assert.equal(resolvePluginDbConnectionLimit({ PLUGIN_DB_CONNECTION_LIMIT: "25" }), 25);
+11 -10
View File
@@ -98,26 +98,27 @@ 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}"`);
}
// 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) return undefined;
// Never echoes the value: a DSN whose password breaks URL parsing (a bare "/") would put the
// password in the boot error and the log.
function assertUrl(key: string, value: string): void {
try {
new URL(value);
} 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;
}
// An absolute URL: defaults to the Ory service; validated so a typo fails at boot.
function readUrl(env: Env, key: string, devDefault: string): string {
const value = env[key] ?? devDefault;
try {
new URL(value);
} catch {
throw new Error(`config: ${key} is not a valid URL: ${value}`);
}
assertUrl(key, value);
return value;
}
+11 -1
View File
@@ -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<typeof postgres> {
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<ProvisionResult> {
// 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) {
+7
View File
@@ -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.
+11 -14
View File
@@ -6,13 +6,13 @@
`Bar = 1`
`Next ID = 56`
`Next ID = 57`
## Items
| 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 |
| 56 | 0.4.22 | | | **Correct the README's two statements that `REQUIRE_SECURE_SECRETS` guards only `CSRF_SECRET`.** | 1 | 2 | 1 | 2 | 1 | code health by 2 | 14.4 |
| 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 |
| 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,19 +34,16 @@
## Details
### 55. Keep a URL config value out of the boot error that rejects it.
### 56. Correct the README's two statements that `REQUIRE_SECURE_SECRETS` guards only `CSRF_SECRET`.
`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.
README → What you must supply (about `:1275`: "refuses to boot on a missing or throwaway
`CSRF_SECRET` and **nothing else**") and README → Security model (about `:1442`:
"`REQUIRE_SECURE_SECRETS` guards only `CSRF_SECRET`") contradict the Configuration table row (about
`:1216`), `src/config.ts:59` and `:190`, and `src/plugin-host/settings.ts`. The flag also refuses a
missing or throwaway `PLUGIN_DB_SECRET` once `PLUGIN_DB_URL` is set, and every plugin setting
declared `secret`. An operator following the checklist underrates the toggle. Make both passages say
what the flag enforces by pointing to the table row, its one home. Four comprehension seats raised it
independently. Markdown only.
### 19. Run the documented Docker commands on macOS and fix whatever misbehaves.