fix(datatables): refuse a malformed role query string instead of ignoring it

`?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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR

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?;
     }
 

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=<name>`."
+            )));
+        }
+        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=<name>`, where <name> 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]

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[://<name>][?role=<role>]' AS <alias>`. 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<AttachedDatatable<'_>> {
+fn parse_attach_datatable(query: &str) -> Result<Option<AttachedDatatable<'_>>> {
     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<Mutex<Vec<String>>>,
     job: &MiniPulledJob,
 ) -> Result<Option<Vec<String>>> {
-    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]

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)?;