fix(server,sdk): stop returning connection credentials from the API

[mlennie]

Connection configs hold warehouse credentials, and the API returned them. A Postgres password, a BigQuery service-account key, a Snowflake private key, S3/GCS/Azure keys, a Databricks token and an SSH bastion private key were all readable by anyone who could reach the port, with no authentication and no prior knowledge of the deployment.

Sha-Bang traced this while reviewing #1047 and offered the redaction as one way to close its must-fix 2. This is that fix. The chain he identified holds: processConfigValue deep-walks the config, so a ${MALLOY_X_PASSWORD} reference is substituted into the real secret before any response is built, and the response then carried it.

What was exposed, and through what

The boundary this crossed is the config file rather than the network posture. publisher.config.json is an operator's file on an operator's disk. The API republished its contents to any reader, including the secret that a ${VAR} reference exists specifically to keep out of the file.

The mechanism is worth stating precisely, because it determines whether a fix is complete or a spot patch. Environment.serialize() returned listApiConnections() directly, which is the live connections array rather than a copy. Every route that serializes an environment therefore served the credential-bearing objects themselves. This is not a single leaking endpoint with a single missing redaction: it is one accessor feeding every read.

I counted the surface from the route table rather than by probing, because probing understates it. Taking every app.<verb> registration in server.ts and server-old.ts and keeping those whose handler reaches serialize(), listEnvironments(), listConnections(), getConnection() or getStatus(): fifteen registrations can return a connection. Roughly half are the legacy /projects aliases, which is exactly what hand-probing misses, and it missed them for me too before I enumerated. I am deliberately not listing the fifteen paths here; the count and the method are what a reviewer needs, and all of them build from the same serializer, so the count is a consequence rather than a boundary and it moves whenever a route is added.

Two surfaces are not affected, both verified rather than assumed. POST /connections/test answers with a ConnectionStatus, whose only properties are status and errorMessage, and it never merges stored config. The MCP surface maps connections to {name, type} explicitly before returning them.

Separately, config.ts spread an entire connection config into a log line when a config entry was missing its name. That is the pattern caught in #736 living on in a second place. A multi-line scan of every logger.* call in packages/server/src found exactly one such site, the one fixed here.

What the fix changes structurally

Responses are built from an allowlist of fields per connection type, applied at the three places that construct one: Environment.serialize() and the two connection controller reads. The internal view keeps its credentials, because compiling and connecting need them, so the split is between what the server holds and what it publishes rather than between one endpoint and another.

An allowlist rather than a denylist of secret-looking names, because the failure modes are asymmetric. An unlisted field is absent, so a credential added to a future connection type is invisible until someone publishes it deliberately; the cost of forgetting a non-secret field is that the UI does not show it yet. The contract cannot supply the list on its own, since api-doc.yaml declares password as a property of PostgresConnection because one schema serves both the write path and the read path.

Credentials are omitted rather than masked. A placeholder is resubmitted as the real credential by any client that sends back what it read, which is what the connection editor does.

withheldFields names what a response withheld, as dotted paths, names only. Without it a client cannot tell a credential that is set from one that was never configured, which is the difference between an empty box that keeps something and an empty box that leaves the connection with nothing.

The half that is a prerequisite rather than an extra

Once responses omit credentials, a caller cannot echo one back, so both write paths had to stop reading omission as deletion. This is the part that makes the change larger than a redaction, and it is not optional: shipping the redaction alone converts the leak into silent credential loss.

The per-connection PATCH merged with a shallow top-level spread, so sending postgresConnection without password replaced the sub-object and destroyed the stored password. The environment PATCH was worse, because it replaces the whole connections list and that is what the connections UI sends for every add, edit and delete: one edit stripped the credentials of every connection in the environment and answered 200. Snowflake and MotherDuck instead failed validation, so the app would have refused any connection change at all. Both behaviours predate this PR; the editor concealed them by reading credentials back out of the response, which is the leak.

Supplying a credential for one method now drops the stored credential of the alternative rather than accumulating both, because the connect path picks by presence: a stale connectionString beats new host/port fields, a stale privateKey routes to key-pair auth, peakaKey short-circuits before password is read, stored S3 beats new GCS, and the Databricks driver prefers OAuth over a token. Those are the five such slots, enumerated from the connect path rather than sampled.

A slot is selected only by a credential the caller actually supplied, non-empty. Both halves are load-bearing and both were learned by getting it wrong. An empty string is an untouched form box rather than a choice, and reading one as a choice destroyed the only credential of a connection described solely by a field the form cannot display. A non-secret field is echoed back by any client resubmitting what it read, so it says nothing about intent: oauthClientId came back on every Databricks write and would have kept the OAuth slot selected permanently.

