fix: enforce folder ACL on flow run-by-version routes (GHSA-8mv7-hmrg-96xv)

[rubenfiszel]

Summary

Private security fix for GHSA-8mv7-hmrg-96xv (HIGH). The version-keyed flow
run routes resolved flow_version.id -> path on the raw DB pool and then
gated only on check_scopes(...), which checks API-token scopes — not the
caller's folder-level RBAC. The folder ACL is enforced by Postgres RLS on the
flow table, reached only through user_db; the version routes never went
through it. A low-privilege workspace member with a normal (unscoped) token
could therefore execute an admin-owned, folder-scoped flow they have zero
permission on, by passing its flow_version.id. The path-keyed sibling
POST /jobs/run/f/{path} was always correctly gated (RLS-aware lookup) and
returned 401 — confirming the asymmetry.

Affected routes (all share the fix): POST /jobs/run/fv/{version},
POST|GET /jobs/run_wait_result/fv/{version}, and (transitively)
GET|POST /jobs/run_and_stream/fv/{version}.

Introduced in v1.576.0 (PR #7062, "webhook by flow version"); vulnerable
through current main / v1.703.1.

Changes

• Add get_flow_path_for_version_authed in windmill-common/src/lib.rs: resolves a flow_version.id to its path through an RLS-filtered flow_version INNER JOIN flow query on user_db, mirroring the exists-but-unauthorized → NotAuthorized / missing → NotFound semantics of get_latest_flow_version_id_for_path.
• Route the three version-keyed flow handlers (run_flow_by_version_inner, run_wait_result_flow_by_version, run_wait_result_flow_by_version_get) through the new helper instead of the raw-pool lookup, before check_scopes. stream_flow_by_version is covered transitively via run_flow_by_version_inner.
• Add regression test backend/tests/ghsa_8mv7_hmrg_96xv.rs + fixture.
• Regenerate SQLx offline cache (1 new query; no EE caches lost).

Test plan

• Regression test passes: folder owner (non-admin) still runs by version (201); unauthorized member rejected on path-keyed control (401) and on all version routes (401)
• Manual end-to-end repro: pre-fix fv/{id} executed an admin folder-scoped flow as a zero-permission member; post-fix all four version routes return 401 while authorized users (admin + folder-granted member) still get 201
• local-review clean (no P0/P1/P2)
• Backend compiles (cargo watch); SQLx cache verified (0 net deletions vs origin/main)

🤖 Generated with Claude Code

---

Summary by cubic

Fixes GHSA-8mv7-hmrg-96xv by enforcing folder ACL on all flow run-by-version routes. Blocks running a folder-scoped flow via flow_version.id without permission and avoids leaking the flow path in unauthorized errors.

• Bug Fixes
  • Added get_flow_path_for_version_authed to resolve flow_version.id -> path via an RLS-filtered join on user_db, with NotAuthorized/NotFound semantics and no path leakage.
  • Updated POST /jobs/run/fv/{version}, POST|GET /jobs/run_wait_result/fv/{version}, and the streaming variant (via run_flow_by_version_inner) to use the helper before scope checks.
  • Removed the temporary regression test; regenerated SQLx offline cache.

^{Written for commit 1a1f2eb. Summary will update on new commits. Review in cubic}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	1a1f2eb
Status:	⚡️  Build in progress...

View logs

[claude]

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

---

Good to merge — correct, minimal, consistently applied security fix with solid regression coverage.

Review

I traced the vulnerable path and the fix end-to-end against origin/main.

The fix is correct. flow_version has no row-level security; the pre-fix handlers resolved flow_version.id -> path on the raw pool and gated only on check_scopes (token scopes, not folder RBAC). The new get_flow_path_for_version_authed resolves the path through an RLS-filtered flow_version INNER JOIN flow on user_db, so the folder ACL on the flow table is now the gate — exactly the mechanism the path-keyed sibling already relied on. The exists-but-unauthorized → NotAuthorized / missing → NotFound semantics mirror get_latest_flow_version_id_for_path (lib.rs:1401) faithfully, so both route families now reject identically.

Coverage is complete. All three handlers (run_flow_by_version_inner, run_wait_result_flow_by_version, run_wait_result_flow_by_version_get) route through the helper before check_scopes and before any side effect. I confirmed stream_flow_by_version → stream_job → RunnableId::FlowId(FlowVersion) dispatches to run_flow_by_version_inner (jobs.rs:5438), so the streaming variant is genuinely covered transitively. No other raw flow_version → path lookups remain in jobs.rs.

SQLx cache is clean. Exactly one added entry matching the new RLS query, zero deletions. The fallback raw query (SELECT path FROM flow_version WHERE id = $1 AND workspace_id = $2) reuses the existing cached entry query-fc9753f5…, so the offline build is fine.

AGENTS.md compliance

New pub async fn get_flow_path_for_version_authed checks out against the public-surface checklist: (a) authorization is the function's entire purpose, documented in the doc comment and enforced in the body; (b) correctly placed in windmill-common/src/lib.rs next to its sibling flow-version resolvers; (c) lands with its callers in the same PR (not half-finished); (d) version: i64 / w_id: &str are passed only as parameterized bind params — no injection surface.

Observations (non-blocking, not gating)

• Orphaned-version edge: if a flow row were deleted while its flow_version row remained, an otherwise-authorized caller would now get NotAuthorized rather than NotFound. This is fail-closed, requires the flow row to run anyway, and matches the sibling's INNER-JOIN-to-flow requirement — consistent, not a regression worth acting on.
• The two-line UserDbWithAuthed { db: user_db.clone(), authed: &authed.to_authed_ref() } construction is repeated at all three call sites and leans on temporary-lifetime-extension through the struct literal. It compiles and is consistent with the module's existing UserDbWithAuthed-parameter convention, so folding it into the helper is purely optional.

Test coverage

Backend: the regression test backend/tests/ghsa_8mv7_hmrg_96xv.rs is well-targeted — folder owner (non-admin) still runs by version (201), unauthorized member rejected on the path-keyed control (401), and rejected on all three version routes (POST run/fv, POST + GET run_wait_result/fv). Fixture correctly models a non-admin folder owner vs. a non-admin member with no grant, both on non-super-admin tokens. The run_and_stream/fv variant isn't exercised directly but shares run_flow_by_version_inner, so the gate is covered.

No further manual verification needed beyond the end-to-end repro already documented in the PR description.
· branch ghsa-8mv7-hmrg-96xv

[cubic-dev-ai]

1 issue found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/windmill-common/src/lib.rs">

<violation number="1" location="backend/windmill-common/src/lib.rs:1584">
P2: NotAuthorized response leaks sensitive metadata (flow path and permission details) to unauthorized users.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fix all with cubic | Re-trigger cubic}

