diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index f008a12d4c..4d93d36307 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -aa05ca8e97fc8265cd724753a80db37f83243254 +94d1b4f0a10bfbc1fdc0c3bfd38d31cdae77d89a \ No newline at end of file diff --git a/backend/tests/postgres_trigger_scope.rs b/backend/tests/postgres_trigger_scope.rs index 1172e03ab6..d07435f414 100644 --- a/backend/tests/postgres_trigger_scope.rs +++ b/backend/tests/postgres_trigger_scope.rs @@ -20,6 +20,7 @@ fn scoped_authed(scopes: Vec<&str>) -> ApiAuthed { folders: vec![], scopes: Some(scopes.into_iter().map(str::to_string).collect()), username_override: None, + username_override_is_token_label: false, token_prefix: None, read_only: false, } diff --git a/backend/tests/trigger_listener_queries.rs b/backend/tests/trigger_listener_queries.rs index c3f6357d4c..16b4e6c55e 100644 --- a/backend/tests/trigger_listener_queries.rs +++ b/backend/tests/trigger_listener_queries.rs @@ -173,6 +173,7 @@ fn make_authed() -> windmill_api_auth::ApiAuthed { folders: vec![], scopes: None, username_override: None, + username_override_is_token_label: false, token_prefix: None, read_only: false, } diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 5219b41e9b..988b22790d 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -196,7 +196,8 @@ impl AuthCache { tracing::error!("JWT auth error: workspace_id mismatch"); return None; } - let username_override = username_override_from_label(claims.label); + let (username_override, username_override_is_token_label) = + username_override_from_label(claims.label); let authed = ApiAuthed { email: claims.email, @@ -211,6 +212,7 @@ impl AuthCache { // WM_TOKEN) keeps full user privileges as before. scopes: claims.scopes, username_override, + username_override_is_token_label, token_prefix: claims.audit_span, read_only: false, }; @@ -265,7 +267,8 @@ impl AuthCache { (Some(owner), Some(email), super_admin, _, label, read_only) if w_id.is_some() => { - let username_override = username_override_from_label(label); + let (username_override, username_override_is_token_label) = + username_override_from_label(label); if let Some((prefix, name)) = owner.split_once('/') { if prefix == "u" { let lookup = if super_admin { @@ -308,6 +311,7 @@ impl AuthCache { folders, scopes: None, username_override, + username_override_is_token_label, token_prefix: Some(safe_token_prefix(token)), read_only, }) @@ -358,6 +362,7 @@ impl AuthCache { folders, scopes: None, username_override, + username_override_is_token_label, token_prefix: Some(safe_token_prefix(token)), read_only, }) @@ -386,7 +391,8 @@ impl AuthCache { } } (_, Some(email), super_admin, scopes, label, read_only) => { - let username_override = username_override_from_label(label); + let (username_override, username_override_is_token_label) = + username_override_from_label(label); if w_id.is_some() { let row_o = sqlx::query!( "SELECT username, is_admin, operator FROM usr WHERE @@ -429,6 +435,7 @@ impl AuthCache { folders, scopes, username_override, + username_override_is_token_label, token_prefix: Some(safe_token_prefix(token)), read_only, }) @@ -450,6 +457,7 @@ impl AuthCache { folders: vec![], scopes, username_override, + username_override_is_token_label, token_prefix: Some(safe_token_prefix(token)), read_only, }), @@ -473,6 +481,7 @@ impl AuthCache { folders: Vec::new(), scopes, username_override, + username_override_is_token_label, token_prefix: Some(safe_token_prefix(token)), read_only, }) @@ -508,6 +517,7 @@ impl AuthCache { folders: Vec::new(), scopes: None, username_override: None, + username_override_is_token_label: false, token_prefix: Some(safe_token_prefix(token)), read_only: false, }; @@ -715,6 +725,7 @@ fn no_auth_admin_authed() -> ApiAuthed { folders: Vec::new(), scopes: None, username_override: None, + username_override_is_token_label: false, token_prefix: None, read_only: false, } @@ -835,27 +846,47 @@ pub async fn resolve_opt_job_authed( Err((Error::NotAuthorized("Unauthorized".to_string()), parts)) } -fn username_override_from_label(label: Option) -> Option { +/// Returns the override and whether it names the token's *label* rather than the entity that +/// fired the request. Callers must not re-derive the second element from the first: the +/// `ephemeral-script-end-user-` arm forwards a `created_by` verbatim, and `created_by` is +/// unconstrained, so it may itself look like any of these shapes. +/// +/// Only namespaces `create_token` rejects (`is_server_minted_label`) are trusted to name the +/// entity acting, so the label can only have come from a server-side mint. Tokens minted +/// before that guard existed are the remaining hole; closing it needs the token row to record +/// who minted it rather than inferring it from the label. +/// +/// Note that a trigger whose identity is set server-side — the SMTP one builds an `email-*` +/// override directly — does not rely on this at all, so its prefix must not be trusted here. +pub(crate) fn username_override_from_label(label: Option) -> (Option, bool) { match label { + Some(label) if label.starts_with("ephemeral-webhook-") => (Some(label), false), + Some(label) if label.starts_with("ephemeral-script-end-user-") => ( + Some( + label + .trim_start_matches("ephemeral-script-end-user-") + .to_string(), + ), + false, + ), + // User-mintable, so they name nobody in particular — the trigger panels merely + // pre-fill `webhook-`/`http-`, and the editor mints the lsp one. The override keeps + // its value because `require_job_read_access` matches it against the `created_by` of + // jobs launched under it, which these shapes produced while they were trusted. + Some(label) if label == "Ephemeral lsp token" => (Some("lsp".to_string()), true), Some(label) - if label.starts_with("ephemeral-webhook-") - || label.starts_with("webhook-") + if label.starts_with("webhook-") || label.starts_with("http-") || label.starts_with("email-") || label.starts_with("ws-") => { - Some(label) + (Some(label), true) } - Some(label) if label.starts_with("ephemeral-script-end-user-") => Some( - label - .trim_start_matches("ephemeral-script-end-user-") - .to_string(), + Some(label) if label != "ephemeral-script" && label != "session" && !label.is_empty() => ( + Some(format!("{}{label}", crate::GENERIC_TOKEN_LABEL_PREFIX)), + true, ), - Some(label) if label == "Ephemeral lsp token" => Some("lsp".to_string()), - Some(label) if label != "ephemeral-script" && label != "session" && !label.is_empty() => { - Some(format!("label-{label}")) - } - _ => None, + _ => (None, false), } } diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 8120c16605..4161710aaf 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -37,6 +37,11 @@ pub use auth::{ // ------------ ApiAuthed & OptJobAuthed types ------------ +/// Prefix `username_override_from_label` puts on the label of a generic user token. The +/// override keeps this form even though `display_username` skips it: `require_job_read_access` +/// matches it against `created_by` to let a token re-read the jobs it launched. +pub const GENERIC_TOKEN_LABEL_PREFIX: &str = "label-"; + #[derive(Default, Clone, Debug)] pub struct OptJobAuthed { pub job_id: Option, @@ -54,6 +59,11 @@ pub struct ApiAuthed { pub folders: Vec<(String, bool, bool)>, pub scopes: Option>, pub username_override: Option, + /// Whether `username_override` is a generic user-token label rather than a name that + /// identifies the requester. It cannot be recovered from the value: the ephemeral + /// end-user override passes a `created_by` through verbatim, and that may itself be a + /// `label-*` string. Only `username_override_from_label` sets it. + pub username_override_is_token_label: bool, pub token_prefix: Option, pub read_only: bool, } @@ -72,8 +82,23 @@ impl ApiAuthed { } } + /// The name a run triggered by this principal is credited to (`v2_job.created_by`, and + /// the audit `end_user`). A trigger-token override names the entity that fired the + /// request and wins; a generic token label does not, so the token owner is credited and + /// stays traceable even when `permissioned_as` is an on-behalf-of identity. pub fn display_username(&self) -> &str { - self.username_override.as_ref().unwrap_or(&self.username) + match self.username_override.as_deref() { + Some(o) if !self.username_override_is_token_label => o, + _ => &self.username, + } + } + + /// Set an override that names the entity acting, e.g. a trigger. Assigning + /// `username_override` on its own would keep the provenance flag of whatever this authed + /// was built from, and a stale `true` makes `display_username` ignore the new value. + pub fn set_acting_username_override(&mut self, username_override: Option) { + self.username_override = username_override; + self.username_override_is_token_label = false; } } @@ -103,6 +128,7 @@ impl From for ApiAuthed { folders: value.folders, scopes: value.scopes, username_override: None, + username_override_is_token_label: false, token_prefix: value.token_prefix, read_only: false, } @@ -852,6 +878,7 @@ pub async fn fetch_api_authed_from_permissioned_as( folders: authed.folders, scopes: authed.scopes, username_override: None, + username_override_is_token_label: false, token_prefix: authed.token_prefix, read_only: false, }; @@ -869,7 +896,8 @@ pub async fn fetch_api_authed_from_permissioned_as( } }; - api_authed.username_override = username_override; + // Callers pass a trigger or app identity here, never a token label. + api_authed.set_acting_username_override(username_override); Ok(api_authed) } @@ -1194,6 +1222,64 @@ mod tests { } } + /// `display_username` is what `push` credits a run to, so a token label standing in for + /// it erases the caller from `created_by` and from the audit trail — irrecoverably when + /// `permissioned_as` is an on-behalf-of identity that also takes the `username` slot. + #[test] + fn generic_token_label_credits_the_token_owner() { + let owner_of = |label: &str| { + let (username_override, username_override_is_token_label) = + auth::username_override_from_label(Some(label.to_string())); + ApiAuthed { + username: "alice".into(), + username_override, + username_override_is_token_label, + ..Default::default() + } + }; + + // Arbitrary user-chosen labels, and the auto-generated MCP OAuth one. + assert_eq!(owner_of("my-personal-token").display_username(), "alice"); + assert_eq!( + owner_of("mcp-oauth-mcp-client-9f3a1c").display_username(), + "alice" + ); + + // A trigger-*shaped* label is just as user-settable as any other, so it is credited + // the same way. Its value is still kept as the override, for `require_job_read_access`. + let webhookish = owner_of("webhook-f/svc/my_script"); + assert_eq!(webhookish.display_username(), "alice"); + assert_eq!( + webhookish.username_override.as_deref(), + Some("webhook-f/svc/my_script") + ); + + // Only labels `create_token` refuses to mint name the entity that fired the request. + assert_eq!( + owner_of("ephemeral-webhook-google-abc12").display_username(), + "ephemeral-webhook-google-abc12" + ); + + // Minted by the editor through the public handler, so it names no principal either. + assert_eq!(owner_of("Ephemeral lsp token").display_username(), "alice"); + + // The SMTP trigger sets its `email-*` identity server-side rather than through a + // label, so a token carrying that prefix is just a user token. + assert_eq!(owner_of("email-f/team/inbox").display_username(), "alice"); + assert_eq!( + owner_of("ephemeral-script-end-user-enduser42").display_username(), + "enduser42" + ); + + // The end-user token forwards a `created_by` verbatim, and `created_by` is not + // constrained to a username — a job launched before the owner was credited still + // carries `label-*`. That is an end user, not this token's label, so it stands. + assert_eq!( + owner_of("ephemeral-script-end-user-label-alice").display_username(), + "label-alice" + ); + } + // Regression tests for the Preview path traversal: a Preview's path skips the // DB `proper_id` CHECK and reaches the worker, where it builds on-disk module // dirs. Traversal must be rejected even for admins, who otherwise bypass the diff --git a/backend/windmill-api-integration-tests/tests/native_triggers.rs b/backend/windmill-api-integration-tests/tests/native_triggers.rs index 1d809ae526..6249105ff6 100644 --- a/backend/windmill-api-integration-tests/tests/native_triggers.rs +++ b/backend/windmill-api-integration-tests/tests/native_triggers.rs @@ -50,6 +50,7 @@ fn test_authed() -> ApiAuthed { folders: vec![], scopes: None, username_override: None, + username_override_is_token_label: false, token_prefix: None, read_only: false, } diff --git a/backend/windmill-api-integration-tests/tests/token_label_idor.rs b/backend/windmill-api-integration-tests/tests/token_label_idor.rs index 92dada92c2..ddec1a481a 100644 --- a/backend/windmill-api-integration-tests/tests/token_label_idor.rs +++ b/backend/windmill-api-integration-tests/tests/token_label_idor.rs @@ -179,6 +179,9 @@ async fn test_trigger_token_labels_still_creatable(db: Pool) -> anyhow "http-test-user-2-cd34", "email-test-user-2-ef56", "my-ci-token", + // Minted client-side by the editor (every TypeScript editor load) and the debugger. + "Ephemeral lsp token", + "debugger-token", ] { let resp = create_token_with_label(port, "SECRET_TOKEN_2", label).await; assert_eq!( @@ -190,3 +193,31 @@ async fn test_trigger_token_labels_still_creatable(db: Pool) -> anyhow Ok(()) } + +/// The mirror of the above: reserved namespaces must NOT be mintable. `username_override_from_label` +/// trusts these shapes to name the entity acting, so a forged one would stamp an arbitrary +/// name onto `v2_job.created_by` and the audit `end_user` — on an `on_behalf_of` runnable, +/// which also takes the `username`/`email` columns, that leaves no trace of the real caller. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_reserved_token_labels_not_creatable(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + for label in [ + "ephemeral-webhook-forged", + "ephemeral-script-end-user-svcaccount", + "ephemeral-script", + "session", + "mcp-oauth-forged", + ] { + let resp = create_token_with_label(port, "SECRET_TOKEN_2", label).await; + assert_eq!( + resp.status(), + 400, + "creating a token with reserved label {label:?} must be rejected" + ); + } + + Ok(()) +} diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index f78a9e85ff..bb7e129b28 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2764,6 +2764,21 @@ async fn create_token( forbid_superadmin_job_token(&db, &authed.email, job_id).await?; check_token_create_rate_limit(&authed.username)?; + // `username_override_from_label` trusts a server-minted label to name the entity acting, + // so a forged one would put an arbitrary name in `created_by` and the audit trail. + // Deliberately narrower than the `is_user_token` guard on relabelling: the editor and the + // debugger mint their own tokens through this handler. Server-side mints bypass it by + // calling `create_token_internal` / `create_token_for_owner` directly. + if token_config + .label + .as_deref() + .is_some_and(windmill_common::auth::is_server_minted_label) + { + return Err(Error::BadRequest( + "label collides with a reserved system-token namespace".to_string(), + )); + } + windmill_api_auth::ensure_scopes_within_caller(&authed, token_config.scopes.as_deref())?; let mut tx = db.begin().await?; diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 6e5e9cecc0..6a133600a1 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -9746,6 +9746,7 @@ async fn load_workspace_authed( folders: vec![], scopes: base_authed.scopes.clone(), username_override: base_authed.username_override.clone(), + username_override_is_token_label: base_authed.username_override_is_token_label, token_prefix: base_authed.token_prefix.clone(), read_only: base_authed.read_only, }); @@ -9775,6 +9776,7 @@ async fn load_workspace_authed( folders, scopes: base_authed.scopes.clone(), username_override: base_authed.username_override.clone(), + username_override_is_token_label: base_authed.username_override_is_token_label, token_prefix: base_authed.token_prefix.clone(), read_only: base_authed.read_only, }) diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index aa320c4167..ab711ae1d2 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -4497,9 +4497,9 @@ async fn build_args( if arg_str.starts_with("\"$ctx:") { let prop = arg_str.trim_start_matches("\"$ctx:").trim_end_matches("\""); let value = match prop { - "username" => authed.as_ref().map(|a| { - serde_json::to_value(a.username_override.as_ref().unwrap_or(&a.username)) - }), + "username" => authed + .as_ref() + .map(|a| serde_json::to_value(a.display_username())), "email" => authed.as_ref().map(|a| serde_json::to_value(&a.email)), "workspace" => Some(serde_json::to_value(&w_id)), "groups" => authed.as_ref().map(|a| serde_json::to_value(&a.groups)), diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 1bd8ec536f..cc37ed097a 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -1103,9 +1103,10 @@ async fn require_job_read_access( // identity, i.e. its `permissioned_as_email` (the token owner's email, never set from // the label) equals `authed.email`. This still admits every legitimate same-owner // re-read (trigger tokens reading their own webhook/http/email jobs, the - // ephemeral-script-end-user worker token, generic labeled tokens) while denying - // cross-principal collisions. The DB hit only happens when an override is present and - // matches, so the common session/token path stays query-free. + // ephemeral-script-end-user worker token, and jobs whose stored `created_by` is a + // `label-*` override) while denying cross-principal collisions. The DB hit only happens + // when an override is present and matches, so the common session/token path stays + // query-free. if authed .username_override .as_deref() @@ -10938,6 +10939,7 @@ mod approval_view_gate_tests { folders: vec![], scopes: None, username_override: None, + username_override_is_token_label: false, token_prefix: None, read_only: false, } diff --git a/backend/windmill-api/src/lib.rs b/backend/windmill-api/src/lib.rs index f4d30cbd77..f1c7ad4909 100644 --- a/backend/windmill-api/src/lib.rs +++ b/backend/windmill-api/src/lib.rs @@ -328,6 +328,7 @@ async fn inject_agent_authed( folders: Vec::new(), scopes: None, username_override: None, + username_override_is_token_label: false, token_prefix: None, read_only: false, }, diff --git a/backend/windmill-common/src/auth.rs b/backend/windmill-common/src/auth.rs index cbaa392cd4..736190295e 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -43,6 +43,22 @@ pub fn is_user_token(label: Option<&str>) -> bool { } } +/// Whether `label` belongs to a namespace only the server mints, and which therefore must be +/// rejected by `create_token`. Narrower than [`is_user_token`], which also drives label +/// editability and expiry notifications and can afford to reserve more: `Ephemeral lsp token` +/// and `debugger-token` are minted by the editor and the debugger through that same handler, +/// so reserving them would break those features. +/// +/// `username_override_from_label` trusts a label to name the entity acting only if it is in +/// here, so anything added must be unmintable by a member. +pub fn is_server_minted_label(label: &str) -> bool { + label.starts_with("ephemeral-webhook-") + || label.starts_with("ephemeral-script-end-user-") + || label == "ephemeral-script" + || label == "session" + || label.starts_with("mcp-oauth-") +} + /// Hash a raw token using SHA-256 (hex-encoded, 64 chars). /// Used to store and look up tokens without keeping plaintext in the DB. pub fn hash_token(token: &str) -> String { diff --git a/frontend/src/lib/components/auditLogs/AuditLogDetails.svelte b/frontend/src/lib/components/auditLogs/AuditLogDetails.svelte index ed02de2dda..392018750d 100644 --- a/frontend/src/lib/components/auditLogs/AuditLogDetails.svelte +++ b/frontend/src/lib/components/auditLogs/AuditLogDetails.svelte @@ -10,6 +10,10 @@ let { logs, selectedId = undefined }: Props = $props() + // `span` holds the caller's token prefix, except for job-minted worker tokens, which + // stamp the job they run for instead. + const JOB_SPAN_PREFIX = 'job-span-' + const ViewFlowOp: AuditLog['operation'][] = ['jobs.run.flow', 'flows.create', 'flows.update'] const ViewAppOp: AuditLog['operation'][] = ['apps.create', 'apps.update'] @@ -24,6 +28,17 @@ ID {log.id} + {#if log.span} + {@const isJobSpan = log.span.startsWith(JOB_SPAN_PREFIX)} +
+ + {isJobSpan ? 'Job' : 'Token prefix'} + + + {isJobSpan ? log.span.slice(JOB_SPAN_PREFIX.length) : log.span} + +
+ {/if}
Parameters
diff --git a/frontend/src/lib/components/auditLogs/AuditLogsTable.svelte b/frontend/src/lib/components/auditLogs/AuditLogsTable.svelte index 5ad2cb2065..b0f5b7256a 100644 --- a/frontend/src/lib/components/auditLogs/AuditLogsTable.svelte +++ b/frontend/src/lib/components/auditLogs/AuditLogsTable.svelte @@ -205,10 +205,19 @@
-
- {logOrDate.log.username} + +
+ + {logOrDate.log.username} + {#if logOrDate.log.parameters && 'end_user' in logOrDate.log.parameters} - ({logOrDate.log.parameters.end_user}) + + ({logOrDate.log.parameters.end_user}) + {/if}