An explicit null clears a sub-object; an empty object keeps what is hidden inside it, on the principle that erring toward keeping a credential beats erring toward destroying one.

What pins it, and the holes found in the pinning

A contract-parity spec holds the allowlist against api-doc.yaml, including the objects DucklakeConnection declares inline, and walks $refs out from Connection to require every reachable schema to be registered. It catches a property added to any connection schema, and a new connection type whose schema nobody registers. It is not hypothetical: rebasing onto #1064 pulled in impersonateServiceAccount on BigqueryConnection and the spec failed until it was classified. It is public (an account email, not a credential).

An integration spec boots a real server with real credentials and asserts none appears in any response body from any endpoint, or in the server log. It is written by behaviour rather than location, because the unit specs pin the three call sites that exist today and say nothing about a fourth added later. The credential reaches it as a ${VAR} reference, so the substitution step is inside the covered path.

Both of those had holes that review found rather than the tests failing, which is worth stating because it bears on how much weight to put on them:

• The sweep asserted no secret in the body but never asserted the route answered 200. A path that 404s returns a body with no secret in it, so it reported clean for routes it never reached, and would have kept reporting clean if a route were renamed. Every swept path now has to answer 200.
• The sweep covered only /environments. The legacy /projects aliases were already covered by the fix, same controller and serializer, but nothing pinned that. Now swept.
• The parity spec's schema-block extractor matched [A-Za-z]+, which does not match a name containing a digit, so GCSConnection silently ran into the next schema's properties.
• An earlier version of the parity check iterated a hand-maintained map, so it proved nothing about a schema nobody added to it. That is what the $ref walk closed.

Every fix here was mutation-tested: reverted in turn to confirm the guarding test goes red, plus injected fields and an injected unregistered connection type to confirm the parity gate fails.

Verification beyond the suites: reproduced on a running server before and after, reading the bytes on the wire; the update paths checked against the stored config in the database rather than against a response, since reading the credential is the thing being prevented; and three counterfactual runs with each fix disabled, each of which destroyed a credential while answering 200.

Gate: typecheck, lint, unit (3255 pass), integration (389 pass), and the environment-connections Playwright suite.

Review coverage, stated plainly

Most of the review here was mine, and mine missed things that others caught. Two rounds of adversarial review found ten defects, and the second round's most serious finding was a data-loss bug created by the first round's own fix. The single most damaging defect, that the connections UI writes through the environment PATCH rather than the endpoint the fix was first written against, was found by review and not by my manual testing, because I had tested the path I had just written. Two holes in my own credential sweep were found by someone else pushing on it rather than by me. Sha-Bang's independent trace is what started this.

So this has had no independent human review yet. Given what independent passes found on #1047 and #1057 that four self-review rounds had not, that is worth weighing.

Follow-ups, deliberately not in this PR

Deleting a connection through the environment PATCH drops it from memory without pruning its stored row, so it returns with its credentials on the next restart. That predates this change and wants its own fix.

Renaming a DuckDB attached database requires re-entering its secret, since entries are matched by name. Documented rather than worked around.

Write access to a connection reaches its credential: anyone who can PATCH one can point it at a host they control and have the stored credential sent there. Publisher authenticates neither operation, so this is not a new boundary, but it is now written down in docs/connections.md because a deployment that gates reads and writes separately should know it.

For operators on an unpatched build

Upgrading is the fix. If you are hardening instead, authenticate the API rather than any particular path: every read builds from the same serializer, so protecting the endpoints you happen to know about leaves the rest.

[Sha-Bang]

the redaction is coherent — one projection, one allowlist, three call sites, and hiddenFields/reinstate derived from the same shape so what the API hides is exactly what an update preserves.

I stood it up rather than only reading it: built the branch, booted a server with ${VAR}-referenced postgres and snowflake credentials, and drove the connections UI in a browser. Gate here — typecheck and lint clean, unit 3252 pass, integration 313 pass with one failure (concurrent_package, GCS seed) that reproduces identically on origin/main, so not yours. Two counterfactuals hold: reverting the serialize() projection turns the sweep red, allowlisting password turns 11 specs red. A third does not, and that's blocking finding 1. Also, the body says 389 integration tests; the suite collects 314 here.

two blocking, both in the half of this change that is a prerequisite rather than an extra:

