From 78a12fdfccb79e4c2442e8675ba2be55561f1455 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 8 Sep 2026 21:29:51 +0200 Subject: [PATCH] fix(datatables): refuse to copy a data table that is under roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pg_dump carries no roles and the import runs with --no-privileges, so a copied data table arrives owned by the admin connection with no GRANT for any role. The settings clone brings `permissions` across, so the fork's tenants pass Windmill's check, connect as the role they were given, and are denied by Postgres on everything: an entry that reads as configured and answers nothing. Refuse the copy — in the import endpoint before any data moves, and in the fork path the CLI takes. Replaying the source's owners and ACLs into the clone is what lifts this, and is a change of its own. Dropping `permissions` from the copy instead would be the unsafe half, since the copy holds the parent's rows. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR --- .../tests/datatable_roles.rs | 30 +++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 54 ++++++++++++++++--- 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index c7621e9996..48ecedf6f2 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -533,3 +533,33 @@ async fn a_rename_has_to_match_the_save_it_claims_to_describe( assert_eq!(entry.unwrap()["datatable"], "other", "the swap did not carry the pointer"); Ok(()) } + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_data_table_under_roles_is_not_copied_into_a_fork( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + // `pg_dump` carries no roles and the restore drops ACLs, so a copy would arrive with the + // parent's tenants and none of the grants behind them: every role but admin denied by + // Postgres in a data table that reads as configured. Refuse the copy rather than ship that, + // and refuse it before any data moves. + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/test-workspace/workspaces/import_pg_database" + )), + "SECRET_TOKEN", + ) + .json(&json!({"source": "datatable://main", "target": "datatable://main", + "fork_behavior": "schema_only"})) + .send() + .await?; + assert_eq!(resp.status(), 400); + assert!( + resp.text().await?.contains("under roles"), + "the copy was refused for some other reason" + ); + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index ef7c832e1c..157f2bd4ba 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -46,7 +46,7 @@ use windmill_common::workspaces::GitRepositorySettings; use windmill_common::workspaces::WorkspaceDeploymentUISettings; use windmill_common::workspaces::{ check_deploy_rules, check_user_against_rule, get_datatable_resource_from_db, - get_datatable_resource_from_db_unchecked, resolve_governing_datatable, + get_datatable_resource_from_db_unchecked, parse_datatable_ref, resolve_governing_datatable, validate_dev_workspace_id, validate_fork_workspace_id, validate_workspace_name, DataTable, DataTableCatalogResourceType, DataTableForkBehavior, DatatableAccess, ProtectionRuleKind, ProtectionRules, ProtectionRuleset, RuleCheckResult, WorkspaceGitSyncSettings, @@ -3378,6 +3378,32 @@ struct ImportPgDatabaseRequest { fork_behavior: DataTableForkBehavior, } +/// Refuse to copy a data table that is under roles. +/// +/// `pg_dump` carries no roles and the import runs with `--no-privileges`, so a clone arrives with +/// its objects owned by the admin connection and no `GRANT` for any role. The settings copy brings +/// `permissions` across, so the fork's tenants pass Windmill's check, connect as the role they were +/// given, and are then denied by Postgres on everything — a data table that looks configured and +/// answers nothing. +/// +/// It fails closed rather than open, so this is a usability cliff rather than a hole, and the fix +/// is to replay the source's owners and ACLs into the clone. That is a change of its own; until it +/// exists, refusing is the honest answer. Dropping `permissions` from the clone instead would be +/// the unsafe half: the copy holds the parent's rows, so an unpermissioned clone hands all of them +/// to everyone in the fork. +async fn ensure_datatable_is_clonable(db: &DB, w_id: &str, name: &str) -> Result<()> { + let governing = resolve_governing_datatable(db, w_id, name).await?; + if governing.datatable.permissions.is_some() { + return Err(Error::BadRequest(format!( + "Data table '{name}' is under roles and cannot be copied yet: a copy carries the \ + role assignments but not the Postgres privileges behind them, so every role but \ + admin would be denied in the copy. Fork it keeping the original database, or turn \ + its roles off first." + ))); + } + Ok(()) +} + /// Import (pg_dump/pg_import) from source to target async fn import_pg_database( authed: ApiAuthed, @@ -3390,6 +3416,11 @@ async fn import_pg_database( return Ok("No action needed for KeepOriginal behavior".to_string()); } + if let Some(reference) = req.source.strip_prefix("datatable://") { + let (name, _) = parse_datatable_ref(reference); + ensure_datatable_is_clonable(&db, &w_id, name).await?; + } + if req.fork_behavior == DataTableForkBehavior::SchemaAndData { require_admin(authed.is_admin, &authed.username)?; if *CLOUD_HOSTED { @@ -3825,10 +3856,12 @@ async fn edit_datatable_config( // moves once, from what it named before this save. Inside the transaction: the rename and the // pointers that name it are one change, and half of it is a fork whose jobs stop. for (i, r) in new_config.renames.iter().enumerate() { - repoint_datatable_references(&mut tx, &w_id, &r.from, &format!("__wm_rename_tmp/{i}")).await?; + repoint_datatable_references(&mut tx, &w_id, &r.from, &format!("__wm_rename_tmp/{i}")) + .await?; } for (i, r) in new_config.renames.iter().enumerate() { - repoint_datatable_references(&mut tx, &w_id, &format!("__wm_rename_tmp/{i}"), &r.to).await?; + repoint_datatable_references(&mut tx, &w_id, &format!("__wm_rename_tmp/{i}"), &r.to) + .await?; } // A deletion cannot be followed the same way — there is nothing to point at any more. Read who @@ -3846,10 +3879,12 @@ async fn edit_datatable_config( ) .fetch_all(&mut *tx) .await?; - stranded.extend(rows.into_iter().map(|r| StrandedReference { - workspace_id: r.workspace_id, - datatable: r.datatable, - })); + stranded.extend( + rows.into_iter().map(|r| StrandedReference { + workspace_id: r.workspace_id, + datatable: r.datatable, + }), + ); } tx.commit().await?; @@ -3866,7 +3901,9 @@ async fn edit_datatable_config( ) .await?; - Ok(Json(EditDataTableConfigResult { stranded_references: stranded })) + Ok(Json(EditDataTableConfigResult { + stranded_references: stranded, + })) } #[derive(Deserialize)] @@ -7885,6 +7922,7 @@ async fn apply_forked_datatable( &DatatableAccess::Authed(authed.to_authed_ref()), ) .await?; + ensure_datatable_is_clonable(db, parent_w_id, &fdt.name).await?; windmill_common::validate_dbname(&fdt.new_dbname)?; if !fdt.new_dbname.starts_with("wm_fork_") { return Err(Error::BadRequest(format!(