mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-12 16:05:43 +00:00
skip workspaced-route duplicate checks on cloud (#9305)
* 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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 `<br />` so multi-line errors stay readable in the toast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 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 `<span>` 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
tristantr
co-authored by
Claude Opus 4.7
parent
9e63f2cd09
commit
c161aff933
@@ -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 {
|
||||
|
||||
@@ -72,7 +72,7 @@ pub enum Error {
|
||||
ExecutionRawError(Box<serde_json::value::RawValue>),
|
||||
#[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<String> = 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::<Vec<_>>()
|
||||
.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<T> {
|
||||
fn or_else_not_found(self, s: impl ToString) -> Result<T>;
|
||||
}
|
||||
@@ -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]");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
// `<span>` around a `u/...` or `f/...` capture that itself cannot match
|
||||
// any HTML metacharacter. Convert `\n` to `<br />` so multi-line errors
|
||||
// stay readable in the toast (without this, HTML collapses newlines).
|
||||
let html = escapeHtml(msg).replaceAll('\n', '<br />')
|
||||
return html.replaceAll(pathRegex, (path) => {
|
||||
return `<span class="bg-surface-secondary p-1 text-xs font-mono whitespace-nowrap rounded-md">${path}</span>`
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user