Merge commit from fork

The database name of a Postgres resource was the bare positional argument of
pg_dump and the value of psql's --dbname. Both forms are interpreted: as a
positional argument a name opening with `-` is read as an option of pg_dump
(`--file=`, `--restrict-key=`), and in both forms a name holding `=` is
expanded as a libpq connection string that overrides the host, port and user
Windmill passed before it. libpq reads PGDATABASE as a plain name, neither
parsed nor expanded, so the name now goes there.

The change is in the shared helpers, so it covers every caller:
import_pg_database, export_pg_schema, datatable_clone::make_copy,
generate_initial_datatable_migration and pg_import_dump. pg_dump's invocation
is extracted into pg_dump_command() beside the existing psql_command() so the
argument vector can be asserted on directly.

Beside that:

- resolve_pg_source_checked_with_kind() now refuses a name opening with `-`
  for every source and target it resolves, where validate_dbname() only ever
  reached a target. The check is deliberately narrower than validate_dbname():
  a source is any database a user already has, and `2024_archive`, `my.db` or
  a non-ASCII name are all names Postgres accepts and resources carry today.

- export_pg_schema and import_pg_database now require the `code` capability,
  the one the data table migration endpoints next door already ask for. Both
  make the API server run pg_dump/psql as children of its own process against
  a database the request names, so the role that may not run code should not
  reach them; they stay open to every author, who can run pg_dump in a job
  anyway. The frontend offers both only from the database manager.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

backend/windmill-api-integration-tests/tests/operator_manage_rights.rs

@@ -406,3 +406,62 @@ async fn test_operators_draft_resources_and_variables(db: Pool<Postgres>) -> 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<Postgres>) -> 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(())
+}

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<DumpFile> {
-    let dump_file = DumpFile::new()?;
-
+    dump_path: &std::path::Path,
+) -> Result<(tokio::process::Command, Option<DumpFile>)> {
     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<DumpFile> {
+    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<D
         .arg(pg_db.port.unwrap_or(5432).to_string())
         .arg("--username")
         .arg(pg_db.login_name())
-        .arg("--dbname")
-        .arg(&pg_db.dbname)
         .arg("--no-psqlrc")
         .stdout(std::process::Stdio::piped())
         .stderr(std::process::Stdio::piped());
 
+    // Not `--dbname`, which expands a name holding `=` as a libpq connection string overriding
+    // the host, port and user set above. See `pg_dump_command`.
+    cmd.env("PGDATABASE", &pg_db.dbname);
+
     if let Some(ref password) = pg_db.password {
         cmd.env("PGPASSWORD", password);
     }
@@ -3715,6 +3738,97 @@ mod pg_tls_env_tests {
     }
 }
 
+/// The database name of a resource reaches `pg_dump` and `psql` through `PGDATABASE`, never in
+/// their argument vector, so neither an option (`--file=`, `--restrict-key=`) nor a libpq
+/// connection string (`host=... port=...`) in that field is parsed as one.
+#[cfg(test)]
+mod pg_dbname_env_tests {
+    use super::{pg_dump_command, pg_dump_database, psql_command, DumpFile, PgDumpOptions};
+    use windmill_common::PgDatabase;
+
+    /// Nothing listens on that port.
+    fn pg_db(dbname: &str) -> 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<std::ffi::OsString> {
+        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<String>,
     Json(req): Json<ImportPgDatabaseRequest>,
 ) -> Result<String> {
+    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<String>,
     Json(req): Json<ExportPgSchemaRequest>,
 ) -> Result<String> {
+    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,

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.