fix(auth): tighten token-owner fallback for unscoped tokens (WIN-1978)

[rubenfiszel]

Fixes WIN-1978

What

The auth cache resolves an API token into an ApiAuthed by reading the token's
owner field and then looking up that owner in the target workspace. When the
owner was u/<username> and the usr lookup returned no row, the fallback
granted operator state:

if let Some(r) = r { (r.is_admin, r.operator) }
else                { (false, true) }   // missing user → operator

That's the wrong default — "I don't recognise this user in this workspace" should
not become "treat them as an operator." This can happen any time an owner
string gets out of sync with workspace membership (stale tokens after a user is
removed, workspace renames, etc.), so the fix is independent of any specific
exploitation path.

The same shape exists in the sibling branches: g/<groupname> silently accepted
any group name as a "group user", and the no-prefix branch granted
is_operator=true from arbitrary owner strings.

Change

backend/windmill-api-auth/src/auth.rs:

• u/<username>: require a matching, non-disabled row in the target workspace's
  usr table (super-admin still bypasses). No row → reject auth (None)
  instead of falling back to operator.
• g/<groupname>: require the named group to exist in the target workspace
  (group_ table).
• Unprefixed / unknown-prefix owner: reject. No legitimate code path creates
  tokens with such owners.

Each rejection logs a warn! so it's visible in the auth audit trail.

Legitimate tokens (owner ∈ {NULL, u/<real_user>, g/<real_group>}) are
unaffected.

Test plan

• cargo check -p windmill-api-auth
• Existing test_permissions_exhaustive still passes against the fix.

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	42a2786
Status:	✅  Deploy successful!
Preview URL:	https://5519d865.windmill.pages.dev
Branch Preview URL:	https://ruben-win-1978-vuln-report-r.windmill.pages.dev

View logs

[claude]

Claude finished @rubenfiszel's task in 5m 12s —— View job

---

Claude PR Review

Verdict: Good to merge.

This is a clean, scoped fix for the unsafe (is_admin=false, is_operator=true) fallback in the unscoped-token / token-owner branch of AuthCache::get_opt_job_authed. The three rejection paths now line up with the only owner shapes any legitimate code path actually emits:

Owner shape	Source in tree	New behavior
u/<username>	users_ee.rs:60 (service-account impersonation), users.rs:713 (rename), native-triggers/src/lib.rs:820 (carried over)	Requires a non-disabled usr row in target workspace; super-admin bypasses (unchanged).
g/<groupname>	native triggers, group-permissioned actions	Requires the group to exist in the target workspace; super-admin bypasses.
No prefix / unknown prefix	none I could find	Rejected.

All legitimate token-creation sites either leave owner NULL (in which case the (_, Some(email), ...) arm at auth.rs:370 handles auth, unaffected by this PR) or set u/<username> / g/<groupname>. So real flows do not regress.

Findings

No P0 / P1 issues. A couple of P2 observations, none worth blocking on:

• auth.rs:300, auth.rs:350 — w_id.as_deref().unwrap_or("") in the warn! is reached only when the match guard already established w_id.is_some() and we have not unwrapped yet, so the unwrap_or("") branch is dead. Cosmetic; current form is harmless.
• auth.rs:284 — On the u/ success path the ApiAuthed.username is still taken from the owner string (name.to_string()), not from the usr row. This is preserved from the previous code, so not a regression, but the usr row's username would be the more authoritative value if you ever want the username to be re-validated and not just the (admin, operator) flags.
• auth.rs:308 — query_scalar!("SELECT EXISTS(...)") returns Option<bool> in sqlx, so .fetch_one(...).await.ok().flatten().unwrap_or(false) is correct. Just noting because it surprised me on first read.

AGENTS.md compliance

• Rust patterns: uses sqlx::query! / query_scalar!, returns Option with a warn! audit trail on rejection — matches the rust-backend skill.
• Validation: cargo check -p windmill-api-auth was listed in the PR; appropriate per docs/validation.md (one crate touched).
• No public-surface check needed — no new pub fn is added; the change is entirely inside AuthCache::get_opt_job_authed.
• Test coverage policy is honored: a backend regression test is added in the right module (windmill-api-integration-tests/tests/permissions.rs), with the same #[ignore] #[cfg(feature = "deno_core")] #[sqlx::test(...)] shape as its siblings.

Test coverage

• Backend — new test_unscoped_token_missing_owner_rejects_cross_workspace covers all three forged-owner shapes (u/missing, g/missing, no-prefix) end-to-end against the real Axum router and asserts 401/403. Solid regression guard against the exact class of forgery the PR addresses.
• Frontend / CI / config — not touched; nothing expected.

Manual verification before merge: nothing strictly required, but worth a smoke pass that legitimate u/<real_user> and g/<real_group> unscoped tokens (e.g. service-account impersonation token from /users/impersonate, native trigger webhook tokens) still authenticate against the workspace they belong to. The integration test exercises the rejection paths, not the legitimate-pass paths in the modified branch.

[github-actions]

Pi Review

Good to merge

Summary: This is a clean, self-contained security fix that closes an auth-bypass gap in the token-owner resolution logic in backend/windmill-api-auth/src/auth.rs. The three branches (u/<name>, g/<name>, no/unrecognized prefix) each receive the correct "validate or reject" treatment instead of an unconditional operator fallback. The new regression test in permissions.rs exercises all three forged-owner shapes and follows the existing #[ignore] convention used by every integration test in that file.

What I verified:

