fix(server): redact connection-test errors and restore duckdb/ducklake testability

[mlennie]

Follow-up to #915, delivering the two items committed to in the review reply, plus two fixes the specs and a security review forced into the open.

What this does

1. Redacts connection-test error messages. testConnectionConfig returned (error as Error).message verbatim in the POST /api/v0/connections/test response body. DuckDB attach failures echo the full connection string (IO Error: Unable to connect to Postgres at "postgres://user:pass@host/db"), so a failed test of a postgres/ducklake/duckdb-attached connection sent the cleartext password back to the API caller. The returned errorMessage now goes through redactPgSecrets. The two log lines on the same paths (testConnectionConfig's catch and attachDatabasesToDuckDB's catch) printed the same DSN to the server log and are redacted the same way. The rethrow in attachDatabasesToDuckDB stays raw on purpose: redaction happens at boundaries (response bodies, log lines), and handleAlreadyAttachedError matches on message content.

2. Fixes connection testing for duckdb/ducklake connections, broken since Adopt MalloyConfig and scope DuckDB paths to package/project roots #682. testConnectionConfig built its throwaway config with an empty environment path, so DuckDB rejected the empty workingDirectory before any attach ran: every duckdb/ducklake connection test failed with workingDirectory is invalid: path must not be empty regardless of the config's validity. CI never noticed because the credential-gated specs skip without credentials and the unguarded invalid-config spec only asserts "failed with some message". The config is now rooted at process.cwd(), which is where the DuckLake cleanup in the finally already expected the connection file to be (see the comment in ducklake.test.ts). Without this fix, item 1 protects an unreachable path and cannot be tested.

3. Guards the connection name against path traversal. Reaching a real attach (item 2) means the connection name now flows into path.join(cwd, + "${name}.duckdb" + ), and here the name comes straight from the request body. A name like ../foo would create a .duckdb file outside the working directory. testConnectionConfig now runs assertSafePackageName on the name before building the config, the same allowlist the DuckLake cleanup path (deleteDuckLakeConnectionFile) already applies to it, so any valid name is unaffected.

4. Tightens the pass-2 comment in pg_helpers.ts per @housejester's note 1 on fix(server): redact URL-form connection strings in redactPgSecrets #915: the raw-/ mop-up only recovers the password when no raw @ precedes the /. With both (postgres://u:p@a/b@h/d), pass 1's inserted ***@ satisfies the mop-up's first-@ before it reaches the raw /, so the password tail after the raw @ stays visible. The comment now says so, and a pinning spec documents the accepted residual.

Tests

• Three new specs in connection.spec.ts drive real attach failures offline (localhost port 1, nothing can listen there unprivileged) through the URL-form, keyword-form, and ducklake-catalog shapes, asserting the returned message never contains the cleartext password (the unconditional security invariant) and, once the DSN has reached the message, that it is redacted. The positive check is conditional on the DSN being present so an environment where a DuckDB extension can't load fails honestly rather than on a missing marker.
• One spec asserts a path-traversal connection name is rejected and no file is created outside the working directory.
• One pinning spec in pg_helpers.spec.ts for the pass-2 residual shape.
• Revert tests (isolating each fix): removing the redaction wrap while keeping the cwd fix fails all three redaction specs; disabling the name guard while keeping the cwd fix fails the traversal spec.
• Full local gate green (typecheck, lint, prettier, unit + integration + skills).
• Verified live against a running server: all three shapes return redacted messages over REST with zero cleartext in the server log, and a traversing name is rejected with no file written outside cwd.

Out of scope, known and left alone

• buildPgConnectionString unquoted-value handling (whitespace passwords), internalErrorToHttpError, and the MCP tool error paths: separate follow-ups from the fix(server): redact URL-form connection strings in redactPgSecrets #915 review.
• The controller's outer catch (Connection test failed: ...) cannot carry a DSN today (the service catches internally; only cleanup errors reach it).
• duckdb-type connection tests leave a <name>.duckdb file in the server's cwd (pre-existing; only ducklake files are cleaned up). The new specs clean up their own.

[housejester]

