From ac9992391ec5615cacd50218c83267d938ae0f64 Mon Sep 17 00:00:00 2001 From: danielshih Date: Wed, 22 Jul 2026 10:44:44 +0000 Subject: [PATCH 1/2] fix(sql_builder): harden quote_literal against standard_conforming_strings=off Consolidate quote_ident/quote_literal onto a shared quote_internal core that mirrors libpq PQescapeInternal: literals containing a backslash now use the escape-string form ( E'...') with backslashes doubled, so the result is safe regardless of the server's standard_conforming_strings setting. Backslash-free literals and all identifiers are byte-for-byte unchanged; null-byte rejection is preserved. Also update the two backend test-only assertions that pinned the pre-hardening quote_literal output for a backslash input. ci: run quote_literal_scs_off in the integration job Wire the new standard_conforming_strings=off roundtrip test into the CI integration run block (regular connection via DATABASE_URL_REGULAR; creates no replication slot, so no slot-cleanup step follows it). --- .github/workflows/ci-checks.yml | 3 + Cargo.toml | 4 + integration-tests/quote_literal_scs_off.rs | 84 ++++++++++ src/connection/libpq.rs | 13 +- src/connection/native/connection.rs | 10 +- src/sql_builder.rs | 174 +++++++++++++++++---- 6 files changed, 250 insertions(+), 38 deletions(-) create mode 100644 integration-tests/quote_literal_scs_off.rs diff --git a/.github/workflows/ci-checks.yml b/.github/workflows/ci-checks.yml index 85b9662..c8faaee 100644 --- a/.github/workflows/ci-checks.yml +++ b/.github/workflows/ci-checks.yml @@ -336,6 +336,9 @@ jobs: # Server-version preflight: end-to-end core CDC path (create slot -> stream -> decode INSERT) plus PG17+ gated slot ops (FAILOVER create + ALTER) that must pass the client-side version gate and execute; the gated portion skips gracefully on PG<17. cargo test --test version_preflight -- --ignored --nocapture --test-threads=1 + # quote_literal escaping: proves quote_literal stays injection-safe under standard_conforming_strings off and on (a backslash+quote payload roundtrips byte-for-byte); uses the regular connection and creates no replication slot. + cargo test --test quote_literal_scs_off -- --ignored --nocapture --test-threads=1 + integration-ssl: name: SSL Integration Tests (PG ${{ matrix.pg_version }}) runs-on: ubuntu-latest diff --git a/Cargo.toml b/Cargo.toml index d70f8b7..6e66496 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -161,6 +161,10 @@ path = "integration-tests/base_backup_options.rs" name = "version_preflight" path = "integration-tests/version_preflight.rs" +[[test]] +name = "quote_literal_scs_off" +path = "integration-tests/quote_literal_scs_off.rs" + [[bench]] name = "wal_pipeline" harness = false diff --git a/integration-tests/quote_literal_scs_off.rs b/integration-tests/quote_literal_scs_off.rs new file mode 100644 index 0000000..192524c --- /dev/null +++ b/integration-tests/quote_literal_scs_off.rs @@ -0,0 +1,84 @@ +#![cfg(any(feature = "libpq", feature = "rustls-tls"))] + +//! Live proof that `quote_literal` is injection-safe under either value of the +//! server's `standard_conforming_strings` setting. +//! +//! With `standard_conforming_strings = off` a backslash is an escape character +//! inside a plain `'…'` string, so quote-doubling alone would let a crafted +//! value terminate the literal early. `quote_literal` defends against this by +//! switching to the escape-string form ` E'…'` (backslashes doubled) whenever +//! the input contains a backslash. This test feeds a payload combining a +//! backslash, a single quote, and trailing SQL through `quote_literal` into a +//! `SELECT`, then asserts the value roundtrips byte-for-byte — proving the +//! string never terminated early and the trailing SQL never executed. +//! +//! ## Prerequisites +//! +//! - PostgreSQL (any supported version) +//! - `DATABASE_URL_REGULAR` — a regular (non-replication) connection, e.g. +//! `postgresql://postgres:postgres@localhost:5432/test_walstream` +//! (falls back to `DATABASE_URL` with `replication=database` stripped) +//! +//! ## Running Locally +//! +//! ```bash +//! export DATABASE_URL_REGULAR="postgresql://postgres:postgres@localhost:5432/test_walstream" +//! cargo test --test quote_literal_scs_off -- --ignored --nocapture +//! ``` + +use pg_walstream::{quote_literal, PgReplicationConnection}; + +fn regular_conn_string() -> String { + std::env::var("DATABASE_URL_REGULAR").unwrap_or_else(|_| { + std::env::var("DATABASE_URL") + .unwrap_or_else(|_| { + "postgresql://postgres:postgres@localhost:5432/test_walstream".to_string() + }) + .replace("?replication=database", "") + .replace("&replication=database", "") + }) +} + +fn init_tracing() { + let _ = tracing_subscriber::fmt() + .with_max_level(tracing::Level::INFO) + .try_init(); +} + +/// `SELECT ` and return the single scalar the server +/// echoes back. +fn roundtrip(conn: &mut PgReplicationConnection, payload: &str) -> String { + let sql = format!( + "SELECT {} AS v", + quote_literal(payload).expect("no null bytes") + ); + conn.exec(&sql) + .expect("SELECT of quoted literal must succeed") + .get_value(0, 0) + .expect("one row, one column") +} + +#[test] +#[ignore = "requires live PostgreSQL"] +fn quote_literal_survives_standard_conforming_strings_off() { + init_tracing(); + + let mut conn = + PgReplicationConnection::connect(®ular_conn_string()).expect("regular connection"); + + // Combines both literal-breakout vectors: a backslash (the scs=off escape + // character) and a single quote, followed by SQL that must NOT run. + let payload = r"weird \ value ' with; DROP TABLE x; --"; + + for scs in ["off", "on"] { + conn.exec(&format!("SET standard_conforming_strings = {scs}")) + .unwrap_or_else(|e| panic!("SET standard_conforming_strings = {scs}: {e}")); + + let got = roundtrip(&mut conn, payload); + assert_eq!( + got, payload, + "payload must roundtrip intact under standard_conforming_strings = {scs}; \ + early string termination would change, truncate, or drop it" + ); + } +} diff --git a/src/connection/libpq.rs b/src/connection/libpq.rs index d9a1e10..e11b254 100644 --- a/src/connection/libpq.rs +++ b/src/connection/libpq.rs @@ -1199,11 +1199,14 @@ mod tests { } #[test] - fn test_sanitize_backslash_and_quote() { - // Backslashes should not be specially treated, only single quotes - let input = "test\\'value"; - let sanitized = sanitize_sql_string_value(input); - assert_eq!(sanitized, "test\\''value"); + fn test_quote_backslash_and_quote() { + // A backslash switches quote_literal to the escape-string form ` E'…'` + // (safe under standard_conforming_strings off or on); both the backslash + // and the single quote are doubled. + assert_eq!( + quote_sql_string_value("test\\'value"), + r#" E'test\\''value'"# + ); } #[test] diff --git a/src/connection/native/connection.rs b/src/connection/native/connection.rs index deb9f13..b473672 100644 --- a/src/connection/native/connection.rs +++ b/src/connection/native/connection.rs @@ -1273,8 +1273,14 @@ mod tests { } #[test] - fn test_sanitize_backslash_and_quote() { - assert_eq!(sanitize_sql_string_value("test\\'value"), "test\\''value"); + fn test_quote_backslash_and_quote() { + // A backslash switches quote_literal to the escape-string form ` E'…'` + // (safe under standard_conforming_strings off or on); both the backslash + // and the single quote are doubled. + assert_eq!( + quote_sql_string_value("test\\'value"), + r#" E'test\\''value'"# + ); } #[test] diff --git a/src/sql_builder.rs b/src/sql_builder.rs index ad18d69..0e620a6 100644 --- a/src/sql_builder.rs +++ b/src/sql_builder.rs @@ -12,6 +12,89 @@ use crate::types::{ INVALID_XLOG_REC_PTR, }; +/// Identifier (double quotes) vs string literal (single quotes). Selects the quote character, backslash handling, and null-byte error wording for [`quote_internal`]. +#[derive(Clone, Copy)] +enum QuoteStyle { + Identifier, + Literal, +} + +impl QuoteStyle { + /// The wrapping/escaping quote character for this style. + #[inline] + fn quote_char(self) -> char { + match self { + QuoteStyle::Identifier => '"', + QuoteStyle::Literal => '\'', + } + } + + /// The error message used when the input contains a null byte. + #[inline] + fn null_byte_error(self) -> &'static str { + match self { + QuoteStyle::Identifier => "SQL identifier must not contain null bytes", + QuoteStyle::Literal => "SQL literal must not contain null bytes", + } + } +} + +/// Shared quoting core for [`quote_ident`] and [`quote_literal`]. +/// +/// Wraps `value` in the style's quote character, doubling any embedded quote character. For string literals only, a backslash switches the result to the escape-string form ` E'…'` (a leading space, then `E'`) with backslashes doubled, so the result is safe regardless of the server's `standard_conforming_strings` setting. Identifiers never interpret backslashes, so they never need this. +/// +/// Rejects null bytes, which are invalid in both identifiers and literals and could otherwise cause truncation-based injection through the C-string wire protocol. +/// +/// Mirrors libpq's `PQescapeInternal` (which rust-postgres's `escape_internal` ports), with the added null-byte rejection. +fn quote_internal(value: &str, style: QuoteStyle) -> Result { + if value.contains('\0') { + return Err(ReplicationError::config( + style.null_byte_error().to_string(), + )); + } + + let quote_char = style.quote_char(); + let escape_backslashes = matches!(style, QuoteStyle::Literal); + + let mut num_quotes = 0usize; + let mut num_backslashes = 0usize; + for ch in value.chars() { + if ch == quote_char { + num_quotes += 1; + } else if escape_backslashes && ch == '\\' { + num_backslashes += 1; + } + } + + // Backslash doubling only happens for literals, so a non-zero count implies literal mode — which is exactly when the escape-string syntax is required. + let use_escape_string = num_backslashes > 0; + let mut out = String::with_capacity( + value.len() + num_quotes + num_backslashes + if use_escape_string { 4 } else { 2 }, + ); + + // ` E'…'` (with a leading space so the literal cannot fuse onto a preceding identifier) is emitted only when backslashes are being doubled. + if use_escape_string { + out.push(' '); + out.push('E'); + } + out.push(quote_char); + + if num_quotes == 0 && num_backslashes == 0 { + // Fast path: nothing to escape. + out.push_str(value); + } else { + for ch in value.chars() { + if ch == quote_char || (escape_backslashes && ch == '\\') { + out.push(ch); + } + out.push(ch); + } + } + + out.push(quote_char); + Ok(out) +} + /// Quote a PostgreSQL identifier by wrapping in double quotes and escaping /// internal double quotes (doubling them). /// @@ -32,26 +115,16 @@ use crate::types::{ /// ``` #[inline] pub fn quote_ident(name: &str) -> Result { - if name.contains('\0') { - return Err(ReplicationError::config( - "SQL identifier must not contain null bytes".to_string(), - )); - } - let mut out = String::with_capacity(name.len() + 2); - out.push('"'); - for ch in name.chars() { - if ch == '"' { - out.push('"'); - } - out.push(ch); - } - out.push('"'); - Ok(out) + quote_internal(name, QuoteStyle::Identifier) } /// Quote a PostgreSQL string literal by wrapping in single quotes and escaping /// internal single quotes (doubling them). /// +/// If `value` contains a backslash, the escape-string form ` E'…'` is emitted +/// with backslashes doubled, so the result is safe under either value of the +/// server's `standard_conforming_strings` setting. +/// /// # Errors /// /// Returns an error if `value` contains a null byte (`\0`), which is invalid in @@ -65,25 +138,14 @@ pub fn quote_ident(name: &str) -> Result { /// /// assert_eq!(quote_literal("hello").unwrap(), "'hello'"); /// assert_eq!(quote_literal("it's").unwrap(), "'it''s'"); +/// // A backslash switches to the escape-string form, safe under any +/// // `standard_conforming_strings` setting. +/// assert_eq!(quote_literal(r"a\b").unwrap(), r" E'a\\b'"); /// assert!(quote_literal("bad\0value").is_err()); /// ``` #[inline] pub fn quote_literal(value: &str) -> Result { - if value.contains('\0') { - return Err(ReplicationError::config( - "SQL literal must not contain null bytes".to_string(), - )); - } - let mut out = String::with_capacity(value.len() + 2); - out.push('\''); - for ch in value.chars() { - if ch == '\'' { - out.push('\''); - } - out.push(ch); - } - out.push('\''); - Ok(out) + quote_internal(value, QuoteStyle::Literal) } // ═══════════════════════════════════════════════════════════════════════════ @@ -972,7 +1034,38 @@ mod tests { #[test] fn quote_literal_backslash_and_quote() { - assert_eq!(quote_literal("test\\'value").unwrap(), "'test\\''value'"); + // A backslash switches to the escape-string form ` E'…'`, with both the + // backslash and the single quote doubled — safe under standard_conforming_strings + // off or on. + assert_eq!( + quote_literal("test\\'value").unwrap(), + r#" E'test\\''value'"# + ); + } + + #[test] + fn quote_literal_backslash_only() { + // A backslash with no quote still uses the escape-string form. + assert_eq!(quote_literal("a\\b").unwrap(), r#" E'a\\b'"#); + } + + #[test] + fn quote_literal_backslash_injection_neutralized() { + // The classic standard_conforming_strings=off breakout payload becomes inert: + // the backslash and quote are both doubled inside an E'' string, so nothing + // after the intended literal can execute. + assert_eq!( + quote_literal("\\'; DROP TABLE t; --").unwrap(), + r#" E'\\''; DROP TABLE t; --'"# + ); + } + + #[test] + fn quote_literal_no_backslash_is_legacy_form() { + // Without a backslash the output is byte-identical to the plain '…' form + // (no E prefix, no leading space) — no existing builder output shifts. + assert_eq!(quote_literal("it's").unwrap(), "'it''s'"); + assert_eq!(quote_literal("plain").unwrap(), "'plain'"); } #[test] @@ -1907,6 +2000,25 @@ mod tests { ); } + #[test] + fn create_subscription_backslash_in_connection_uses_escape_string() { + // A backslash in a free-form literal propagates the escape-string form + // through a real builder. The doubled space (template space + the escape's + // leading space) is valid SQL. + let opts = CreateSubscriptionOptions { + subscription_name: "sub", + connection_string: r"host=db\x", + publication: "pub", + slot_name: "slot", + ..Default::default() + }; + let sql = build_create_subscription_sql(&opts).unwrap(); + assert!( + sql.contains(r"CONNECTION E'host=db\\x'"), + "connection literal not escaped as expected: {sql}" + ); + } + // ── build_disable_subscription_sql ─────────────────────────────────── #[test] From 1edc543d47cfcc3f151ea32e7442987696f98e01 Mon Sep 17 00:00:00 2001 From: danielshih Date: Thu, 23 Jul 2026 02:58:52 +0000 Subject: [PATCH 2/2] enhance documentation for quote_internal and add tests for quote_ident backslash handling --- integration-tests/quote_literal_scs_off.rs | 5 ++++- src/connection/libpq.rs | 16 +++++++++++++++ src/sql_builder.rs | 24 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/integration-tests/quote_literal_scs_off.rs b/integration-tests/quote_literal_scs_off.rs index 192524c..93509eb 100644 --- a/integration-tests/quote_literal_scs_off.rs +++ b/integration-tests/quote_literal_scs_off.rs @@ -34,8 +34,11 @@ fn regular_conn_string() -> String { .unwrap_or_else(|_| { "postgresql://postgres:postgres@localhost:5432/test_walstream".to_string() }) - .replace("?replication=database", "") + // Drop the replication flag: this test needs a *regular* (non-replication) connection so `SET` and a plain `SELECT` work. + // Handle it as the first, a later, or the sole query parameter without leaving a dangling `?`/`&`. + .replace("?replication=database&", "?") .replace("&replication=database", "") + .replace("?replication=database", "") }) } diff --git a/src/connection/libpq.rs b/src/connection/libpq.rs index e11b254..0d31518 100644 --- a/src/connection/libpq.rs +++ b/src/connection/libpq.rs @@ -217,6 +217,22 @@ impl PgReplicationConnection { } } + // Force UTF-8 client_encoding so quote_literal/quote_ident's ASCII-only escaping is multibyte-safe regardless of the server/locale default , parity with the native backend, which sends client_encoding=UTF8 in its startup packet. Under an ASCII-unsafe encoding (SJIS/GBK/BIG5) a raw lead byte could otherwise fuse with an escaping quote and reopen a literal breakout. + if unsafe { PQsetClientEncoding(conn, c"UTF8".as_ptr()) } != 0 { + let error_msg = unsafe { + let error_ptr = PQerrorMessage(conn); + if error_ptr.is_null() { + "Unknown error".to_string() + } else { + CStr::from_ptr(error_ptr).to_string_lossy().into_owned() + } + }; + unsafe { PQfinish(conn) }; + return Err(ReplicationError::permanent_connection(format!( + "Failed to set client_encoding=UTF8: {error_msg}" + ))); + } + // Check server version - logical replication requires PostgreSQL 14+ let server_version = unsafe { PQserverVersion(conn) }; if server_version < 140000 { diff --git a/src/sql_builder.rs b/src/sql_builder.rs index 0e620a6..42a56c5 100644 --- a/src/sql_builder.rs +++ b/src/sql_builder.rs @@ -46,6 +46,8 @@ impl QuoteStyle { /// Rejects null bytes, which are invalid in both identifiers and literals and could otherwise cause truncation-based injection through the C-string wire protocol. /// /// Mirrors libpq's `PQescapeInternal` (which rust-postgres's `escape_internal` ports), with the added null-byte rejection. +/// +/// Assumes the connection uses a UTF-8 (ASCII-safe) `client_encoding`, which both backends force at connect time (the native backend sends `client_encoding=UTF8` in its startup packet; the libpq backend calls `PQsetClientEncoding(conn, "UTF8")` after connecting). Because `'` and `\` are ASCII and a Rust `&str` is UTF-8, scanning by `char` can never mistake a multibyte continuation byte for a quote or backslash — so libpq's `PQescapeInternal` multibyte handling (needed only for ASCII-unsafe server encodings such as SJIS/GBK) is safely omitted here. fn quote_internal(value: &str, style: QuoteStyle) -> Result { if value.contains('\0') { return Err(ReplicationError::config( @@ -969,6 +971,17 @@ mod tests { ); } + #[test] + fn quote_ident_backslash_stays_plain() { + // Identifiers never interpret backslashes: a backslash passes through verbatim inside "…" — no doubling and, crucially, no ` E'…'` escape-string form (that is literal-only). Locks the "backslash handling is literal-only" invariant so a future QuoteStyle refactor cannot silently regress quote_ident. + assert_eq!(quote_ident(r"a\b").unwrap(), r#""a\b""#); + // Even a backslash + single-quote + trailing SQL stays a plain double-quoted identifier (only " would be doubled; there is none). + assert_eq!( + quote_ident(r"a\'; DROP TABLE t; --").unwrap(), + r#""a\'; DROP TABLE t; --""# + ); + } + #[test] fn quote_ident_unicode() { assert_eq!(quote_ident("テスト").unwrap(), r#""テスト""#); @@ -1060,6 +1073,17 @@ mod tests { ); } + #[test] + fn quote_literal_backslash_ordering_regression() { + // Placement/counting guards (all safe today — these lock the behavior): + // consecutive backslashes are doubled individually, + assert_eq!(quote_literal("\\\\").unwrap(), r#" E'\\\\'"#); + // a quote *before* a backslash keeps both doubled, in order, + assert_eq!(quote_literal("'\\").unwrap(), r#" E'''\\'"#); + // and a lone trailing backslash is doubled so it cannot fuse the closing quote. + assert_eq!(quote_literal("x\\").unwrap(), r#" E'x\\'"#); + } + #[test] fn quote_literal_no_backslash_is_legacy_form() { // Without a backslash the output is byte-identical to the plain '…' form