From 54ce80d755d769cc38bbc06c1ecc8da58b9f6979 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 08:50:29 +0000 Subject: [PATCH 1/2] fix(security): a bound credentialsRef reaches the postgres server on the DSN branch (#8873) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The postgres arm emitted `{ connectionString: url, password: spec.secret }`, which is correct at the knex-config layer and discarded one layer lower: `pg` merges `parse(config.connectionString)` OVER the explicit config, so the injected credential never reached the handshake. Measured on pg 8.22.0 + knex 3.3.0, a credential-free DSN with a secret bound resolved to password `null`; a stored pre-#8082 url embedding a password resolved to that url's own password instead of the bound one. Two independent mechanisms destroyed it: `parse()` emits a `password` key for every url (`''` when there is no userinfo password) which `Object.assign` copies over the injected value, and knex's `setHiddenProperty` has already made `password` non-enumerable, which `Object.assign` does not copy at all. On the DSN branch, and only when a secret is bound, `connectionString` is dropped: the arm hands `pg` its own parse of the url (`pg-connection-string`, the client's parser, so no second `postgresql://` dialect lives in this repo) with the credential applied after it, where nothing re-parses over it. A datasource that binds no secret is byte-for-byte unchanged. Not fixed by symmetry with either sibling arm: `mysql2` lets the explicit key win and mongodb rides in `options.auth`, while `pg` lets the DSN win. The competing userinfo-splice remedy was measured and rejected — a stored `?password=` beats userinfo in `pg-connection-string`, and it would put the cleartext credential into a string knex does not hide. The pin asserts on what the `pg` client resolves, never on the connection object the factory built: that assertion passed throughout this defect's life. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza --- ...ostgres-dsn-bound-secret-reaches-server.md | 74 ++++ .../services/service-datasource/package.json | 3 +- .../postgres-dsn-bound-secret.test.ts | 355 ++++++++++++++++++ .../src/default-datasource-driver-factory.ts | 169 ++++++++- pnpm-lock.yaml | 3 + 5 files changed, 592 insertions(+), 12 deletions(-) create mode 100644 .changeset/postgres-dsn-bound-secret-reaches-server.md create mode 100644 packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts diff --git a/.changeset/postgres-dsn-bound-secret-reaches-server.md b/.changeset/postgres-dsn-bound-secret-reaches-server.md new file mode 100644 index 0000000000..bebd80a09f --- /dev/null +++ b/.changeset/postgres-dsn-bound-secret-reaches-server.md @@ -0,0 +1,74 @@ +--- +"@objectstack/service-datasource": patch +--- + +fix(security): a bound `external.credentialsRef` reaches the postgres SERVER on the DSN branch, not just the knex config (#8873) + +A postgres datasource whose `config.url` is a DSN and whose credential is bound +through `external.credentialsRef` (or the connection form's secret field) opened +its connection **with no password at all**. Not a disclosure — a broken binding, +of the fail-quietly kind: `DatasourceConnectionService` resolved the secret +fail-closed, the operator saw a bound credential and a datasource reporting +connected, and the handshake carried nothing. + +**This arm was the one that looked correct.** It had an explicit secret branch +and a comment declaring the intent — *"For a DSN, a separately-supplied secret +overrides the embedded password"* — and it emitted +`{ connectionString: url, password: spec.secret }`, which passes any assertion +written against the factory's own output. `pg` discarded the credential one +layer lower: + +```js +// pg 8.22.0, lib/connection-parameters.js +if (config.connectionString) { + config = Object.assign({}, config, parse(config.connectionString)) +} +``` + +Two independent mechanisms destroyed it, either sufficient on its own. `parse()` +emits a `password` key for **every** url — `''` when the url carries no userinfo +password — and `Object.assign` copies that over the injected value, after which +`val('password', …)` falls through to `PGPASSWORD` and the defaults; and knex's +`setHiddenProperty` has already made `password` a non-enumerable own property of +`connectionSettings`, which `Object.assign` does not copy at all. Measured on pg +8.22.0 + knex 3.3.0: `postgresql://app@db.internal:5432/app` with a secret bound +resolved to password `null`, and a stored pre-#8082 url embedding +`app:embedded-legacy@` resolved to `'embedded-legacy'` — the DSN beating the +credential an operator deliberately bound. Since #8082 refuses a +`user:password@` userinfo at the publish door, the credential-free DSN is the +only authorable URL shape for this driver, so this was the shape the connection +form produces. + +**The remedy is a third shape, not either sibling's.** The clients merge a DSN +against explicit keys in opposite directions: `mysql2` lets the explicit key win +(`{ uri, password }`, #8875) and mongodb rides in `options.auth` beside an +untouched url (#9042), while `pg` lets the DSN win. So on the postgres DSN +branch — and only when a secret is bound — `connectionString` is gone: the arm +hands `pg` **pg's own parse of the url** (`pg-connection-string`, the client's +parser, so there is no second dialect of `postgresql://…` in this repo to drift +out of agreement) with the credential applied afterwards, where nothing +re-parses over it. Everything else resolves exactly as before, verified +key-by-key across the sslmode, unix-socket, `?options=`, credential-free, +embedded-password and no-userinfo forms. + +The competing remedy — keep `connectionString` and splice the secret into the +userinfo — was measured and rejected on two counts: `pg-connection-string` +honours a `?password=` query parameter **over** userinfo, so a stored pre-#8337 +row would still lose the bound secret; and it would materialise the cleartext +credential into a string nothing hides (`JSON.stringify` of knex's +`connectionSettings` prints the whole DSN, while a discrete `password` stays +hidden), re-creating at connect time the hardest-to-redact credential spelling +that #8082 refuses to let anyone author. + +**What changes for an existing deployment.** A DSN datasource that binds no +secret is byte-for-byte unaffected — it still hands `pg` the url unparsed. One +behaviour worth knowing: a stored pre-#8082 row that embeds a password in its +url *and* binds a credential now authenticates with the **bound** credential, +which is the precedence this arm's own comment always claimed and both sibling +arms already apply. A DSN naming no user still receives the credential (unlike +the mongodb arm's deliberate no-op there): `pg` sends a password only when the +server asks for one, so injecting cannot break a datasource that connects today. +Finally, a url `pg`'s own parser rejects (a multi-host DSN, which node-postgres +does not implement) is now refused when the driver is built rather than on first +query — the same error, named and located, with the url deliberately not echoed +because it may itself embed a credential. diff --git a/packages/services/service-datasource/package.json b/packages/services/service-datasource/package.json index 6cad408cf9..560fe8b502 100644 --- a/packages/services/service-datasource/package.json +++ b/packages/services/service-datasource/package.json @@ -28,7 +28,8 @@ "dependencies": { "@objectstack/core": "workspace:*", "@objectstack/spec": "workspace:*", - "@objectstack/types": "workspace:*" + "@objectstack/types": "workspace:*", + "pg-connection-string": "^2.14.0" }, "devDependencies": { "@objectstack/driver-memory": "workspace:*", diff --git a/packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts b/packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts new file mode 100644 index 0000000000..80f4776df9 --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts @@ -0,0 +1,355 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8873 — a bound `external.credentialsRef` reaches the SERVER on the postgres + * arm's DSN branch, not merely the knex config. + * + * ## The defect, and why it survived a passing sibling pin + * + * `DatasourceConnectionService` resolves `external.credentialsRef` to a + * cleartext secret and hands it to this factory as `spec.secret`. The postgres + * arm returned `{ connectionString: url, password: spec.secret }` — an explicit + * secret branch, a comment declaring the intent, and a config object that + * passes any assertion written against it. `pg` then threw the credential away + * one layer lower: + * + * ```js + * // pg 8.22.0, lib/connection-parameters.js + * if (config.connectionString) { + * config = Object.assign({}, config, parse(config.connectionString)) + * } + * ``` + * + * Measured on `origin/main` @ c308a4fd8, driver `postgres`, secret bound: + * + * ```text + * config.url 'postgresql://app@db.internal:5432/app' -> password null + * config.url 'postgresql://app:embedded-legacy@db.internal/app' -> password 'embedded-legacy' + * ``` + * + * Two independent mechanisms destroy it, and either alone is sufficient: + * `parse()` emits a `password` key for EVERY url (`''` when the url carries no + * userinfo password) which `Object.assign` copies over the injected value; and + * knex's `setHiddenProperty` has already made `password` a NON-ENUMERABLE own + * property of `connectionSettings`, which `Object.assign` does not copy at all. + * + * ## What is asserted, and at which layer — the constraint this file exists for + * + * Every credential assertion below reads what the **`pg` client resolved**, via + * the `pg` module knex itself will use (`knex.client.driver.Client`) fed the + * exact `connectionSettings` knex will hand it. Nothing asserts on the + * `connection` object this factory emitted, deliberately: that assertion PASSED + * throughout this defect's entire life. It is the same lesson the mongodb half + * of `bound-secret-dsn-branches.test.ts` records, whose header names this arm by + * name — *"the postgres arm passes the equivalent config-layer assertion while + * still being broken below it"* — inherited here rather than re-learned. + * + * No connection is opened anywhere in this file: `new Client(config)` resolves + * `connectionParameters` in its constructor and does no I/O, which is exactly + * what makes this seam assertable without a server. + * + * ## Why the remedy is a third shape again, not either sibling's + * + * `mysql2` merges a `uri` UNDER its sibling keys (explicit key wins → + * `{ uri, password }`); mongodb rides in `options.auth` beside an untouched + * url. `pg` merges the other way, so the only shape that survives is a config + * it will not re-parse: pg's own `parse()` of the url, spread as the connection + * itself, with `password` applied afterwards. The competing remedy — splice the + * secret into the url's userinfo — is measured in this file too: it does not + * even fix the defect, because `pg-connection-string` honours `?password=` over + * userinfo. + * + * ## Reverse verification (predicted in writing before running) + * + * Predicted with the pre-fix branch (`{ connectionString: url, ...(secret ? + * { password } : {}) }`) restored over these tests at their fixed state: the + * FIVE injecting cases go red on the resolved password, and the remaining cases + * stay green — the no-secret passthrough, the two discrete-branch controls, the + * "nothing else changed" equivalence sweep (it excludes `password` on purpose) + * and the unparseable-DSN refusal, which the old branch never reached. The + * shape matters: an arm-wide regression would mean this file measures something + * other than the branch-local defect. Measured exactly that set — see the PR + * body for the run. + */ + +import { describe, it, expect } from 'vitest'; +import { createDefaultDatasourceDriverFactory } from '../default-datasource-driver-factory.js'; + +const factory = () => createDefaultDatasourceDriverFactory({ dev: false }); + +/** The cleartext `DatasourceConnectionService` resolves a `credentialsRef` to. */ +const BOUND_SECRET = 's3cr3t-from-sys_secret'; + +/** + * The one authorable postgres URL shape post-#8082: a username, never a + * password. `PostgresConfigSchema.url` states the contract this arm failed to + * keep, verbatim — *"bind the secret (`external.credentialsRef` / the + * connection form's secret field) and it is injected at connect time. A bare + * username (`user@host`) stays writable."* + */ +const BARE_USERNAME_DSN = 'postgresql://app@db.internal:5432/app'; + +/** Build the driver and return the `pg` client knex would open the pool with. */ +async function pgClient(spec: Record): Promise { + const handle: any = await factory().create({ driver: 'postgres', ...spec } as any); + try { + const knexClient = (handle.driver ?? handle).knex.client; + // `knex.client.driver` is the `pg` module knex resolved for itself, and + // `connectionSettings` is the object it passes to `new Client(...)` in + // `acquireRawConnection`. Reading both from knex rather than importing `pg` + // here keeps the pin on the code path that actually runs. + return new knexClient.driver.Client(knexClient.connectionSettings); + } finally { + // The pool is never opened — nothing in this file connects. + try { await handle.disconnect?.(); } catch { /* noop */ } + } +} + +/** Everything `pg` resolved for the handshake, credential included. */ +async function pgResolved(spec: Record): Promise> { + const client = await pgClient(spec); + const p = client.connectionParameters; + return { + user: p.user, + // Direct property access, never JSON.stringify or Object.keys: `pg` hides + // `password` exactly as knex does, so a serialising probe reports a dropped + // secret that is in fact present. + password: p.password, + host: p.host, + port: p.port, + database: p.database, + ssl: p.ssl, + application_name: p.application_name, + statement_timeout: p.statement_timeout, + options: p.options, + connect_timeout: p.connect_timeout, + client_encoding: p.client_encoding, + }; +} + +/** The `connection` object the factory emitted — the layer that CANNOT judge this. */ +async function emittedConnection(spec: Record): Promise { + const handle: any = await factory().create({ driver: 'postgres', ...spec } as any); + try { + const driver = handle.driver ?? handle; + return (driver?.config ?? {}).connection; + } finally { + try { await handle.disconnect?.(); } catch { /* noop */ } + } +} + +describe('#8873 — postgres: a bound secret reaches the CLIENT on the DSN branch', () => { + it('resolves the bound secret as the handshake password instead of nothing', async () => { + // The whole card in one assertion. Before the fix this resolved to `null`: + // `parse()` contributed `password: ''` and `Object.assign` copied it over + // the injected value, so the datasource reported connected while having + // authenticated with no credential at all. + const resolved = await pgResolved({ + name: 'orders', + config: { url: BARE_USERNAME_DSN }, + secret: BOUND_SECRET, + }); + + expect(resolved.password).toBe(BOUND_SECRET); + // And the rest of the DSN still describes the same server: the credential + // is attached to pg's own reading of the url, not to a re-parsed one. + expect(resolved).toMatchObject({ + user: 'app', + host: 'db.internal', + port: 5432, + database: 'app', + }); + }); + + it('lets the bound secret win over a legacy password embedded in a stored DSN', async () => { + // #8082 refuses this url at the publish door, so it can only arrive as a + // stored pre-#8082 row. Before the fix the DSN's own password won — the + // arm's comment promised the opposite ("a separately-supplied secret + // overrides the embedded password") and `pg` decided otherwise. + const resolved = await pgResolved({ + name: 'legacy-userinfo', + config: { url: 'postgresql://app:embedded-legacy@db.internal:5432/app' }, + secret: BOUND_SECRET, + }); + + expect(resolved.password).toBe(BOUND_SECRET); + expect(resolved.user).toBe('app'); + }); + + it('wins over a `?password=` query parameter — the spelling that defeats a userinfo splice', async () => { + // `pg-connection-string` copies every query parameter into the config and + // `?password=` beats userinfo (measured; the reason #8337 refuses it at + // authoring, so this too is a stored-row-only shape). This case is what + // rules OUT the competing remedy: splicing the secret into the userinfo + // resolves to 'from-query-param' here, i.e. it leaves the card's defect + // live for this row. Overriding the PARSED key wins over every spelling + // because it is applied after the parse. + const resolved = await pgResolved({ + name: 'legacy-query', + config: { url: 'postgresql://app@db.internal:5432/app?password=from-query-param' }, + secret: BOUND_SECRET, + }); + + expect(resolved.password).toBe(BOUND_SECRET); + }); + + it('carries the credential on a DSN that names no user', async () => { + // Deliberately UNLIKE the mongodb arm, which no-ops here. The asymmetry is + // mechanical, not a second policy: `MongoClient` cannot carry a password + // without a username and injecting `{username:''}` would turn a working + // anonymous connection into a guaranteed failure, whereas `pg` sends a + // password only when the server asks for one — so injecting cannot break a + // datasource that connects today, and refusing would silently drop a + // credential the operator bound. Making the contradictory pair loud belongs + // at the authoring door (#9041), which this card lands before. + const resolved = await pgResolved({ + name: 'anonymous-url', + config: { url: 'postgresql://db.internal:5432/app' }, + secret: BOUND_SECRET, + }); + + expect(resolved.password).toBe(BOUND_SECRET); + expect(resolved.database).toBe('app'); + }); + + it('keeps the datasource\'s own `ssl` block and pg extras reaching the client', async () => { + // The DSN branch carries more than a credential, and dropping any of it + // while fixing the credential would be the same class of defect one key + // over. #4410 made these keys real; they must stay real on the branch this + // card rewrites. + const resolved = await pgResolved({ + name: 'tls', + config: { + url: BARE_USERNAME_DSN, + applicationName: 'objectstack', + statementTimeout: 30000, + }, + ssl: { enabled: true, rejectUnauthorized: false }, + secret: BOUND_SECRET, + }); + + expect(resolved.password).toBe(BOUND_SECRET); + expect(resolved.ssl).toMatchObject({ rejectUnauthorized: false }); + expect(resolved.application_name).toBe('objectstack'); + expect(resolved.statement_timeout).toBe(30000); + }); + + it('changes NOTHING a bound secret does not touch (equivalence sweep)', async () => { + // The safety half of the remedy. Dropping `connectionString` means this arm + // no longer lets `pg` read the url itself, so every non-credential key the + // url contributes has to arrive by the new route unchanged — including the + // query parameters only `pg-connection-string` knows to copy. Compared + // key-by-key against the same datasource with no secret bound, which still + // takes the untouched `connectionString` path. + const urls = [ + BARE_USERNAME_DSN, + 'postgres://app@db.internal/app?sslmode=require', + 'postgres://app@db.internal/app?sslmode=disable', + 'postgresql://app@db.internal:5432/app?application_name=from-url&connect_timeout=10', + 'postgresql://app@db.internal:5432/app?options=-c%20geqo%3Doff', + 'postgresql:///app?host=/var/run/postgresql', + 'postgresql://app@db.internal:5432/app', + ]; + + for (const url of urls) { + const config = { url, applicationName: 'objectstack', statementTimeout: 30000 }; + const withSecret = await pgResolved({ name: 'sweep', config, secret: BOUND_SECRET }); + const without = await pgResolved({ name: 'sweep', config }); + + // `password` is the one key this card changes; everything else must be + // what `pg` derived from the url before, verbatim. + const { password: injected, ...restWith } = withSecret; + const { password: _none, ...restWithout } = without; + expect(restWith, url).toEqual(restWithout); + expect(injected, url).toBe(BOUND_SECRET); + } + }); + + it('leaves a DSN with nothing bound exactly as it was (no behaviour change)', async () => { + // Blast radius is "a secret was bound". A datasource that binds none must + // reach `pg` byte-for-byte as before — still through `connectionString`, + // still parsed by the client, still with whatever password its own url + // implies, which is not this change's business to alter. + const spec = { name: 'anon', config: { url: BARE_USERNAME_DSN } }; + + expect(await emittedConnection(spec)).toMatchObject({ connectionString: BARE_USERNAME_DSN }); + expect((await pgResolved(spec)).password).toBeNull(); + }); + + it('hands `pg` no `connectionString` to re-parse once a secret is bound', async () => { + // The one deliberate config-layer assertion in this file, and it is about + // the MECHANISM rather than the credential: a future refactor that puts the + // url back beside the password would restore the exact defect this card + // closed, and would do it while every credential assertion above still + // reads as intentional code. `connectionString`'s absence is what makes + // them true. + const conn = await emittedConnection({ + name: 'orders', + config: { url: BARE_USERNAME_DSN }, + secret: BOUND_SECRET, + }); + + expect(conn.connectionString).toBeUndefined(); + expect(conn.host).toBe('db.internal'); + }); + + it('refuses a DSN pg cannot parse, without echoing the url', async () => { + // Multi-host DSNs are a libpq feature `pg` does not implement: measured, + // `parse` and `new ConnectionParameters` BOTH throw ERR_INVALID_URL on this + // url, so the datasource has never been able to connect. The parse moving + // to build time makes the failure earlier and named instead of arriving as + // a bare `Invalid URL` on first query — and both `create()` call sites turn + // a throw here into a located datasource failure, not a boot crash. + const url = 'postgresql://app:embedded-legacy@h1:5432,h2:5433/app'; + await expect(factory().create({ + name: 'multi-host', + driver: 'postgres', + config: { url }, + secret: BOUND_SECRET, + } as any)).rejects.toThrow(/pg's own parser rejects/); + + // The message must not carry the url: a stored row may embed a credential + // in exactly that string, which is why `pg` redacts it in its own error. + let err: Error | undefined; + try { + await factory().create({ + name: 'multi-host', + driver: 'postgres', + config: { url }, + secret: BOUND_SECRET, + } as any); + } catch (e) { + err = e as Error; + } + expect(err?.message).not.toContain('embedded-legacy'); + expect(err?.message).not.toContain(url); + expect(err?.message).toContain('multi-host'); + }); + + it('still reads the bound secret on the discrete-fields branch (control)', async () => { + // Green before this change and after it: the branch that already worked is + // what makes the DSN branch's silence a per-branch asymmetry rather than an + // arm that never read the secret at all. + const resolved = await pgResolved({ + name: 'discrete', + config: { host: 'db.internal', port: 5432, database: 'app', username: 'app' }, + secret: BOUND_SECRET, + }); + + expect(resolved.password).toBe(BOUND_SECRET); + expect(resolved.user).toBe('app'); + }); + + it('keeps preferring the bound secret over an inline `config.password` (control)', async () => { + // `config.password` is `z.never()` at every authoring door since #7990, so + // this is a stored-row-only shape; the discrete branch's precedence is + // unchanged by this card. + const resolved = await pgResolved({ + name: 'discrete-legacy', + config: { host: 'db.internal', database: 'app', username: 'app', password: 'inline-legacy' }, + secret: BOUND_SECRET, + }); + + expect(resolved.password).toBe(BOUND_SECRET); + }); +}); diff --git a/packages/services/service-datasource/src/default-datasource-driver-factory.ts b/packages/services/service-datasource/src/default-datasource-driver-factory.ts index 11bdd58fff..89caf401d8 100644 --- a/packages/services/service-datasource/src/default-datasource-driver-factory.ts +++ b/packages/services/service-datasource/src/default-datasource-driver-factory.ts @@ -46,6 +46,7 @@ */ import { join } from 'node:path'; +import { parse as parsePostgresConnectionString } from 'pg-connection-string'; import { resolveDriverId, urlUserinfoUsername, type BuiltinDriverId } from '@objectstack/spec/data'; import type { IDatasourceDriverFactory, @@ -370,6 +371,59 @@ function resolveSslOption(spec: DatasourceConnectionSpec): unknown { return shorthand == null ? undefined : shorthand; } +/** + * What this factory says when `pg`'s own parser rejects a postgres DSN that + * carries a bound credential (#8873). + * + * Reached only on the secret-bound path, and only for a url `pg` itself cannot + * read: {@link postgresDsnFields} runs the client's own parser, so anything it + * rejects would raise the identical error at connect time today (measured on + * pg 8.22.0 — `postgresql://app@h1:5432,h2:5433/app` throws `ERR_INVALID_URL` + * from `new ConnectionParameters` exactly as it does from `parse`). The failure + * is therefore moved earlier and named, never invented: this datasource has + * never been able to open a connection. + * + * ⚠️ The url is deliberately NOT echoed. It is the one string in this arm that + * can still carry a credential — a stored pre-#8082 row may embed a userinfo + * password, and a pre-#8337 row a `?password=` — which is why `pg` redacts it + * in its own error (`input: '*****REDACTED*****'`, measured). A message that + * quoted the url to be helpful would put that credential into every log that + * records a failed datasource build. + */ +function unparseablePostgresDsnMessage(args: { datasource?: string; cause: unknown }): string { + const where = args.datasource ? `datasource '${args.datasource}'` : 'this postgres datasource'; + const cause = args.cause instanceof Error ? args.cause.message : String(args.cause); + return ( + `The postgres ${where} binds a credential to a connection url that pg's own parser rejects ` + + `(${cause}). The url is not shown here because it may itself embed a credential. ` + + `pg raises the same error when it opens a connection, so this datasource cannot connect ` + + `with or without the bound secret — correct \`config.url\` to a form pg accepts ` + + `(\`postgresql://[user@][host][:port][/dbname][?params]\`, single host only).` + ); +} + +/** + * `pg`'s own decomposition of a DSN, used as the connection config itself. + * + * Parsing with the client's parser rather than a hand-rolled one is the whole + * safety argument for {@link buildSqlConnection}'s postgres DSN branch: the + * fields handed to `pg` are the fields `pg` would have derived from the same + * string, by construction, so there is no second dialect of `postgresql://…` + * in this repo to drift out of agreement with the client. It also carries the + * parts a partial parse would silently drop — `pg-connection-string` copies + * EVERY query parameter into the config, which is how `?sslmode=`, + * `?application_name=`, `?options=` and `?connect_timeout=` reach the client at + * all (the same mechanism that makes `?password=` a credential spelling, which + * `PostgresConfigSchema` refuses at authoring since #8337). + */ +function postgresDsnFields(url: string, datasource?: string): Record { + try { + return parsePostgresConnectionString(url) as unknown as Record; + } catch (err) { + throw new Error(unparseablePostgresDsnMessage({ datasource, cause: err })); + } +} + /** Build the Knex `connection` for a SQL driver from a spec's config + secret. */ function buildSqlConnection(spec: DatasourceConnectionSpec, client: 'pg' | 'better-sqlite3'): unknown { const cfg = (spec.config ?? {}) as Record; @@ -387,15 +441,104 @@ function buildSqlConnection(spec: DatasourceConnectionSpec, client: 'pg' | 'bett const ssl = resolveSslOption(spec); const url = cfg.url as string | undefined; if (url) { - // For a DSN, a separately-supplied secret overrides the embedded password. - // TLS still applies: `sslmode` in a DSN and the `ssl` option are separate - // channels to `pg`, and a datasource that declares one should get it. - return { - connectionString: url, - ...(spec.secret ? { password: spec.secret } : {}), + // The sibling keys, in the order they have always been layered. `pg` lets + // a DSN's own `sslmode` override the datasource's `ssl` block and a DSN's + // `?application_name=` override `config.applicationName`; both branches + // below keep that precedence by putting the DSN's contribution last. + const siblings = { ...(ssl !== undefined ? { ssl } : {}), ...pgConnectionExtras(cfg), }; + + // Nothing bound: byte-for-byte the shape this arm has always emitted. The + // blast radius of #8873 is "a secret was bound", so a datasource that binds + // none must not change at all — including keeping the DSN unparsed here, so + // a url `pg` rejects still fails where it fails today. + if (!spec.secret) return { connectionString: url, ...siblings }; + + // A bound secret, on the DSN branch (#8873). + // + // ## Why `connectionString` is gone rather than accompanied + // + // This arm used to return `{ connectionString: url, password: spec.secret }` + // and it was the one arm that LOOKED right — an explicit secret branch and a + // comment declaring the intent — while dropping the credential one layer + // below, where no assertion on this function's output can see it. `pg` + // merges a DSN over the config rather than under it: + // + // ```js + // // pg 8.22.0, lib/connection-parameters.js + // if (config.connectionString) { + // config = Object.assign({}, config, parse(config.connectionString)) + // } + // ``` + // + // So the injected password is destroyed TWICE over, by two independent + // mechanisms (both measured on pg 8.22.0 + knex 3.3.0): + // + // 1. `parse()` emits a `password` key for every url — `''` when the DSN + // carries no userinfo password — and `Object.assign` copies it over the + // injected value. `val('password', …)` then reads `''`, falls through to + // `PGPASSWORD` and `defaults`, and the effective password is `null`. + // 2. knex hides the key first: `setHiddenProperty` makes `password` a + // NON-ENUMERABLE own property of `connectionSettings`, and + // `Object.assign` copies only enumerable ones — so it never reaches the + // merge in the first place. + // + // Measured before this change, `postgresql://app@db.internal:5432/app` with + // a secret bound: effective password `null`. With a stored pre-#8082 url + // embedding `app:embedded-legacy@`: effective password `'embedded-legacy'` + // — the DSN beating the credential an operator deliberately bound. + // + // ⛔ Not fixable by symmetry with either sibling arm, and this is the whole + // point of the card: `mysql2` merges a `uri` UNDER its sibling keys (the + // explicit key wins, which is why `buildMysqlConnection` returns + // `{ uri, password }`), and the mongo arm rides beside an untouched url in + // `options.auth`. `pg` merges the other way, so the only place a credential + // survives is a config the client will not re-parse. + // + // ## Why pg's own parse, and not a re-serialised userinfo + // + // The competing remedy — keep `connectionString` and splice the secret into + // the userinfo — was measured and rejected on two counts: + // + // - **It does not even fix the defect.** `pg-connection-string` honours a + // `?password=` query parameter OVER the userinfo (the reason #8337 + // refuses that spelling at authoring). Measured: a stored pre-#8337 url + // `postgresql://app@db.internal:5432/app?password=from-query-param` + // with the secret spliced into the userinfo still resolves to + // `'from-query-param'`. Overriding the parsed `password` key wins over + // every spelling, because it is applied after the parse. + // - **It would materialise the cleartext credential into a string nothing + // hides.** Measured on knex 3.3.0: with the secret in a discrete + // `password`, `JSON.stringify(client.connectionSettings)` prints + // `{"host":…,"user":"app"}` and the secret is absent; with the secret + // spliced into the url it prints the whole DSN, credential included. + // That is also the shape #8082 refuses to let anyone author and + // `redactUrlPassword` exists to scrub — synthesising it at connect time + // would push the platform's own hardest-to-redact credential spelling + // back into circulation. + // + // Everything except `password` is therefore what `pg` would have computed + // from the same url: verified key-by-key (`user`/`database`/`port`/`host`/ + // `ssl`/`application_name`/`statement_timeout`/`options`/`connect_timeout`/ + // `client_encoding`) across the sslmode, unix-socket, `?options=`, + // credential-free, embedded-password and no-userinfo forms — identical in + // every case. + // + // A url naming no user still gets the credential, unlike the mongo arm's + // deliberate no-op there. The asymmetry is mechanical, not a second policy: + // `MongoClient` cannot carry a password without a username and would turn a + // working anonymous connection into a guaranteed failure, whereas `pg` sends + // a password only when the server asks for one — so injecting cannot break + // a datasource that connects today, and refusing would drop a credential the + // operator bound. Making that contradictory pair loud belongs at the + // authoring door, where both halves are visible at once (#9041). + return { + ...siblings, + ...postgresDsnFields(url, spec.name), + password: spec.secret, + }; } return { host: cfg.host, @@ -474,9 +617,12 @@ function buildSqlPool(spec: DatasourceConnectionSpec): Record { * ⛔ Do NOT copy this shape to the postgres arm. `pg` does the OPPOSITE merge — * `Object.assign({}, config, parse(config.connectionString))`, i.e. the DSN * overrides the explicit key — so `{connectionString, password}` there resolves - * to the DSN's own (absent) password. That is a live defect, filed separately; - * it is NOT fixed by symmetry with this arm, and the two clients disagreeing is - * exactly why each arm's precedence is measured rather than assumed. + * to the DSN's own (absent) password. That was a live defect for as long as this + * comment described it as one; #8873 closed it by dropping `connectionString` + * entirely on that branch and handing `pg` its own parse of the url with the + * credential attached. It was NOT fixed by symmetry with this arm, and the two + * clients disagreeing is exactly why each arm's precedence is measured rather + * than assumed. * * A DSN with NOTHING bound still passes through as the bare string, so a * datasource that never bound a secret is byte-for-byte unaffected. @@ -661,8 +807,9 @@ function buildMongoUrl(spec: DatasourceConnectionSpec): string { * ⛔ Do NOT reach this shape by symmetry from the mysql arm. The clients merge * a DSN against explicit keys in OPPOSITE directions — `pg` merges * `parse(connectionString)` OVER the explicit config, which is why the postgres - * arm looks correct and is broken one layer lower (filed separately). Each - * arm's precedence is measured against its own client. + * arm looked correct while being broken one layer lower, and why #8873 had to + * close it with a THIRD shape again (no `connectionString` at all). Each arm's + * precedence is measured against its own client. * * ## Why a userinfo-free url gets NOTHING, deliberately * diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 83e5677be1..7862ce4468 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2272,6 +2272,9 @@ importers: '@objectstack/types': specifier: workspace:* version: link:../../types + pg-connection-string: + specifier: ^2.14.0 + version: 2.14.0 devDependencies: '@objectstack/driver-memory': specifier: workspace:* From e36872f428ba031459bd76a7452619932f77bead Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 08:54:27 +0000 Subject: [PATCH 2/2] test(service-datasource): record the #8873 reverse verification, predicted then measured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8 failed / 3 passed with the pre-fix branch restored, case for case as predicted: the five credential cases red on the resolved password (null for the credential-free urls, someone else's credential for the two stored rows), the equivalence sweep red on its separate injected-value assertion, the connectionString-absence mechanism pin red, and the unparseable-DSN refusal red by a different route — the old branch never parses, so it never throws. Green throughout: the no-secret passthrough and both discrete-branch controls, which is the branch-local shape this file needs to be measuring. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza --- .../postgres-dsn-bound-secret.test.ts | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts b/packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts index 80f4776df9..939ebd11e0 100644 --- a/packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts +++ b/packages/services/service-datasource/src/__tests__/postgres-dsn-bound-secret.test.ts @@ -62,14 +62,33 @@ * ## Reverse verification (predicted in writing before running) * * Predicted with the pre-fix branch (`{ connectionString: url, ...(secret ? - * { password } : {}) }`) restored over these tests at their fixed state: the - * FIVE injecting cases go red on the resolved password, and the remaining cases - * stay green — the no-secret passthrough, the two discrete-branch controls, the - * "nothing else changed" equivalence sweep (it excludes `password` on purpose) - * and the unparseable-DSN refusal, which the old branch never reached. The - * shape matters: an arm-wide regression would mean this file measures something - * other than the branch-local defect. Measured exactly that set — see the PR - * body for the run. + * { password } : {}) }`) restored over these tests at their fixed state: + * **8 failed / 3 passed**, and specifically these — + * + * - RED, the five credential cases, each on the resolved password: `null` for + * the bare-username, no-userinfo and ssl/extras urls, `'embedded-legacy'` + * for the stored userinfo row, `'from-query-param'` for the stored query + * row. Those last two are the sharper direction: the credential is not + * merely missing, it is someone else's. + * - RED, the equivalence sweep — it excludes `password` from the key-by-key + * comparison on purpose, but still asserts the injected value separately, + * and that half fails. + * - RED, the `connectionString`-absence mechanism pin: the old branch emits it. + * - RED, the unparseable-DSN refusal, by a DIFFERENT route from all the + * others — the old branch never parses, so it never throws and the + * assertion fails for want of a rejection rather than on a credential. + * - GREEN, the three cases that must not move: the no-secret passthrough and + * both discrete-branch controls. The defect is branch-local, so an arm-wide + * regression would mean this file measures something else. + * + * Measured exactly that set: 8 failed / 3 passed, first two failures verbatim — + * + * ```text + * × resolves the bound secret as the handshake password instead of nothing + * AssertionError: expected null to be 's3cr3t-from-sys_secret' + * × lets the bound secret win over a legacy password embedded in a stored DSN + * AssertionError: expected 'embedded-legacy' to be 's3cr3t-from-sys_secret' + * ``` */ import { describe, it, expect } from 'vitest';