From 45d3c951bb8827c67daaaf75ed52f3c3e1215911 Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Fri, 23 Oct 2026 18:56:08 -0400 Subject: [PATCH] fix: restore workspace pg_dump changes reverted by 812ff6ea5e (#11988) 812ff6ea5e was squashed from a branch based before 8af48429 and reverted its changes to these three files. Restore them as they were in 8af48429. Co-authored-by: Claude Opus 5.5 --- .../tests/operator_manage_rights.rs | 59 +++++++ .../windmill-api-workspaces/src/workspaces.rs | 163 ++++++++++++++++-- backend/windmill-common/src/lib.rs | 64 +++++++ 3 files changed, 273 insertions(+), 13 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/operator_manage_rights.rs b/backend/windmill-api-integration-tests/tests/operator_manage_rights.rs index cfbeae7a69..dbc5f36e01 100644 --- a/backend/windmill-api-integration-tests/tests/operator_manage_rights.rs +++ b/backend/windmill-api-integration-tests/tests/operator_manage_rights.rs @@ -406,3 +406,62 @@ async fn test_operators_draft_resources_and_variables(db: Pool) -> any Ok(()) } + +/// The two endpoints that make the API server run `pg_dump`/`psql` against a database a request +/// names are closed to a role that cannot run code. +/// +/// Both polarities on each route: a 403 alone would equally be a route that does not exist or one +/// refusing the source for some other reason, and the admin's 400 shows the gate is about the role +/// rather than the request. The source is deliberately unresolvable, so the gate is the only thing +/// that can answer 403 and no database is ever reached. +#[sqlx::test(migrations = "../migrations", fixtures("base", "permissions_test"))] +#[serial] +async fn test_dumping_a_database_needs_the_code_right(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + windmill_common::roles::invalidate_all_roles_cache(); + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let api = format!("http://localhost:{port}/api/w/{WS}"); + + let cases = [ + ( + "workspaces/export_pg_schema", + json!({"source": "u/operator/not-a-source"}), + "export a database schema", + ), + ( + "workspaces/import_pg_database", + json!({"source": "u/operator/not-a-source", "target": "u/operator/not-a-source", + "fork_behavior": "schema_only"}), + "import a database", + ), + ]; + + for (route, body, action) in cases { + let resp = operator_client() + .post(format!("{api}/{route}")) + .json(&body) + .send() + .await?; + let status = resp.status(); + let text = resp.text().await?; + assert_eq!(status, 403, "{route}: {text}"); + assert!( + text.contains(&format!("Your role cannot {action} in this workspace")), + "{route} refused for another reason: {text}" + ); + + let resp = reqwest::Client::new() + .post(format!("{api}/{route}")) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&body) + .send() + .await?; + let status = resp.status(); + let text = resp.text().await?; + assert_eq!(status, 400, "{route}: {text}"); + assert!(text.contains("Invalid source format"), "{route}: {text}"); + } + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 0534f3e7ee..9a6efec148 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3366,6 +3366,10 @@ pub(crate) async fn resolve_pg_source_checked_with_kind( let pg: PgDatabase = serde_json::from_value(db_resource) .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))?; + // Checked here rather than at each caller: every source and target of a dump, restore or copy + // comes through this function, and `validate_dbname` only ever reaches a target. The check is + // deliberately the narrow one — see `validate_dbname_not_option_like`. + windmill_common::validate_dbname_not_option_like(&pg.dbname)?; Ok((pg, managed_kind)) } @@ -3455,21 +3459,20 @@ pub(crate) struct PgDumpOptions<'a> { pub(crate) no_acl: bool, } -/// Run pg_dump against a PgDatabase, writing output to a temp file on disk. -/// Returns a DumpFile handle; the file is deleted when the handle is dropped. -pub(crate) async fn pg_dump_database( +/// A pg_dump invocation against `pg_db` writing to `dump_path`, carrying the connection settings +/// the CLI reads from the environment. The returned file, when there is one, holds the root +/// certificate the command points at and must outlive it. +fn pg_dump_command( pg_db: &PgDatabase, opts: PgDumpOptions<'_>, -) -> Result { - let dump_file = DumpFile::new()?; - + dump_path: &std::path::Path, +) -> Result<(tokio::process::Command, Option)> { let host = &pg_db.host; let port = pg_db.port.unwrap_or(5432).to_string(); let user = pg_db.login_name(); - let dbname = &pg_db.dbname; let mut cmd = tokio::process::Command::new("pg_dump"); - cmd.arg("--format=plain").arg("--file").arg(&dump_file.path); + cmd.arg("--format=plain").arg("--file").arg(dump_path); if opts.schema_only { cmd.arg("--schema-only"); } @@ -3487,13 +3490,31 @@ pub(crate) async fn pg_dump_database( .arg("--port") .arg(&port) .arg("--username") - .arg(user) - .arg(dbname); + .arg(user); + + // The name comes from a resource, so it never goes on the command line: as the positional + // argument a name starting with `-` is read as an option of pg_dump (`--file`, + // `--restrict-key`), and as `--dbname` a name holding `=` is expanded as a libpq connection + // string overriding the host, port and user set above. libpq takes the environment variable + // as a plain database name, neither parsed as an option nor expanded. + cmd.env("PGDATABASE", &pg_db.dbname); if let Some(ref password) = pg_db.password { cmd.env("PGPASSWORD", password); } - let _root_cert = apply_pg_tls_env(&mut cmd, pg_db)?; + let root_cert = apply_pg_tls_env(&mut cmd, pg_db)?; + Ok((cmd, root_cert)) +} + +/// Run pg_dump against a PgDatabase, writing output to a temp file on disk. +/// Returns a DumpFile handle; the file is deleted when the handle is dropped. +pub(crate) async fn pg_dump_database( + pg_db: &PgDatabase, + opts: PgDumpOptions<'_>, +) -> Result { + let dump_file = DumpFile::new()?; + + let (mut cmd, _root_cert) = pg_dump_command(pg_db, opts, &dump_file.path)?; let output = cmd .output() @@ -3619,12 +3640,14 @@ fn psql_command(pg_db: &PgDatabase) -> Result<(tokio::process::Command, Option PgDatabase { + PgDatabase { + host: "127.0.0.1".to_string(), + user: None, + password: None, + port: Some(1), + sslmode: None, + dbname: dbname.to_string(), + root_certificate_pem: None, + accept_invalid_certs: None, + use_iam_auth: None, + region: None, + options: None, + } + } + + fn pgdatabase_of(cmd: &std::process::Command) -> Option { + cmd.get_envs() + .find(|(k, _)| *k == "PGDATABASE") + .and_then(|(_, v)| v.map(|v| v.to_os_string())) + } + + #[test] + fn pg_dump_takes_the_dbname_from_the_environment() { + let dump_file = DumpFile::new().unwrap(); + let name = "--file=/tmp/written_by_the_caller"; + let (cmd, _root_cert) = + pg_dump_command(&pg_db(name), PgDumpOptions::default(), &dump_file.path).unwrap(); + let cmd = cmd.as_std(); + assert!( + cmd.get_args().all(|arg| arg != name), + "the dbname is still in pg_dump's argument vector" + ); + assert_eq!(pgdatabase_of(cmd).as_deref(), Some(name.as_ref())); + // The only `--file` pg_dump is given is still Windmill's own. + assert_eq!( + cmd.get_args() + .filter(|arg| arg.to_string_lossy().starts_with("--file")) + .count(), + 1 + ); + } + + #[test] + fn psql_takes_the_dbname_from_the_environment() { + let name = "host=elsewhere"; + let (cmd, _root_cert) = psql_command(&pg_db(name)).unwrap(); + let cmd = cmd.as_std(); + assert!( + cmd.get_args().all(|arg| arg != name), + "the dbname is still in psql's argument vector" + ); + assert_eq!(pgdatabase_of(cmd).as_deref(), Some(name.as_ref())); + } + + /// The same, observed on the real binary: an option-shaped name must reach the connection + /// attempt rather than pg_dump's option parser. Skipped where pg_dump is not installed — + /// which is why the assertion also requires that pg_dump ran at all (our `pg_dump failed:` + /// prefix) instead of only that the option message is absent. + #[tokio::test] + async fn pg_dump_does_not_read_an_option_shaped_dbname_as_an_option() { + if std::process::Command::new("pg_dump") + .arg("--version") + .output() + .is_err() + { + eprintln!("skipped: pg_dump is not installed"); + return; + } + let Err(err) = pg_dump_database(&pg_db("--no-such-option"), PgDumpOptions::default()).await + else { + panic!("pg_dump reached a server on 127.0.0.1:1"); + }; + let err = err.to_string(); + assert!( + err.contains("pg_dump failed:"), + "pg_dump did not run: {err}" + ); + assert!(!err.contains("unrecognized option"), "{err}"); + } +} + /// GUC names the server backing `pg_db` knows about. /// /// Asked through psql rather than a tokio-postgres connection so the lookup reaches @@ -4061,6 +4175,25 @@ fn ensure_copied_without_roles(governing: &GoverningDatatable) -> Result<()> { Ok(()) } +/// Gate for the endpoints that make the API server run `pg_dump`/`psql` against a database a +/// request names. +/// +/// Those two processes are children of the API server, outside any job sandbox and holding its +/// environment, and the dump they exchange is a `psql` script assembled from strings the source +/// server returns — so reaching them is close enough to running code on the server that the role +/// which may not run code at all must not reach them either. `code` is the capability the data +/// table migration endpoints next door already ask for (`datatable_migrations::ensure_not_operator`), +/// and not admin: the database manager offers an export and an import of a user's own Postgres +/// resource to every author, who can run the same `pg_dump` in a job anyway. +async fn ensure_can_dump_databases( + db: &DB, + w_id: &str, + authed: &ApiAuthed, + action: &str, +) -> Result<()> { + windmill_common::workspaces::check_role_can_code(db, w_id, authed, action).await +} + /// Import (pg_dump/pg_import) from source to target async fn import_pg_database( authed: ApiAuthed, @@ -4069,6 +4202,8 @@ async fn import_pg_database( Path(w_id): Path, Json(req): Json, ) -> Result { + ensure_can_dump_databases(&db, &w_id, &authed, "import a database").await?; + if req.fork_behavior == DataTableForkBehavior::KeepOriginal { return Ok("No action needed for KeepOriginal behavior".to_string()); } @@ -4185,6 +4320,8 @@ async fn export_pg_schema( Path(w_id): Path, Json(req): Json, ) -> Result { + ensure_can_dump_databases(&db, &w_id, &authed, "export a database schema").await?; + let pg = resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.source).await?; let dump_file = pg_dump_database( &pg, diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 4e1524580e..2d60008373 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -843,6 +843,50 @@ mod validate_dbname_tests { } } +#[cfg(test)] +mod validate_dbname_not_option_like_tests { + use super::validate_dbname_not_option_like; + + /// A source is any database a user already has, so everything `validate_dbname` refuses but + /// Postgres accepts has to keep working. + #[test] + fn accepts_every_name_that_is_not_an_option() { + for name in [ + "mydb", + "My-Db_1", + "2024_archive", + "my.db", + "db with spaces", + "Grüße", + "host=elsewhere port=5433", + "", + "a-dash-inside--it", + ] { + assert!( + validate_dbname_not_option_like(name).is_ok(), + "refused the legitimate name {name:?}" + ); + } + } + + #[test] + fn rejects_a_name_that_an_argument_vector_would_read_as_an_option() { + for name in [ + "--file=/tmp/x", + "--restrict-key=k", + "-f", + "-", + " --file=/tmp/x", + "\t-f", + ] { + let err = validate_dbname_not_option_like(name) + .expect_err(&format!("accepted the option-like name {name:?}")) + .to_string(); + assert!(err.contains("cannot start with '-'"), "{err}"); + } + } +} + #[cfg(test)] mod pg_tls_tests { use super::PgDatabase; @@ -1595,6 +1639,26 @@ pub fn validate_dbname(dbname: &str) -> error::Result<()> { Ok(()) } +/// The one restriction on the name of a database Windmill only *reads from*: it may not look like +/// a command-line option. +/// +/// [`validate_dbname`] is the rule for a name Windmill creates or interpolates into SQL, and is +/// far too narrow here: a source is any Postgres resource a user already has, and an existing +/// database may legitimately be named `2024_archive`, `my.db`, `Grüße` or anything else Postgres +/// accepts as a quoted identifier. Refusing those would break working resources. A name opening +/// with `-` is the one shape that is never a database and always a mistake or an attempt to smuggle +/// an option into a `pg_dump`/`psql` argument vector, so it is refused wherever a source is +/// resolved — the commands themselves pass the name through `PGDATABASE` and never on the command +/// line, and this keeps a future caller that forgets from re-opening that door. +pub fn validate_dbname_not_option_like(dbname: &str) -> error::Result<()> { + if dbname.trim_start().starts_with('-') { + return Err(error::Error::BadRequest( + "Database name cannot start with '-'".to_string(), + )); + } + Ok(()) +} + /// Lock the instance databases among `names` until `tx` ends, in a stable order. Taken by every /// settings save naming an instance database, and by [`drop_unused_instance_database`]: a save /// cannot start using a database between that drop's check that nothing does and the drop.