• u/<username>: switched from fetch_one + fallback-to-operator on no-row to fetch_optional + explicit None rejection. Super-admin bypass is preserved (unchanged from prior behavior). Correct.
• g/<groupname>: added an EXISTS check against group_; previously accepted any string as a group identity. Correct.
• No prefix / unknown prefix: both previously granted is_operator=true plus is_admin=super_admin; now unconditionally rejected with a tracing::warn! audit log. Correct.
• unwrap() safety: every unwrap() call is inside a branch gated on w_id.is_some() from the outer match arm, or inside if let Some(lookup) which only executes after the DB round-trip succeeds. No panics on expected paths.
• Test fixture compatibility: the three forged owners (u/candidate1_missing_user, g/candidate1_missing_group, raw_no_prefix) are deliberately absent from the permissions_test.sql fixture, so the rejection paths are genuinely exercised.

No P0, P1, or P2 findings.

Test coverage: The new integration test covers all three rejection paths. It follows the #[ignore] convention in permissions.rs (all integration tests there are #[ignore] + #[cfg(feature = "deno_core")]). The PR body states cargo check passed and the test_permissions_exhaustive test still passes. No additional automated tests are expected for this class of change.

Manual verification: No in-app UI surface to exercise — this is a backend auth-layer change. The integration test, when run with cargo test --features deno_core permissions -- --ignored, provides the definitive verification.

[github-actions]

Codex Review

Good to merge

No issues found. Checked for bugs, security, and AGENTS.md compliance.

Test coverage

Backend: the PR adds an ignored SQLx integration regression covering the missing u/, missing g/, and no-prefix owner cases. That matches the auth-cache behavior changed here.

Manual verification: this is backend auth behavior with no UI surface to exercise. I did not complete a local cargo check; this devbox failed early because clang rejected the configured -fuse-ld=mold linker flag before reaching this crate.

[cubic-dev-ai]

No issues found across 1 file

_{Re-trigger cubic}

[github-actions]

Codex Review

cc @rubenfiszel
Mergeable, but should ideally address nits: missing auth regression coverage

Found 1 issue:

1. [P2] The final diff changes security-sensitive auth resolution for unscoped token owners, including new DB-backed rejection paths for missing u/, missing g/, and unprefixed owners, but the regression test that covered those cases was removed from the PR. Given this is an auth fallback fix, the changed branch should have automated coverage for the rejected forged-owner shapes so the operator fallback cannot silently regress.
   backend/windmill-api-auth/src/auth.rs:247

Test coverage

Backend: the final diff only changes backend/windmill-api-auth/src/auth.rs and does not include any test changes. I would expect a regression test that inserts unscoped tokens with u/<missing>, g/<missing>, and unprefixed owners and verifies workspace requests are rejected.

Manual verification: no UI surface is involved. Before merge, exercise the forged unscoped-token cases above and smoke-test legitimate u/<real_user> and g/<real_group> owner tokens against their workspace.

[github-actions]

Now I have all the context I need. Let me write the review.

Pi Review

cc @rubenfiszel

Mergeable, but should ideally address nits: stale PR body (test claimed but removed), no regression test coverage

Security analysis — fix is correct

The auth fix is sound across all three changed branches in get_opt_job_authed:

• u/<username>: switches from fetch_one + fallback-to-operator to fetch_optional + None rejection. Super-admin bypass is preserved. Correct.
• g/<groupname>: adds EXISTS check against group_ table in the target workspace. Previously accepted any group name. Correct.
• No-prefix / unknown-prefix: now unconditionally rejected with a warn! audit log. Previously both granted operator state. Correct.

All three rejection paths log via tracing::warn!, creating an audit trail. No legitimate token-creation path (service-account impersonation, native triggers, group-permissioned actions) is affected — those all emit u/<real_user> or g/<real_group> owners.

Findings

P2 — PR body is stale; claims test that was removed (backend/windmill-api-integration-tests/tests/permissions.rs)

The PR body's test plan section claims:

New regression test_unscoped_token_missing_owner_rejects_cross_workspace in windmill-api-integration-tests/tests/permissions.rs covers all three forged-owner shapes (u/missing, g/missing, no-prefix) and asserts 401/403.

But commit 42a278604 (chore: drop integration regression for auth fallback) removed that test entirely because it required sqlx offline-cache regeneration. The final diff touches only auth.rs — there is no test file in the diff at all. The PR body wasn't updated to reflect the removal.

P2 — No regression test coverage for the security fix

The integration test was the only automated coverage for the three rejection paths. With it removed, there is zero automated verification that the fix actually rejects tokens with forged owners. While the fix is straightforward to verify by reading the code, security fixes benefit from regression guards. Consider adding a unit-level test in windmill-api-auth (rather than an integration test) that doesn't depend on sqlx offline-cache regeneration, or regenerating the offline cache and restoring the test before merge.

No new public surfaces

No new pub fn or exported component added. The change is entirely within the private AuthCache::get_opt_job_authed method.

Test coverage

• Backend — this PR touches only windmill-api-auth/src/auth.rs. The integration test that was authored alongside the fix was removed in the final commit. No other tests cover the changed rejection paths. The existing test_permissions_exhaustive integration test in permissions.rs exercises the legitimate-pass paths for unscoped tokens (user/group owners that do exist in the workspace), which this PR does not alter — so the existing test suite validates the non-regression property, but not the newly-added rejection paths.

• Frontend / CI / config — not touched.

Manual verification

This is a backend auth-layer change with no in-app UI surface to exercise directly. Before merging, verify with a smoke test that legitimate unscoped tokens (e.g., a service-account impersonation token with owner = u/<real_user>, or a native trigger webhook token with owner = g/<real_group>) still authenticate against their target workspace. The auth logic changed for the fallback-on-missing case only, but confirming the happy path is worth the few minutes it takes.