From 4f015401bed781ed8afdffee6a413dc2e128a644 Mon Sep 17 00:00:00 2001 From: Shuu Date: Thu, 17 Sep 2026 20:16:28 +0900 Subject: [PATCH 1/2] fix(pg_connection): re-validate ConnConfig on every connect path --- async_postgres/pg_connection/dsn.nim | 6 +- async_postgres/pg_connection/lifecycle.nim | 18 ++- tests/api_surface.golden | 1 + tests/test_ssl.nim | 147 +++++++++++++++++++++ 4 files changed, 166 insertions(+), 6 deletions(-) diff --git a/async_postgres/pg_connection/dsn.nim b/async_postgres/pg_connection/dsn.nim index 67cbf42..3abe6d2 100644 --- a/async_postgres/pg_connection/dsn.nim +++ b/async_postgres/pg_connection/dsn.nim @@ -752,8 +752,10 @@ proc parseUriDsn*(dsn: string): ConnConfig = result.port = result.hosts[0].port validateClientCertConfig(result) -proc validateConnConfig(config: var ConnConfig) = - ## Mirror DSN guards for ``initConnConfig`` (DSN parsers validate inline). +proc validateConnConfig*(config: var ConnConfig) = + ## Mirror DSN guards for ``initConnConfig`` and the ``connect`` chokepoint + ## (DSN parsers validate inline; hand-built ``ConnConfig`` is re-checked at + ## connect time so numeric / hostaddr faults become ``PgConfigError``). ## Negative ``connectTimeout`` becomes ``ZeroDuration``. if config.connectTimeout < ZeroDuration: config.connectTimeout = ZeroDuration diff --git a/async_postgres/pg_connection/lifecycle.nim b/async_postgres/pg_connection/lifecycle.nim index e6e8afb..65efbe1 100644 --- a/async_postgres/pg_connection/lifecycle.nim +++ b/async_postgres/pg_connection/lifecycle.nim @@ -115,9 +115,15 @@ proc connectToHost*( ): Future[PgConnection] {.async.} = ## Connect to single host (dial ``hostaddr`` else ``host``; verify via ``host``). - # Re-check the mTLS pairing here as well: `connect` validates it in `wrapped`, - # but this proc is public and a direct caller would otherwise have the certs - # silently dropped by a successful sslAllow plaintext attempt. + # Local mutable copy: ``validateConnConfig`` may normalize ``connectTimeout``. + var config = config + + # Re-check numeric / hostaddr / mTLS pairing here as well: `connect` validates + # them in `wrapped`, but this proc is public and a direct caller would + # otherwise bypass the parsers (port wrap, keepalive ``cint`` RangeDefect, + # negative timeout footgun) or have certs silently dropped by a successful + # sslAllow plaintext attempt. + validateConnConfig(config) validateClientCertConfig(config) # Validate before the sslAllow branch rewrites sslMode to sslDisable, which @@ -581,6 +587,8 @@ proc orderedHosts*(config: ConnConfig): seq[HostEntry] = proc connect*(config: ConnConfig): Future[PgConnection] = ## Connect with multi-host failover, ``targetSessionAttrs``, per-host ``connectTimeout``. ## Per-host failures fold into one ``PgConnectionError``; a ``PgConfigError`` escapes the fold. + # Local mutable copy: ``validateConnConfig`` may normalize ``connectTimeout``. + var config = config proc perform(hosts: seq[HostEntry]): Future[PgConnection] {.async.} = # `hosts` is already ordered by the caller (shuffled under lbhRandom), so # both the preferStandby two-pass loop and the single-pass loop below share @@ -654,7 +662,9 @@ proc connect*(config: ConnConfig): Future[PgConnection] = proc wrapped(): Future[PgConnection] {.async.} = # ConnConfig may be built or mutated without passing through the parsers' - # validation — re-check here so every connect path rejects bad cert config. + # validation — re-check here so every connect path rejects bad numeric / + # hostaddr / cert config (``initConnConfig`` alone is not enough). + validateConnConfig(config) validateClientCertConfig(config) if config.channelBinding == cbRequire and config.sslMode == sslDisable: # Knowable before any dial; the per-host check in selectScramMechanism diff --git a/tests/api_surface.golden b/tests/api_surface.golden index 0a4f641..b582e9f 100644 --- a/tests/api_surface.golden +++ b/tests/api_surface.golden @@ -511,6 +511,7 @@ async_postgres/pg_connection/dsn.nim parseSslMode async_postgres/pg_connection/dsn.nim parseSslNegotiation async_postgres/pg_connection/dsn.nim parseTargetSessionAttrs async_postgres/pg_connection/dsn.nim parseUriDsn +async_postgres/pg_connection/dsn.nim validateConnConfig async_postgres/pg_connection/lifecycle.nim close async_postgres/pg_connection/lifecycle.nim closeImpl async_postgres/pg_connection/lifecycle.nim connect diff --git a/tests/test_ssl.nim b/tests/test_ssl.nim index f67af46..a6d3f55 100644 --- a/tests/test_ssl.nim +++ b/tests/test_ssl.nim @@ -814,6 +814,153 @@ suite "Client certificate config validation": check raised check configFault +suite "connect hand-built ConnConfig numeric validation": + # #631 gap: `validateConnConfig` used to run only in `initConnConfig`. + # Hand-built `ConnConfig` must hit the same guards at the `connect` chokepoint + # (no mock server — failure is client-side before dial). + test "out-of-range port is a config fault, not a connection failure": + var raised = false + var configFault = false + + proc testBody() {.async.} = + let config = ConnConfig( + host: "127.0.0.1", + port: 99999, + user: "test", + database: "test", + sslMode: sslDisable, + ) + try: + let conn = await connect(config) + await conn.close() + except PgError as e: + raised = true + configFault = e of PgConfigError + + waitFor testBody() + check raised + check configFault + + test "port 0 is a config fault": + var raised = false + var configFault = false + + proc testBody() {.async.} = + let config = ConnConfig( + host: "127.0.0.1", port: 0, user: "test", database: "test", sslMode: sslDisable + ) + try: + let conn = await connect(config) + await conn.close() + except PgError as e: + raised = true + configFault = e of PgConfigError + + waitFor testBody() + check raised + check configFault + + test "slash hostaddr is a config fault": + var raised = false + var configFault = false + + proc testBody() {.async.} = + let config = ConnConfig( + host: "db", + hostaddr: "/tmp", + port: 5432, + user: "test", + database: "test", + sslMode: sslDisable, + ) + try: + let conn = await connect(config) + await conn.close() + except PgError as e: + raised = true + configFault = e of PgConfigError + + waitFor testBody() + check raised + check configFault + + test "negative keepAliveIdle is a config fault": + var raised = false + var configFault = false + + proc testBody() {.async.} = + let config = ConnConfig( + host: "127.0.0.1", + port: 1, + user: "test", + database: "test", + sslMode: sslDisable, + keepAliveIdle: -1, + ) + try: + let conn = await connect(config) + await conn.close() + except PgError as e: + raised = true + configFault = e of PgConfigError + + waitFor testBody() + check raised + check configFault + + test "keepAliveIdle exceeding cint is a config fault": + when sizeof(cint) < sizeof(int): + var raised = false + var configFault = false + + proc testBody() {.async.} = + let config = ConnConfig( + host: "127.0.0.1", + port: 1, + user: "test", + database: "test", + sslMode: sslDisable, + keepAliveIdle: int(high(cint)) + 1, + ) + try: + let conn = await connect(config) + await conn.close() + except PgError as e: + raised = true + configFault = e of PgConfigError + + waitFor testBody() + check raised + check configFault + + test "negative connectTimeout normalizes and does not raise PgConfigError": + # Normalization must happen before dial; use an immediately-refused port so + # the attempt finishes without hanging (ZeroDuration = no timeout). + var configFault = false + var connected = false + + proc testBody() {.async.} = + let config = ConnConfig( + host: "127.0.0.1", + port: 1, + user: "test", + database: "test", + sslMode: sslDisable, + connectTimeout: seconds(-5), + ) + try: + let conn = await connect(config) + connected = true + await conn.close() + except PgConfigError: + configFault = true + except PgError: + discard + + waitFor testBody() + check not configFault + check not connected + suite "SSL negotiation - sslAllow": test "sslAllow connects without SSL when server accepts plaintext": var connState: PgConnState From 56dfccac34ce3bf6a47ab5529d7e8df8d8779e92 Mon Sep 17 00:00:00 2001 From: Shuu Date: Fri, 18 Sep 2026 14:36:32 +0900 Subject: [PATCH 2/2] fix --- async_postgres/pg_connection/dsn.nim | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/async_postgres/pg_connection/dsn.nim b/async_postgres/pg_connection/dsn.nim index 3abe6d2..51dfac7 100644 --- a/async_postgres/pg_connection/dsn.nim +++ b/async_postgres/pg_connection/dsn.nim @@ -771,11 +771,15 @@ proc validateConnConfig*(config: var ConnConfig) = "Invalid hostaddr: must be a numeric IP address, not a Unix socket path (use host for Unix sockets)", ) - checkPort(config.port) - checkHostaddr(config.hostaddr) - for entry in config.hosts: - checkPort(entry.port) - checkHostaddr(entry.hostaddr) + # Once `hosts` is populated the scalar host/port pair is an unused back-compat + # mirror, left zeroed by hand-built configs. + if config.hosts.len > 0: + for entry in config.hosts: + checkPort(entry.port) + checkHostaddr(entry.hostaddr) + else: + checkPort(config.port) + checkHostaddr(config.hostaddr) if config.keepAliveIdle < 0: raise newException(PgConfigError, "keepalives_idle must be non-negative")