fix: credit the token owner instead of the token label in the audit trail (#10423)

* fix: credit the token owner instead of the token label in the audit trail Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: address review findings on token-owner audit attribution Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: carry token-label provenance explicitly instead of inferring it Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: point ee-repo-ref at the companion branch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: trust only non-forgeable token labels to name the acting entity Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: reject reserved system-token labels at token creation Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: narrow the token-label guard to server-minted namespaces Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: add the provenance field to the remaining ApiAuthed literals Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: stop trusting the email- label, which no mint produces Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-21 08:02:38 +00:00 · 2026-07-31 11:21:14 +02:00
parent 08827121a9
commit 3716a71fd7
15 changed files with 239 additions and 28 deletions
@@ -1 +1 @@
-aa05ca8e97fc8265cd724753a80db37f83243254
+94d1b4f0a10bfbc1fdc0c3bfd38d31cdae77d89a
@@ -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,
    }
@@ -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,
    }
@@ -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<String>) -> Option<String> {
+/// 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<String>) -> (Option<String>, 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),
    }
 }

@@ -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<uuid::Uuid>,
@@ -54,6 +59,11 @@ pub struct ApiAuthed {
    pub folders: Vec<(String, bool, bool)>,
    pub scopes: Option<Vec<String>>,
    pub username_override: Option<String>,
+    /// 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<String>,
    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<String>) {
+        self.username_override = username_override;
+        self.username_override_is_token_label = false;
    }
 }

@@ -103,6 +128,7 @@ impl From<Authed> 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
@@ -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,
    }
@@ -179,6 +179,9 @@ async fn test_trigger_token_labels_still_creatable(db: Pool<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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(())
+}
@@ -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?;
@@ -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,
    })
@@ -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)),
@@ -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,
        }
@@ -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,
            },
@@ -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 {
@@ -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 @@
 					<span class="font-semibold text-xs text-emphasis">ID</span>
 					<span class="text-xs">{log.id}</span>
 				</div>
+				{#if log.span}
+					{@const isJobSpan = log.span.startsWith(JOB_SPAN_PREFIX)}
+					<div class="flex flex-col gap-1">
+						<span class="font-semibold text-xs text-emphasis">
+							{isJobSpan ? 'Job' : 'Token prefix'}
+						</span>
+						<span class="text-xs break-all">
+							{isJobSpan ? log.span.slice(JOB_SPAN_PREFIX.length) : log.span}
+						</span>
+					</div>
+				{/if}
 				<div class="flex flex-col gap-1">
 					<span class="font-semibold text-xs text-emphasis">Parameters</span>
 					<div class="text-xs p-2 bg-surface-secondary rounded-md">
@@ -205,10 +205,19 @@
 									</div>
 									<div class={showWorkspace ? 'w-2/12 text-xs' : 'w-3/12 text-xs'}>
 										<div class="flex flex-row gap-2 items-center">
-											<div class="whitespace-nowrap overflow-x-auto no-scrollbar max-w-60">
-												{logOrDate.log.username}
+											<!-- end_user can be an arbitrarily long token label; truncate it rather
+											than let it push the username out of the cell. -->
+											<div class="flex flex-row min-w-0 max-w-60 overflow-hidden">
+												<span class="whitespace-nowrap shrink-0" title={logOrDate.log.username}>
+													{logOrDate.log.username}
+												</span>
 												{#if logOrDate.log.parameters && 'end_user' in logOrDate.log.parameters}
-													<span> ({logOrDate.log.parameters.end_user})</span>
+													<span
+														class="truncate pl-1"
+														title={String(logOrDate.log.parameters.end_user)}
+													>
+														({logOrDate.log.parameters.end_user})
+													</span>
 												{/if}
 											</div>
 											<Button