Skip to content

fix(service-datasource): a bound credentialsRef reaches the mysql client on the DSN branch (#8696) - #8875

Merged
qq9340100 merged 3 commits into
mainfrom
claude/issue-8696-credentialsref-dsn-arms
Aug 15, 2026
Merged

fix(service-datasource): a bound credentialsRef reaches the mysql client on the DSN branch (#8696)#8875
qq9340100 merged 3 commits into
mainfrom
claude/issue-8696-credentialsref-dsn-arms

Conversation

@qq9340100

Copy link
Copy Markdown
Collaborator

Part of #8696the mysql arm only. The mongodb arm named in that card is not implemented here and #8696 remains open for it; see "What is deliberately not in this PR" below.

Premise re-verified first (the card was deferred behind #8081)

#8696 was held for a day because #8081 was rewriting the datasource credential read/write paths this defect sits on, so the first deliverable was a measurement, not a fix. Measured on origin/main @ 20067c56b, driver arm by driver arm, with a bound secret injected exactly as DatasourceConnectionService injects it:

mysql config.url = mysql://app@db.internal:3306/app
knex connection: typeof=string value="mysql://app@db.internal:3306/app" secret ABSENT
mongodb config.url = mongodb://app@db.internal:27017/app
driver.config.url = mongodb://app@db.internal:27017/app secret ABSENT
postgres config.url = postgresql://app@db.internal:5432/app
connection.password = the bound secret secret PRESENT

The premise still holds.#8081 did not touch these branches — the three sites the card cites (:442, :542, :389) are unchanged, and DatasourceConnectionService still resolves external.credentialsRef into spec.secret and hands it to the factory.

The fork the card left open is settled by the spec, not by preference

The card asked whether to inject the secret or refuse loudly, and the dispatch treated refusal as presumptive. Both readings turned out to be answerable from the contract rather than by taste: packages/spec already declares injection on all three URL-bearing SQL arms. MysqlConfigSchema.url, verbatim:

Credential-free by contract since #8082: a user:password@ userinfo is refused at publish exactly like an inline password (#7990) — 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.

The same sentence appears on MongoConfigSchema.url and PostgresConfigSchema.url. So this is not a fork between two defensible behaviours — it is Prime Directive #10 one layer down: the contract says injected at connect time, and the runtime dropped it. Refusing loudly would have required changing the spec, which is a domain:spec contract decision, and would additionally have rejected the exact configuration datasource-credential-migration.ts (landed today in #8155) instructs operators to write:

Edit the datasource in Setup → Datasources: remove the password from the URL (a bare user@host is accepted) and enter it in the connection form's secret field, which binds it into the secret store.

That same file names the remedy this PR implements — "the driver factory's DSN branches would have to honour the injected secret" — as the producer-side change its own refusal is waiting on.

The dispatch's stop-condition did not fire. It was "stop and report if a URL rewrite is required for mongo": measured false. MongoClient's auth option injects beside an unmodified url and wins over an embedded userinfo password (mongodb 7.5.0), so no rewrite and no URL-encoding of the secret is involved on either arm.

The change

buildMysqlConnection's DSN branch was if (url) return url; — the DSN string became the whole knex connection, which has no key a credential could live in. It now hands mysql2 the DSN and the secret together:

if(url){if(!spec.secret)returnurl;return{uri: url,password: spec.secret};}

{ uri, password } rather than a hand-parsed URL is the point of the shape: mysql2 keeps owning its own DSN grammar, so there is no second dialect of mysql://… in this repo, no re-encoding, and no place for a parser bug to change which host is connected to. Measured on mysql2 3.23.1 / knex 3.3.0 / pg 8.22.0:

mysql2 ConnectionConfig({uri:'mysql://app@db.internal:3306/app', password:'INJECTED'})
-> user=app host=db.internal port=3306 database=app password=INJECTED
mysql2 ConnectionConfig({uri:'mysql://app:embedded@db.internal:3306/app', password:'INJECTED'})
-> password=INJECTED # bound secret wins, as the postgres arm's comment already declares
knex connectionSettings for {uri, password}
-> Object.keys = ['uri'] ; getOwnPropertyNames = ['uri','password'] ; cs.password = 'INJECTED'

That last line is a trap worth naming: knex stores password as a non-enumerable own property (setHiddenProperty), so JSON.stringify and Object.keys both report a bare {uri}. A serialising probe reads as "the secret was dropped" when it was not — every assertion in the pin uses direct property access instead.

Blast radius is exactly the broken class. A DSN with nothing bound still returns the bare string it always did, so any datasource that binds no secret is byte-for-byte unaffected. The only datasources whose behaviour changes are the ones that bound a credential and were connecting without it.

Verification

  • New pin bound-secret-dsn-branches.test.ts — 5 cases: injection on the DSN branch, bound-wins-over-embedded-legacy-DSN, the no-secret passthrough, and two controls on the discrete branch.
  • Reverse verification, direction predicted in writing before running. Predicted: restoring if (url) return url; turns the two DSN cases RED and leaves the no-secret and both discrete cases GREEN — branch-local, so a whole-arm regression would mean the pin measures the wrong thing. Measured exactly that set, 2 failed / 3 passed:
    × 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'
    
  • @objectstack/service-datasource: 426 tests / 19 files pass, typecheck clean.
  • Gate union re-derived from the actual changed paths with scripts/pm/dispatch-gates.mjs and run at 9e0e95f87, which is this branch's head (the union was re-run after merging origin/main, since the merge commit moved the head): check:test-source-alias, check:type-source-resolution, check:nul-bytes, check:error-code-casing, check:changeset-gate-self-tests, check:objectui-changeset, check:query-options-erasure, check:type-check-coverage, check:type-check-debt --re-measure, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset — all green. The re-derivation added seven families beyond the dispatch list (the changeset family and the test-file convention ratchets).

No new error code is minted, so nothing lands on the ADR-0112 surface: the refusals this file already raises are plain errors carrying a fix instruction, and this change adds none.

What is deliberately not in this PR

The mongodb arm — #8696 stays open for it.buildMongoUrl's if (explicit) return explicit; still drops the bound secret, so a mongo DSN datasource still reaches MongoClient with an empty password (measured). The remedy is known and cheap — options.auth, which MongoDBDriver already forwards verbatim — but auth requires a username as well as a password, and reading the url's userinfo username needs the platform's own DSN grammar: new URL() rejects the multi-host form MongoConfigSchema documents (mongodb://app@h1:27017,h2:27017/app throws ERR_INVALID_URL, measured), which is precisely why @objectstack/spec/data owns urlUserinfoPassword / redactUrlPassword instead of using WHATWG parsing. Those two export the password half of that grammar and no username half. Adding one belongs beside them in packages/spec, which is outside the file surface this card was dispatched with, and hand-rolling a second copy of the userinfo boundaries here is the shape #8082's ruling rejected by name ("a single value-level parse as the mechanism, precisely so no second copy exists to disagree with this one"). Left for an explicit decision rather than guessed at; the reasoning is recorded on buildMongoUrl itself so the next author does not have to re-derive it.

Two defects measured in passing, filed unassigned rather than fixed here:


Generated by Claude Code

…ent 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn
…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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn
@vercel

vercelBot commented Aug 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 15, 2026 12:37pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

No hand-written docs reference the 1 changed package(s). ✅

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 15, 2026
@qq9340100
qq9340100 marked this pull request as ready for review August 15, 2026 12:54
@qq9340100
qq9340100 added this pull request to the merge queueAug 15, 2026
Merged via the queue into main with commit 72050ccAug 15, 2026
27 checks passed
@qq9340100
qq9340100 deleted the claude/issue-8696-credentialsref-dsn-arms branch August 15, 2026 13:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@qq9340100@claude