From b5785e8e7ab4d2565d35e596b6f99c89bbc9f395 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 9 Sep 2026 16:00:19 +0200 Subject: [PATCH] fix(datatables): refuse a malformed role query string instead of ignoring it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `?Role=analytics`, `?role=` and `?x=1&role=…` all fell through the reference parser's exact-match rule, so the connection resolved to the data table's default role and ran under a login the caller never asked for — the URI half of the same trap as a malformed `-- role` annotation. The key now matches case-insensitively, and anything else in the query string is an error naming it; `role` is the only parameter a reference takes. Callers that only need the entry keep a lenient `datatable_ref_name`, since they never act on the role. The DuckDB `ATTACH` parser propagates it rather than attaching under the default. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR --- .../windmill-api-workspaces/src/workspaces.rs | 6 +- backend/windmill-common/src/workspaces.rs | 91 ++++++++++++++++--- .../windmill-worker/src/duckdb_executor.rs | 45 ++++++--- backend/windmill-worker/src/pg_executor.rs | 2 +- 4 files changed, 111 insertions(+), 33 deletions(-) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 3be8f45fbe..9526c526fa 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, parse_datatable_ref, resolve_governing_datatable, + datatable_ref_name, get_datatable_resource_from_db_unchecked, resolve_governing_datatable, validate_dev_workspace_id, validate_fork_workspace_id, validate_workspace_name, DataTable, DataTableCatalogResourceType, DataTableForkBehavior, DatatableAccess, ProtectionRuleKind, ProtectionRules, ProtectionRuleset, RuleCheckResult, WorkspaceGitSyncSettings, @@ -3313,7 +3313,7 @@ async fn create_pg_database( // database that no data table entry names. Refuse here too, so the clone stops before one // exists rather than leaving an empty registered `wm_fork_…` behind. if let Some(reference) = req.source.strip_prefix("datatable://") { - let (name, _) = parse_datatable_ref(reference); + let name = datatable_ref_name(reference); ensure_datatable_is_clonable(&db, &w_id, name).await?; } @@ -3425,7 +3425,7 @@ async fn import_pg_database( } if let Some(reference) = req.source.strip_prefix("datatable://") { - let (name, _) = parse_datatable_ref(reference); + let name = datatable_ref_name(reference); ensure_datatable_is_clonable(&db, &w_id, name).await?; } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 6201e367ed..bddf85f0c1 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1858,8 +1858,12 @@ pub async fn ensure_can_use_datatable_role( return Ok(()); } let catalog = crate::datatable_roles::read_role_catalog(db).await?; - let Some((role_id, tenants)) = - datatable_role_entry(governing.datatable.permissions.as_ref(), &catalog, name, role)? + let Some((role_id, tenants)) = datatable_role_entry( + governing.datatable.permissions.as_ref(), + &catalog, + name, + role, + )? else { return Ok(()); }; @@ -2102,14 +2106,54 @@ pub fn strip_datatable_permissions( Some(datatable) } +/// The data table a `datatable://` reference names, ignoring its query string. For callers that +/// only need to find the entry; use [`parse_datatable_ref`] wherever the role is acted on. +pub fn datatable_ref_name(reference: &str) -> &str { + reference + .split_once('?') + .map(|(name, _)| name) + .unwrap_or(reference) +} + /// Split a `datatable://` reference into its name and the role its query string names. -pub fn parse_datatable_ref(reference: &str) -> (&str, Option<&str>) { +/// +/// A query string that does not parse is an error rather than an absent role. Falling back would +/// resolve the reference to the data table's default role, so `?Role=analytics` or a mistyped +/// `?role=` would quietly connect as something the caller did not ask for — the same trap as a +/// malformed `-- role` annotation, and `role` is the only parameter a reference takes. +pub fn parse_datatable_ref(reference: &str) -> Result<(&str, Option<&str>)> { let (name, query) = reference.split_once('?').unwrap_or((reference, "")); - let role = query - .split('&') - .find_map(|param| param.strip_prefix("role=")) - .filter(|role| !role.is_empty()); - (name, role) + let mut role = None; + for param in query.split('&').filter(|p| !p.is_empty()) { + let (key, value) = param.split_once('=').unwrap_or((param, "")); + if !key.eq_ignore_ascii_case("role") { + return Err(Error::BadRequest(format!( + "Data table reference '{name}' carries an unknown parameter '{key}'. \ + The only one it takes is `?role=`." + ))); + } + if role.is_some() { + return Err(Error::BadRequest(format!( + "Data table reference '{name}' names a role more than once." + ))); + } + if value.is_empty() || !is_datatable_role_name(value) { + return Err(Error::BadRequest(format!( + "Data table reference '{name}' has a malformed role '{value}'. Write it as \ + `?role=`, where is letters, digits, '_' or '-'." + ))); + } + role = Some(value); + } + Ok((name, role)) +} + +fn is_datatable_role_name(role: &str) -> bool { + !role.is_empty() + && role.len() <= 63 + && role + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') } #[derive(Deserialize, Serialize, Debug)] @@ -3348,7 +3392,10 @@ mod tests { assert!(can_use_datatable_role(&tenants(&["f/finance"]), &authed)); assert!(can_use_datatable_role(&tenants(&["*"]), &authed)); assert!(!can_use_datatable_role(&tenants(&[]), &authed)); - assert!(!can_use_datatable_role(&tenants(&["u/bob", "g/ops"]), &authed)); + assert!(!can_use_datatable_role( + &tenants(&["u/bob", "g/ops"]), + &authed + )); // A bare name is not a principal: only the three prefixes and the wildcard match. assert!(!can_use_datatable_role(&tenants(&["alice"]), &authed)); @@ -3360,17 +3407,33 @@ mod tests { #[test] fn a_datatable_ref_splits_off_its_role() { - assert_eq!(parse_datatable_ref("sales"), ("sales", None)); + assert_eq!(parse_datatable_ref("sales").unwrap(), ("sales", None)); assert_eq!( - parse_datatable_ref("sales?role=analytics"), + parse_datatable_ref("sales?role=analytics").unwrap(), ("sales", Some("analytics")) ); + // The key matches case-insensitively, the way the `-- role` annotation does. assert_eq!( - parse_datatable_ref("sales?x=1&role=analytics"), + parse_datatable_ref("sales?Role=analytics").unwrap(), ("sales", Some("analytics")) ); - // An empty role is no role rather than a role named "". - assert_eq!(parse_datatable_ref("sales?role="), ("sales", None)); + + // A query string that does not parse is refused rather than read as "no role": resolving + // it to the data table's default would connect as a login the caller never asked for. + for malformed in [ + "sales?role=", + "sales?role=an;alytics", + "sales?x=1&role=analytics", + "sales?role=a&role=b", + ] { + assert!( + parse_datatable_ref(malformed).is_err(), + "silently ignored: {malformed}" + ); + } + + // The name-only helper stays lenient — it is used where the role is never acted on. + assert_eq!(datatable_ref_name("sales?role="), "sales"); } #[test] diff --git a/backend/windmill-worker/src/duckdb_executor.rs b/backend/windmill-worker/src/duckdb_executor.rs index 88b367b191..7fb973d3ce 100644 --- a/backend/windmill-worker/src/duckdb_executor.rs +++ b/backend/windmill-worker/src/duckdb_executor.rs @@ -1496,7 +1496,7 @@ pub async fn do_duckdb( probe_blocks.extend(q); } else if let Some(q) = transform_attach_datatable(&query_block, conn, &mut hidden_passwords, job) - .await? + .await? { probe_blocks.extend(q); } else { @@ -1573,7 +1573,7 @@ pub async fn do_duckdb( v.extend(ducklake_query); } else if let Some(datatable_query) = transform_attach_datatable(&query_block, conn, &mut hidden_passwords, job) - .await? + .await? { v.extend(datatable_query); } else { @@ -2609,19 +2609,24 @@ struct AttachedDatatable<'a> { /// `ATTACH 'datatable[://][?role=]' AS `. A bare `datatable` names the default /// data table, so the role query string has to be accepted with and without an explicit name. -fn parse_attach_datatable(query: &str) -> Option> { +fn parse_attach_datatable(query: &str) -> Result>> { lazy_static::lazy_static! { static ref RE: regex::Regex = regex::Regex::new( r"(?i)ATTACH\s*'datatable(://[^'?:]+)?(\?[^':]*)?'\s*AS\s+([^ ;]+)" ).unwrap(); } - let cap = RE.captures(query)?; + let Some(cap) = RE.captures(query) else { + return Ok(None); + }; let name = cap.get(1).map(|m| &m.as_str()[3..]).unwrap_or("main"); - let role = cap - .get(2) - .and_then(|m| windmill_common::workspaces::parse_datatable_ref(m.as_str()).1); + // A query string that does not parse is refused rather than dropped: attaching under the + // default role when the statement asked for another one is the failure this guards. + let role = match cap.get(2) { + Some(m) => windmill_common::workspaces::parse_datatable_ref(m.as_str())?.1, + None => None, + }; let alias = cap.get(3).map(|m| m.as_str()).unwrap_or(""); - Some(AttachedDatatable { name, role, alias }) + Ok(Some(AttachedDatatable { name, role, alias })) } async fn transform_attach_datatable( @@ -2630,7 +2635,7 @@ async fn transform_attach_datatable( hidden_passwords: &mut Arc>>, job: &MiniPulledJob, ) -> Result>> { - let Some(attached) = parse_attach_datatable(query) else { + let Some(attached) = parse_attach_datatable(query)? else { return Ok(None); }; @@ -2787,16 +2792,26 @@ mod tests { #[test] fn attach_datatable_parses_name_and_role() { - let named = parse_attach_datatable("ATTACH 'datatable://sales?role=analytics' AS dt").unwrap(); - assert_eq!((named.name, named.role, named.alias), ("sales", Some("analytics"), "dt")); + let named = + parse_attach_datatable("ATTACH 'datatable://sales?role=analytics' AS dt").unwrap(); + assert_eq!( + (named.name, named.role, named.alias), + ("sales", Some("analytics"), "dt") + ); // A bare `datatable` is the default one, and still takes a role. - let default = parse_attach_datatable("ATTACH 'datatable?role=analytics' AS dt").unwrap(); + let default = parse_attach_datatable("ATTACH 'datatable?role=analytics' AS dt") + .unwrap() + .unwrap(); assert_eq!((default.name, default.role), ("main", Some("analytics"))); - let no_role = parse_attach_datatable("ATTACH 'datatable://sales' AS dt").unwrap(); + let no_role = parse_attach_datatable("ATTACH 'datatable://sales' AS dt") + .unwrap() + .unwrap(); assert_eq!((no_role.name, no_role.role), ("sales", None)); - let bare = parse_attach_datatable("ATTACH 'datatable' AS dt").unwrap(); + let bare = parse_attach_datatable("ATTACH 'datatable' AS dt").unwrap().unwrap(); assert_eq!((bare.name, bare.role), ("main", None)); - assert!(parse_attach_datatable("SELECT 1").is_none()); + assert!(parse_attach_datatable("SELECT 1").unwrap().is_none()); + // A malformed role is refused rather than attached under the default one. + assert!(parse_attach_datatable("ATTACH 'datatable://sales?Role=analytics' AS dt").is_err()); } #[test] diff --git a/backend/windmill-worker/src/pg_executor.rs b/backend/windmill-worker/src/pg_executor.rs index 3844f4d0cc..03560690f3 100644 --- a/backend/windmill-worker/src/pg_executor.rs +++ b/backend/windmill-worker/src/pg_executor.rs @@ -683,7 +683,7 @@ pub async fn do_postgresql( match pg_args.get("database").cloned() { Some(Value::String(db_str)) if db_str.starts_with("datatable://") => { let reference = db_str.trim_start_matches("datatable://"); - let (db_str, uri_role) = parse_datatable_ref(reference); + let (db_str, uri_role) = parse_datatable_ref(reference)?; // The annotation wins: a generated query can carry a `?role=` in the reference it // was handed, but only the script's author writes the leading comment block. let annotated = SqlAnnotations::datatable_role(&query)?;