From c645537ee6bd24a8bddf9692247315167a054358 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 9 Sep 2026 18:41:35 +0200 Subject: [PATCH] fix(datatables): honour `-- role: x`, and fix the DuckDB attach test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both real. `attach_datatable_parses_name_and_role` never compiled: `parse_attach_datatable` returns `Result>` now and one call site kept a single `unwrap`. Its `?Role=analytics` case also asserted a refusal, contradicting the parser in the same commit, which matches the key case-insensitively. Replaced with the cases that are genuinely malformed, and a positive one for the cased key. `-- role: analytics` fell through to the default role — the silent fallback the strict parser exists to remove, for the spelling most likely to be typed. The keyword now accepts an optional colon, attached or spaced, while a word that merely starts with it (`rolebased`) is still not an attempt. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb --- backend/windmill-common/src/worker.rs | 37 +++++++++++++++---- .../windmill-worker/src/duckdb_executor.rs | 30 ++++++++++++--- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/backend/windmill-common/src/worker.rs b/backend/windmill-common/src/worker.rs index 59d76a9d99..6b3c63e3c0 100644 --- a/backend/windmill-common/src/worker.rs +++ b/backend/windmill-common/src/worker.rs @@ -1105,16 +1105,27 @@ impl SqlAnnotations { if !line.starts_with("--") { break; } - let mut tokens = line[2..].split_whitespace(); - if !tokens - .next() - .is_some_and(|t| t.eq_ignore_ascii_case("role")) - { + // `role`, `Role`, `role:` and `role:name` all open an attempt; `rolexyz` does not. + // The colon is worth accepting rather than skipping past: `-- role: x` is the likelier + // spelling, and skipping it is exactly the silent fallback this refuses. + let body = line[2..].trim_start(); + let Some(after) = body + .get(..4) + .filter(|kw| kw.eq_ignore_ascii_case("role")) + .map(|_| &body[4..]) + else { + continue; + }; + let colon = after.starts_with(':'); + let after = after.strip_prefix(':').unwrap_or(after); + if !after.is_empty() && !colon && !after.starts_with(char::is_whitespace) { continue; } + // Past this point the line is an attempt to name a role, so a malformed one is an // error rather than a miss. Falling through would run the query as the data table's // default role — quietly, and under a login the author did not choose. + let mut tokens = after.split_whitespace(); let role = tokens .next() .map(|role| role.strip_suffix(';').unwrap_or(role)); @@ -2731,9 +2742,15 @@ mod tests { assert_eq!(role("SELECT 1;\n-- role analytics").unwrap(), None); assert_eq!(role("SELECT 1").unwrap(), None); - // Unambiguous intent is honoured: the keyword matches case-insensitively, and a trailing - // semicolon is a habit carried over from SQL rather than a different role. - for accepted in ["-- Role operator\nSELECT 1", "-- role operator;\nSELECT 1"] { + // Unambiguous intent is honoured: the keyword matches case-insensitively, a trailing + // semicolon is a habit carried over from SQL rather than a different role, and the colon + // spelling is the one most likely to be typed. + for accepted in [ + "-- Role operator\nSELECT 1", + "-- role operator;\nSELECT 1", + "-- role: operator\nSELECT 1", + "-- role:operator\nSELECT 1", + ] { assert_eq!( role(accepted).unwrap(), Some("operator".to_string()), @@ -2747,10 +2764,14 @@ mod tests { "-- role operator -- why\nSELECT 1", "-- role an;alytics\nSELECT 1", "-- role\nSELECT 1", + "-- role:\nSELECT 1", "-- role based access is handled below\nSELECT 1", ] { assert!(role(near_miss).is_err(), "silently ignored: {near_miss}"); } + + // A word that merely starts with the keyword is not an attempt. + assert_eq!(role("-- rolebased notes\nSELECT 1").unwrap(), None); } fn matcher(id: &str) -> WorkspaceMatcher { diff --git a/backend/windmill-worker/src/duckdb_executor.rs b/backend/windmill-worker/src/duckdb_executor.rs index 7fb973d3ce..764859d29b 100644 --- a/backend/windmill-worker/src/duckdb_executor.rs +++ b/backend/windmill-worker/src/duckdb_executor.rs @@ -2792,8 +2792,9 @@ mod tests { #[test] fn attach_datatable_parses_name_and_role() { - let named = - parse_attach_datatable("ATTACH 'datatable://sales?role=analytics' AS dt").unwrap(); + let named = parse_attach_datatable("ATTACH 'datatable://sales?role=analytics' AS dt") + .unwrap() + .unwrap(); assert_eq!( (named.name, named.role, named.alias), ("sales", Some("analytics"), "dt") @@ -2807,11 +2808,30 @@ mod tests { .unwrap() .unwrap(); assert_eq!((no_role.name, no_role.role), ("sales", None)); - let bare = parse_attach_datatable("ATTACH 'datatable' AS dt").unwrap().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").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()); + + // The key matches case-insensitively, as the `-- role` annotation does. + let cased = parse_attach_datatable("ATTACH 'datatable://sales?Role=analytics' AS dt") + .unwrap() + .unwrap(); + assert_eq!(cased.role, Some("analytics")); + + // A query string that does not parse is refused rather than attached under the default + // role: the statement asked for a specific one. + for malformed in [ + "ATTACH 'datatable://sales?role=' AS dt", + "ATTACH 'datatable://sales?role=an;alytics' AS dt", + "ATTACH 'datatable://sales?x=1&role=analytics' AS dt", + ] { + assert!( + parse_attach_datatable(malformed).is_err(), + "silently ignored: {malformed}" + ); + } } #[test]