1. Add the environment PATCH to the credential sweep. The merge at environment.ts:431 — the path the app writes through, and the worst bug this PR fixes — is covered by nothing. I reverted it and the whole suite stayed green.
2. Gate the editor's "stored value" text on withheldFields. Right now a snowflake connection that stores only a password tells the operator a private key and a passphrase are stored too. The field this PR added to make that distinguishable is read in exactly one place, and it isn't this one.

three non-blocking inline. Separately, one pre-existing bug I tripped over that isn't this PR's: deleting a connection in the app does nothing. Connections.tsx:212 reads if (!conn.resource) deleteConnection.mutateAsync(payload) — inverted, and every API connection carries resource, so the branch never fires and the user gets "Cannot delete this connection". Identical on main. It only touches this PR because resource is in your allowlist; keeping it there keeps the behaviour as-is. Worth its own fix.

[mlennie]

Thanks for standing it up rather than reading it, and for running the counterfactuals rather than taking mine on faith. All five items are addressed, three of them as you prescribed. The tip is f3dae8f7. Two corrections to claims of mine up front, because both were mine to get right and one of them is the same class of error the PR body warns about.

Blocking 1: the environment PATCH had no coverage

Fixed, and your framing of why it matters is the part I want to keep. The PR body says the sweep is behaviour-based so that a fourth call site would be covered. It was not true of the second call site it already had, which makes it a false completeness claim rather than a gap in coverage, and those are worse because they tell the next reviewer to look elsewhere.

Your counterfactual is now the test. It GETs …/connections, PATCHes that list straight back to PATCH /api/v0/environments/{env} with one connection appended, asserts 200, then asserts every original connection still reports its withheldFields. Reverting mergedConnections to payloadConnections turns exactly that test red and nothing else. It also carries a guard that at least three connections were withholding something before the PATCH, so it cannot pass by having had nothing to lose.

Worth noting for anyone reading later why the unit spec did not catch this: mergeConnectionUpdate has two call sites, the spec exercises the function against literals, and environment.ts was the caller nothing reached. Pinning a function is not pinning its callers.

Blocking 2: the editor did not read the field added for it

Fixed as you described. There is now one reader of withheldFields in that file, a Set keyed by dotted path, and both the placeholder and the helper text derive isStored from ${attributesFieldName[type]}.${field.name}. A box with nothing behind it says "No value stored." rather than promising to keep something. The line-310 .includes("postgresConnection.connectionString") now reads the same set, so there is one convention.

You flagged the main field loop. The DuckLake storage secrets had the same unconditional promise, at the secretAccessKey and secret placeholders, so those read the set too. Same finding, so it seemed wrong to fix the instance you happened to cite and leave its siblings.

Your point that it lands harder because the "Either password or private key is required" check came out in this same diff is fair, and it is the reason this was blocking rather than cosmetic: the form stopped pushing back at the same moment it started making a promise it had not checked.

Non-blocking 1: selecting on the sub-object

Accepted and fixed. You are right that it is the oauthClientId failure mode, and the uncomfortable part is that the docstring three lines above claims the design avoids it, so the invariant held for four slots out of five and the comment asserted five.

Reproduced your case before changing anything, and the merged object matched yours. Selection now names the nested credential (s3Connection.secretAccessKey, gcsConnection.secret, azureConnection.sasUrl, azureConnection.clientSecret) and isSupplied walks one dotted level.

One honest residual, since you will see it if you re-run your case: the stale s3Connection STUMP still survives when the caller echoes it back, because it is in the patch and the merge does not delete what a caller sent. What changes is the outcome. secretAccessKey is gone, so attachCloudStorage throws on the missing secret instead of the connection quietly authenticating against the old store. Loud and wrong-request beats silent and wrong-store, and stripping caller-supplied content felt like more overreach than the problem justified. Say the word if you would rather it dropped the whole sub-object.

Non-blocking 2: what withheldFields actually reports

Accepted, and the reword is the honest option for the reason you give: hiddenFields returns any unlisted field whole, so a stored field the allowlist does not know about lands there too. It now reads "Dotted paths of the fields this connection has stored that this response does not return", noting that in practice that is its credentials but that it is any withheld stored field. Your someFutureNonSecretField case is exactly why it mattered once the editor started rendering from it: a non-secret box would have been badged as a held credential.

Non-blocking 3: the malformed-scalar branch

Accepted. if (!isPlainObject(patch)) return hidden; made the stored object win and dropped the host and user the patch did send, which made the rule stated two lines above it false. The patch now wins and validation downstream rejects the shape. There is a test, mostly so nobody restores the old line to "fix" a malformed input.

Correction: the integration test count

You collect 314 and the body says 389. The body is wrong and yours is right.

