From e439dfe7c5960cb6135c70bed0705f1567a2c626 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 11:56:42 +0000 Subject: [PATCH 1/2] fix(service-datasource): a bound credentialsRef reaches the mysql client on the DSN branch (#8696) `DatasourceConnectionService` resolves `external.credentialsRef` to a cleartext secret and hands it to the driver factory as `spec.secret`. The mysql arm threw it away whenever `config.url` was present -- `if (url) return url;` made the DSN string the whole knex `connection`, so the resolved credential reached nothing. Since #8082 refuses a `user:password@` userinfo at the publish door, a bare username DSN plus a bound secret is the only authorable URL shape for this driver, so the arm dropped the credential of precisely the configuration the platform tells operators to write: the datasource connected unauthenticated, or failed with a driver-level auth error naming nothing about the binding. The fix hands mysql2 the DSN and the secret together (`{ uri, password }`) rather than parsing the URL here: mysql2 keeps owning its own DSN grammar, and its merge gives the explicit key precedence, so the bound secret also wins over a legacy password embedded in a stored pre-#8082 row. A DSN with nothing bound still passes through as the bare string, so nothing that binds no secret changes behaviour. Measured on mysql2 3.23.1, knex 3.3.0 and pg 8.22.0. The mongodb arm and a separate pg client-layer defect are recorded in the changeset and left to their own changes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn --- .changeset/mysql-dsn-bound-secret.md | 67 +++++++ .../bound-secret-dsn-branches.test.ts | 165 ++++++++++++++++++ .../src/default-datasource-driver-factory.ts | 79 ++++++++- 3 files changed, 307 insertions(+), 4 deletions(-) create mode 100644 .changeset/mysql-dsn-bound-secret.md create mode 100644 packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts diff --git a/.changeset/mysql-dsn-bound-secret.md b/.changeset/mysql-dsn-bound-secret.md new file mode 100644 index 0000000000..380c76d31f --- /dev/null +++ b/.changeset/mysql-dsn-bound-secret.md @@ -0,0 +1,67 @@ +--- +"@objectstack/service-datasource": patch +--- + +fix(service-datasource): a bound `external.credentialsRef` reaches the mysql client on the DSN branch instead of being dropped (#8696) + + + +`DatasourceConnectionService` resolves a datasource's `external.credentialsRef` +to a cleartext secret and hands it to the driver factory as `spec.secret`. The +mysql arm then **threw it away** whenever `config.url` was present: the DSN +string became the whole knex `connection`, and the resolved credential reached +nothing. Measured on `origin/main`, driver `mysql`, `config.url` +`mysql://app@db.internal:3306/app`, secret bound: + +```text +knex connection: typeof=string value="mysql://app@db.internal:3306/app" +``` + +**This is a broken binding, not a disclosure.** Since #8082 refuses a +`user:password@` userinfo at the publish door, a bare-username DSN plus a bound +secret is the *only* authorable URL shape for this driver — the exact shape the +connection form produces and the exact shape #8155's re-homing remedy tells +operators to write. Such a datasource therefore connected **unauthenticated**, +or failed with a driver-level auth error naming nothing about the binding, while +its Setup page showed a credential bound and the connect path reported success. +It is the declared-≠-enforced shape one layer below Prime Directive #10: +`MysqlConfigSchema.url` already states the contract this code failed to keep — +*"bind the secret … and it is injected at connect time. A bare username +(`user@host`) stays writable."* + +**The fix hands mysql2 the DSN and the secret together** — `{ uri, password }` +rather than a hand-parsed URL. mysql2 keeps owning its own DSN grammar (no URL +parsing, no re-encoding, no second dialect of `mysql://…` in this repo), and its +merge gives the **explicit** key precedence, so the bound credential also wins +over a legacy password embedded in a stored pre-#8082 row — the precedence the +postgres arm's DSN branch already declares. Measured on mysql2 3.23.1, knex +3.3.0 and pg 8.22.0. + +A DSN with **nothing bound passes through unchanged**, as the bare string it has +always been, so the entire blast radius is datasources that bind a secret — the +ones that are broken today. + +Two measured findings this change deliberately does **not** act on, each filed +on its own: + +- **The mongodb arm is still open.** `buildMongoUrl`'s `if (explicit) return + explicit;` drops the bound secret the same way, so a mongo DSN datasource + still reaches `MongoClient` with an **empty** password. The remedy is not a URL + rewrite — `MongoClient`'s `auth` option injects beside an unmodified url, and + it wins over an embedded userinfo password (measured on mongodb 7.5.0) — but it + requires a username as well, and reading the url's userinfo username needs the + platform's own DSN grammar (`new URL()` rejects the multi-host form + `MongoConfigSchema` documents). `@objectstack/spec/data` exports the password + half of that grammar and no username half; adding one belongs beside it rather + than as a second copy of the userinfo boundaries here. +- **The postgres arm passes this assertion at the config layer and is broken one + layer below it.** `pg` merges `parse(connectionString)` **over** the explicit + `password`, so `{connectionString, password}` resolves to the DSN's own + (absent) password — effective `password: null`, measured on pg 8.22.0. Its + `if (url)` branch is not fixed by symmetry with this one; the two clients merge + in opposite directions, which is why each arm's precedence is measured rather + than assumed. diff --git a/packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts b/packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts new file mode 100644 index 0000000000..7cfa9c7cef --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts @@ -0,0 +1,165 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8696 — a bound `external.credentialsRef` reaches the client on the mysql + * arm's DSN branch, not only on its discrete-fields branch. + * + * ## The defect + * + * `DatasourceConnectionService` resolves `external.credentialsRef` to a + * cleartext secret and hands it to this factory as `spec.secret`. The mysql arm + * then threw it away whenever `config.url` was present (`if (url) return url;` + * — the DSN string became the whole knex `connection`). Measured on + * `origin/main` @ 20067c56b, driver `mysql`, `config.url` + * `mysql://app@db.internal:3306/app`, secret bound: + * + * ```text + * knex connection: typeof=string value="mysql://app@db.internal:3306/app" + * ``` + * + * No credential anywhere. Since #8082 refuses a `user:password@` userinfo at + * the publish door, a bare-username DSN plus a bound secret is the only + * authorable URL shape for this driver — so the arm dropped the credential of + * precisely the configuration the platform tells operators to write, and the + * datasource then connected unauthenticated (or failed with a driver-level auth + * error naming nothing about the binding). This is the third branch of the + * family #7314 / #7385 / #8152 closed one arm at a time. + * + * ## What is asserted, and at which layer + * + * The factory's own output — the `connection` it hands `SqlDriver`. That is + * this module's contract, but it is deliberately NOT the whole story, and this + * file says so because the sibling arm proves the difference matters: the + * postgres arm passes the equivalent assertion at this layer and is STILL + * broken, because `pg` merges `parse(connectionString)` OVER the explicit + * `password` (measured on pg 8.22.0: effective password `null`). Filed + * separately. mysql2 merges the other way — the explicit key wins — which is + * what makes the shape below correct HERE and wrong there. Measured on + * mysql2 3.23.1 and knex 3.3.0: + * + * ```text + * mysql2 ConnectionConfig({uri:'mysql://app@db.internal:3306/app', password:'INJECTED'}) + * -> user=app host=db.internal port=3306 database=app password=INJECTED + * knex connectionSettings for that object + * -> Object.keys = ['uri'] (password is NON-ENUMERABLE, setHiddenProperty) + * Object.getOwnPropertyNames = ['uri','password'] cs.password = 'INJECTED' + * ``` + * + * ⚠️ That last line is why every assertion below reads `conn.password` + * directly. `JSON.stringify(conn)` / `Object.keys(conn)` on a knex-normalised + * connection hide the very key under test — a serialising probe reports a + * dropped secret that is in fact present. + * + * ## Reverse verification (predicted before running, then measured) + * + * Predicted: with `if (url) return url;` restored, the two DSN cases go RED on + * the injected password and the discrete + no-secret cases stay GREEN — the + * defect is branch-local, so a whole-arm regression would be the wrong shape. + * Measured exactly that: 2 failed / 3 passed, both failures reading + * `expected 'mysql://app@db.internal:3306/app' to be an object`. + * + * ## The mongodb half is NOT closed here + * + * `buildMongoUrl`'s `if (explicit) return explicit;` still drops the bound + * secret, so a mongo DSN datasource still reaches MongoClient with an empty + * password. It is not pinned as expected behaviour below — a test asserting the + * defect would read as a contract. The remedy and what blocks it are recorded + * on `buildMongoUrl` itself; #8696 stays open for it. + */ + +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 URL shape post-#8082: a username, never a password. */ +const BARE_USERNAME_DSN = 'mysql://app@db.internal:3306/app'; + +/** + * The `connection` this arm hands `SqlDriver`, read off the constructed driver + * rather than from a private export — the same seam the #4456 pins in + * `default-datasource-driver-factory.test.ts` read. + */ +async function mysqlConnection(spec: Record): Promise { + const handle: any = await factory().create({ driver: 'mysql', ...spec } as any); + try { + const driver = handle.driver ?? handle; + return (driver?.config ?? driver?.knexConfig ?? driver?.options ?? {}).connection; + } finally { + // The pool is never opened — nothing connects in this file. + try { await handle.disconnect?.(); } catch { /* noop */ } + } +} + +describe('#8696 — mysql: a bound secret reaches the client on the DSN branch', () => { + it('injects the bound secret beside the DSN instead of dropping it', async () => { + const conn = await mysqlConnection({ + name: 'orders', + config: { url: BARE_USERNAME_DSN }, + secret: BOUND_SECRET, + }); + + // Not the bare-string passthrough any more: mysql2 needs a sibling key to + // merge the credential into, and a string has no siblings. + expect(typeof conn).toBe('object'); + // Direct property access, never JSON.stringify — see the header note on + // knex's setHiddenProperty. + expect(conn.password).toBe(BOUND_SECRET); + // The DSN itself is handed over untouched: mysql2 owns its grammar, and + // nothing here re-encodes or rewrites the authored value. + expect(conn.uri).toBe(BARE_USERNAME_DSN); + }); + + 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. The bound credential is the one an operator + // deliberately bound, and mysql2's merge gives the explicit key precedence + // — the same rule the postgres arm's DSN branch already declares. + const conn = await mysqlConnection({ + name: 'legacy', + config: { url: 'mysql://app:embedded-legacy@db.internal:3306/app' }, + secret: BOUND_SECRET, + }); + + expect(conn.password).toBe(BOUND_SECRET); + expect(conn.uri).toBe('mysql://app:embedded-legacy@db.internal:3306/app'); + }); + + it('leaves a DSN with nothing bound exactly as it was (no behaviour change)', async () => { + // The whole blast radius of this change is "a secret was bound". A + // datasource that binds none must be byte-for-byte what it was before, and + // the bare-string passthrough is what knex has always parsed for it. + const conn = await mysqlConnection({ name: 'anon', config: { url: BARE_USERNAME_DSN } }); + + expect(conn).toBe(BARE_USERNAME_DSN); + }); + + 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 conn = await mysqlConnection({ + name: 'discrete', + config: { host: 'db.internal', port: 3306, database: 'app', username: 'app' }, + secret: BOUND_SECRET, + }); + + expect(conn.password).toBe(BOUND_SECRET); + expect(conn.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 too is a stored-row-only shape; the precedence is unchanged here. + const conn = await mysqlConnection({ + name: 'both', + config: { host: 'db.internal', database: 'app', username: 'app', password: 'inline-legacy' }, + secret: BOUND_SECRET, + }); + + expect(conn.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 21f5fdb7e9..7073d1769f 100644 --- a/packages/services/service-datasource/src/default-datasource-driver-factory.ts +++ b/packages/services/service-datasource/src/default-datasource-driver-factory.ts @@ -431,15 +431,69 @@ function buildSqlPool(spec: DatasourceConnectionSpec): Record { /** * Build the Knex `connection` for mysql2 from a spec's config + secret. A DSN - * (`url`/`connectionString`) passes through as-is — knex's mysql2 dialect - * accepts a connection string; otherwise discrete fields, with the secret as - * the password (never part of `config`). + * (`url`) selects the connection-string form; otherwise discrete fields, with + * the secret as the password (never part of `config`). + * + * ## A bound secret reaches the client on the DSN branch too (#8696) + * + * This arm used to be `if (url) return url;` — the DSN string became the whole + * knex `connection` and an injected `spec.secret` was dropped on the floor, + * with no diagnostic. That is the declared-≠-enforced shape one layer below + * Prime Directive #10: `MysqlConfigSchema.url` states the contract this code + * 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."* Since #8082 refuses a + * `user:password@` userinfo at the publish door, that bare-username DSN plus a + * bound secret is the ONLY authorable shape for a URL-shaped mysql datasource + * — so the dropped secret meant it connected with no credential at all, or + * failed with a driver-level auth error naming nothing about the binding. + * + * ## Why `{ uri, password }` and not a hand-parsed DSN + * + * `mysql2` merges a `uri` with sibling keys itself, and the EXPLICIT key wins + * (`ConnectionConfig`: uri-derived values are only filled in for keys the + * caller did not supply). So the DSN keeps being parsed by the client that + * owns its grammar — no URL parsing, no re-encoding, no second dialect of + * `mysql://…` in this repo — and the bound secret overrides even a legacy + * embedded password on a stored pre-#8082 row. Measured on mysql2 3.23.1: + * + * ```text + * {uri:'mysql://app@db.internal:3306/app', password:'INJECTED'} + * -> user=app host=db.internal port=3306 database=app password=INJECTED + * {uri:'mysql://app:embedded@db.internal:3306/app', password:'INJECTED'} + * -> password=INJECTED (the bound secret wins, as postgres' arm declares) + * ``` + * + * ⚠️ knex hides the key rather than passing it visibly: `Client`'s constructor + * calls `setHiddenProperty`, so `password` survives on `connectionSettings` as + * a NON-ENUMERABLE own property (measured on knex 3.3.0). `JSON.stringify` and + * `Object.keys` therefore both report a bare `{uri}` — a probe that serialises + * this object reads as "the secret was dropped" when it was not. Assert it with + * direct property access. + * + * ⛔ 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. + * + * 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. */ function buildMysqlConnection(spec: DatasourceConnectionSpec): unknown { const cfg = (spec.config ?? {}) as Record; const mysqlSsl = resolveSslOption(spec); const url = cfg.url as string | undefined; - if (url) return url; + if (url) { + if (!spec.secret) return url; + // `ssl` is deliberately NOT carried here: this arm drops a declared `ssl` + // block on the DSN branch whether or not a secret is bound (postgres' + // branch does carry it), and making TLS appear only for datasources that + // happen to bind a credential would be a second, stranger asymmetry. That + // gap is a defect of its own and is filed separately, not fixed in passing. + return { uri: url, password: spec.secret }; + } return { host: cfg.host, port: cfg.port, @@ -531,6 +585,23 @@ function buildMemoryConfig(spec: DatasourceConnectionSpec): Record; From 328cd84dc51b5ba60abf73702a350c0ffb3d7acd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 11:58:33 +0000 Subject: [PATCH 2/2] test(service-datasource): quote the measured ablation output in the #8696 pin header The reverse-verification note carried the predicted failure text rather than the observed one. Replaced with the two real assertion messages from the ablated run (2 failed / 3 passed, the predicted set). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn --- .../bound-secret-dsn-branches.test.ts | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts b/packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts index 7cfa9c7cef..d0b69ffb13 100644 --- a/packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts +++ b/packages/services/service-datasource/src/__tests__/bound-secret-dsn-branches.test.ts @@ -52,11 +52,22 @@ * * ## Reverse verification (predicted before running, then measured) * - * Predicted: with `if (url) return url;` restored, the two DSN cases go RED on - * the injected password and the discrete + no-secret cases stay GREEN — the - * defect is branch-local, so a whole-arm regression would be the wrong shape. - * Measured exactly that: 2 failed / 3 passed, both failures reading - * `expected 'mysql://app@db.internal:3306/app' to be an object`. + * Predicted, in writing, before running it: with `if (url) return url;` + * restored, the two DSN cases go RED on the injected password while the + * no-secret DSN case and both discrete-branch cases stay GREEN — the defect is + * branch-local, so a whole-arm regression would be the wrong shape and would + * mean the pin is measuring something else. Measured exactly that set, 2 failed + * / 3 passed: + * + * ```text + * × injects the bound secret beside the DSN instead of dropping it + * AssertionError: expected 'string' to be 'object' + * × lets the bound secret win over a legacy password embedded in a stored DSN + * AssertionError: expected undefined to be 's3cr3t-from-sys_secret' + * ``` + * + * The first failure is the whole defect in one line: the arm answered with the + * DSN *string*, which has no key for a credential to live in. * * ## The mongodb half is NOT closed here *