mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-26 08:02:31 +00:00
fix(http-triggers): scope route exists check by workspace, skip non-workspaced clones in forks
The non-CLOUD branch of `route_path_key_exists` self-excluded by trigger path alone, which silently masked cross-workspace collisions once forks started cloning trigger rows verbatim. Tighten it to exclude only the exact `(workspace_id, path)` row. Fork creation also now skips non-workspaced HTTP triggers — their URL has no workspace prefix, so a clone collides with the parent at the matchit router (which silently drops one of two duplicates) and there is no namespacing escape hatch. The clone copies all rows when CLOUD_HOSTED or HTTP_ROUTE_WORKSPACED_ROUTE forces every route workspaced regardless. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
29f5880809
commit
0cf1426450
+3
-2
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "\n SELECT EXISTS(\n SELECT 1\n FROM http_trigger\n WHERE\n ((workspaced_route IS TRUE AND workspace_id || '/' || route_path_key = $1)\n OR (workspaced_route IS FALSE AND route_path_key = $1))\n AND http_method = $2\n AND ($3::TEXT IS NULL OR path != $3)\n )\n ",
|
||||
"query": "\n SELECT EXISTS(\n SELECT 1\n FROM http_trigger\n WHERE\n ((workspaced_route IS TRUE AND workspace_id || '/' || route_path_key = $1)\n OR (workspaced_route IS FALSE AND route_path_key = $1))\n AND http_method = $2\n AND ($3::TEXT IS NULL OR NOT (workspace_id = $4 AND path = $3))\n )\n ",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
@@ -26,6 +26,7 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
@@ -33,5 +34,5 @@
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "fe464b8b3ade86743d82c5e3fb14f457e07f07e44c7b693d5d755899d4210dee"
|
||||
"hash": "aa0a2f90d15a642ad3caaa3876d9cb4a5391ff8663da61b48bfeb73bfa005bbe"
|
||||
}
|
||||
+4
-3
@@ -1,15 +1,16 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "INSERT INTO http_trigger (\n path, route_path, route_path_key, script_path, is_flow, workspace_id,\n edited_by, edited_at, extra_perms, authentication_method, http_method,\n static_asset_config, is_static_website, workspaced_route, wrap_body,\n raw_string, authentication_resource_path, summary, description,\n error_handler_path, error_handler_args, retry, request_type, mode,\n permissioned_as, labels\n )\n SELECT\n path, route_path, route_path_key, script_path, is_flow, $1,\n edited_by, edited_at, extra_perms, authentication_method, http_method,\n static_asset_config, is_static_website, workspaced_route, wrap_body,\n raw_string, authentication_resource_path, summary, description,\n error_handler_path, error_handler_args, retry, request_type, 'disabled'::TRIGGER_MODE,\n permissioned_as, labels\n FROM http_trigger WHERE workspace_id = $2",
|
||||
"query": "INSERT INTO http_trigger (\n path, route_path, route_path_key, script_path, is_flow, workspace_id,\n edited_by, edited_at, extra_perms, authentication_method, http_method,\n static_asset_config, is_static_website, workspaced_route, wrap_body,\n raw_string, authentication_resource_path, summary, description,\n error_handler_path, error_handler_args, retry, request_type, mode,\n permissioned_as, labels\n )\n SELECT\n path, route_path, route_path_key, script_path, is_flow, $1,\n edited_by, edited_at, extra_perms, authentication_method, http_method,\n static_asset_config, is_static_website, workspaced_route, wrap_body,\n raw_string, authentication_resource_path, summary, description,\n error_handler_path, error_handler_args, retry, request_type, 'disabled'::TRIGGER_MODE,\n permissioned_as, labels\n FROM http_trigger\n WHERE workspace_id = $2\n AND (workspaced_route IS TRUE OR $3)",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Varchar",
|
||||
"Text"
|
||||
"Text",
|
||||
"Bool"
|
||||
]
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "94901314288c2b37c942b35d88b6de6cfa0f18fc191f1b1d90dfacd4ea8d2f60"
|
||||
"hash": "aee9bf16e37a5361f96d6b35dddffa6cc37346cdb6b38ecbf6e1530eef3a57bd"
|
||||
}
|
||||
@@ -30,6 +30,7 @@ use uuid::Uuid;
|
||||
use windmill_audit::audit_oss::{audit_log, AuditAuthorable};
|
||||
use windmill_audit::ActionKind;
|
||||
use windmill_common::db::UserDB;
|
||||
use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE;
|
||||
use windmill_common::users::username_to_permissioned_as;
|
||||
use windmill_common::variables::{
|
||||
build_crypt, decrypt, encrypt, SECRET_SALT, WORKSPACE_CRYPT_CACHE,
|
||||
@@ -3825,6 +3826,15 @@ async fn clone_triggers_and_schedules(
|
||||
.execute(&mut **tx)
|
||||
.await?;
|
||||
|
||||
// Skip non-workspaced HTTP triggers: their URL has no workspace prefix, so
|
||||
// a clone would collide with the parent's row at runtime (matchit::Router
|
||||
// silently drops one of two duplicates) and `route_path_key_exists` would
|
||||
// also fail to spot the cross-workspace conflict cleanly. The instance
|
||||
// settings `CLOUD_HOSTED` and `HTTP_ROUTE_WORKSPACED_ROUTE` force every
|
||||
// route to be workspace-prefixed regardless of the column, so when either
|
||||
// is on we clone everything.
|
||||
let force_workspaced =
|
||||
*CLOUD_HOSTED || HTTP_ROUTE_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed);
|
||||
sqlx::query!(
|
||||
r#"INSERT INTO http_trigger (
|
||||
path, route_path, route_path_key, script_path, is_flow, workspace_id,
|
||||
@@ -3841,9 +3851,12 @@ async fn clone_triggers_and_schedules(
|
||||
raw_string, authentication_resource_path, summary, description,
|
||||
error_handler_path, error_handler_args, retry, request_type, 'disabled'::TRIGGER_MODE,
|
||||
permissioned_as, labels
|
||||
FROM http_trigger WHERE workspace_id = $2"#,
|
||||
FROM http_trigger
|
||||
WHERE workspace_id = $2
|
||||
AND (workspaced_route IS TRUE OR $3)"#,
|
||||
target_workspace_id,
|
||||
source_workspace_id,
|
||||
force_workspaced,
|
||||
)
|
||||
.execute(&mut **tx)
|
||||
.await?;
|
||||
|
||||
@@ -62,7 +62,8 @@ pub async fn route_path_key_exists(
|
||||
.await?
|
||||
.unwrap_or(false)
|
||||
} else {
|
||||
let http_route_workspaced = HTTP_ROUTE_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed);
|
||||
let http_route_workspaced =
|
||||
HTTP_ROUTE_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed);
|
||||
let effective_workspaced = workspaced_route.unwrap_or(false) || http_route_workspaced;
|
||||
let route_path_key = if effective_workspaced {
|
||||
std::borrow::Cow::Owned(format!("{}/{}", w_id, route_path_key.trim_matches('/')))
|
||||
@@ -70,6 +71,10 @@ pub async fn route_path_key_exists(
|
||||
std::borrow::Cow::Borrowed(route_path_key)
|
||||
};
|
||||
|
||||
// Self-exclusion is by `(workspace_id, path)` not just `path`: workspace
|
||||
// forks clone trigger rows verbatim, so the same trigger path can exist
|
||||
// in multiple workspaces. Excluding by path alone would silently mask a
|
||||
// real collision against the parent's row.
|
||||
sqlx::query_scalar!(
|
||||
r#"
|
||||
SELECT EXISTS(
|
||||
@@ -79,12 +84,13 @@ pub async fn route_path_key_exists(
|
||||
((workspaced_route IS TRUE AND workspace_id || '/' || route_path_key = $1)
|
||||
OR (workspaced_route IS FALSE AND route_path_key = $1))
|
||||
AND http_method = $2
|
||||
AND ($3::TEXT IS NULL OR path != $3)
|
||||
AND ($3::TEXT IS NULL OR NOT (workspace_id = $4 AND path = $3))
|
||||
)
|
||||
"#,
|
||||
&route_path_key,
|
||||
http_method as &HttpMethod,
|
||||
trigger_path
|
||||
trigger_path,
|
||||
w_id
|
||||
)
|
||||
.fetch_one(db)
|
||||
.await?
|
||||
@@ -145,7 +151,8 @@ async fn require_admin_for_instance_wide_route(
|
||||
is_admin: bool,
|
||||
workspaced_route: Option<bool>,
|
||||
) -> Result<bool> {
|
||||
let http_route_workspaced = HTTP_ROUTE_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed);
|
||||
let http_route_workspaced =
|
||||
HTTP_ROUTE_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed);
|
||||
let effective_workspaced = workspaced_route.unwrap_or(false) || http_route_workspaced;
|
||||
if !is_admin && !effective_workspaced {
|
||||
return Err(Error::NotAuthorized(
|
||||
@@ -465,7 +472,8 @@ impl TriggerCrud for HttpTrigger {
|
||||
let resolved_edited_by = trigger.base.resolve_edited_by(authed);
|
||||
let resolved_permissioned_as = trigger.base.resolve_permissioned_as(authed);
|
||||
|
||||
let http_route_workspaced = HTTP_ROUTE_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed);
|
||||
let http_route_workspaced =
|
||||
HTTP_ROUTE_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed);
|
||||
let effective_workspaced =
|
||||
trigger.config.workspaced_route.unwrap_or(false) || http_route_workspaced;
|
||||
|
||||
|
||||
@@ -32,6 +32,18 @@ with two invariants:
|
||||
cloned**. Those triggers manage external webhook state we don't want
|
||||
duplicated.
|
||||
|
||||
**Non-workspaced HTTP triggers are also skipped.** A row with
|
||||
`workspaced_route=false` (and where neither `CLOUD_HOSTED` nor the
|
||||
`HTTP_ROUTE_WORKSPACED_ROUTE` instance setting is on) has a runtime URL
|
||||
without any workspace prefix. A clone would collide with the parent's row at
|
||||
the matchit router level, where duplicate inserts are silently dropped — one
|
||||
trigger would invisibly hijack the other. There is no namespacing escape
|
||||
hatch for these (the whole point of `workspaced_route=false` is to skip the
|
||||
prefix), so the clone filter excludes them. The fork user can re-create one
|
||||
manually if they need it. When `CLOUD_HOSTED` or `HTTP_ROUTE_WORKSPACED_ROUTE`
|
||||
is on, every route is workspace-prefixed at runtime regardless of the column,
|
||||
and the clone copies all rows.
|
||||
|
||||
## Merge-direction filter (always on)
|
||||
|
||||
Whenever the source workspace ID matches `wm-fork-*`, the tarball export at
|
||||
|
||||
Reference in New Issue
Block a user