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..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 @@ -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( + "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); + 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..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 @@ -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 ); @@ -458,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, @@ -468,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 039c6338cf..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 @@ -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", () => { @@ -218,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 }]); @@ -273,10 +278,36 @@ 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 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); + }); + + 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(conn?.sslmode).toBe("verify-full"); + expect(tlsExplicitlyRequested(conn!)).toBe(true); }); it("is true when a root cert (file path or inline PEM) is set", () => {