From 9c7d96927df01851ab1c5cab8f599214b2908a09 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Thu, 17 Sep 2026 15:56:05 +0100 Subject: [PATCH 1/2] fix(cli): treat only verifying sslmodes as a local TLS demand (CLI-2366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Honoring an explicit `--db-url` TLS request against a loopback target keyed off any populated `sslmode`, which is not the same as the DSN asking for TLS. `prefer` and `allow` name a fallback libpq performs and `sslConfigsFor` does not, and `sslmode` is also filled from `PGSSLMODE` and libpq service files when the DSN omits it. A developer with `PGSSLMODE=prefer` exported, or a DSN carrying `?sslmode=prefer`, therefore lost the local plaintext exemption and failed the handshake against a plaintext Postgres — on the shared connection seam, so `db query`, dump and pull were affected as well as `gen types`. Only `require`, `verify-ca`, `verify-full` or a supplied root cert now count as a demand. A TLS tunnel on loopback keeps working, and a local target that names nothing, `prefer`, `allow` or `disable` connects in plaintext as before. Co-Authored-By: Claude Opus 5 --- .../db-connection.sql-pg.integration.test.ts | 48 +++++++++++++++++++ .../db-connection.sql-pg.layer.ts | 17 +++++-- .../db-connection.sql-pg.unit.test.ts | 33 ++++++++++++- 3 files changed, 92 insertions(+), 6 deletions(-) diff --git a/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts b/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts index 2d98b4a564..c0b706e956 100644 --- a/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts +++ b/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts @@ -885,6 +885,54 @@ describe("a local target's explicit TLS request (CLI-2366: honor --db-url's own }), ); + it.live( + "stays plaintext for a local target with sslmode=prefer (CLI-2366 regression guard: " + + "libpq's TLS-then-plaintext fallback is not implemented by sslConfigsFor)", + () => + Effect.gen(function* () { + const server = yield* Effect.promise(fakeStartupServer); + yield* Effect.gen(function* () { + const pool = yield* acquirePgPool( + { + host: "127.0.0.1", + port: server.port, + user: "postgres", + password: "postgres", + database: "postgres", + sslmode: "prefer", + }, + { isLocal: true, dnsResolver: "native" }, + ); + yield* Effect.tryPromise(() => pool.query("select 1")); + }).pipe(Effect.scoped, Effect.ensuring(Effect.sync(server.close))); + expect(server.sawSslRequest()).toBe(false); + }), + ); + + it.live( + "stays plaintext for a local target with sslmode=allow (libpq's plaintext-then-TLS fallback " + + "is not implemented by sslConfigsFor)", + () => + Effect.gen(function* () { + const server = yield* Effect.promise(fakeStartupServer); + yield* Effect.gen(function* () { + const pool = yield* acquirePgPool( + { + host: "127.0.0.1", + port: server.port, + user: "postgres", + password: "postgres", + database: "postgres", + sslmode: "allow", + }, + { isLocal: true, dnsResolver: "native" }, + ); + yield* Effect.tryPromise(() => pool.query("select 1")); + }).pipe(Effect.scoped, Effect.ensuring(Effect.sync(server.close))); + expect(server.sawSslRequest()).toBe(false); + }), + ); + it.live( "stays plaintext for a local target with no sslmode/sslrootcert set (the default must not regress)", () => diff --git a/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts b/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts index 7fdf023767..95692df026 100644 --- a/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts +++ b/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts @@ -397,13 +397,22 @@ export interface ClientCert { } /** - * Whether the DSN itself asked for TLS behavior: `--db-url`'s `sslmode`/`sslrootcert` are honored - * even against a target classified local (e.g. a TLS tunnel on the loopback stack), so `isLocal` - * alone must not force plaintext when one of these is set. + * `sslmode` values that demand TLS. `prefer` and `allow` describe a fallback libpq would perform + * and {@link sslConfigsFor} does not, so neither counts as a demand; treating them as one would + * turn a plaintext-capable target into a handshake failure. + */ +const TLS_DEMANDING_SSLMODES = new Set(["require", "verify-ca", "verify-full"]); + +/** + * Whether the connection demands TLS, which keeps `--db-url`'s `sslmode`/`sslrootcert` honored + * against a target classified local (e.g. a TLS tunnel on the loopback stack) instead of being + * forced to plaintext by `isLocal` alone. An unset, `prefer`, `allow` or `disable` mode is not a + * demand: `sslmode` is also filled from `PGSSLMODE` and libpq service files, so a merely present + * value cannot be read as the DSN asking for TLS. */ export function tlsExplicitlyRequested(cfg: PgConnInput): boolean { return ( - cfg.sslmode !== undefined || + (cfg.sslmode !== undefined && TLS_DEMANDING_SSLMODES.has(cfg.sslmode)) || (cfg.sslrootcert?.length ?? 0) > 0 || (cfg.sslrootcertInline?.length ?? 0) > 0 ); diff --git a/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts b/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts index 039c6338cf..831de4a370 100644 --- a/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts +++ b/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts @@ -25,6 +25,7 @@ import { toConnectError, toExecError, } from "./db-connection.sql-pg.layer.ts"; +import { parseConnectionString } from "./db-config.parse.ts"; import type { PgConnInput } from "./db-connection.service.ts"; describe("buildConnectionUrl", () => { @@ -273,10 +274,38 @@ describe("tlsExplicitlyRequested (CLI-2366: --db-url TLS against a local target) expect(tlsExplicitlyRequested(base)).toBe(false); }); - it("is true for any sslmode, including disable (still resolved by sslConfigsFor)", () => { + it("is true for require/verify-ca/verify-full, the modes sslConfigsFor cannot fall back from", () => { expect(tlsExplicitlyRequested({ ...base, sslmode: "require" })).toBe(true); + expect(tlsExplicitlyRequested({ ...base, sslmode: "verify-ca" })).toBe(true); expect(tlsExplicitlyRequested({ ...base, sslmode: "verify-full" })).toBe(true); - expect(tlsExplicitlyRequested({ ...base, sslmode: "disable" })).toBe(true); + }); + + it("is false for prefer/allow/disable, whose libpq fallback sslConfigsFor does not implement", () => { + // `prefer` and `allow` describe a TLS-then-plaintext (or reverse) fallback that + // `sslConfigsFor` never performs, so reading either as a demand would turn a + // plaintext-capable local target into a handshake failure (the CLI-2366 regression). + expect(tlsExplicitlyRequested({ ...base, sslmode: "prefer" })).toBe(false); + expect(tlsExplicitlyRequested({ ...base, sslmode: "allow" })).toBe(false); + expect(tlsExplicitlyRequested({ ...base, sslmode: "disable" })).toBe(false); + }); + + it("leaves a loopback DSN plaintext when PGSSLMODE filled sslmode the URL never set", () => { + // The real trigger: `sslmode` is also filled from `PGSSLMODE` and libpq service files, so an + // ambient `PGSSLMODE=prefer` must not make a bare loopback `--db-url` demand TLS. + const conn = parseConnectionString( + "postgresql://postgres:postgres@127.0.0.1:54322/postgres", + (name) => (name === "PGSSLMODE" ? "prefer" : undefined), + ); + expect(conn?.sslmode).toBe("prefer"); + expect(tlsExplicitlyRequested(conn!)).toBe(false); + }); + + it("still demands TLS when PGSSLMODE asks for verification", () => { + const conn = parseConnectionString( + "postgresql://postgres:postgres@127.0.0.1:54322/postgres", + (name) => (name === "PGSSLMODE" ? "verify-full" : undefined), + ); + expect(tlsExplicitlyRequested(conn!)).toBe(true); }); it("is true when a root cert (file path or inline PEM) is set", () => { From ab2a308cb1023189e5ecd7364f1ee8b6a518fc05 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Fri, 18 Sep 2026 11:07:05 +0100 Subject: [PATCH 2/2] fix(cli): keep sslmode=allow's TLS fallback for a local-classified target (CLI-2366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #6664: narrowing tlsExplicitlyRequested to drop ambient sslmode=allow/prefer as a TLS demand also silently dropped allow's own plaintext-then-TLS fallback for local-classified targets, since sslConfigsFor's isLocal short-circuit ran before the allow branch. allow's fallback is safe regardless of isLocal — its first attempt is already plaintext — so it now bypasses that exemption instead of being collapsed to plaintext-only. Also corrects test titles/comments that claimed sslConfigsFor never implements allow's fallback, and adds a missing parse-success assertion before a non-null dereference. Co-Authored-By: Claude Sonnet 5 --- .../db-connection.sql-pg.integration.test.ts | 4 ++-- .../src/command-internal/db-connection.sql-pg.layer.ts | 8 +++++--- .../command-internal/db-connection.sql-pg.unit.test.ts | 10 ++++++---- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts b/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts index c0b706e956..06dc217ffc 100644 --- a/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts +++ b/apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts @@ -910,8 +910,8 @@ describe("a local target's explicit TLS request (CLI-2366: honor --db-url's own ); it.live( - "stays plaintext for a local target with sslmode=allow (libpq's plaintext-then-TLS fallback " + - "is not implemented by sslConfigsFor)", + "connects plaintext for a local target with sslmode=allow, whose first fallback attempt " + + "is already plaintext", () => Effect.gen(function* () { const server = yield* Effect.promise(fakeStartupServer); diff --git a/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts b/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts index 95692df026..72de3279e2 100644 --- a/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts +++ b/apps/cli/src/command-internal/db-connection.sql-pg.layer.ts @@ -467,7 +467,9 @@ export function sslOptionFor( * DoH-resolved IP was substituted; `caCert` promotes `require` to `verify-ca` when set. * `isLocal` is the caller's TLS-exemption decision, not the raw target classification: a local * target that explicitly set `sslmode`/`sslrootcert` (see {@link tlsExplicitlyRequested}) is not - * exempt, so the caller passes `false` for it in that case. + * exempt, so the caller passes `false` for it in that case. `allow`'s fallback bypasses that + * exemption regardless: its first attempt is plaintext, identical to the exempt case, and the + * second only helps a local-classified target that actually requires TLS. */ export function sslConfigsFor( sslmode: string | undefined, @@ -477,14 +479,14 @@ export function sslConfigsFor( host?: string, clientCert?: ClientCert, ): Array { - if (isLocal) return [false]; // A unix-socket host always connects in plaintext, regardless of `sslmode`; never send an SSL // negotiation over the socket. Independent of `isLocal`, since a socket path isn't the local // services hostname. if (host !== undefined && isUnixSocketHost(host)) return [false]; - if (sslmode === "disable") return [false]; if (sslmode === "allow") return [false, sslOptionFor("require", false, servername, caCert, clientCert)]; + if (isLocal) return [false]; + if (sslmode === "disable") return [false]; // `require` plus a root cert behaves like `verify-ca`. const effectiveMode = sslmode === "require" && caCert !== undefined ? "verify-ca" : sslmode; if ( diff --git a/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts b/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts index 831de4a370..72e59e03d1 100644 --- a/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts +++ b/apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts @@ -219,6 +219,10 @@ describe("sslConfigsFor (pgconn fallback list)", () => { ]); }); + it("allow keeps its TLS fallback for a target classified local", () => { + expect(sslConfigsFor("allow", true, undefined)).toEqual([false, { rejectUnauthorized: false }]); + }); + it("prefer and unset are TLS only (ConnectByUrl strips the plaintext fallback)", () => { expect(sslConfigsFor("prefer", false, undefined)).toEqual([{ rejectUnauthorized: false }]); expect(sslConfigsFor(undefined, false, undefined)).toEqual([{ rejectUnauthorized: false }]); @@ -280,10 +284,7 @@ describe("tlsExplicitlyRequested (CLI-2366: --db-url TLS against a local target) expect(tlsExplicitlyRequested({ ...base, sslmode: "verify-full" })).toBe(true); }); - it("is false for prefer/allow/disable, whose libpq fallback sslConfigsFor does not implement", () => { - // `prefer` and `allow` describe a TLS-then-plaintext (or reverse) fallback that - // `sslConfigsFor` never performs, so reading either as a demand would turn a - // plaintext-capable local target into a handshake failure (the CLI-2366 regression). + it("is false for prefer, allow and disable, none of which name a verification intent", () => { expect(tlsExplicitlyRequested({ ...base, sslmode: "prefer" })).toBe(false); expect(tlsExplicitlyRequested({ ...base, sslmode: "allow" })).toBe(false); expect(tlsExplicitlyRequested({ ...base, sslmode: "disable" })).toBe(false); @@ -305,6 +306,7 @@ describe("tlsExplicitlyRequested (CLI-2366: --db-url TLS against a local target) "postgresql://postgres:postgres@127.0.0.1:54322/postgres", (name) => (name === "PGSSLMODE" ? "verify-full" : undefined), ); + expect(conn?.sslmode).toBe("verify-full"); expect(tlsExplicitlyRequested(conn!)).toBe(true); });