From bd13b2f0b38fb302249763cc588f04dbdec12847 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 18 Sep 2026 08:08:30 +0000 Subject: [PATCH 1/3] fix(worker): detach client-attached catalogs at the session boundary Attached databases are instance-global, like secrets, so a client's `ATTACH ... AS db` survived its session on a hot-idle worker and was inherited by whichever session that worker served next. Two failures: - Correctness: clients attach with `ATTACH IF NOT EXISTS ... AS db`, a no-op when a previous session left a `db` behind, so the new session silently reads the previous session's target. On 2026-09-18 a tenant's sqlmesh run inherited an analyst's `db` (a different Postgres endpoint) and its parallel postgres scans failed with `SET TRANSACTION SNAPSHOT ... snapshot does not exist`. Every failing worker had served such an attach beforehand; every worker that had not was clean. - Isolation: an attached catalog freezes its connection string, credentials included, at ATTACH time, so it outlives wipeUserSecrets. Another user of the org could query it without holding the credential. detachUserCatalogs runs next to the secrets wipe: mandatory on every shared-warm CreateSession (a failure fails the session), best-effort at DestroySession. It preserves DuckDB-internal catalogs, the instance's primary database (resolved by lowest oid, since it is a file stem rather than `memory` when DataDir is set) and the worker-managed allowlist (ducklake, delta, __ducklake_metadata_*). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_016ZnZTbiHkGWQ7485DoJB3E --- CLAUDE.md | 27 +++- duckdbservice/service.go | 21 +++ duckdbservice/user_catalogs.go | 101 +++++++++++++++ duckdbservice/user_catalogs_test.go | 193 ++++++++++++++++++++++++++++ tests/mw-dev/e2e/harness.sh | 19 ++- 5 files changed, 356 insertions(+), 5 deletions(-) create mode 100644 duckdbservice/user_catalogs.go create mode 100644 duckdbservice/user_catalogs_test.go diff --git a/CLAUDE.md b/CLAUDE.md index fc65e541..3eafb673 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -740,6 +740,24 @@ Invariants for anyone touching this path: + the `__default_*`/`duckgres_*` prefixes, which activation re-creates). It MUST run before replay on every CreateSession in shared-warm mode, and a wipe failure MUST fail the session. +- **Client-ATTACHed catalogs get the same treatment, in the same place.** + `detachUserCatalogs` (`duckdbservice/user_catalogs.go`) runs right after the + secrets wipe on every shared-warm CreateSession and a failure MUST fail the + session (best-effort again at DestroySession). Attached databases are + instance-global exactly like secrets, and an attached catalog freezes its + connection string — credentials included — at ATTACH time, so it outlives + the wipe of the secret it was built from: without the detach, user B can + query user A's attached Postgres without ever holding A's credential. It is + also a correctness bug, not just isolation: clients attach with + `ATTACH IF NOT EXISTS ... AS db`, a no-op when the previous session left a + `db` behind, so the new session silently reads the PREVIOUS session's target + (2026-09-18: a tenant's sqlmesh run inherited an analyst's `db` endpoint and + its parallel postgres scans died with `SET TRANSACTION SNAPSHOT ... snapshot + does not exist`). Preserved: DuckDB-internal catalogs, the instance's primary + database (resolved by lowest oid — it is a file stem, not `memory`, when + DataDir is set), and the `isSystemCatalog` allowlist (`ducklake`, `delta`, + `__ducklake_metadata_*`). **A new worker-managed ATTACH must be added to that + allowlist** or the next session create will detach it. - **Execute-then-persist ordering.** Persist only statements DuckDB accepted; a store failure after a successful exec is an ERROR telling the user the secret will NOT survive the session. Replay failures at session create are @@ -762,10 +780,11 @@ Invariants for anyone touching this path: `usersecrets.RedactErrorForLog(query, errMsg)` guards those error sinks (`logQueryError`, `logQuery`); keep new error logging behind it too, and pass the original (un-redacted) query so it can classify. -- Touching the interception, wipe/replay, or payload shape → update - `server/conn_user_secrets_test.go`, `duckdbservice/user_secrets_test.go`, - and the `persistent_user_secret`(+`_isolation`) assertions in - `tests/mw-dev/e2e/harness.sh`. +- Touching the interception, wipe/replay, catalog detach, or payload shape → + update `server/conn_user_secrets_test.go`, + `duckdbservice/user_secrets_test.go`, `duckdbservice/user_catalogs_test.go`, + and the `persistent_user_secret`(+`_isolation`, which also carries the + attached-catalog leak check) assertions in `tests/mw-dev/e2e/harness.sh`. ## Admin Console (VPC-private web UI, `kubernetes` tag) diff --git a/duckdbservice/service.go b/duckdbservice/service.go index 3e695406..5a2f09b3 100644 --- a/duckdbservice/service.go +++ b/duckdbservice/service.go @@ -1257,6 +1257,22 @@ func (p *SessionPool) CreateSession(username, memoryLimit string, threads int, s if len(wiped) > 0 { slog.Info("Wiped user secrets left by previous session.", "user", username, "count", len(wiped)) } + // Same isolation step for client-ATTACHed databases: they are + // instance-global too, and an attached catalog keeps the credentials + // it was built from even after the secret above is gone. Mandatory for + // the same reason as the wipe — see detachUserCatalogs. + detached, detachErr := detachUserCatalogs(secretCtx, conn) + if detachErr != nil { + secretCancel() + _ = conn.Close() + p.mu.Lock() + p.reserved-- + p.mu.Unlock() + return nil, nil, fmt.Errorf("detach user catalogs before session start: %w", detachErr) + } + if len(detached) > 0 { + slog.Info("Detached user catalogs left by previous session.", "user", username, "count", len(detached), "catalogs", detached) + } secretWarnings = replayUserSecrets(secretCtx, conn, username, secretStatements) secretCancel() } @@ -1453,6 +1469,11 @@ func (p *SessionPool) DestroySession(token string) error { if _, err := wipeUserSecrets(wipeCtx, session.DB); err != nil { log.Warn("Failed to wipe user secrets on session destroy.", "user", session.Username, "error", err) } + // Likewise for client-ATTACHed databases, which carry their own copy + // of the credentials. Best-effort here; mandatory at CreateSession. + if _, err := detachUserCatalogs(wipeCtx, session.DB); err != nil { + log.Warn("Failed to detach user catalogs on session destroy.", "user", session.Username, "error", err) + } wipeCancel() } diff --git a/duckdbservice/user_catalogs.go b/duckdbservice/user_catalogs.go new file mode 100644 index 00000000..365748a2 --- /dev/null +++ b/duckdbservice/user_catalogs.go @@ -0,0 +1,101 @@ +package duckdbservice + +import ( + "context" + "fmt" + "strings" +) + +// systemCatalogNames are the attached databases the worker itself manages on a +// shared-warm instance: activation attaches (and re-attaches) them, so a +// session-boundary detach must leave them alone. +var systemCatalogNames = map[string]struct{}{ + "ducklake": {}, + "delta": {}, +} + +// systemCatalogPrefixes covers catalogs DuckDB extensions attach on the +// worker's behalf — DuckLake attaches its metadata store under this prefix. +var systemCatalogPrefixes = []string{"__ducklake_metadata_"} + +// isSystemCatalog reports whether an attached database is worker-managed. +// DuckDB catalog names are case-insensitive. +func isSystemCatalog(name string) bool { + name = strings.ToLower(name) + if _, ok := systemCatalogNames[name]; ok { + return true + } + for _, p := range systemCatalogPrefixes { + if strings.HasPrefix(name, p) { + return true + } + } + return false +} + +// detachUserCatalogs detaches every database a client ATTACHed on the shared +// per-org instance, preserving only what the worker manages itself: DuckDB's +// internal catalogs (system/temp), the instance's primary database, and the +// isSystemCatalog allowlist. +// +// Attached databases are instance-global, exactly like secrets, so an ATTACH +// survives its session on a hot-idle worker and is inherited by whichever +// session the worker serves next. Two distinct failures follow: +// +// - Cross-user isolation: an attached catalog freezes its connection string — +// credentials included — at ATTACH time, so it keeps working after +// wipeUserSecrets has dropped the secret it was built from. User B of the +// org could query user A's attached Postgres without ever holding A's +// credential. +// - Silent mis-routing: clients attach with `ATTACH IF NOT EXISTS ... AS db`, +// which is a no-op when a previous session left a `db` behind, so the new +// session reads whatever target the PREVIOUS session chose. On 2026-09-18 a +// tenant's sqlmesh run inherited an analyst's `db` (a different endpoint) +// and its parallel postgres scans failed with `SET TRANSACTION SNAPSHOT +// ... snapshot does not exist`. +// +// Like the secrets wipe this is only safe with one session per worker: with a +// concurrent session it would detach a catalog out from under a live query. +// +// Returns the names that were detached. +func detachUserCatalogs(ctx context.Context, h secretDBHandle) ([]string, error) { + // The primary database is the one the instance was opened on — the lowest + // non-internal oid, since everything else is attached afterwards. It is + // `memory` on k8s workers but a file stem when DataDir is set, so resolve it + // rather than hard-coding a name. + rows, err := h.QueryContext(ctx, ` + SELECT database_name, + database_oid = (SELECT MIN(database_oid) FROM duckdb_databases() WHERE NOT internal) AS is_primary + FROM duckdb_databases() + WHERE NOT internal`) + if err != nil { + return nil, fmt.Errorf("list attached databases: %w", err) + } + var candidates []string + for rows.Next() { + var name string + var isPrimary bool + if err := rows.Scan(&name, &isPrimary); err != nil { + _ = rows.Close() + return nil, err + } + if isPrimary || isSystemCatalog(name) { + continue + } + candidates = append(candidates, name) + } + if err := rows.Err(); err != nil { + _ = rows.Close() + return nil, err + } + _ = rows.Close() + + var detached []string + for _, name := range candidates { + if _, err := h.ExecContext(ctx, "DETACH DATABASE IF EXISTS "+quoteSecretIdent(name)); err != nil { + return detached, fmt.Errorf("detach database %q: %w", name, err) + } + detached = append(detached, name) + } + return detached, nil +} diff --git a/duckdbservice/user_catalogs_test.go b/duckdbservice/user_catalogs_test.go new file mode 100644 index 00000000..e54c283a --- /dev/null +++ b/duckdbservice/user_catalogs_test.go @@ -0,0 +1,193 @@ +package duckdbservice + +import ( + "context" + "database/sql" + "path/filepath" + "sort" + "strings" + "testing" + "time" + + "github.com/posthog/duckgres/server" +) + +func attachedDatabases(t *testing.T, db *sql.DB) []string { + t.Helper() + rows, err := db.Query("SELECT database_name FROM duckdb_databases() WHERE NOT internal ORDER BY database_name") + if err != nil { + t.Fatalf("list databases: %v", err) + } + defer func() { _ = rows.Close() }() + var names []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + t.Fatalf("scan: %v", err) + } + names = append(names, n) + } + return names +} + +func mustExecDB(t *testing.T, db *sql.DB, q string) { + t.Helper() + if _, err := db.Exec(q); err != nil { + t.Fatalf("exec %q: %v", q, err) + } +} + +// Regression for the 2026-09-18 incident: a client `ATTACH ... AS db` survived +// its session on the hot-idle worker, so the next session's +// `ATTACH IF NOT EXISTS ... AS db` was a no-op and it read the PREVIOUS +// session's target. The detach must remove client catalogs and keep the +// worker-managed ones. +func TestDetachUserCatalogs(t *testing.T) { + db := openSecretTestDB(t) + dir := t.TempDir() + // Worker-managed catalogs, stood in for by plain DuckDB files: only the + // names matter to the allowlist. + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "lake.duckdb")+"' AS ducklake") + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "meta.duckdb")+"' AS __ducklake_metadata_ducklake") + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "delta.duckdb")+"' AS delta") + // Client catalogs, including one needing identifier quoting. + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "a.duckdb")+"' AS db") + mustExecDB(t, db, "ATTACH ':memory:' AS \"My \"\"odd\"\" Db\"") + + detached, err := detachUserCatalogs(context.Background(), db) + if err != nil { + t.Fatalf("detachUserCatalogs: %v", err) + } + sort.Strings(detached) + if got, want := strings.Join(detached, ","), `My "odd" Db,db`; got != want { + t.Errorf("detached = %q, want %q", got, want) + } + + got := strings.Join(attachedDatabases(t, db), ",") + if want := "__ducklake_metadata_ducklake,delta,ducklake,memory"; got != want { + t.Errorf("remaining databases = %q, want %q", got, want) + } + + // The point of the fix: the next session's IF NOT EXISTS attach must now + // take effect instead of silently inheriting the old target. + mustExecDB(t, db, "ATTACH IF NOT EXISTS '"+filepath.Join(dir, "b.duckdb")+"' AS db") + var path string + if err := db.QueryRow("SELECT path FROM duckdb_databases() WHERE database_name = 'db'").Scan(&path); err != nil { + t.Fatalf("read db path: %v", err) + } + if !strings.HasSuffix(path, "b.duckdb") { + t.Errorf("db path = %q, want the new session's target (b.duckdb)", path) + } +} + +// The primary database is a file stem, not `memory`, when the instance is +// opened on a file. It must never be a detach candidate regardless of name. +func TestDetachUserCatalogsKeepsFileBackedPrimary(t *testing.T) { + dir := t.TempDir() + db, err := sql.Open("duckdb", filepath.Join(dir, "alice.duckdb")) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "other.duckdb")+"' AS db") + + detached, err := detachUserCatalogs(context.Background(), db) + if err != nil { + t.Fatalf("detachUserCatalogs: %v", err) + } + if len(detached) != 1 || detached[0] != "db" { + t.Errorf("detached = %v, want [db]", detached) + } + if got := strings.Join(attachedDatabases(t, db), ","); got != "alice" { + t.Errorf("remaining databases = %q, want %q", got, "alice") + } +} + +func TestDetachUserCatalogsNoop(t *testing.T) { + db := openSecretTestDB(t) + detached, err := detachUserCatalogs(context.Background(), db) + if err != nil { + t.Fatalf("detachUserCatalogs: %v", err) + } + if len(detached) != 0 { + t.Errorf("detached = %v, want none", detached) + } +} + +func TestIsSystemCatalog(t *testing.T) { + for name, want := range map[string]bool{ + "ducklake": true, + "DuckLake": true, + "delta": true, + "__ducklake_metadata_ducklake": true, + "db": false, + "ducklake2": false, + "memory": false, // protected as the primary, not by name + } { + if got := isSystemCatalog(name); got != want { + t.Errorf("isSystemCatalog(%q) = %v, want %v", name, got, want) + } + } +} + +// End-to-end at the pool level: two consecutive sessions on one shared-warm +// worker. Session A attaches `db`; session B — possibly a different user of the +// org — must not inherit it, and B's own IF NOT EXISTS attach must win. +func TestCreateSessionDetachesPreviousSessionCatalogs(t *testing.T) { + db := openSecretTestDB(t) + dir := t.TempDir() + pool := &SessionPool{ + sessions: make(map[string]*Session), + stopRefresh: make(map[string]func()), + duckLakeSem: make(chan struct{}, 1), + warmupDB: db, + warmupDone: make(chan struct{}), + cfg: server.Config{SessionInitTimeout: time.Second}, + maxSessions: 1, + sharedWarmMode: true, + activation: &activatedTenantRuntime{payload: ActivationPayload{OrgID: "analytics"}, db: db}, + } + close(pool.warmupDone) + + a, _, err := pool.CreateSession("alice", "", 0, nil) + if err != nil { + t.Fatalf("CreateSession(alice): %v", err) + } + if _, err := a.Conn.ExecContext(context.Background(), + "ATTACH IF NOT EXISTS '"+filepath.Join(dir, "alice_target.duckdb")+"' AS db"); err != nil { + t.Fatalf("alice attach: %v", err) + } + if err := pool.DestroySession(a.ID); err != nil { + t.Fatalf("DestroySession(alice): %v", err) + } + // The destroy-time detach is best-effort, but with nothing in the way it + // should already have cleared the catalog off the hot-idle worker. + if got := strings.Join(attachedDatabases(t, db), ","); got != "memory" { + t.Errorf("databases after alice's session = %q, want only memory", got) + } + + // Simulate the destroy-time detach having been skipped (it is best-effort): + // the CreateSession detach is the one that must hold. + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "alice_target.duckdb")+"' AS db") + + b, _, err := pool.CreateSession("bob", "", 0, nil) + if err != nil { + t.Fatalf("CreateSession(bob): %v", err) + } + defer func() { _ = pool.DestroySession(b.ID) }() + if got := strings.Join(attachedDatabases(t, db), ","); got != "memory" { + t.Fatalf("bob inherited alice's catalogs: %q", got) + } + if _, err := b.Conn.ExecContext(context.Background(), + "ATTACH IF NOT EXISTS '"+filepath.Join(dir, "bob_target.duckdb")+"' AS db"); err != nil { + t.Fatalf("bob attach: %v", err) + } + var path string + if err := b.Conn.QueryRowContext(context.Background(), + "SELECT path FROM duckdb_databases() WHERE database_name = 'db'").Scan(&path); err != nil { + t.Fatalf("read db path: %v", err) + } + if !strings.HasSuffix(path, "bob_target.duckdb") { + t.Errorf("bob's db path = %q, want bob_target.duckdb", path) + } +} diff --git a/tests/mw-dev/e2e/harness.sh b/tests/mw-dev/e2e/harness.sh index 5bfb09d8..054b6421 100755 --- a/tests/mw-dev/e2e/harness.sh +++ b/tests/mw-dev/e2e/harness.sh @@ -2548,11 +2548,28 @@ persistent_user_secret_isolation() { # org rootpw # TEMPORARY secret left behind on the instance-global worker DuckDB. n="$(pg "$org" "$u2pw" ducklake "SELECT count(*) FROM duckdb_secrets() WHERE name = '$tname'" "$u2")" [ "$n" = "0" ] || fail "user secret: user $u2 sees root's TEMPORARY secret — cross-user leak (count=$n)" + # Same leak path for client-ATTACHed catalogs, which are instance-global too + # and keep the credentials they were attached with. Regression for the + # 2026-09-18 incident: one session's `ATTACH ... AS db` survived on the + # hot-idle worker, the next session's `ATTACH IF NOT EXISTS ... AS db` was a + # no-op, and it silently queried the PREVIOUS session's target. Root leaves a + # catalog holding a marker table; $u2's IF NOT EXISTS attach under the same + # alias must come up EMPTY (a fresh catalog), not inherit root's. Both sides + # are pinning statements, so they land on the org's standard worker rather + # than being split across the exploratory tier. In-memory catalogs keep the + # check free of external dependencies; the name carries no digits so the + # result survives the command-tag noise psql prints for the ATTACH. + pg "$org" "$pw" ducklake "ATTACH ':memory:' AS leakcat; CREATE TABLE leakcat.main.marker AS SELECT 42 AS x" >/dev/null + n="$(pg "$org" "$u2pw" ducklake "ATTACH IF NOT EXISTS ':memory:' AS leakcat; SELECT count(*) FROM duckdb_tables() WHERE database_name = 'leakcat'" "$u2" | tr -dc '0-9')" + [ "$n" = "0" ] || fail "user catalog: user $u2 inherited root's attached catalog — cross-session ATTACH leak (tables=$n)" + # And within one user: root's NEXT session must not see its own stale attach. + n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_databases() WHERE database_name = 'leakcat'" | tr -dc '0-9')" + [ "$n" = "0" ] || fail "user catalog: attached catalog survived into root's next session (count=$n)" # Root's stored (persistent) copy must be unaffected by $u2's session-create wipe. n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_secrets() WHERE name = '$sname'")" [ "$n" = "1" ] || fail "user secret: root's persistent secret lost after $u2's session (count=$n)" pg "$org" "$pw" ducklake "DROP PERSISTENT SECRET $sname" >/dev/null - log "user secret isolation OK on $org ($u2 blind to root's persistent AND temporary secrets; root's stored copy intact)" + log "user secret isolation OK on $org ($u2 blind to root's persistent AND temporary secrets AND attached catalogs; root's stored copy intact)" } # ---- resilience ----------------------------------------------------------- From 8107e83624f157fbb22f3bcae84c11fef1cb32ce Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 18 Sep 2026 08:20:34 +0000 Subject: [PATCH 2/3] fix(worker): keep the catalog detach off DuckLake and off the replay budget Review follow-ups on the session-boundary detach: - Catalog-qualify the listing (system.main.duckdb_databases()). By this point in session create the default catalog is ducklake, and sessionmeta documents that an unqualified duckdb_databases() there starts a DuckLake transaction, which pays a full catalog reload when the schema version has moved. The session-init probe has normally paid for it already, so this is defensive rather than a measured win. - Give the detach its own deadline instead of sharing the secrets wipe's, so a slow detach cannot starve the persistent-secret replay that follows. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_016ZnZTbiHkGWQ7485DoJB3E --- duckdbservice/service.go | 11 ++++++++--- duckdbservice/user_catalogs.go | 12 ++++++++++-- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/duckdbservice/service.go b/duckdbservice/service.go index 5a2f09b3..8cbeac90 100644 --- a/duckdbservice/service.go +++ b/duckdbservice/service.go @@ -1261,7 +1261,10 @@ func (p *SessionPool) CreateSession(username, memoryLimit string, threads int, s // instance-global too, and an attached catalog keeps the credentials // it was built from even after the secret above is gone. Mandatory for // the same reason as the wipe — see detachUserCatalogs. - detached, detachErr := detachUserCatalogs(secretCtx, conn) + // Own deadline: a slow detach must not eat the replay's budget below. + detachCtx, detachCancel := context.WithTimeout(context.Background(), userSecretOpTimeout) + detached, detachErr := detachUserCatalogs(detachCtx, conn) + detachCancel() if detachErr != nil { secretCancel() _ = conn.Close() @@ -1471,10 +1474,12 @@ func (p *SessionPool) DestroySession(token string) error { } // Likewise for client-ATTACHed databases, which carry their own copy // of the credentials. Best-effort here; mandatory at CreateSession. - if _, err := detachUserCatalogs(wipeCtx, session.DB); err != nil { + wipeCancel() + detachCtx, detachCancel := context.WithTimeout(context.Background(), userSecretOpTimeout) + if _, err := detachUserCatalogs(detachCtx, session.DB); err != nil { log.Warn("Failed to detach user catalogs on session destroy.", "user", session.Username, "error", err) } - wipeCancel() + detachCancel() } // Best-effort restore of the cache-proxy S3 transport if this session diff --git a/duckdbservice/user_catalogs.go b/duckdbservice/user_catalogs.go index 365748a2..8e4e273f 100644 --- a/duckdbservice/user_catalogs.go +++ b/duckdbservice/user_catalogs.go @@ -63,10 +63,18 @@ func detachUserCatalogs(ctx context.Context, h secretDBHandle) ([]string, error) // non-internal oid, since everything else is attached afterwards. It is // `memory` on k8s workers but a file stem when DataDir is set, so resolve it // rather than hard-coding a name. + // + // The function is catalog-qualified on purpose. By this point in session + // create the default catalog is `ducklake`, and sessionmeta documents that + // an unqualified `duckdb_databases()` there starts a DuckLake transaction — + // which pays a full catalog reload (tens of seconds on a large tenant) when + // the schema version has moved. Naming `system` means the lookup never has + // to consult the default catalog. The session-init probe has normally paid + // for that reload already, so this is belt and braces, not a measured win. rows, err := h.QueryContext(ctx, ` SELECT database_name, - database_oid = (SELECT MIN(database_oid) FROM duckdb_databases() WHERE NOT internal) AS is_primary - FROM duckdb_databases() + database_oid = (SELECT MIN(database_oid) FROM system.main.duckdb_databases() WHERE NOT internal) AS is_primary + FROM system.main.duckdb_databases() WHERE NOT internal`) if err != nil { return nil, fmt.Errorf("list attached databases: %w", err) From 8d01aaace564e9f422ba950faf8c65c853d0fb4f Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 18 Sep 2026 08:23:26 +0000 Subject: [PATCH 3/3] test(e2e): run the attached-catalog leak check over one connection per side Every `pg` call is its own session, whose session-create detach removes the catalog before a second call could observe it, so the check could only ever pass vacuously. Feed each side's statements to psql on stdin (one Q message per statement, same session), make the catalog name run-unique, assert the marker is visible in root's own session, and assert ducklake + memory survive the detach. Statement shapes verified against a local standalone server. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_016ZnZTbiHkGWQ7485DoJB3E --- tests/mw-dev/e2e/harness.sh | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/tests/mw-dev/e2e/harness.sh b/tests/mw-dev/e2e/harness.sh index 054b6421..176f8620 100755 --- a/tests/mw-dev/e2e/harness.sh +++ b/tests/mw-dev/e2e/harness.sh @@ -2554,17 +2554,32 @@ persistent_user_secret_isolation() { # org rootpw # hot-idle worker, the next session's `ATTACH IF NOT EXISTS ... AS db` was a # no-op, and it silently queried the PREVIOUS session's target. Root leaves a # catalog holding a marker table; $u2's IF NOT EXISTS attach under the same - # alias must come up EMPTY (a fresh catalog), not inherit root's. Both sides - # are pinning statements, so they land on the org's standard worker rather - # than being split across the exploratory tier. In-memory catalogs keep the - # check free of external dependencies; the name carries no digits so the - # result survives the command-tag noise psql prints for the ATTACH. - pg "$org" "$pw" ducklake "ATTACH ':memory:' AS leakcat; CREATE TABLE leakcat.main.marker AS SELECT 42 AS x" >/dev/null - n="$(pg "$org" "$u2pw" ducklake "ATTACH IF NOT EXISTS ':memory:' AS leakcat; SELECT count(*) FROM duckdb_tables() WHERE database_name = 'leakcat'" "$u2" | tr -dc '0-9')" - [ "$n" = "0" ] || fail "user catalog: user $u2 inherited root's attached catalog — cross-session ATTACH leak (tables=$n)" - # And within one user: root's NEXT session must not see its own stale attach. - n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_databases() WHERE database_name = 'leakcat'" | tr -dc '0-9')" - [ "$n" = "0" ] || fail "user catalog: attached catalog survived into root's next session (count=$n)" + # alias must come up EMPTY (a fresh catalog), not inherit root's. + # + # Each side's statements must ride ONE connection — every `pg` call is its + # own session, whose session-create detach would already have removed the + # catalog — so they are fed to psql on stdin (one Q message per statement, + # same session). ATTACH is not PostgreSQL syntax, so it parse-fails into the + # pin set and both sides land on the org's standard worker rather than being + # split across the exploratory tier. In-memory catalogs keep the check free of + # external dependencies; the name is run-unique. + lcat="e2e_leakcat_$$" + out="$(printf "ATTACH ':memory:' AS %s;\nCREATE TABLE %s.main.marker AS SELECT 42 AS x;\nSELECT count(*) FROM duckdb_tables() WHERE database_name = '%s';\n" "$lcat" "$lcat" "$lcat" | \ + PGPASSWORD="$pw" psql \ + "sslmode=require host=$org$SNI_SUFFIX hostaddr=$CP_IP port=5432 user=root dbname=ducklake" \ + -v ON_ERROR_STOP=1 -tA 2>&1)" || fail "user catalog: root attach session failed: $out" + [ "$(printf '%s' "$out" | tail -1)" = "1" ] || fail "user catalog: root's marker table not visible in its own session ($out)" + out="$(printf "ATTACH IF NOT EXISTS ':memory:' AS %s;\nSELECT count(*) FROM duckdb_tables() WHERE database_name = '%s';\n" "$lcat" "$lcat" | \ + PGPASSWORD="$u2pw" psql \ + "sslmode=require host=$org$SNI_SUFFIX hostaddr=$CP_IP port=5432 user=$u2 dbname=ducklake" \ + -v ON_ERROR_STOP=1 -tA 2>&1)" || fail "user catalog: $u2 attach session failed: $out" + [ "$(printf '%s' "$out" | tail -1)" = "0" ] || fail "user catalog: user $u2 inherited root's attached catalog — cross-session ATTACH leak ($out)" + # Gone from the next fresh session too, while the worker-managed catalogs + # must survive every session-create detach. + n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_databases() WHERE database_name = '$lcat'")" + [ "$n" = "0" ] || fail "user catalog: attached catalog survived into a fresh session (count=$n)" + n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_databases() WHERE database_name IN ('ducklake','memory')")" + [ "$n" = "2" ] || fail "user catalog: worker-managed catalogs missing after the detach (count=$n)" # Root's stored (persistent) copy must be unaffected by $u2's session-create wipe. n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_secrets() WHERE name = '$sname'")" [ "$n" = "1" ] || fail "user secret: root's persistent secret lost after $u2's session (count=$n)"