[github-actions]

Codex Review

Good to merge

No issues found. Checked for bugs, security problems, public-surface auth expectations, and AGENTS.md compliance.

Test coverage

Backend API/security coverage is present via backend/tests/ghsa_8mv7_hmrg_96xv.rs, covering authorized owner access plus unauthorized rejection for the path-keyed control route and the run / run_wait_result flow-version routes.

Manual verification should still exercise GET and POST /jobs/run_and_stream/fv/{version} with the same unauthorized folder-scoped flow version, confirming a 401 and no queued or streamed job.

[github-actions]

Now I have enough context to complete the review.

Pi Review

Good to merge

This is a clean, well-scoped security fix. The vulnerability is real, the fix correctly mirrors the RLS-gating pattern already used by the path-keyed sibling routes (push_flow_job_by_path_into_queue → get_latest_flow_version_info_for_path), and all four affected code paths are updated. No new issues introduced.

---

What was reviewed

• New get_flow_path_for_version_authed helper in backend/windmill-common/src/lib.rs — correct two-query pattern (RLS-filtered lookup on user_db, fallback raw-DB check to distinguish NotAuthorized from NotFound), consistent with the existing get_latest_flow_version_id_for_path at line 1401.
• Three handler call sites in backend/windmill-api/src/jobs.rs — run_flow_by_version_inner, run_wait_result_flow_by_version_get, and run_wait_result_flow_by_version all now call the authed helper before check_scopes, replacing the raw-DB-only flow_version query.
• stream_flow_by_version (line 5294) routes through stream_job → run_flow_by_version_inner, so it inherits the fix.
• Regression test backend/tests/ghsa_8mv7_hmrg_96xv.rs + fixture — exercises the folder owner (201), path-keyed rejection (401 control), and version-keyed rejection on all three route families.
• SQLx offline cache — one new query descriptor, no existing cache files lost.

