From f87945e56b6339fb83233762bf606d741b95bd2c Mon Sep 17 00:00:00 2001 From: nettrash Date: Sun, 6 Sep 2026 09:59:19 +0200 Subject: [PATCH] #244 --- .github/workflows/rust.yml | 32 ++++++ CHANGELOG | 41 ++++++-- app/src/config/dump_config.rs | 72 +++++++++---- app/src/config/tests/dump_config.rs | 158 +++++++++++++++++++++++++--- app/src/dump/core.rs | 26 ++++- app/tests/live_database.rs | 62 +++++++++++ 6 files changed, 346 insertions(+), 45 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 6991e4c..4a2e565 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -166,6 +166,38 @@ jobs: USE_SINGLE_TRANSACTION=false EOF + - name: Assert a password of URL-reserved characters connects + env: + PGPASSWORD: postgres + run: | + # The connection details used to be interpolated into a + # postgres://user:password@host:port/db string and parsed back, so + # every RFC 3986 reserved character in a password was read as syntax + # (issue #244). `#`, `/` and `?` failed as "invalid port number" + # before any socket was opened, and `%41` was silently decoded, so + # `pass%41word` authenticated as `passAword`. + # + # Nothing about a schema diff exercises this, so it gets its own step: + # create a role whose password holds every character that misbehaved, + # then dump as that role. + password='p#ss/w?rd%41@x%' + psql -h 127.0.0.1 -U postgres -d a -v ON_ERROR_STOP=1 -c \ + "drop role if exists pgc_issue244_role;" + psql -h 127.0.0.1 -U postgres -d a -v ON_ERROR_STOP=1 -c \ + "create role pgc_issue244_role login password '${password}';" + if ! ./app/target/release/pgc --command dump \ + --server 127.0.0.1 --port 5432 --database a --scheme public \ + --output /tmp/issue244.dump \ + --user pgc_issue244_role --password "${password}" > /tmp/issue244.log 2>&1; then + echo "ERROR: a password of URL-reserved characters was refused on PG ${{ matrix.pg_version }}" + cat /tmp/issue244.log + psql -h 127.0.0.1 -U postgres -d a -c "drop role if exists pgc_issue244_role;" || true + exit 1 + fi + psql -h 127.0.0.1 -U postgres -d a -v ON_ERROR_STOP=1 -c \ + "drop role if exists pgc_issue244_role;" + echo "OK: reserved characters in a password survive on PG ${{ matrix.pg_version }}" + - name: Run pgc compare (round 1) — generate diff run: | ./app/target/release/pgc --config /tmp/pgc.conf diff --git a/CHANGELOG b/CHANGELOG index 7c0dc2b..bf018ae 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,19 +1,44 @@ -2026-09-05 v1.0.27 +2026-09-06 v1.0.27 - Fixes four migrations that did not do what they said: + Fixes four migrations that did not do what they said - the routine ordering bug behind issue #240, with one opt-in flag for it, the schema-moved extension of issue #241, the materialized view rebuilt on an unrelated column change of issue #242, and the identity column dropped in the wrong order of issue - #243. Beyond those this release reorganises the test - suite, adds a library target and integration tests, - and documents the crate. The dump gains one optional - field, which older dumps may omit, and the generated - SQL is unchanged for any schema none of the four bugs - touched. + #243 - and one class of password that could not be + used to connect at all (issue #244). Beyond those + this release reorganises the test suite, adds a + library target and integration tests, and documents + the crate. The dump gains one optional field, which + older dumps may omit, and the generated SQL is + unchanged for any schema none of the four migration + bugs touched. Bug fixes: + - Resolved issue #244 (a password containing + characters that are reserved in a URL could not be + used, and one kind of password was silently + changed). Connection details were interpolated + into a postgres://user:password@host:port/database + string and handed back to sqlx to parse, so every + reserved character in a credential was read as URL + syntax rather than as data. A password containing + #, / or ? ended the authority section early and + the parser eventually read the port out of + something that was not a number, reporting invalid + port number - naming the one part of the + connection that was never wrong, before any socket + was opened. Worse and quieter, a password + containing a valid percent-escape was decoded: + pass%41word was sent as passAword, so the wrong + password was accepted and the real one could not + be used at all. The connection now goes through + PgConnectOptions with the fields set individually, + which sqlx passes to the wire protocol unaltered, + and a port that is not a number is reported as a + port instead of as a failed connection. The masked + connection string in log output is unchanged. - Resolved issue #243 (a column that stopped being an identity column produced a migration PostgreSQL refused to run). One column's ALTER statements diff --git a/app/src/config/dump_config.rs b/app/src/config/dump_config.rs index 11f495e..ccb9359 100644 --- a/app/src/config/dump_config.rs +++ b/app/src/config/dump_config.rs @@ -4,6 +4,7 @@ //! [`Dump`](crate::dump::core::Dump): the password must never reach a dump file. use serde::{Deserialize, Serialize}; +use sqlx::postgres::{PgConnectOptions, PgSslMode}; /// This is a database dump configuration structure. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -27,7 +28,33 @@ pub struct DumpConfig { } impl DumpConfig { - /// Returns the connection string for the database. + /// Connection options for the database, ready to hand to + /// `PgPoolOptions::connect_with`. + /// + /// Deliberately **not** a URL. The credentials go into the fields as they + /// were typed, and sqlx passes them to the PostgreSQL wire protocol + /// unaltered; nothing ever parses them as URL syntax. Building a + /// `postgres://user:password@host:port/db` string and letting sqlx parse it + /// back means every RFC 3986 reserved character in a password is read as + /// syntax rather than data, and that failed three different ways + /// (issue #244, all verified live): + /// + /// - `#`, `/` and `?` end the authority section early, so the parser + /// eventually reads the port out of something that is not a number and + /// reports `invalid port number` — naming the one part of the URL that + /// was never wrong; + /// - `%` followed by two hex digits is silently percent-**decoded**, so + /// `pass%41word` authenticates as `passAword`. A password that really + /// contains `%41` cannot be used at all, and the failure blames the + /// credentials rather than pgc; + /// - `@` and a bare `%` happened to survive, which is worse than if they + /// had not: the class of bug looked narrower than it was. + /// + /// Built on `new_without_pgpass`, not `new`, because that is what sqlx's + /// own URL parser uses (`PgConnectOptions::parse_from_url`). Starting from + /// `new` instead would newly consult `~/.pgpass` and could silently + /// substitute a password the configuration did not ask for. + /// /// # Examples /// /// ``` @@ -37,32 +64,39 @@ impl DumpConfig { /// host: "db.example".to_string(), /// port: "5432".to_string(), /// user: "alice".to_string(), - /// password: "s3cret".to_string(), + /// // Reserved in a URL, ordinary in a password. + /// password: "p#ss/w?rd%41".to_string(), /// database: "shop".to_string(), /// scheme: "public".to_string(), /// ssl: true, /// file: "dump.from".to_string(), /// }; /// - /// assert_eq!( - /// config.get_connection_string(), - /// "postgres://alice:s3cret@db.example:5432/shop?sslmode=require" - /// ); + /// let options = config.get_connect_options().unwrap(); + /// assert_eq!(options.get_host(), "db.example"); + /// assert_eq!(options.get_port(), 5432); /// ``` /// - /// This string contains the password — use - /// [`get_masked_connection_string`](Self::get_masked_connection_string) - /// for anything that gets printed or logged. - pub fn get_connection_string(&self) -> String { - format!( - "postgres://{}:{}@{}:{}/{}?sslmode={}", - self.user, - self.password, - self.host, - self.port, - self.database, - if self.ssl { "require" } else { "disable" } - ) + /// # Errors + /// + /// [`sqlx::Error::Configuration`] when [`port`](Self::port) is not a `u16`. + /// The field is a `String` because it arrives from a config file or a CLI + /// flag, so the parse has to happen somewhere; doing it here means a bad + /// port is reported as a bad port. + pub fn get_connect_options(&self) -> Result { + let port: u16 = self.port.trim().parse().map_err(sqlx::Error::config)?; + + Ok(PgConnectOptions::new_without_pgpass() + .host(&self.host) + .port(port) + .username(&self.user) + .password(&self.password) + .database(&self.database) + .ssl_mode(if self.ssl { + PgSslMode::Require + } else { + PgSslMode::Disable + })) } /// Returns a masked connection string for the database. diff --git a/app/src/config/tests/dump_config.rs b/app/src/config/tests/dump_config.rs index b2e7e00..eeed585 100644 --- a/app/src/config/tests/dump_config.rs +++ b/app/src/config/tests/dump_config.rs @@ -1,4 +1,5 @@ use super::*; +use sqlx::postgres::{PgConnectOptions, PgSslMode}; #[test] fn test_dump_config_new() { @@ -38,7 +39,7 @@ fn test_dump_config_default() { } #[test] -fn test_get_connection_string_with_ssl_disabled() { +fn test_get_connect_options_with_ssl_disabled() { let config = DumpConfig { host: "localhost".to_string(), port: "5432".to_string(), @@ -50,13 +51,16 @@ fn test_get_connection_string_with_ssl_disabled() { file: "test.dump".to_string(), }; - let connection_string = config.get_connection_string(); - let expected = "postgres://testuser:testpass@localhost:5432/testdb?sslmode=disable"; - assert_eq!(connection_string, expected); + let options = config.get_connect_options().expect("valid configuration"); + assert_eq!(options.get_host(), "localhost"); + assert_eq!(options.get_port(), 5432); + assert_eq!(options.get_username(), "testuser"); + assert_eq!(options.get_database(), Some("testdb")); + assert!(matches!(options.get_ssl_mode(), PgSslMode::Disable)); } #[test] -fn test_get_connection_string_with_ssl_enabled() { +fn test_get_connect_options_with_ssl_enabled() { let config = DumpConfig { host: "remotehost".to_string(), port: "5433".to_string(), @@ -68,9 +72,12 @@ fn test_get_connection_string_with_ssl_enabled() { file: "prod.dump".to_string(), }; - let connection_string = config.get_connection_string(); - let expected = "postgres://produser:securepass@remotehost:5433/proddb?sslmode=require"; - assert_eq!(connection_string, expected); + let options = config.get_connect_options().expect("valid configuration"); + assert_eq!(options.get_host(), "remotehost"); + assert_eq!(options.get_port(), 5433); + assert_eq!(options.get_username(), "produser"); + assert_eq!(options.get_database(), Some("proddb")); + assert!(matches!(options.get_ssl_mode(), PgSslMode::Require)); } #[test] @@ -109,8 +116,12 @@ fn test_get_masked_connection_string_with_ssl_enabled() { assert_eq!(masked_string, expected); } +/// The test this replaces asserted only that `format!` had produced the +/// string it was told to produce, and never parsed the result — so it passed +/// while every connection using that string failed (issue #244). What matters +/// is that the credentials survive, so that is what is asserted here. #[test] -fn test_connection_string_with_special_characters() { +fn test_connect_options_with_special_characters() { let config = DumpConfig { host: "test-host.example.com".to_string(), port: "5432".to_string(), @@ -122,11 +133,13 @@ fn test_connection_string_with_special_characters() { file: "special.dump".to_string(), }; - let connection_string = config.get_connection_string(); - let expected = - "postgres://user@domain:pass!@#$%@test-host.example.com:5432/test_db-name?sslmode=disable"; - assert_eq!(connection_string, expected); + let options = config.get_connect_options().expect("valid configuration"); + assert_eq!(options.get_host(), "test-host.example.com"); + assert_eq!(options.get_port(), 5432); + assert_eq!(options.get_username(), "user@domain"); + assert_eq!(options.get_database(), Some("test_db-name")); + // The masked form is log output only and stays a plain string. let masked_string = config.get_masked_connection_string(); let expected_masked = "postgres://*:*@test-host.example.com:5432/test_db-name?sslmode=disable"; assert_eq!(masked_string, expected_masked); @@ -223,11 +236,124 @@ fn test_edge_cases_empty_strings() { file: "".to_string(), }; - let connection_string = config.get_connection_string(); - let expected = "postgres://:@:/?sslmode=disable"; - assert_eq!(connection_string, expected); + // An empty port is not a port. The URL path used to carry it as far as + // the parser and come back with `invalid port number`; now it is refused + // where it can still be described as what it is. + assert!(config.get_connect_options().is_err()); let masked_string = config.get_masked_connection_string(); let expected_masked = "postgres://*:*@:/?sslmode=disable"; assert_eq!(masked_string, expected_masked); } + +// ── issue #244: credentials are data, not URL syntax ─────────────────── +// The connection details used to be interpolated into a +// `postgres://user:password@host:port/db` string and parsed back by sqlx. +// Every RFC 3986 reserved character in a password was then read as syntax. +// These tests pin both halves: that the URL round-trip really is lossy, +// and that `get_connect_options` does not take part in it. + +use std::str::FromStr; + +fn config_with_password(password: &str) -> DumpConfig { + DumpConfig { + host: "localhost".to_string(), + port: "5432".to_string(), + user: "alice".to_string(), + password: password.to_string(), + database: "shop".to_string(), + scheme: "public".to_string(), + ssl: false, + file: "test.dump".to_string(), + } +} + +/// The URL the old code would have built for this configuration. +fn legacy_url(config: &DumpConfig) -> String { + format!( + "postgres://{}:{}@{}:{}/{}?sslmode=disable", + config.user, config.password, config.host, config.port, config.database + ) +} + +#[test] +fn reserved_characters_that_broke_the_url_leave_the_options_intact() { + // `#`, `/` and `?` each end the authority section early, so the parser + // reads the port out of something that is not a number — the reported + // `invalid port number`, naming the one part that was never wrong. + for password in ["repro#pass", "repro/pass", "repro?pass"] { + let config = config_with_password(password); + + assert!( + PgConnectOptions::from_str(&legacy_url(&config)).is_err(), + "`{password}` was expected to break the URL form" + ); + + let options = config + .get_connect_options() + .unwrap_or_else(|e| panic!("`{password}` must still configure cleanly: {e}")); + assert_eq!(options.get_host(), "localhost"); + assert_eq!(options.get_port(), 5432); + assert_eq!(options.get_username(), "alice"); + assert_eq!(options.get_database(), Some("shop")); + } +} + +#[test] +fn a_percent_escape_in_a_password_is_no_longer_decoded() { + // The quiet one. `%41` is a valid percent-escape, so the URL parser + // decoded it and `pass%41word` authenticated as `passAword` — verified + // live against PostgreSQL 16 in both directions: the wrong password was + // accepted, and the real one could not be used at all. + // + // There is no password getter on `PgConnectOptions`, so this reads it out + // of the `Debug` rendering, which prints it. That couples the test to + // sqlx's formatting; if sqlx ever redacts it the assertion fails loudly, + // which is the right way round for a test whose whole subject is a + // credential being rewritten in transit. + let config = config_with_password("pass%41word"); + + let from_url = PgConnectOptions::from_str(&legacy_url(&config)) + .expect("this one parses — that is exactly the problem"); + assert!( + format!("{from_url:?}").contains(r#"password: Some("passAword")"#), + "the URL form is supposed to decode %41; if it no longer does, this \ + test has stopped describing the bug: {from_url:?}" + ); + + let from_options = config.get_connect_options().expect("valid configuration"); + assert!( + format!("{from_options:?}").contains(r#"password: Some("pass%41word")"#), + "the password must reach sqlx exactly as it was typed: {from_options:?}" + ); +} + +#[test] +fn a_port_that_is_not_a_number_is_reported_as_a_port() { + let mut config = config_with_password("plain"); + config.port = "not-a-port".to_string(); + + let error = config + .get_connect_options() + .expect_err("a non-numeric port must be refused"); + + assert!( + matches!(error, sqlx::Error::Configuration(_)), + "expected a configuration error, got: {error:?}" + ); +} + +#[test] +fn a_port_with_surrounding_whitespace_is_accepted() { + // Config files are hand-edited; `PORT = 5432 ` is a typo, not an outage. + let mut config = config_with_password("plain"); + config.port = " 5432 ".to_string(); + + assert_eq!( + config + .get_connect_options() + .expect("whitespace around a port is not an error") + .get_port(), + 5432 + ); +} diff --git a/app/src/dump/core.rs b/app/src/dump/core.rs index 9abdeaf..00769d1 100644 --- a/app/src/dump/core.rs +++ b/app/src/dump/core.rs @@ -277,9 +277,20 @@ impl Dump { ); } + // A bad port is a configuration error, not a connection failure, and + // saying so is the whole point of issue #244 — the old URL path + // reported every unparseable credential as `invalid port number`. + let connect_options = self.configuration.get_connect_options().map_err(|e| { + Error::other(format!( + "Invalid connection configuration ({}): {}.", + self.configuration.get_masked_connection_string(), + e + )) + })?; + let pool = PgPoolOptions::new() .max_connections(max_connections) - .connect(self.configuration.get_connection_string().as_str()) + .connect_with(connect_options) .await .map_err(|e| { Error::other(format!( @@ -3552,9 +3563,20 @@ impl Dump { /// Connect to the database and fill the dump without saving to a file. pub async fn inspect(&mut self, max_connections: u32) -> Result<(), Error> { + // A bad port is a configuration error, not a connection failure, and + // saying so is the whole point of issue #244 — the old URL path + // reported every unparseable credential as `invalid port number`. + let connect_options = self.configuration.get_connect_options().map_err(|e| { + Error::other(format!( + "Invalid connection configuration ({}): {}.", + self.configuration.get_masked_connection_string(), + e + )) + })?; + let pool = PgPoolOptions::new() .max_connections(max_connections) - .connect(self.configuration.get_connection_string().as_str()) + .connect_with(connect_options) .await .map_err(|e| { Error::other(format!( diff --git a/app/tests/live_database.rs b/app/tests/live_database.rs index 14a693f..a9f7fc8 100644 --- a/app/tests/live_database.rs +++ b/app/tests/live_database.rs @@ -97,3 +97,65 @@ async fn inspect_populates_a_dump_without_writing_a_file() { // buildable straight after. assert!(!dump.generate_clear_script(true, true, false).is_empty()); } + +/// Issue #244: a password full of URL-reserved characters must reach the +/// server as typed. +/// +/// The unit tests can only show that the credential is not rewritten on its +/// way into `PgConnectOptions`. Whether the server accepts it is a question +/// only the server can answer, and it is the question that was actually +/// wrong: `#`, `/` and `?` used to fail as `invalid port number` before any +/// socket was opened, and `pass%41word` used to authenticate as `passAword`. +/// +/// Needs a role-creating connection (the `PG*` user must have CREATEROLE or +/// be a superuser), which is what the documented local and CI setups use. +#[tokio::test] +#[ignore = "needs a reachable PostgreSQL server; run with --ignored"] +async fn a_password_of_reserved_characters_authenticates() { + // Every character the URL form mangled, in one password: the three that + // ended the authority section, the escape that was silently decoded, and + // the two that survived by luck. + const PASSWORD: &str = "p#ss/w?rd%41@x%"; + const ROLE: &str = "pgc_issue244_role"; + + let admin = env_config(""); + let mut admin_dump = Dump::new(admin.clone()); + admin_dump + .inspect(4) + .await + .expect("connect to the live database as the configured user"); + + let pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .connect_with(admin.get_connect_options().expect("admin configuration")) + .await + .expect("open an admin connection"); + + // `CREATE ROLE` takes the password as a plain SQL literal, so only the + // single quotes need escaping — this is not a URL either. + sqlx::query(&format!(r#"drop role if exists {ROLE}"#)) + .execute(&pool) + .await + .expect("clean up any leftover role"); + sqlx::query(&format!( + r#"create role {ROLE} login password '{}'"#, + PASSWORD.replace('\'', "''") + )) + .execute(&pool) + .await + .expect("create the test role"); + let dir = ScratchDir::new("live-issue244"); + let mut as_role = env_config(&dir.path_str("issue244.dump")); + as_role.user = ROLE.to_string(); + as_role.password = PASSWORD.to_string(); + + let result = Dump::new(as_role).inspect(4).await; + + // Drop the role before asserting, so a failure does not leave it behind. + let _ = sqlx::query(&format!(r#"drop role if exists {ROLE}"#)) + .execute(&pool) + .await; + pool.close().await; + + result.expect("a password of URL-reserved characters must authenticate"); +}