From 7619a7a51e6c69576e24ccec21b8935fdf61e28b Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 1 Sep 2026 04:09:12 +0200 Subject: [PATCH] fix(datatables): keep role passwords out of audit, and role names out of SQL The audit parameter of a data table config save carried the whole settings blob, generated role passwords included. Redact it the way every other export of that blob already is. Both SDKs pasted the caller's role straight into the `-- role` annotation, where a newline ends the comment and leaves the rest running as whatever the first line named. Check the value against the role-name grammar the server enforces. --- .../windmill-api-workspaces/src/workspaces.rs | 8 +++++++- python-client/wmill/wmill/client.py | 9 +++++++++ typescript-client/sqlUtils.ts | 9 +++++++++ typescript-client/tests/sqlUtils.test.ts | 16 ++++++++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index d7eeabea73..ce39e2a457 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3712,7 +3712,13 @@ async fn edit_datatable_config( .and_then(|old| old.permissions.clone()); } - let args_for_audit = format!("{:?}", new_config.settings); + // The settings carry each role's generated login password, and an audit + // parameter is stored in the clear and traced. + let args_for_audit = serde_json::to_value(&new_config.settings) + .ok() + .and_then(|v| redact_datatable_settings_for_export(Some(v))) + .map(|v| v.to_string()) + .unwrap_or_default(); audit_log( &mut *tx, &authed, diff --git a/python-client/wmill/wmill/client.py b/python-client/wmill/wmill/client.py index d9bf502c70..713bf6a1e3 100644 --- a/python-client/wmill/wmill/client.py +++ b/python-client/wmill/wmill/client.py @@ -2369,6 +2369,11 @@ def stream_result(stream) -> None: for text in stream: append_to_result_stream(text) +#: Role names the server accepts, so a value carrying a newline cannot close the +#: annotation and append statements of its own. +_ROLE_NAME_RE = re.compile(r"^[A-Za-z0-9_-]{1,63}$") + + class DataTableClient: """Client for executing SQL queries against Windmill DataTables.""" @@ -2381,6 +2386,10 @@ class DataTableClient: role: DataTable role to run as, on a datatable with permissions enabled (default: the data table's default role) """ + if role is not None and not _ROLE_NAME_RE.match(role): + raise ValueError( + f"Invalid data table role '{role}': must be 1-63 characters of letters, digits, '_' or '-'" + ) self.client = client self.role = role self.name, self.schema = parse_sql_client_name(name) diff --git a/typescript-client/sqlUtils.ts b/typescript-client/sqlUtils.ts index 0f04698ba9..4cbc815906 100644 --- a/typescript-client/sqlUtils.ts +++ b/typescript-client/sqlUtils.ts @@ -134,11 +134,20 @@ interface SqlProvider { providerName: string; } +/** Role names the server accepts, so a value carrying a newline cannot close the + * annotation and append statements of its own. */ +const ROLE_NAME_RE = /^[A-Za-z0-9_-]{1,63}$/; + function datatableProvider( name: string, schema?: string, role?: string ): SqlProvider { + if (role !== undefined && !ROLE_NAME_RE.test(role)) { + throw new Error( + `Invalid data table role '${role}': must be 1-63 characters of letters, digits, '_' or '-'` + ); + } return { providerName: "datatable", language: "postgresql", diff --git a/typescript-client/tests/sqlUtils.test.ts b/typescript-client/tests/sqlUtils.test.ts index 2f7197cac8..967d06bb53 100644 --- a/typescript-client/tests/sqlUtils.test.ts +++ b/typescript-client/tests/sqlUtils.test.ts @@ -38,11 +38,18 @@ interface SqlProvider { providerName: string; } +const ROLE_NAME_RE = /^[A-Za-z0-9_-]{1,63}$/; + function datatableProvider( name: string, schema?: string, role?: string ): SqlProvider { + if (role !== undefined && !ROLE_NAME_RE.test(role)) { + throw new Error( + `Invalid data table role '${role}': must be 1-63 characters of letters, digits, '_' or '-'` + ); + } return { providerName: "datatable", language: "postgresql", @@ -545,6 +552,15 @@ describe("datatable() — template tag", () => { const out = datatableQuery("main", "analyst")("SELECT $1", 42); expect(out.content.split("\n")[0]).toBe("-- role analyst"); }); + + test("a role that is not a role name is refused, not annotated", () => { + // The annotation is a comment line: a newline in the value would end it and + // leave the rest as SQL running under whatever role the first line named. + expect(() => dt("main", "admin\nDELETE FROM customers")).toThrow( + /Invalid data table role/ + ); + expect(() => dt("main", "operator-1")).not.toThrow(); + }); }); // =============================================================================