From 0a2ebfb3db73932e966db0662708aadcadcc1e4b Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 16 Sep 2026 23:55:43 +0200 Subject: [PATCH 1/3] fix: hold the ACL connection to the database that was authorized Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- .../windmill-api-workspaces/src/datatable_acl.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 8ec966d475..ce63555267 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -328,6 +328,22 @@ async fn connect_as_admin_unchecked( .await?; let pg: PgDatabase = serde_json::from_value(resource) .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?; + // Resolving reads the settings again, and a save since `governing` was authorized can point + // the entry elsewhere and back. An instance entry's database is its `resource_path`, so the + // connection is held to the database that was authorized, and a later check of the entry + // cannot pass while this talks to another one. + if governing + .datatable + .database + .as_ref() + .map(|d| d.resource_path.as_str()) + != Some(&pg.dbname) + { + return Err(Error::BadRequest(format!( + "Data table '{}' was pointed at another database while this ran; try again", + governing.name + ))); + } let dbname = pg.dbname.clone(); let (client, mut connection) = pg.connect(Some(db)).await?; // Unbounded: the driver must never wait on the receiver, which only drains once the statement From b7623f12ebafbbc6bbc296119bfec356093a327e Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 00:09:29 +0200 Subject: [PATCH 2/3] fix: build the ACL connection from the authorized data table entry Resolving the settings again could land on a resource with the same database name on another server, which the later entry checks never see. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- .../src/datatable_acl.rs | 34 +++++++------------ 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index ce63555267..1dac05beb0 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -36,10 +36,7 @@ use windmill_common::datatable_roles::{ ADMIN_DATATABLE_ROLE, CUSTOM_INSTANCE_USER, }; use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; -use windmill_common::workspaces::{ - get_datatable_resource_from_db_unchecked, resolve_governing_datatable, DataTable, - GoverningDatatable, -}; +use windmill_common::workspaces::{resolve_governing_datatable, DataTable, GoverningDatatable}; use windmill_common::{PgDatabase, DB}; use crate::datatable_permissions::{ensure_governs_datatable, ensure_reaches_datatable}; @@ -323,27 +320,20 @@ async fn connect_as_admin_unchecked( mpsc::UnboundedReceiver, String, )> { - let resource = - get_datatable_resource_from_db_unchecked(db, &governing.workspace_id, &governing.name) - .await?; - let pg: PgDatabase = serde_json::from_value(resource) - .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?; - // Resolving reads the settings again, and a save since `governing` was authorized can point - // the entry elsewhere and back. An instance entry's database is its `resource_path`, so the - // connection is held to the database that was authorized, and a later check of the entry - // cannot pass while this talks to another one. - if governing + ensure_instance(governing)?; + // Built from the authorized entry, never by resolving the settings again: a save in between + // could point the entry at a resource on another server and back, and this connection would + // then alter a database the later checks of the entry never see. + let mut pg = PgDatabase::parse_uri(&windmill_common::get_database_url().await?.as_str().await)?; + pg.dbname = governing .datatable .database .as_ref() - .map(|d| d.resource_path.as_str()) - != Some(&pg.dbname) - { - return Err(Error::BadRequest(format!( - "Data table '{}' was pointed at another database while this ran; try again", - governing.name - ))); - } + .expect("a governing entry owns a database") + .resource_path + .clone(); + pg.user = Some(CUSTOM_INSTANCE_USER.to_string()); + pg.password = Some(windmill_common::utils::get_custom_pg_instance_password(db).await?); let dbname = pg.dbname.clone(); let (client, mut connection) = pg.connect(Some(db)).await?; // Unbounded: the driver must never wait on the receiver, which only drains once the statement From e79bc14903e03938897aa7f5e4320c8b9655b6d0 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 09:00:04 +0200 Subject: [PATCH 3/3] fix: check ACL read reach against the entry it connects from Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- backend/ee-repo-ref.txt | 2 +- .../src/datatable_acl.rs | 4 ++-- .../src/datatable_permissions.rs | 12 ++++++++++++ .../src/datatable_permissions_oss.rs | 18 ++++++++++++++++-- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 1ba2346e6c..8cab265586 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -85e15fc93b238159fc90acfc8427d29e219b88ca +0edd40979cf36bfba59323f3f6a0811ae1369cf5 diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 1dac05beb0..6bd14316fc 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -39,7 +39,7 @@ use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::workspaces::{resolve_governing_datatable, DataTable, GoverningDatatable}; use windmill_common::{PgDatabase, DB}; -use crate::datatable_permissions::{ensure_governs_datatable, ensure_reaches_datatable}; +use crate::datatable_permissions::{ensure_governs_datatable, ensure_reaches_governing_datatable}; pub(crate) fn routes() -> Router { Router::new() @@ -1018,8 +1018,8 @@ async fn get_datatable_acl( ) -> JsonResult { crate::datatable_acl_oss::ensure_datatable_acl_available()?; let target: AclTarget = query.try_into()?; - ensure_reaches_datatable(&db, &w_id, &datatable_name, &authed).await?; let governing = resolve_governing_datatable(&db, &w_id, &datatable_name).await?; + ensure_reaches_governing_datatable(&db, &w_id, &datatable_name, &governing, &authed).await?; ensure_instance(&governing)?; let editable = ensure_governs_datatable(&db, &authed, &w_id, &governing) .await diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 5cb1f3c1c3..69e04e328d 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -65,3 +65,15 @@ pub(crate) async fn ensure_reaches_datatable( ) -> Result<()> { roles::ensure_reaches_datatable(db, w_id, datatable_name, authed).await } + +/// [`ensure_reaches_datatable`] against an entry already resolved, for a caller that goes on to +/// connect from that same entry. +pub(crate) async fn ensure_reaches_governing_datatable( + db: &DB, + w_id: &str, + datatable_name: &str, + governing: &GoverningDatatable, + authed: &ApiAuthed, +) -> Result<()> { + roles::ensure_reaches_governing_datatable(db, w_id, datatable_name, governing, authed).await +} diff --git a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs b/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs index f4d8c6a7ad..c7c7314885 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs @@ -12,8 +12,8 @@ #[cfg(all(feature = "private", feature = "enterprise"))] pub(crate) use crate::datatable_permissions_ee::{ - ensure_governs_datatable, ensure_reaches_datatable, get_datatable_permissions, - list_usable_datatable_roles, set_datatable_permissions, + ensure_governs_datatable, ensure_reaches_datatable, ensure_reaches_governing_datatable, + get_datatable_permissions, list_usable_datatable_roles, set_datatable_permissions, }; #[cfg(not(all(feature = "private", feature = "enterprise")))] @@ -56,6 +56,20 @@ mod ce { } } + pub(crate) async fn ensure_reaches_governing_datatable( + _db: &DB, + _w_id: &str, + _datatable_name: &str, + governing: &GoverningDatatable, + _authed: &ApiAuthed, + ) -> Result<()> { + if governing.datatable.permissions.is_none() { + Ok(()) + } else { + Err(unavailable()) + } + } + // The routes stay registered so the API has one shape; each answers after authentication, // before anything is read.