From 2d2cdb7a99f26a6c7d278744683d4e913f257097 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 4 Aug 2026 15:07:56 +0000 Subject: [PATCH] allowlist resource_type and escape fuzzy-search highlights (#10509) * fix: allowlist resource_type and escape search highlights * fix: bound path length and keep marked-label offsets entity-aware * fix: match postgres word-char semantics and drop double-escaping * fix: sanitize db constraint and rls errors instead of relying on the regex --- backend/windmill-common/src/utils.rs | 135 ++++++++++++++++++ backend/windmill-store/src/resources.rs | 24 +++- backend/windmill-store/src/variables.rs | 14 +- .../lib/components/ContentSearchInner.svelte | 17 +-- .../src/lib/components/SearchItems.svelte | 14 +- .../src/lib/components/instanceSettings.ts | 7 + .../instanceSettingsMarkedLabel.test.ts | 27 ++++ 7 files changed, 217 insertions(+), 21 deletions(-) create mode 100644 frontend/src/lib/components/instanceSettingsMarkedLabel.test.ts diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index 2bd6f1b204..6d13515d1f 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -222,6 +222,89 @@ pub fn escape_ilike_pattern(s: &str) -> String { .replace('_', "\\_") } +lazy_static::lazy_static! { + /// `[\p{Alphabetic}\p{Nd}_-]`, not `[\w-]`: Postgres' `\w` is `alnum` plus + /// underscore, while Rust's also covers combining marks, connector + /// punctuation and join controls. Spelling it out keeps these in step with + /// the CHECK constraints below, which are the real authority — a character + /// accepted here and rejected there puts the raw constraint violation back + /// on the wire, which is what this guard exists to prevent. + static ref PROPER_CHAR: &'static str = r"\p{Alphabetic}\p{Nd}_-"; + + /// Mirrors the `proper_id` CHECK shared by `script`, `flow`, `variable`, + /// `resource` and `schedule`. + static ref PROPER_PATH_RE: regex::Regex = + regex::Regex::new(&format!(r"^[ufg](/[{}]+){{2,}}$", *PROPER_CHAR)).unwrap(); + /// Mirrors the `proper_name` CHECK on `resource_type.name`, which + /// `resource.resource_type` references without a foreign key of its own. + static ref PROPER_TYPE_NAME_RE: regex::Regex = + regex::Regex::new(&format!(r"^[{}]{{1,50}}$", *PROPER_CHAR)).unwrap(); +} + +/// Reject a path the `proper_id` constraint would reject anyway, so the caller +/// gets a plain 400 instead of the raw Postgres constraint-violation string, +/// which names the table and constraint and echoes the input back. +pub fn check_proper_path(path: &str) -> Result<()> { + // The column is varchar(255); without this an over-long but well-formed path + // still reaches Postgres and leaks the same kind of message back. + if path.chars().count() > 255 { + return Err(Error::BadRequest( + "Invalid path: it must be at most 255 characters".to_string(), + )); + } + if !PROPER_PATH_RE.is_match(path) { + return Err(Error::BadRequest( + "Invalid path: it must be of the form u//, f// or \ + g//, where every segment contains only alphanumeric characters, \ + '_' or '-'" + .to_string(), + )); + } + Ok(()) +} + +/// Replace a Postgres rejection with a message that says what the caller did +/// wrong and nothing about the schema. The raw error names the table and the +/// constraint and echoes the input back. +/// +/// This, not `check_proper_path`, is what makes the leak unreachable: Postgres +/// classifies `\w` by the database's `LC_CTYPE`, so no fixed Rust charset can +/// mirror `proper_id` across deployments (`u/usér/nom` is valid under a UTF-8 +/// locale and rejected under `C`). The pre-checks exist to give the common cases +/// a precise message; this catches whatever they let through. +pub fn sanitize_db_error(e: sqlx::Error) -> Error { + let Some(db_err) = e.as_database_error() else { + return Error::from(e); + }; + match db_err.code().as_deref() { + // check_violation — `proper_id` / `proper_name` and friends + Some("23514") => Error::BadRequest( + "Invalid path or name: it does not match the required format".to_string(), + ), + // string_data_right_truncation + Some("22001") => Error::BadRequest("A field exceeds its maximum length".to_string()), + // insufficient_privilege — row-level security rejected the row + Some("42501") => { + Error::NotAuthorized("You don't have write permission at this path".to_string()) + } + _ => Error::from(e), + } +} + +/// Confine a resource type name to the charset `resource_type.name` already +/// enforces. `resource.resource_type` has no such constraint of its own, so +/// without this any string up to 50 chars can be stored and later rendered as +/// the type of a resource everyone in the workspace sees. +pub fn check_proper_type_name(name: &str) -> Result<()> { + if !PROPER_TYPE_NAME_RE.is_match(name) { + return Err(Error::BadRequest( + "Invalid resource type: it must be 1 to 50 alphanumeric characters, '_' or '-'" + .to_string(), + )); + } + Ok(()) +} + pub fn require_admin(is_admin: bool, username: &str) -> Result<()> { if !is_admin { Err(Error::RequireAdmin(username.to_string())) @@ -1388,6 +1471,58 @@ pub fn truncate_with_ellipsis(s: &str, max_chars: usize) -> String { mod tests { use super::*; + /// The guards are only safe because they are never stricter than the DB + /// constraints they front. Narrowing `\w` to ASCII reads equivalent and + /// compiles, but would start rejecting paths that already deploy today. + #[test] + fn proper_path_matches_the_db_constraint() { + for ok in [ + "u/admin/foo", + "f/some-folder/bar/baz", + "g/all/x", + "u/usér/nom", + ] { + assert!(check_proper_path(ok).is_ok(), "{ok} should be accepted"); + } + for bad in [ + "a/admin/foo", + "u/admin", + "u/admin/foo/", + "u/admin/lawful_variable/x", + "", + // Postgres' `\w` covers neither join controls nor combining marks; + // Rust's `\w` covers both, so `[\w-]` here would pass these through + // to the constraint and leak its message back to the caller. + "u/admin/a\u{200C}b", + "u/admin/a\u{0301}b", + "u/admin/a\u{203F}b", + ] { + assert!(check_proper_path(bad).is_err(), "{bad} should be rejected"); + } + assert!(check_proper_path(&format!("u/admin/{}", "a".repeat(300))).is_err()); + } + + #[test] + fn proper_type_name_matches_the_db_constraint() { + for ok in ["postgresql", "c_aws_account", "my-type", &"a".repeat(50)] { + assert!( + check_proper_type_name(ok).is_ok(), + "{ok} should be accepted" + ); + } + for bad in [ + "", + &"a".repeat(51), + "", + "a\u{200C}b", + ] { + assert!( + check_proper_type_name(bad).is_err(), + "{bad} should be rejected" + ); + } + } + #[test] fn truncate_handles_multibyte_at_boundary() { // Byte 25 of this string falls inside a 2-byte 'а'; naive `&s[..25]` would panic. diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 4ccf364cf8..a184a094b7 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -18,7 +18,10 @@ use windmill_common::workspaces::{check_deploy_rules, RuleCheckResult}; use crate::secret_backend_ext::rename_vault_secret; use crate::var_resource_cache::{auth_identity, cache_resource, get_cached_resource}; -use windmill_common::utils::{escape_ilike_pattern, BulkDeleteRequest}; +use windmill_common::utils::{ + check_proper_path, check_proper_type_name, escape_ilike_pattern, sanitize_db_error, + BulkDeleteRequest, +}; use windmill_common::webhook::{WebhookMessage, WebhookShared}; use axum::{ @@ -1023,6 +1026,8 @@ async fn create_resource( Json(resource): Json, ) -> Result<(StatusCode, String)> { check_scopes(&authed, || format!("resources:write:{}", resource.path))?; + check_proper_path(&resource.path)?; + check_proper_type_name(&resource.resource_type)?; if let RuleCheckResult::Blocked(msg) = check_deploy_rules( &w_id, AuditAuthorable::username(&authed), @@ -1100,7 +1105,8 @@ async fn create_resource( resource.labels.as_deref() as Option<&[String]> ) .execute(&mut *tx) - .await?; + .await + .map_err(sanitize_db_error)?; } else { // Create-only (the default): DO NOTHING + a row-count guard, so a path that appears between // check_path_conflict above and this insert is rejected rather than overwritten. A plain @@ -1119,7 +1125,8 @@ async fn create_resource( resource.labels.as_deref() as Option<&[String]> ) .execute(&mut *tx) - .await?; + .await + .map_err(sanitize_db_error)?; if inserted.rows_affected() == 0 { return Err(Error::BadRequest(format!( "Resource {} already exists", @@ -1691,6 +1698,10 @@ async fn update_resource( // source path. if let Some(npath) = ns.path.as_deref() { check_scopes(&authed, || format!("resources:write:{}", npath))?; + check_proper_path(npath)?; + } + if let Some(nrt) = ns.resource_type.as_deref() { + check_proper_type_name(nrt)?; } if let RuleCheckResult::Blocked(msg) = check_deploy_rules( &w_id, @@ -1810,7 +1821,10 @@ async fn update_resource( } let sql = sqlb.sql().map_err(|e| Error::internal_err(e.to_string()))?; - let npath_o: Option = sqlx::query_scalar(&sql).fetch_optional(&mut *tx).await?; + let npath_o: Option = sqlx::query_scalar(&sql) + .fetch_optional(&mut *tx) + .await + .map_err(sanitize_db_error)?; let npath = not_found_if_none(npath_o, "Resource", path)?; @@ -2161,6 +2175,8 @@ async fn create_resource_type( return Err(Error::PermissionDenied(msg)); } + check_proper_type_name(&resource_type.name)?; + let mut tx = user_db.begin(&authed).await?; check_rt_path_conflict(&mut tx, &w_id, &resource_type.name).await?; diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 3b70a3e3a7..8f288dc85e 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -17,7 +17,9 @@ use crate::secret_backend_ext::{ delete_secret_from_backend, get_secret_value, is_external_stored_value, is_vault_stored_value, rename_vault_secret, store_secret_value, }; -use windmill_common::utils::{escape_ilike_pattern, BulkDeleteRequest}; +use windmill_common::utils::{ + check_proper_path, escape_ilike_pattern, sanitize_db_error, BulkDeleteRequest, +}; use windmill_common::webhook::{WebhookMessage, WebhookShared}; use axum::{ @@ -593,6 +595,7 @@ async fn create_variable( Json(variable): Json, ) -> Result<(StatusCode, String)> { check_scopes(&authed, || format!("variables:write:{}", variable.path))?; + check_proper_path(&variable.path)?; if let RuleCheckResult::Blocked(msg) = check_deploy_rules( &w_id, AuditAuthorable::username(&authed), @@ -659,7 +662,8 @@ async fn create_variable( &authed.username ) .execute(&mut *tx) - .await?; + .await + .map_err(sanitize_db_error)?; if variable.ws_specific.unwrap_or(false) { sqlx::query!( @@ -1095,6 +1099,7 @@ async fn update_variable( // source path. if let Some(npath) = ns.path.as_deref() { check_scopes(&authed, || format!("variables:write:{}", npath))?; + check_proper_path(npath)?; } let authed = maybe_refresh_folders(&path, &w_id, authed, &db).await; @@ -1289,7 +1294,10 @@ async fn update_variable( sqlb.set_str("edited_by", &authed.username); sqlb.returning("path"); let sql = sqlb.sql().map_err(|e| Error::internal_err(e.to_string()))?; - let npath_o: Option = sqlx::query_scalar(&sql).fetch_optional(&mut *tx).await?; + let npath_o: Option = sqlx::query_scalar(&sql) + .fetch_optional(&mut *tx) + .await + .map_err(sanitize_db_error)?; not_found_if_none(npath_o, "Variable", path)? } else { // `has_sql_updates` is guaranteed true whenever `ns.path` is provided diff --git a/frontend/src/lib/components/ContentSearchInner.svelte b/frontend/src/lib/components/ContentSearchInner.svelte index 4b04b9b07a..34f3815a47 100644 --- a/frontend/src/lib/components/ContentSearchInner.svelte +++ b/frontend/src/lib/components/ContentSearchInner.svelte @@ -68,15 +68,6 @@ return ` (${n})` } - function escape(htmlStr) { - return htmlStr - .replace(/&/g, '&') - .replace(//g, '>') - .replace(/"/g, '"') - .replace(/'/g, ''') - } - let showNbScripts = $state(10) let showNbApps = $state(10) let showNbResources = $state(10) @@ -127,7 +118,7 @@ filter={search} items={scripts} f={(s) => { - return escape(s.content) + return s.content }} bind:filteredItems={filteredScriptItems} /> @@ -136,7 +127,7 @@ filter={search} items={resources} f={(s) => { - return escape(YAML.stringify(s.value)) + return YAML.stringify(s.value) }} bind:filteredItems={filteredResourceItems} /> @@ -145,7 +136,7 @@ filter={search} items={flows} f={(s) => { - return escape(YAML.stringify(s.value, null, 4)) + return YAML.stringify(s.value, null, 4) }} bind:filteredItems={filteredFlowItems} /> @@ -154,7 +145,7 @@ filter={search} items={apps} f={(s) => { - return escape(YAML.stringify(s.value, null, 4)) + return YAML.stringify(s.value, null, 4) }} bind:filteredItems={filteredAppItems} /> diff --git a/frontend/src/lib/components/SearchItems.svelte b/frontend/src/lib/components/SearchItems.svelte index 069fabfe99..1413059ff5 100644 --- a/frontend/src/lib/components/SearchItems.svelte +++ b/frontend/src/lib/components/SearchItems.svelte @@ -1,6 +1,7 @@