Nice tight follow-up — the redaction, the process.cwd() rooting, and the traversal guard all read correctly, and the guard is properly hoisted ahead of path.join so it matches the allowlist deleteDuckLakeConnectionFile already applies. I confirmed the pg_helpers.ts change is comment + pinning-spec only (the three redactPgSecrets regexes are byte-identical), and traced the pass-2 residual by hand (postgres://u:p@a/b@h/d → postgres://u:***@a/b@h/d) — the comment and the accepted-residual framing from the #915 note both hold.

A couple of things worth a look before merge (no verdict from me, just flags):

1. A third raw log boundary the redaction pass didn't cover. The description says the two log lines on these paths are redacted, but there's a third: testDuckDBConnection builds

const errorMessage = `Attached database '${attachedDb.name}' (${attachedDb.type}) test failed: ${(error as Error).message}`;
logger.error(errorMessage);            // connection.ts:1775 — raw
failedAttachments.push(errorMessage);

and logs it raw at connection.ts:1775 before rethrowing it (where it is redacted at the 1874 boundary). Same message shape that can embed the DSN, not wrapped in redactPgSecrets.

Whether it fires for the connect-refused case the new specs exercise depends on eager-vs-lazy attach: the postgres extension normally connects at ATTACH time, so the throw originates in attachDatabasesToDuckDB (→ redacted at 1874) and 1775 is skipped — which is why the specs still pass. But if an attach half-succeeds and the per-db probe query at line 1699 fails with a message carrying connection detail, 1775 logs it unredacted. Cheapest fix is to redact it there too for the same defense-in-depth as the rest of the PR: logger.error(redactPgSecrets(errorMessage)).

2. The cwd fix makes a dormant file-write side effect live. Restoring the attach path means every POST /api/v0/connections/test with a duckdb type now creates/overwrites a <name>.duckdb in the server's cwd — and unlike the ducklake file it's never cleaned up (the finally only removes <name>_ducklake.duckdb). Names can't traverse (the guard holds), so it's clutter/disk-growth bounded by distinct valid names on an unauthenticated endpoint, not an escape — and the PR notes it as pre-existing/out-of-scope. But since this PR is what makes the path reachable, it may be worth folding the symmetric cleanup (delete <name>.duckdb too) into the same finally rather than leaving it owed.

Minor, non-blocking: redactPgSecrets is pg/URI-shaped, so it wouldn't catch a raw S3 secretAccessKey if a storage-attach error ever echoed one. The ducklake test fails at the port-1 catalog connect before storage is touched, so this looks unreachable today — just noting the redactor's shape for future storage-side error paths.

[kylenesbit]

Reviewed the diff against the tree, ran the new specs locally, and probed one behavior I wasn't sure about.

The three claimed fixes are real and correctly implemented. I traced the pass-2 residual by hand (postgres://u:p@a/b@h/d -> postgres://u:***@a/b@h/d) and confirmed the pg_helpers.ts change is comment-plus-spec only, and both notes from the #915 review are addressed. All 87 tests in connection.spec.ts + pg_helpers.spec.ts pass locally, and the redaction specs do exercise the real attach path here — the returned message comes back as IO Error: Unable to connect to Postgres at "postgres://alice:***@127.0.0.1:1/mydb".

One finding I'd want fixed before merge, because the new cleanup deletes files the connection test never created.

1. The <name>.duckdb cleanup deletes an operator's pre-existing database

The finally now unconditionally removes <cwd>/<name>.duckdb, but nothing checks that the test created that file. Since this PR makes the attach path reachable, the endpoint also opens that path read-write first. Verified by seeding a real DuckDB database in the server's cwd and then testing a duckdb connection with the matching name:

seed:   CREATE TABLE precious AS SELECT 42 AS answer   ->  review_probe_real.duckdb
POST:   { name: "review_probe_real", type: "duckdb", ... }   (attach fails, port 1)
log:    Removed connection file review_probe_real.duckdb from .../packages/server
after:  test status: failed | operator database still present: false

The database is gone. A second probe with a non-DuckDB file at that path returned The file ... exists, but it is not a valid DuckDB database file! and then deleted it anyway, which shows the path is opened before the delete and that the delete is unconditional. POST /api/v0/connections/test has no auth middleware (server.ts:1121; no auth app.use in the file), so this is an unauthenticated primitive for deleting any <cwd>/<allowlisted-name>.duckdb. Running the server from a directory that holds your own DuckDB files is normal, so the name collision isn't exotic. The shared file also means two concurrent tests of one name clobber each other: one request's finally deletes the file the other still has open.

The cleanest fix removes the class rather than the symptom — give the throwaway config a throwaway directory:

const testRoot = await fs.mkdtemp(path.join(os.tmpdir(), "publisher-conn-test-"));
environmentConfig = buildEnvironmentMalloyConfig([connectionConfig], testRoot);
// finally:
await fs.rm(testRoot, { recursive: true, force: true });

That handles the deletion hazard, the litter from note 2 of the last review, the concurrency clobber, and the traversal exposure at once, and it keeps the specs from writing into the repo tree. Keep assertSafePackageName as defense in depth. If you'd rather stay in cwd, at minimum stat the file before the test and skip the delete when it already existed.

2. The name guard now rejects connection names the rest of the system accepts

assertSafePackageName runs for every connection type, but nothing else constrains connection names to [A-Za-z0-9._-]: assembleEnvironmentConnections only rejects empty, duplicate, and the reserved duckdb (connection_config.ts:546-560), and Connection.name in api-doc.yaml:3658 is a bare string with no pattern. So a connection named my prod db can be created and served, but the console's Test button on it now returns Invalid package name: must be 1-255 characters... — for a Postgres or BigQuery connection that never touches the filesystem. Either scope the assert to the branch that derives a filename, or (better) validate names at create/update so the whole system agrees.

3. An invalid name is reported as a 200 with status: "failed"

The assert sits inside the try, so its BadRequestError is caught and returned as a failed test — the traversal spec pins that. Everywhere else that error is what produces a 400 through the controller's mapper. The controller already throws BadRequestError for a missing name and type before calling the service, and the route maps those to 400, so moving the name check up next to them keeps the semantics consistent for one line.

Smaller notes

• The cleanup warning loses its reason. The traversal spec run logs warn: Error cleaning up connection test file {"error":{}} — an Error doesn't JSON-serialize, and the other new log sites correctly pass .message. That warn also fires on every rejected-name request, since the cleanup helper re-runs the assert that just failed; skipping cleanup when the name never validated removes guaranteed noise from a path that is by definition fine.
• logger.error("Connection test failed", { error: message }) drops the stack that logger.error(error) carried. redactPgSecrets(error.stack ?? error.message) keeps the diagnostics and stays redacted.
• The redactor is pg-shaped but the boundary is now every connection type. The ducklake spec plants an S3 secretAccessKey while expectRedacted only checks the pg password, which reads as more coverage than exists; Snowflake privateKeyPass, Databricks token, and an Azure SAS URL's ?sig= all flow through this same errorMessage unredacted. Not a regression, and the shape was already flagged, but since this PR establishes the boundary a follow-up looks owed.
• The redaction specs can pass vacuously. expectRedacted gates the marker on the message containing 127.0.0.1, and when a DuckDB extension can't load the failure message carries no DSN at all — so the not.toContain(password) invariant is trivially satisfied too and the specs go green whether or not the redaction exists. The Connection Integration Tests workflow has network and runs on same-repo PRs, so the real path is covered today. Since sinon is already imported in this file, one spec stubbing the lookup to throw an error carrying a DSN would pin the wiring regardless of extension availability.
• The raw rethrow in attachDatabasesToDuckDB is justified for handleAlreadyAttachedError, but that message also reaches lookupConnection callers on query and model paths, where internalErrorToHttpError doesn't redact. Noted as out of scope from fix(server): redact URL-form connection strings in redactPgSecrets #915 — worth keeping visible so the residual doesn't get lost.

The branch is behind main and needs a rebase. The ducklake asymmetry is correct as written: connection_config.ts:570-574 gives a ducklake connection only a <name>_ducklake.duckdb path, so the else if isn't missing a case.

[mlennie]

Thanks Kyle, this is a great review. Finding 1 is a real regression I introduced in the last round, and your temp-dir suggestion is clearly the right fix. All three are addressed (force-pushed, rebased onto current main).

1. The cleanup deleting an operator's database. You're right, and it's worse than litter: the attach opens the path read-write and the finally deleted it unconditionally on an unauthenticated endpoint. I took your cleaner fix rather than the stat-guard: the throwaway config is now rooted in a fresh fs.mkdtemp directory and the whole directory is removed in the finally. That closes the deletion hazard, the concurrency clobber, and the cwd litter in one move, and the test files stop landing in the repo tree. assertSafePackageName stays as defense in depth. New spec seeds a sentinel <cwd>/<name>.duckdb and asserts a same-named duckdb test leaves it byte-for-byte untouched.

2. The name guard rejecting names the rest of the system accepts. Scoped the check to duckdb/ducklake, the only types that derive a <name>.duckdb filename. A Postgres or BigQuery connection with any name is testable again; new spec pins that my prod db on a Postgres connection fails with a connection error, not "Invalid package name". Validating names at create/update is the better long-term answer, but that's its own change.

3. Invalid name as 200 status:failed. Moved the scoped check up into the controller next to the existing config/type checks, so an unsafe duckdb/ducklake name is now a 400 through the mapper; new controller spec covers it. Empty names still fall through to the service's missing-name failure.

On the smaller notes: the log now keeps the redacted stack (error.stack ?? error.message); the {"error":{}} warn is gone with the temp-dir cleanup, which also no longer re-runs the assert on a rejected name. I took your sinon suggestion too, there's now a spec that stubs runSQL to throw a DSN-bearing error, so the redaction wiring is pinned regardless of whether an extension loads, rather than passing vacuously. That stubbed test also surfaced one more raw-error log in the same attach flow (isDatabaseAttached's SHOW DATABASES catch), which I redacted for consistency, its SHOW DATABASES can't carry a DSN in practice, but it's the same pattern and was printing the test's fake secret to stderr.

Left as follow-ups, tracked: the pg-shaped redactor doesn't cover Snowflake privateKeyPass, a Databricks token, an Azure SAS ?sig=, or an S3 secretAccessKey at this boundary, so a redactor pass that widens the shape is owed now that every connection type funnels through here; and the raw rethrow reaching query/model paths via internalErrorToHttpError stays out of scope from the #915 line. Also noted from the earlier review: attachedDatabases[].name flows unescaped into DuckDB identifiers, and environment.ts's deleteDuckDBConnection builds its path without the guard its ducklake sibling uses.

Rebased onto current main (past the materialization tier); full gate green (typecheck, lint, prettier, unit + integration + skills).