From f9464c2b8ed0b66c6cc2a06d4990ee80d4cfa829 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sun, 6 Sep 2026 22:00:17 +0200 Subject: [PATCH] fix(datatables): a second door is refused where it resolves, by the database itself rather than the login --- ...1d1d28e19e3658b333e3248c203eb4895b68.json} | 4 +- backend/ee-repo-ref.txt | 2 +- .../tests/datatable_tenants.rs | 58 ++++++++++++-- .../src/datatable_permissions.rs | 28 ++++--- backend/windmill-common/src/workspaces.rs | 77 ++++++++++++++++--- backend/windmill-store/src/resources.rs | 43 ++++++++++- 6 files changed, 180 insertions(+), 32 deletions(-) rename backend/.sqlx/{query-c5ca2b9382ece7f23fce8e3327d5b373617be55edd5efdf73fd71ec5847b0359.json => query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json} (82%) diff --git a/backend/.sqlx/query-c5ca2b9382ece7f23fce8e3327d5b373617be55edd5efdf73fd71ec5847b0359.json b/backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json similarity index 82% rename from backend/.sqlx/query-c5ca2b9382ece7f23fce8e3327d5b373617be55edd5efdf73fd71ec5847b0359.json rename to backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json index 32c480fb8c..2799bc59a2 100644 --- a/backend/.sqlx/query-c5ca2b9382ece7f23fce8e3327d5b373617be55edd5efdf73fd71ec5847b0359.json +++ b/backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt\n WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)\n AND dt.value->'permissions'->>'database_identity' = $1\n ORDER BY ws.workspace_id, dt.key", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt\n WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)\n AND dt.value->'permissions'->>'physical_identity' = $1\n ORDER BY ws.workspace_id, dt.key", "describe": { "columns": [ { @@ -24,5 +24,5 @@ null ] }, - "hash": "c5ca2b9382ece7f23fce8e3327d5b373617be55edd5efdf73fd71ec5847b0359" + "hash": "e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68" } diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 142f90f469..1266c658d3 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -1e70ab9734682756988f184e850a3b2c5a0595fe +0c220600e7ceece78a7623fd4185adfb0054d1c9 diff --git a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs index 41ecfb2681..25dcf862e2 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs @@ -733,9 +733,11 @@ async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database( let text = preview().await; assert!(!text.contains("cannot be enabled"), "{text}"); - // Another path, the same database: a copy, wherever the resource was moved. + // Another path and another login, the same database: a copy, wherever the + // resource was moved and whoever it connects as. sqlx::query( - "UPDATE resource SET path = 'f/moved/pg', value = jsonb_set(value, '{dbname}', '\"prod\"') + "UPDATE resource SET path = 'f/moved/pg', + value = jsonb_set(jsonb_set(value, '{dbname}', '\"prod\"'), '{user}', '\"postgres\"') WHERE workspace_id = 'detached' AND path = 'u/test-user/pg'", ) .execute(&db) @@ -768,13 +770,13 @@ async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database( .await?; assert_eq!(resp.status(), 200); let value: serde_json::Value = resp.json().await?; - windmill_common::workspaces::datatable_database_identity(&value) + windmill_common::workspaces::physical_database_identity(&value) }; sqlx::query( r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, '{datatables,byo,permissions}', jsonb_build_object('enabled', true, 'roles', '{"admin": {"tenants": []}}'::jsonb, - 'database_identity', $1::text)) + 'physical_identity', $1::text)) WHERE workspace_id = 'test-workspace'"#, ) .bind(&identity) @@ -822,8 +824,54 @@ async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database( let status = resp.status().as_u16(); let text = resp.text().await?; assert_eq!(status, 400, "{text}"); + assert!(text.contains("a data table of another workspace"), "{text}"); + // Nor deleted and recreated at the path the entry still names. + let resp = authed( + client().delete(format!( + "http://localhost:{port}/api/w/detached/resources/delete/f/moved/pg" + )), + "SECRET_TOKEN", + ) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let resp = authed( + client().post(format!("http://localhost:{port}/api/w/detached/resources/create")), + "SECRET_TOKEN", + ) + .json(&json!({ + "path": "f/moved/pg", + "resource_type": "postgresql", + "value": { "host": "db.example", "port": 5432, "dbname": "prod", "user": "app", "password": "pw", "sslmode": "disable" } + })) + .send() + .await?; + let status = resp.status().as_u16(); + let text = resp.text().await?; + assert_eq!(status, 400, "{text}"); + assert!(text.contains("a data table of another workspace"), "{text}"); + // A second door that got past every write-time guard — a `$var:` changed + // under the resource, say — is refused where it is used. + sqlx::query( + "INSERT INTO resource (workspace_id, path, value, resource_type, created_by, edited_at) + VALUES ('detached', 'f/moved/pg', $1, 'postgresql', 'test-user', now())", + ) + .bind(json!({ "host": "db.example", "port": 5432, "dbname": "prod", "user": "postgres", "password": "pw" })) + .execute(&db) + .await?; + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/detached/workspaces/get_datatable_table_schema?datatable_name=door&schema_name=public&table_name=t" + )), + "SECRET_TOKEN", + ) + .send() + .await?; + let status = resp.status().as_u16(); + let text = resp.text().await?; + assert_eq!(status, 401, "{text}"); assert!( - text.contains("data table 'byo' of workspace test-workspace"), + text.contains("whose role permissions are enabled"), "{text}" ); sqlx::query( diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 74327e4d2e..a592fdcd57 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -34,7 +34,8 @@ use windmill_common::utils::require_admin; use windmill_common::worker::SqlAnnotations; use windmill_common::workspaces::{ can_use_datatable_role, datatable_database_identity, get_datatable_resource_from_db_unchecked, - DataTable, DataTableCatalogResourceType, DataTablePermissions, ADMIN_DATATABLE_ROLE, + physical_database_identity, DataTable, DataTableCatalogResourceType, DataTablePermissions, + ADMIN_DATATABLE_ROLE, }; use windmill_common::{PgDatabase, DB}; @@ -234,6 +235,9 @@ pub(crate) struct AdminConnection { /// resolution can tell it has not moved. `None` for an instance database, /// which no workspace edit can repoint. pub(crate) database_identity: Option, + /// The database without the login, instance databases included: what every + /// other data table entry is held apart from. + pub(crate) physical_identity: String, pub(crate) admin_pg_role: String, pub(crate) pg_roles: PgRoleInventory, /// Whether `PUBLIC` holds CREATE on schema `public`, i.e. every role in this @@ -352,6 +356,7 @@ pub(crate) async fn connect_as_admin_unchecked( .resource_type == DataTableCatalogResourceType::Postgresql) .then(|| datatable_database_identity(&db_resource)); + let physical_identity = physical_database_identity(&db_resource); let pg_db: PgDatabase = serde_json::from_value(db_resource) .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?; let dbname = pg_db.dbname.clone(); @@ -451,6 +456,7 @@ pub(crate) async fn connect_as_admin_unchecked( AdminConnection { dbname, database_identity, + physical_identity, admin_pg_role, pg_roles: PgRoleInventory { existing, adoptable }, public_schema_is_open, @@ -491,6 +497,7 @@ async fn build_plan( // Stamped from the connection the roles are about to be created through, so // a resolution that lands anywhere else later can refuse. plan.permissions.database_identity = conn.database_identity; + plan.permissions.physical_identity = Some(conn.physical_identity); if !req.enabled { // Opting out is never refused; what it strands is said out loud. let roles: HashSet<&str> = datatable @@ -870,8 +877,9 @@ async fn refuse_enabling_permissions_over_shared_access( /// resource is a second door. /// /// An instance database is matched by name. A resource-backed entry reaches -/// wherever its resource points, whatever path it names, so it is matched by the -/// resource's host, port, database and user. Read from the stored resource rows +/// wherever its resource points, whatever path it names and whichever login it +/// carries, so it is matched by the resource's host, port and database. Read +/// from the stored resource rows /// in one query: a field that is a `$var:` / `$res:` reference is only resolved /// — a per-row read, and a secret backend call where the workspace uses one — /// when every plain field already agrees, so the loop that decrypts runs for @@ -920,7 +928,7 @@ async fn entries_reaching( } DataTableCatalogResourceType::Postgresql => { let ours = get_datatable_resource_from_db_unchecked(db, w_id, datatable_name).await?; - let identity = datatable_database_identity(&ours); + let identity = physical_database_identity(&ours); let is_reference = |v: &serde_json::Value| { v.as_str() .is_some_and(|s| s.starts_with("$var:") || s.starts_with("$res:")) @@ -933,7 +941,7 @@ async fn entries_reaching( // says what it reaches once resolved. let mut references = !raw.is_object(); let mut plain_fields_agree = true; - for field in DATABASE_IDENTITY_FIELDS { + for field in PHYSICAL_DATABASE_FIELDS { let theirs = raw.get(field).unwrap_or(&serde_json::Value::Null); if is_reference(theirs) { references = true; @@ -966,7 +974,7 @@ async fn entries_reaching( ))) } }; - if datatable_database_identity(&resolved) == identity { + if physical_database_identity(&resolved) == identity { reaching.push(describe(&o.workspace_id, &o.name, o.deleted)); } } @@ -975,8 +983,8 @@ async fn entries_reaching( Ok(reaching) } -/// The fields [`datatable_database_identity`] hashes. -const DATABASE_IDENTITY_FIELDS: [&str; 4] = ["host", "port", "dbname", "user"]; +/// The fields [`physical_database_identity`] hashes. +const PHYSICAL_DATABASE_FIELDS: [&str; 3] = ["host", "port", "dbname"]; /// Refuse a data table entry that reaches a database another data table governs. /// @@ -1021,9 +1029,9 @@ pub(crate) async fn refuse_reaching_a_governed_database( db, ) .await?; - windmill_common::workspaces::governed_datatable_with_identity( + windmill_common::workspaces::governed_datatable_reaching( db, - &datatable_database_identity(&resource), + &physical_database_identity(&resource), &[(w_id, datatable_name)], ) .await? diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index b5113af656..fd2935dd43 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1247,6 +1247,13 @@ pub struct DataTablePermissions { /// repointed by editing a resource. #[serde(default, skip_serializing_if = "Option::is_none")] pub database_identity: Option, + /// The database itself, as [`physical_database_identity`] fingerprints it — + /// the login left out. Stamped at the opt-in, instance databases included, + /// and what every other data table entry is held apart from: one reaching + /// this database would hand its users the owning connection, whichever login + /// its own resource carries. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub physical_identity: Option, } impl DataTablePermissions { @@ -1554,9 +1561,11 @@ fn resource_backs_permissioned_datatable(names: &[String]) -> Error { /// table may not be pointed at a database another data table governs — that /// entry would be a second door onto it, open to every member as the owning /// connection. The settings form refuses such an entry; this refuses the -/// resource edit that would turn an existing entry into one. The new value is +/// resource write — an edit, a create at a path an entry still names, a rename +/// onto one — that would turn an existing entry into one. The new value is /// resolved for it, `$var:` references included, since that is what the entry -/// would reach. +/// would reach. A resource owner need not be an admin anywhere, so a governed +/// data table of another workspace is not named to them. pub async fn ensure_resource_identity_change_allowed( db: &DB, w_id: &str, @@ -1595,12 +1604,12 @@ pub async fn ensure_resource_identity_change_allowed( return Ok(()); }; let resolved = transform_json_unchecked(new_value, w_id, db).await?; - let new_identity = datatable_database_identity(&resolved); + let new_identity = physical_database_identity(&resolved); let own: Vec<(&str, &str)> = backing .iter() .map(|(name, _)| (w_id, name.as_str())) .collect(); - if let Some((gw, gname)) = governed_datatable_with_identity(db, &new_identity, &own).await? { + if let Some((gw, gname)) = governed_datatable_reaching(db, &new_identity, &own).await? { let mut names: Vec<&str> = backing.iter().map(|(n, _)| n.as_str()).collect(); names.sort(); return Err(Error::BadRequest(format!( @@ -1611,30 +1620,31 @@ pub async fn ensure_resource_identity_change_allowed( if gw == w_id { format!("data table '{gname}'") } else { - format!("data table '{gname}' of workspace {gw}") + "a data table of another workspace".to_string() } ))); } Ok(()) } -/// The permissioned data table, in any workspace, whose opt-in stamped -/// `identity` — other than the `except` entries — if there is one. +/// The permissioned data table, in any workspace, whose opt-in stamped this +/// [`physical_database_identity`] — other than the `except` entries — if there +/// is one. /// /// Authorization: performs none, and needs none: it answers with a name, for a /// caller that only ever refuses on it. -pub async fn governed_datatable_with_identity( +pub async fn governed_datatable_reaching( db: &DB, - identity: &str, + physical_identity: &str, except: &[(&str, &str)], ) -> Result> { let governed = sqlx::query!( r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!" FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false) - AND dt.value->'permissions'->>'database_identity' = $1 + AND dt.value->'permissions'->>'physical_identity' = $1 ORDER BY ws.workspace_id, dt.key"#, - identity, + physical_identity, ) .fetch_all(db) .await?; @@ -1671,6 +1681,23 @@ pub fn datatable_database_identity(resolved: &serde_json::Value) -> String { hex::encode(hasher.finalize()) } +/// Fingerprint the database a connection resolves to, without the login: the +/// same host, port and database reached as another user is the same database, +/// and for the question of who else reaches it, the login does not matter. +pub fn physical_database_identity(resolved: &serde_json::Value) -> String { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + for field in ["host", "port", "dbname"] { + let value = resolved + .get(field) + .map(|v| v.to_string()) + .unwrap_or_default(); + hasher.update(value.as_bytes()); + hasher.update([0u8]); + } + hex::encode(hasher.finalize()) +} + /// Whether a resolution has to prove the data table still points where its roles /// live. /// @@ -2137,6 +2164,33 @@ async fn get_datatable_resource_inner( ensure_datatable_database_unchanged(name, &datatable, &db_resource)?; } + // An unpermissioned data table must not be a second door onto a database + // another data table governs. The writes that could make it one — the + // settings form, a resource write — refuse, but none of them is transactional + // with the opt-in, and a `$var:` a resource references changes under it + // through no write of the resource at all; here, on the resolved connection, + // is where the answer is authoritative. Internal callers are the guards + // themselves and the data table's own administration. + if !internal && !datatable.permissions.as_ref().is_some_and(|p| p.enabled) { + if let Some((gw, gname)) = governed_datatable_reaching( + db, + &physical_database_identity(&db_resource), + &[(w_id, name)], + ) + .await? + { + return Err(Error::NotAuthorized(format!( + "Data table '{name}' reaches the database of {}, whose role permissions are \ + enabled: its own connection would bypass them. Use that data table instead.", + if gw == w_id { + format!("data table '{gname}'") + } else { + "a data table of another workspace".to_string() + } + ))); + } + } + // The role logs in as itself rather than through `SET ROLE`, which a script // could `RESET ROLE` its way back out of and regain admin's privileges. if let Some((pg_rolename, pg_password)) = role_override { @@ -3452,6 +3506,7 @@ mod tests { roles: map, default_role: None, database_identity: None, + physical_identity: None, }), } } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index eea1476c9d..a90c5b2b83 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -134,7 +134,10 @@ pub struct EditResourceType { /// `Option` conflates: an absent field leaves the extension alone, while an /// explicit `null` clears it. A hub pull relies on both — a type that stops /// being a file type has to stop being one locally too. - #[serde(default, deserialize_with = "windmill_common::more_serde::double_option")] + #[serde( + default, + deserialize_with = "windmill_common::more_serde::double_option" + )] pub format_extension: Option>, } @@ -1231,6 +1234,17 @@ async fn create_resource( .await .map_err(sanitize_db_error)?; } else { + // A data table entry may already name this path, its resource deleted since. + let nvalue: serde_json::Value = serde_json::from_str(raw_json.0.get()) + .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?; + windmill_common::workspaces::ensure_resource_identity_change_allowed( + &db, + &w_id, + &resource.path, + None, + Some(&nvalue), + ) + .await?; // Create-only (the default): DO NOTHING + a row-count guard, so a path that appears between // check_path_conflict above and this insert is rejected rather than overwritten. A plain // DO UPDATE here would clobber a concurrently-created resource, breaking create-only callers @@ -1842,9 +1856,32 @@ async fn update_resource( return Err(Error::PermissionDenied(msg)); } - // A rename takes the resource out from under whatever names its path. - if ns.path.as_deref().is_some_and(|npath| npath != path) { + // A rename takes the resource out from under whatever names its path — and + // puts it under whatever names the new one. + if let Some(npath) = ns.path.as_deref().filter(|npath| *npath != path) { windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?; + let arriving: Option = match ns.value.as_ref() { + Some(v) => Some( + serde_json::from_str(v.get()) + .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?, + ), + None => sqlx::query_scalar!( + "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", + path, + &w_id + ) + .fetch_optional(&db) + .await? + .flatten(), + }; + windmill_common::workspaces::ensure_resource_identity_change_allowed( + &db, + &w_id, + npath, + None, + arriving.as_ref(), + ) + .await?; } // Same as `set_resource_value`: the identity a permissioned data table's // roles were created against is not free to move underneath them.