From c161aff933ec5c4388d3e5bcc657428bd3665b14 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 25 May 2026 16:13:28 +0000 Subject: [PATCH] skip workspaced-route duplicate checks on cloud (#9305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(settings): skip workspaced-route duplicate checks on cloud The pre-write validation hooks for `app_workspaced_route` and `http_route_workspaced_route` query the DB for cross-workspace duplicates and fail the save when any are found. On cloud both `custom_path_exists` (apps) and `route_path_key_exists` (HTTP triggers) already scope lookups by `workspace_id` regardless of these settings, so duplicates across workspaces are expected and the validation has no runtime meaning. The result was that any cloud super-admin attempting to save instance settings with these toggles set to false received `Duplicate HTTP route paths detected` even though the setting has no effect on cloud routing. Fixes WIN-1983 Co-Authored-By: Claude Opus 4.7 (1M context) * fix(error): render JsonErr as readable text and return 400 `Error::JsonErr` previously rendered through `#[error("Error: {0:#?}")]`, leaking Rust's `Debug` output (`Object { "error": String(...), "details": Array [...] }`) into the HTTP response body, and was bucketed into the catch-all 500 branch in `IntoResponse`. The result was a 500 status with a wall of Rust debug syntax in the toast — confusing and user-hostile. - Bucket `JsonErr` into 400 (Bad Request): every current call site (workspaced-route duplicate checks, OAuth client errors, etc.) is a client/validation issue, not an internal server fault. - Add `format_json_err_message` which surfaces the `error` field as the headline, summarises `details` (with a `- key=value` per entry), and pretty-prints the rest as JSON for unknown shapes. The frontend toast now reads e.g. Duplicate HTTP route paths detected - route_path=a, workspace_id=admins, http_method=post - route_path=a, workspace_id=starter, http_method=post Co-Authored-By: Claude Opus 4.7 (1M context) * fix(toast): preserve newlines and escape HTML in multi-line errors The toast renders via `{@html processMessage(message)}`, so server-side error bodies that span multiple lines (e.g. the duplicate-route response from the settings endpoint) collapsed into a single line because HTML treats consecutive whitespace (including `\n`) as a single space. When the message contains a newline, escape HTML first (defends against injected markup in server error bodies) and convert `\n` to `
` so multi-line errors stay readable in the toast. Co-Authored-By: Claude Opus 4.7 (1M context) * fixup: address CI review feedback - toast.ts: escape HTML unconditionally. The previous gate on `\n` left single-line server error bodies unsafe under {@html}, which cubic flagged as P0. The path regex below only inserts a `` around a `u/...` or `f/...` capture that can't contain HTML metacharacters, so escaping the whole input is the simpler and correct fix. - error.rs: add unit tests pinning the rendered shape of `format_json_err_message` (error+details, error-only, truncation cap, non-object fallback to pretty JSON). Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- backend/windmill-api-settings/src/lib.rs | 13 ++- backend/windmill-common/src/error.rs | 106 ++++++++++++++++++++++- frontend/src/lib/components/toast.ts | 18 +++- 3 files changed, 132 insertions(+), 5 deletions(-) diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 3078be9818..644973f894 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -63,7 +63,7 @@ use windmill_common::{ server::Smtp, worker::is_cloud_production_host, }; -use windmill_common::{error::to_anyhow, PgDatabase}; +use windmill_common::{error::to_anyhow, worker::CLOUD_HOSTED, PgDatabase}; /// Unauthenticated settings routes. /// @@ -588,7 +588,10 @@ async fn run_setting_pre_write_hook( ))); }; - if !*workspaced_route { + // Cloud always scopes app custom paths by workspace_id (see + // `custom_path_exists` in apps.rs), so duplicates across workspaces + // are expected and this setting has no runtime effect on cloud. + if !*workspaced_route && !*CLOUD_HOSTED { #[derive(Debug, Deserialize, Serialize)] #[allow(unused)] struct DuplicateApp { @@ -651,7 +654,11 @@ async fn run_setting_pre_write_hook( ))); }; - if !*workspaced_route { + // Cloud always scopes routes by workspace_id (see + // `route_path_key_exists` in windmill-trigger-http), so duplicates + // across workspaces are expected and this setting has no runtime + // effect on cloud. + if !*workspaced_route && !*CLOUD_HOSTED { #[derive(Debug, Deserialize, Serialize)] #[allow(unused)] struct DuplicateRoute { diff --git a/backend/windmill-common/src/error.rs b/backend/windmill-common/src/error.rs index 49dcf25450..f202bf27f9 100644 --- a/backend/windmill-common/src/error.rs +++ b/backend/windmill-common/src/error.rs @@ -72,7 +72,7 @@ pub enum Error { ExecutionRawError(Box), #[error("Error: {error:#} @{location:#}")] Anyhow { error: anyhow::Error, location: String }, - #[error("Error: {0:#?}")] + #[error("{}", format_json_err_message(.0))] JsonErr(serde_json::Value), #[error("{0}")] AIError(String), @@ -256,6 +256,7 @@ impl IntoResponse for Error { Self::SqlErr { .. } | Self::BadRequest(_) | Self::AIError(_) + | Self::JsonErr(_) | Self::QuotaExceeded(_) => axum::http::StatusCode::BAD_REQUEST, Self::BadGateway(_) => axum::http::StatusCode::BAD_GATEWAY, Self::Generic(status_code, _) => status_code, @@ -280,6 +281,59 @@ impl IntoResponse for Error { } } +/// Render a `JsonErr` payload as a readable message suitable for direct +/// display in a toast: surface the `error` field as the headline, append a +/// short summary of `details` (e.g. duplicate paths) when present, and fall +/// back to pretty JSON for unknown shapes. Avoids the Rust `Debug` output +/// (`Object { "error": String("..."), ... }`) that previously leaked to users. +fn format_json_err_message(v: &serde_json::Value) -> String { + if let Some(obj) = v.as_object() { + let headline = obj + .get("error") + .and_then(|e| e.as_str()) + .map(|s| s.to_string()); + let details_summary = obj.get("details").and_then(|d| { + let arr = d.as_array()?; + if arr.is_empty() { + return None; + } + let preview = arr + .iter() + .take(5) + .map(|item| match item { + serde_json::Value::Object(o) => { + let parts: Vec = o + .iter() + .map(|(k, val)| match val { + serde_json::Value::String(s) => format!("{k}={s}"), + _ => format!("{k}={val}"), + }) + .collect(); + format!("- {}", parts.join(", ")) + } + serde_json::Value::String(s) => format!("- {s}"), + other => format!("- {other}"), + }) + .collect::>() + .join("\n"); + let suffix = if arr.len() > 5 { + format!("\n... ({} more)", arr.len() - 5) + } else { + String::new() + }; + Some(format!("{preview}{suffix}")) + }); + + match (headline, details_summary) { + (Some(h), Some(d)) => return format!("{h}\n{d}"), + (Some(h), None) => return h, + (None, Some(d)) => return d, + (None, None) => {} + } + } + serde_json::to_string_pretty(v).unwrap_or_else(|_| v.to_string()) +} + pub trait OrElseNotFound { fn or_else_not_found(self, s: impl ToString) -> Result; } @@ -316,3 +370,53 @@ where Self(err.into()) } } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn json_err_message_error_and_details() { + let v = json!({ + "error": "Duplicate HTTP route paths detected", + "details": [ + { "route_path": "a", "workspace_id": "admins", "http_method": "post" }, + { "route_path": "a", "workspace_id": "starter", "http_method": "post" }, + ], + }); + let rendered = Error::JsonErr(v).to_string(); + assert_eq!( + rendered, + "Duplicate HTTP route paths detected\n\ + - route_path=a, workspace_id=admins, http_method=post\n\ + - route_path=a, workspace_id=starter, http_method=post" + ); + } + + #[test] + fn json_err_message_error_only() { + let v = json!({ "error": "Something went wrong" }); + assert_eq!(Error::JsonErr(v).to_string(), "Something went wrong"); + } + + #[test] + fn json_err_message_truncates_long_details() { + let details: Vec<_> = (0..8).map(|i| json!({ "k": i })).collect(); + let v = json!({ "error": "boom", "details": details }); + let rendered = Error::JsonErr(v).to_string(); + assert!(rendered.starts_with("boom\n- k=0\n- k=1\n- k=2\n- k=3\n- k=4")); + assert!(rendered.ends_with("... (3 more)")); + // Items beyond the cap aren't enumerated. + assert!(!rendered.contains("- k=5")); + } + + #[test] + fn json_err_message_fallback_to_pretty_json() { + let v = json!([1, 2, 3]); + // Non-object payload falls back to pretty JSON instead of leaking + // Rust `Debug` syntax. + let rendered = Error::JsonErr(v).to_string(); + assert_eq!(rendered, "[\n 1,\n 2,\n 3\n]"); + } +} diff --git a/frontend/src/lib/components/toast.ts b/frontend/src/lib/components/toast.ts index 6a68f76f26..d013d20aca 100644 --- a/frontend/src/lib/components/toast.ts +++ b/frontend/src/lib/components/toast.ts @@ -1,12 +1,28 @@ const pathRegex = /\b(u|f)(\/[^\/\s]+){2,}\b/g +function escapeHtml(s: string): string { + return s + .replaceAll('&', '&') + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('"', '"') + .replaceAll("'", ''') +} + export function processMessage(message: string | undefined): string { let msg = !message ? 'Error without message' : typeof message != 'string' ? JSON.stringify(message, null, 2) : message - return msg.replaceAll(pathRegex, (path) => { + // Toast renders via {@html}, so escape unconditionally — server error + // bodies can contain arbitrary content (e.g. `<` from a SQL fragment in + // an error message), and the path regex below only ever inserts a safe + // `` around a `u/...` or `f/...` capture that itself cannot match + // any HTML metacharacter. Convert `\n` to `
` so multi-line errors + // stay readable in the toast (without this, HTML collapses newlines). + let html = escapeHtml(msg).replaceAll('\n', '
') + return html.replaceAll(pathRegex, (path) => { return `${path}` }) }