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

@@ -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/<user>/<name>, f/<folder>/<name> or \
+             g/<group>/<name>, 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<script>alert(1)</script>",
+            "<script>alert(1)</script>",
+            // 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),
+            "<img src=x onerror=prompt('hacked')>",
+            "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.

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<CreateResource>,
 ) -> 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<String> = sqlx::query_scalar(&sql).fetch_optional(&mut *tx).await?;
+    let npath_o: Option<String> = 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?;

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<CreateVariable>,
 ) -> 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<String> = sqlx::query_scalar(&sql).fetch_optional(&mut *tx).await?;
+        let npath_o: Option<String> = 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

frontend/src/lib/components/ContentSearchInner.svelte

@@ -68,15 +68,6 @@
 		return ` (${n})`
 	}
 
-	function escape(htmlStr) {
-		return htmlStr
-			.replace(/&/g, '&')
-			.replace(/</g, '&lt;')
-			.replace(/>/g, '&gt;')
-			.replace(/"/g, '&quot;')
-			.replace(/'/g, '&#39;')
-	}
-
 	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}
 />

frontend/src/lib/components/SearchItems.svelte

@@ -1,6 +1,7 @@
 <script lang="ts">
 	import uFuzzy from '@leeoniya/ufuzzy'
 	import { untrack } from 'svelte'
+	import { escapeHtml } from '$lib/utils'
 
 	interface Props {
 		filter?: string
@@ -14,6 +15,13 @@
 
 	let uf = new uFuzzy(untrack(() => opts))
 
+	// Consumers render `marked` with {@html}, and the searched text is workspace
+	// content (paths, descriptions, resource types, ...). uFuzzy's default mark
+	// concatenates the raw substrings, so escape every part and let only the
+	// <mark> wrapper through as markup.
+	const markEscaped = (part: string, matched: boolean) =>
+		matched ? `<mark>${escapeHtml(part)}</mark>` : escapeHtml(part)
+
 	function filterItems() {
 		let trimmed = filter.trim()
 		if (items == undefined || trimmed.length == 0) {
@@ -32,7 +40,11 @@
 			let infoIdx = order[i]
 			result.push({
 				...items[info.idx[infoIdx]],
-				marked: uFuzzy.highlight(plaintextItems[info.idx[infoIdx]], info.ranges[infoIdx])
+				marked: uFuzzy.highlight(
+					plaintextItems[info.idx[infoIdx]],
+					info.ranges[infoIdx],
+					markEscaped
+				)
 			})
 		}
 		filteredItems = result

frontend/src/lib/components/instanceSettings.ts

@@ -1291,6 +1291,13 @@ export function extractMarkedLabel(marked: string | undefined, labelLength: numb
 		if (marked[markedIdx] === '<') {
 			while (markedIdx < marked.length && marked[markedIdx] !== '>') markedIdx++
 			markedIdx++
+		} else if (marked[markedIdx] === '&') {
+			// SearchItems escapes the haystack, so one plain character can arrive
+			// as an entity. Skipping the whole entity keeps this offset walk in
+			// step with `labelLength`, which counts unescaped characters.
+			const end = marked.indexOf(';', markedIdx)
+			markedIdx = end === -1 ? markedIdx + 1 : end + 1
+			plainIdx++
 		} else {
 			plainIdx++
 			markedIdx++

frontend/src/lib/components/instanceSettingsMarkedLabel.test.ts

@@ -0,0 +1,27 @@
+import { describe, it, expect } from 'vitest'
+import { extractMarkedLabel } from './instanceSettings'
+
+// SearchItems escapes the haystack before highlighting, so `marked` carries
+// entities where the label had `& < > " '`. The offset walk counts unescaped
+// characters, so an entity must advance it by one — otherwise the label is
+// truncated early and can be sliced mid-entity.
+describe('extractMarkedLabel', () => {
+	it('counts an escaped character as one, not as its entity length', () => {
+		const label = 'A & B'
+		expect(extractMarkedLabel('A &amp; B — category', label.length)).toBe('A &amp; B')
+	})
+
+	it('keeps the mark wrapper and stops at the label boundary', () => {
+		const label = 'Base URL'
+		expect(extractMarkedLabel('<mark>Base</mark> URL — general', label.length)).toBe(
+			'<mark>Base</mark> URL'
+		)
+	})
+
+	it('never slices an entity in half', () => {
+		const label = '"q" & <x>'
+		const out = extractMarkedLabel('&quot;q&quot; &amp; &lt;x&gt; — trailing', label.length)
+		expect(out).toBe('&quot;q&quot; &amp; &lt;x&gt;')
+		expect(out.endsWith(';')).toBe(true)
+	})
+})