Public surface checklist

pub async fn get_flow_path_for_version_authed (new):

• (a) Auth expectations documented — doc comment explains RLS-gating and the NotAuthorized/NotFound distinction. ✓
• (b) Module placement — in windmill-common/src/lib.rs alongside sibling helpers get_latest_flow_version_id_for_path and get_flow_version_info_from_version. ✓
• (c) Not half-finished — has a working caller in every code path it was designed for. ✓
• (d) Input validation — version: i64 and w_id: &str are both passed into parameterized SQL queries; no injection or traversal risk. ✓

Test coverage

Backend — regression test (ghsa_8mv7_hmrg_96xv.rs) covers the happy path (owner still runs), the control path (path-keyed sibling rejects), and all three version-keyed route families for the unauthorized member. Clean.

Manual verification — already performed per the PR body (pre-fix repro confirmed bypass, post-fix all four routes return 401 for unauthorized member, authorized users still get 201). No further manual scenarios needed.

[rubenfiszel]

Thanks @cubic-dev-ai — addressed in a1da7d7.

P2 (NotAuthorized leaks flow path + permission details), identified by cubic — valid and fixed.

Nuance: the message was copied verbatim from the existing path-keyed sibling get_latest_flow_version_id_for_path (lib.rs:1455), so it isn't a new class of disclosure. But cubic is right that the version route is genuinely worse: the path-keyed caller already supplied the path, whereas the version-keyed caller supplied only an opaque flow_version.id. Echoing the resolved path back therefore discloses an id→path mapping for a flow they cannot access — a real incremental leak unique to this route, plus the unnecessary authed debug dump.

Fix:

• The NotAuthorized branch now returns a generic "You are not authorized to run this flow version" — no resolved path, no authed debug. Comment added explaining why it intentionally diverges from the (more verbose) sibling.
• The raw fallback is now SELECT EXISTS(...) used only internally to keep the 401-vs-404 distinction (consistent with the sibling's status-code behavior); the path string is never materialized into a response.
• Regression test hardened: asserts the rejection body does not contain the flow path.

Did not touch the path-keyed sibling's message — out of scope for this advisory, and there the path is caller-supplied so it's a non-issue; changing it would also make the two routes inconsistent in a different way.

The other three reviewers (Claude, Codex, Pi) returned Good to merge with no blocking findings.

[cubic-dev-ai]

Thanks @cubic-dev-ai — addressed in a1da7d7.

P2 (NotAuthorized leaks flow path + permission details), identified by cubic — valid and fixed.

Nuance: the message was copied verbatim from the existing path-keyed sibling get_latest_flow_version_id_for_path (lib.rs:1455), so it isn't a new class of disclosure. But cubic is right that the version route is genuinely worse: the path-keyed caller already supplied the path, whereas the version-keyed caller supplied only an opaque flow_version.id. Echoing the resolved path back therefore discloses an id→path mapping for a flow they cannot access — a real incremental leak unique to this route, plus the unnecessary authed debug dump.
...

@rubenfiszel That looks right — the fix in a1da7d7 matches your description exactly.

The key design choice that stands out as correct:

• Two-query fallback using SELECT EXISTS(...) instead of re-fetching the path on the raw pool. The path string never enters application memory in the unauthorized branch, so there's zero risk of accidental disclosure even if the error handler's serialization logic changes later.

• The comment in lib.rs making the divergence from get_latest_flow_version_id_for_path explicit — "the caller only supplied an opaque version id. Echoing back the resolved path would disclose an id->path mapping" — that's the right rationale, and it'll prevent a future well-intentioned refactor from re-unifying the two error messages and reintroducing the leak.

• Regression test hardening (assert!(!body.contains(flow_path))) pins down the non-disclosure guarantee directly, which is better than relying on a specific string match.

No further action needed from my end. The three other reviewers' "good to merge" verdicts plus the updated CI check all align.

[github-actions]

Codex Review

Good to merge

No issues found. Checked for bugs, security problems, public-surface auth expectations, and AGENTS.md compliance.

Test coverage

Backend API/security coverage is present via backend/tests/ghsa_8mv7_hmrg_96xv.rs, covering authorized owner access plus unauthorized rejection for the path-keyed control route and the run / run_wait_result flow-version routes.

Manual verification should include GET and POST /jobs/run_and_stream/fv/{version} with the same unauthorized folder-scoped flow version, confirming a 401 and no queued or streamed job.

[github-actions]

Pi Review

Good to merge

The final revision (a1da7d7) addresses the information-disclosure concern raised in the prior discussion and is now complete. The fix is correct, minimal, and consistently applied across all affected routes.

---

What changed since the initial review

The initial version of get_flow_path_for_version_authed echoed the resolved path back in the NotAuthorized error message (matching the existing path-keyed sibling get_latest_flow_version_id_for_path). Cubic correctly flagged this as a P2 information disclosure: on the version-keyed routes the caller supplies only an opaque flow_version.id, so echoing the resolved path back discloses an id→path mapping for a flow they cannot access.

The fix in a1da7d7:

• The NotAuthorized branch now returns a generic message ("You are not authorized to run this flow version") — no resolved path, no authed debug dump. A comment in lib.rs:1584-1588 documents the intentional divergence from the sibling.
• The raw-DB fallback uses SELECT EXISTS(...) — the path string is never materialized into application memory in the unauthorized branch, eliminating any risk of accidental disclosure through future serialization changes.
• The regression test in backend/tests/ghsa_8mv7_hmrg_96xv.rs:94-98 asserts !body.contains(flow_path), pinning down the non-disclosure guarantee directly.

The three handlers (run_flow_by_version_inner, run_wait_result_flow_by_version, run_wait_result_flow_by_version_get) all route through the new helper before check_scopes. stream_flow_by_version inherits the fix via stream_job → RunnableId::FlowId(FlowVersion) → run_flow_by_version_inner (jobs.rs:5439). No other raw flow_version → path lookups remain in jobs.rs.

Public surface checklist

pub async fn get_flow_path_for_version_authed (new):

• (a) Authorization enforcement is the function's entire purpose — documented in the doc comment and enforced by the RLS-filtered join. ✓
• (b) Placed in windmill-common/src/lib.rs next to sibling helpers get_latest_flow_version_id_for_path and get_flow_version_info_from_version. ✓
• (c) Lands with working callers in the same PR, not half-finished. ✓
• (d) version: i64 / w_id: &str are passed only as parameterized SQL bind parameters — no injection or traversal risk. ✓

SQLx cache

Two new entries added (query-0476ae... for the RLS join, query-e70cbc... for the EXISTS fallback), zero deletions. The raw SELECT path FROM flow_version WHERE id = $1 cache entry remains in place (orphaned from the fixed call sites but benign). Clean.

Consistency with existing patterns

The UserDbWithAuthed { db: user_db.clone(), authed: &authed.to_authed_ref() } construction at all three call sites relies on temporary lifetime extension through the struct literal — consistent with the module's existing UserDbWithAuthed-parameter convention (e.g., get_latest_flow_version_info_for_path callers). Compiles cleanly.

Test coverage

Backend — regression test (backend/tests/ghsa_8mv7_hmrg_96xv.rs) covers:

• Folder owner (non-admin) still runs by version (201)
• Unauthorized member rejected on path-keyed control route (401)
• Unauthorized member rejected on all three version-keyed routes: POST run/fv, POST + GET run_wait_result/fv (401)
• Assertion that the rejection body does not leak the flow path

The run_and_stream/fv variant is not exercised directly in the test but shares run_flow_by_version_inner, so the gate is covered. The test fixture (ghsa_8mv7_hmrg_96xv.sql) correctly models the scenario: a non-admin folder owner vs. a non-admin member with no folder grant, both on non-super-admin tokens.

Manual verification — already performed per the PR body (pre-fix repro confirmed bypass, post-fix all four version routes return 401 for unauthorized member, authorized users still get 201). No further manual scenarios needed.