I measured 389 in a worktree carrying an accumulated publisher.db and publisher_data from dozens of runs. From clean runtime state the same command gives 317 on the branch as you reviewed it, and 318 now that the environment-PATCH test exists. So the number in the body was inflated by my own local state, which is the same category of mistake as reporting a probed sample as a census, one layer further out.

The number I can defend, from a clean checkout: unit 3257 across 151 files, integration 318 across 38 files, both zero failures. Plus lint and typecheck clean, and the environment-connections Playwright suite at 5 passed, re-run because this change touches the editor.

The delete button, which is yours to have found and not mine to fix

Confirmed, and confirmed identical on main: Connections.tsx reads if (!conn.resource) where it means the opposite, and convertConnectionsToApiConnections sets resource on every connection, so the delete branch cannot fire and the user always gets "Cannot delete this connection".

I have deliberately not touched it, and specifically have not changed the allowlist for it. resource stays public, which keeps the behaviour exactly as it is on main rather than having this PR silently change what the delete button does. Filing it as its own issue.

Where the review coverage now stands

Recording this because the PR body makes a claim about it. Before your pass, this branch had two rounds of adversarial self-review that found ten defects, plus two holes in my own credential sweep found by a colleague pushing on it. Your pass found two more, and the more serious of the two is a hole in the same sweep, found the same way: by reverting the fix and watching the tests stay green. Three separate passes have now found something in that one test file, which is a reasonable argument that the file was doing less than its description claimed.

[Sha-Bang]

Re-reviewed at f3dae8f7. All five findings from 5025989993 are closed, and I mutation-tested the two that were about missing coverage rather than taking the new tests at face value.

• The environment PATCH is covered. The new case does the counterfactual I asked for — GET the list, PATCH the whole thing back with one appended, then assert every original connection still reports its withheldFields — with a >= 3 guard so it cannot pass vacuously. I killed it two ways: reverting the argument to payloadConnections (fails, 500 on the PATCH), and no-op'ing reinstate inside the merge (fails on the withheldFields assertion, alongside 10 unit failures). The new test is named in both.
• The editor reads the field. One withheldFields Set, the second reader at the old line 310 folded into it, isStored off the dotted path, and both placeholder and helperText reading it. It goes further than I asked and gates the two ducklake storage placeholders too.
• The slots select on the credential, suppliedAt walks the dotted level, and the new test drives toPublicConnection rather than a hand-built literal, so it reproduces the round-trip instead of approximating it. Reverting selects to the sub-object kills exactly that test and nothing else.
• The malformed-scalar case returns patch, and the withheldFields doc now says what it actually returns rather than "which is to say its credentials".

One blocking, and it is new. The object-store fix traded the API-only bug I filed for a UI-reachable one: switching DuckLake storage from S3 to GCS while leaving the stored secret box blank now carries the old S3 credential forward, and hasS3 beats GCS on the connect path, so the connection keeps authenticating against S3 while the operator believes they moved. It is a regression against 56fd02e1, not a leftover — I ran the same probe against both heads. Details and a fix that keeps both of the lessons in your selects docstring are inline.

What I ran

• 3254 pass / 3 skip / 0 fail on the server unit suite; 5/5 on tests/integration/connection_secrets.
• Four mutations, each restored afterwards: two against the environment PATCH, one against the selects change, and one against reinstate. Every one of them killed the test it was supposed to.
• The store-switch probe below, against f3dae8f7 and 56fd02e1 in turn.

🤖 Reviewed with Claude Code

[mlennie]

@Sha-Bang, you are right, and it is a regression I introduced rather than something you missed the first time. I ran your probe against both heads before touching anything, and it reproduces exactly as you have it:

switch ducklake storage s3 to gcs	56fd02e	f3dae8f
gcs block WITH a secret	clean	clean
gcs block WITHOUT a secret	clean	S3-SECRET reinstated

So the fix for your original finding traded an API-clients-only bug for a UI-reachable one, which is a worse trade than the one it undid. Fixed at 22fd25f5, using your patch.

Why I took it rather than argued it

I checked the two claims that decide it rather than reasoning from the diff.

The UI flow is as you describe. EditConnectionDialog.tsx collects only truthy form values into gcsConfig, so leaving the secret box blank emits gcsConnection: { keyId } with no secret, and the comment sitting above it, which I wrote, says a blank secret means keep the stored value. So switching store while leaving the credential alone is the documented flow, and it is precisely the shape that supplies no credential in any slot.

