diff --git a/CHANGELOG.md b/CHANGELOG.md index c06348d..3370e35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Security + +- **Pin `pg_temp` last in the migration `search_path`.** Each migration was applied + with `SET LOCAL search_path TO `, which left `pg_temp` at its implicit + highest-priority position. Migrations reference objects by unqualified name and + rely on the `search_path` to resolve them to the target schema, so a same-named + temporary object could be resolved instead while a migration runs with elevated + (DDL) privileges. Migrations now run with `SET LOCAL search_path TO , + pg_temp`, pinning the temporary-object schema to the lowest priority so it can no + longer shadow those unqualified references. This is + defense-in-depth following the PostgreSQL `search_path` hardening guidance + (CVE-2018-1058); `pg_temp` is per-session, so there is no live escalation path for + the trusted SQL the runner executes today. No schema or behavioral change for + well-behaved callers. Existing deployments are unaffected and require no + re-migration — the change only governs how future migrations are applied. + ## [0.1.34] - 2026-05-25 ### Security diff --git a/migrations/0002_create_stored_procedures.sql b/migrations/0002_create_stored_procedures.sql index c3427ba..0ebcee1 100644 --- a/migrations/0002_create_stored_procedures.sql +++ b/migrations/0002_create_stored_procedures.sql @@ -1,6 +1,6 @@ -- Migration 0002: Create stored procedures for PostgreSQL provider -- This migration creates schema-qualified stored procedures to replace inline SQL queries --- Note: This migration runs with SET LOCAL search_path TO {schema_name}, so procedures +-- Note: This migration runs with SET LOCAL search_path TO {schema_name}, pg_temp, so procedures -- will be created in the target schema automatically. However, procedures need to use -- schema-qualified table names to work correctly when called from different contexts. diff --git a/migrations/README.md b/migrations/README.md index 6dcee3c..3114817 100644 --- a/migrations/README.md +++ b/migrations/README.md @@ -43,7 +43,8 @@ CREATE TABLE {schema}._duroxide_migrations ( When writing migrations: -1. **Use schema-relative names**: Migrations are executed with `SET LOCAL search_path`, so use unqualified table names: +1. **Use schema-relative names**: Migrations are executed with + `SET LOCAL search_path TO , pg_temp`, so use unqualified table names: ```sql CREATE TABLE instances (...); ``` @@ -93,7 +94,9 @@ Migrations are automatically applied when creating a `PostgresProvider`. Each te - Migrations run inside transactions - Each migration is executed atomically (all-or-nothing) -- The `search_path` is set to the target schema for each migration +- The `search_path` is set to `, pg_temp` for each migration + (`pg_temp` is pinned last so temporary objects cannot shadow the unqualified + references migrations rely on the `search_path` to resolve) - Migrations run in the order specified by their version numbers ## Troubleshooting diff --git a/src/migrations.rs b/src/migrations.rs index eb3b6d3..e8c9a7d 100644 --- a/src/migrations.rs +++ b/src/migrations.rs @@ -469,6 +469,20 @@ impl MigrationRunner { statements } + /// Build the `SET LOCAL search_path` statement used while applying a migration. + /// + /// `pg_temp` is pinned last so it sits at the lowest priority instead of its + /// implicit highest-priority position, preventing a temporary object from + /// shadowing the unqualified references a migration resolves via the + /// `search_path`. This is defense-in-depth (CVE-2018-1058); `pg_catalog` is left + /// unlisted so PostgreSQL keeps it implicitly first. + /// + /// `schema_name` is validated at provider construction + /// (`^[A-Za-z_][A-Za-z0-9_]*$`), so direct interpolation here is safe. + fn migration_search_path_stmt(schema_name: &str) -> String { + format!("SET LOCAL search_path TO {schema_name}, pg_temp") + } + /// Apply a single migration async fn apply_migration( &self, @@ -478,8 +492,14 @@ impl MigrationRunner { // Start transaction let mut tx = conn.begin().await?; - // Set search_path for this transaction - sqlx::query(&format!("SET LOCAL search_path TO {}", self.schema_name)) + // Set search_path for this transaction. + // + // `pg_temp` is pinned explicitly at the lowest priority. Without it, + // `pg_temp` keeps its implicit highest-priority position, which would let a + // temporary object shadow the unqualified references this migration relies on + // the search_path to resolve. This is defense-in-depth (CVE-2018-1058 + // guidance); see `migration_search_path_stmt` for the threat model. + sqlx::query(&Self::migration_search_path_stmt(&self.schema_name)) .execute(&mut *tx) .await?; @@ -560,3 +580,17 @@ impl MigrationRunner { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn migration_search_path_pins_pg_temp_last() { + // The security property: pg_temp must be present and last, so temporary + // objects cannot shadow the unqualified references a migration relies on + // the search_path to resolve. + let stmt = MigrationRunner::migration_search_path_stmt("duroxide"); + assert_eq!(stmt, "SET LOCAL search_path TO duroxide, pg_temp"); + } +} diff --git a/tests/migration_search_path_tests.rs b/tests/migration_search_path_tests.rs new file mode 100644 index 0000000..35d7851 --- /dev/null +++ b/tests/migration_search_path_tests.rs @@ -0,0 +1,106 @@ +//! Behavioral verification that pinning `pg_temp` last in the migration +//! `search_path` (see `MigrationRunner::migration_search_path_stmt`) prevents a +//! same-named temporary object from shadowing the unqualified references a +//! migration relies on the `search_path` to resolve. +//! +//! The unit test `migration_search_path_pins_pg_temp_last` locks the *string* +//! the runner emits; this test proves that string has the intended PostgreSQL +//! name-resolution semantics. It is `#[ignore]`-d because it needs a live +//! database (`DATABASE_URL`). + +use sqlx::{Connection, PgConnection, Row}; + +fn get_database_url() -> String { + dotenvy::dotenv().ok(); + std::env::var("DATABASE_URL").expect("DATABASE_URL must be set") +} + +fn unique_schema() -> String { + let guid = uuid::Uuid::new_v4().to_string(); + format!("sp_test_{}", &guid[guid.len() - 8..]) +} + +/// In a single connection we create a real `widget` table in `` and a +/// temporary `widget` that would shadow it. We then show that: +/// 1. with `SET LOCAL search_path TO , pg_temp` (the runner's +/// statement) an unqualified `widget` resolves to the SCHEMA table, and +/// 2. the legacy `SET LOCAL search_path TO ` (pg_temp implicit first) +/// resolves the same reference to the TEMP table — the behavior the fix +/// hardens against. +#[tokio::test] +#[ignore = "requires a live PostgreSQL (DATABASE_URL)"] +async fn pg_temp_pinned_last_does_not_shadow_unqualified_reference() { + let url = get_database_url(); + let schema = unique_schema(); + let mut conn = PgConnection::connect(&url).await.expect("connect"); + + // Real object in the target schema, tagged so we can tell it apart. + sqlx::query(&format!("CREATE SCHEMA {schema}")) + .execute(&mut conn) + .await + .expect("create schema"); + sqlx::query(&format!("CREATE TABLE {schema}.widget (src text NOT NULL)")) + .execute(&mut conn) + .await + .expect("create schema table"); + sqlx::query(&format!( + "INSERT INTO {schema}.widget (src) VALUES ('schema')" + )) + .execute(&mut conn) + .await + .expect("seed schema table"); + + // A temporary `widget` that shadows it unless pg_temp is demoted. + sqlx::query("CREATE TEMP TABLE widget (src text NOT NULL)") + .execute(&mut conn) + .await + .expect("create temp table"); + sqlx::query("INSERT INTO widget (src) VALUES ('temp')") + .execute(&mut conn) + .await + .expect("seed temp table"); + + // (1) The runner's statement: pg_temp pinned last -> schema wins. + let safe_stmt = format!("SET LOCAL search_path TO {schema}, pg_temp"); + let mut tx = conn.begin().await.expect("begin safe tx"); + sqlx::query(&safe_stmt) + .execute(&mut *tx) + .await + .expect("set safe search_path"); + let resolved: String = sqlx::query("SELECT src FROM widget LIMIT 1") + .fetch_one(&mut *tx) + .await + .expect("read unqualified widget (safe)") + .get("src"); + tx.rollback().await.expect("rollback safe tx"); + assert_eq!( + resolved, "schema", + "with pg_temp pinned last, an unqualified reference must resolve to the schema object" + ); + + // (2) Legacy statement: pg_temp implicit first -> temp shadows the schema. + let legacy_stmt = format!("SET LOCAL search_path TO {schema}"); + let mut tx = conn.begin().await.expect("begin legacy tx"); + sqlx::query(&legacy_stmt) + .execute(&mut *tx) + .await + .expect("set legacy search_path"); + let shadowed: String = sqlx::query("SELECT src FROM widget LIMIT 1") + .fetch_one(&mut *tx) + .await + .expect("read unqualified widget (legacy)") + .get("src"); + tx.rollback().await.expect("rollback legacy tx"); + assert_eq!( + shadowed, "temp", + "the legacy search_path leaves pg_temp first, so the temp object shadows the schema \ + object — this is the behavior the fix hardens against" + ); + + // Cleanup (temp table disappears with the session). + sqlx::query(&format!("DROP SCHEMA {schema} CASCADE")) + .execute(&mut conn) + .await + .expect("drop schema"); + conn.close().await.ok(); +}