Keep a URL config value out of the boot error that rejects it #202
@@ -6,6 +6,12 @@ at boot. Entries start at 0.3.0.
|
|||||||
|
|
||||||
## Unreleased
|
## 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
|
### 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
|
||||||
|
|||||||
@@ -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
|
- 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, 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
|
- 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/`
|
||||||
|
|||||||
@@ -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
|
||||||
|
|||||||
@@ -39,6 +39,23 @@ 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
|
||||||
});
|
});
|
||||||
|
|
||||||
|
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", () => {
|
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);
|
||||||
|
|||||||
+11
-10
@@ -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}"`);
|
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.
|
||||||
const value = env[key] || undefined;
|
function assertUrl(key: string, value: string): void {
|
||||||
if (value === undefined) return undefined;
|
|
||||||
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;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -16,10 +16,20 @@ export interface ProvisionResult {
|
|||||||
provisioned: string[];
|
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> {
|
export async function provisionStorage(options: ProvisionOptions): Promise<ProvisionResult> {
|
||||||
// Notices are left to surface: a REVOKE the account cannot perform only *warns*, and silencing
|
// 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.
|
// 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 {
|
try {
|
||||||
const provisioned: string[] = [];
|
const provisioned: string[] = [];
|
||||||
for (const pluginId of options.pluginIds) {
|
for (const pluginId of options.pluginIds) {
|
||||||
|
|||||||
@@ -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 -----------------------
|
// --- 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
|
// 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.
|
// tests cannot prove lives here: the owner may create tables, and a peer role is locked out.
|
||||||
|
|||||||
@@ -6,13 +6,13 @@
|
|||||||
|
|
||||||
`Bar = 1`
|
`Bar = 1`
|
||||||
|
|
||||||
`Next ID = 56`
|
`Next ID = 57`
|
||||||
|
|
||||||
## Items
|
## Items
|
||||||
|
|
||||||
| 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 |
|
| 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 |
|
| 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,19 +34,16 @@
|
|||||||
|
|
||||||
## Details
|
## 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}`,
|
README → What you must supply (about `:1275`: "refuses to boot on a missing or throwaway
|
||||||
so a malformed value reaches the boot error and the log. A DSN whose password breaks parsing, such as
|
`CSRF_SECRET` and **nothing else**") and README → Security model (about `:1442`:
|
||||||
`postgres://admin:pa/ss@db:5432/plugins`, leaks that password. The keys that can carry credentials:
|
"`REQUIRE_SECURE_SECRETS` guards only `CSRF_SECRET`") contradict the Configuration table row (about
|
||||||
`PLUGIN_DB_URL` (`readCredentiallessUrl` refuses userinfo only after `readOptionalUrl` has parsed it,
|
`:1216`), `src/config.ts:59` and `:190`, and `src/plugin-host/settings.ts`. The flag also refuses a
|
||||||
so a pasted admin DSN that fails to parse is echoed), and `OTLP_ENDPOINT`, whose collector URL may
|
missing or throwaway `PLUGIN_DB_SECRET` once `PLUGIN_DB_URL` is set, and every plugin setting
|
||||||
embed basic-auth userinfo. The other URL keys, `APP_URL`, `HYDRA_ADMIN_URL`, `JWKS_URL`,
|
declared `secret`. An operator following the checklist underrates the toggle. Make both passages say
|
||||||
`KETO_READ_URL`, `KETO_WRITE_URL`, `KRATOS_ADMIN_URL` and `KRATOS_PUBLIC_URL`, pass through the same
|
what the flag enforces by pointing to the table row, its one home. Four comprehension seats raised it
|
||||||
two readers, so the fix covers them too. Name the variable and what it accepts, never the value, and
|
independently. Markdown only.
|
||||||
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.
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user