And the precedence is what makes it dangerous rather than merely wrong: connection.ts reads if (hasS3) … else if (hasGCS), and hasS3 is a presence check on the sub-object, so the reinstated stump wins and the connection authenticates against the store the operator just left.

Your discriminator is the right one because it is the thing this group has and the flat groups do not. Exactly one declared sub-object selects that slot; a round-tripped read declares both, which is the case that made presence-selection wrong in the first place, so it falls through to the credential check. Flat groups are untouched, since their fields are scalars and nothing is ever declared. And a selected slot never drops its own fields, so the rule that an empty box must not destroy a stored credential still holds, which was my worry about widening selects and is the reason your version is better than widening it.

Two tests, and each fails only for its own mutation

Beside the case you asked for I added its converse, because the two failure directions are what the design has to hold apart:

• Switch stores, secret left blank: the sibling S3 secret must be gone. Fails when the discriminator is removed, which is the f3dae8f behaviour.
• Same slot, secret left blank: the slot's own stored secret must survive. This is the credential-loss direction.

Mutation-tested both ways as you asked. Removing the discriminator fails only the blank-secret switch. Reverting selection to the sub-object fails only the round-tripped read. Neither mutation fails both, so the two tests are pinning different things rather than one thing twice.

One residual, unchanged from last round and still disclosed rather than hidden: a caller that echoes back the old sub-object's non-secret fields still leaves a stump, because the merge does not delete what a caller sent. That now ends in a loud attach error rather than the wrong store.

The Windows check

I have not guessed at it, and I cannot settle it from here, so here is what I did establish.

My diff contains nothing in the class that bit the sibling branch: no chmod, no symlink, no file-mode assertion, no path.sep or platform branch. I checked the whole diff rather than the files I remembered touching.

The new integration spec is the obvious suspect since it is the newest thing running there, and the Windows job does run test:integration. But its two platform-sensitive constructs are both already in specs that run on Windows today: spawn("bun", ["src/server.ts"]) is the pattern from readiness_line.integration.spec.ts, and proc.kill("SIGKILL") is the pattern from port_conflict.integration.spec.ts. So it introduces no construct the suite does not already exercise there.

What it does add is one more real server boot to that job, with a 120s readiness poll. If the failure is a timeout rather than an assertion, that is the first thing I would look at, and the fix would be to gate the spec to Linux the way create-malloy-package test:e2e already is, on the grounds that booting a server per spec is what that exemption exists for. But that is a hypothesis about a log I have not read, so I would rather see the failing step than act on it. Please do send it.

Gate

Merged origin/main at c9aa9926 and re-ran from clean runtime state, since I have been caught once already reporting counts inflated by an accumulated publisher.db:

Unit 3288 across 153 files, integration 320 across 38 files, both zero failures. Lint and typecheck clean.

Thanks for mutation-testing the previous round's tests rather than reading them. Killing the environment-PATCH test two separate ways is what turned "there is a test now" into evidence, and it is the check I should have run on my own test before claiming the sweep was behaviour-based.

[mlennie]

Correction, and it retracts the Windows paragraph in my previous comment.

The 22 unit failures were the contract-parity specs, and the contract was fine. Every extractor in them anchors on \n, so the CRLF checkout the Windows leg gets leaves each anchor unmatched and fails all twenty-odd cases at once. Reproduced by converting api-doc.yaml to CRLF locally: 21 failures in connection_public_view.spec.ts plus the re-anchored one in db_utils.spec.ts, which is the 22 exactly, with the same test names. Fixed at 0a3f887d by normalising line endings in both readers, and I checked that the normalisation did not neuter the gate rather than assuming it: with CRLF still in place, an unclassified property injected into MysqlConnection still fails that schema's case.

So please disregard my timeout hypothesis for the earlier Windows failure. It was almost certainly this, and the guess was worth less than the two cheap things I had not done: running the suite the way CI runs it, test:unit --serial, and reproducing the platform's checkout. My local gate was green because my checkout is LF, which is a local environment too clean to show the defect rather than a disagreement about behaviour.

Worth naming rather than filing under "the gate worked": this was a false positive. It failed on a newline convention rather than on the contract, and the next person to see 22 red parity tests would reasonably assume the contract had drifted and look in the wrong place. The underlying fragility is that those specs extract from api-doc.yaml as text, which I chose so they would not depend on a YAML parser this package does not declare. It bit where that trade predicted, and a structural reformat of the spec file would break them the same way; that is now recorded in the commit message.

Gate on the current tip, run as CI runs it: test:unit --serial 3301 across 153 files and integration 320 across 38, both zero failures, matching CI's collected count.