mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-24 16:01:42 +00:00
hugocasa
9022dc9d44
* fix: confine job tokens to workspace-scoped API routes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep the object-storage connection test reachable from a job token Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep the workspace-exists check the CLI makes reachable from a job Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: reconcile the job-token caps after #10124 The workspace-confinement middleware answers a workspace-less route before the privilege gate behind it runs, so the cases #10124 added on those routes now see 403 rather than 401. Rejection is what they assert, but two of them needed more than a status change: - `list_worker_groups` asserted only that the response body omits the static env value, which an error body satisfies for the wrong reason. It now asserts the status, keeping the secret check as a second assertion. - `require_super_admin` lost its only unshadowed route. `GET /api/w/{workspace}/users/list_addable` is gated solely by that call and names a workspace, so it reaches the gate and pins it at 401. The module doc states the two-layer rule once; the file covers both caps, so it is no longer named for either one alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct the workspaces/exists rationale and the parquet gate note `workspace` carries no row-level security, so `exists_workspace` running through `user_db` does not filter by membership as the comment claimed. State what the route actually discloses — whether a workspace id is taken. The object-storage case explained why a 404 would satisfy the assertion for the wrong reason, which described the earlier `assert_ne!(403)`; against the 422 it now asserts, a 404 fails. Say instead why the case is gated on the feature. * fix: let a job token keep the workspace-less routes that carry no workspace Confinement refused every route outside the allowlist, including ones that answer purely from the caller's own account or from the request body. Those cross no workspace boundary, so refusing them buys nothing: - `users/email` returns a value already inside the token, and `workspaces/allowed_domain_auto_invite` tests the caller's own address against a static list. Neither opens a transaction. - `users/usage` reads the caller's own row; `users/tutorial_progress` reads and upserts a UI bitfield keyed on the same email. - `schedules/preview` takes no `ApiAuthed` at all — it computes the occurrences of the cron expression in the body and returns nothing the caller did not send. The rule, not the list, is what the doc comment states: answers from the caller's own account, the request body, or content identical for every workspace; never naming another workspace, never instance configuration. The candidates it excludes are written down with their reasons, since `users/list_invites` reads as caller-scoped until you notice the response carries a workspace id per invite. Regression covers both directions — the new entries answer, and the rejected caller-scoped reads stay refused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: reach the privilege gates confinement hides `require_devops_role` and `require_instance_admin` gate only workspace-less routes, so confinement answers every request that would reach them: no HTTP case can tell whether they still cap job tokens, and both could lose that check with this suite green. `require_super_admin` has a workspace-scoped route to reach it; these two have none, so call them directly instead. The identity used is the fixture's real superadmin, so the passing half proves the rejection keys off `job_id` rather than off the user. Also separate two claims the write-allowlist doc had merged into one sentence: no entry writes outside the caller's own account, but what each may read differs, and the workspace-existence check answers for any id. * docs: say that the object-storage probe writes The write-allowlist lead claimed no entry writes state outside the caller's own account. `test_s3_bucket` puts an object into the store the body names and deletes it again, so it does write; a failure between the two leaves the object behind. The invariant that holds is about Windmill state. Say so in the lead, and describe the put/delete in the entry itself rather than leaving "acts only on the store the request body describes" to imply a read. * test: cover the last two job-token gates confinement hides Seven guards key on `ApiAuthed::job_id`. Three keep a workspace-scoped route and are exercised over HTTP; the other four are reachable only through workspace-less routes, which confinement now answers first, so nothing observed whether they still cap job tokens. `require_devops_role` and `require_instance_admin` were already called directly. Add the two that were not: `forbid_superadmin_job_token`, and `forbid_elevated_job_token`, whose call sites are `create_token`, `update_token_scopes` and `set_password` — all workspace-less. Both key on two conditions rather than one, so all three combinations are pinned: neither fires without job provenance, and neither fires for an unelevated identity. The second matters — collapsing either into a blanket job-token refusal would stop ordinary users creating tokens, and no other case would catch it. The doc comment records which of the seven each route covers. * docs: correct which job-token gates have no observable route The previous commit put `forbid_elevated_job_token` among the guards reachable only through workspace-less routes, and its message named three call sites. It has six, and two are workspaced: `mint_app_embed_token` and `mint_raw_app_sdk_token`. Its superadmin branch is therefore already exercised over HTTP — the 401 the embed-token case asserts is this gate. So three of the seven lack an observable route, not four. Its direct assertions stay: the embed-token case only ever reaches it with an elevated identity, and the unelevated-negative case is what would catch the gate being collapsed into a blanket job-token refusal. * test: pin is_instance_admin, and stop enumerating gates in prose `is_instance_admin` is `authed.is_admin && authed.job_id.is_none()`, so a census built by searching for `job_id.is_some()` could not see it. Both its call sites are workspace-less, and it returns a bool that selects obfuscation rather than refusing — a job token reading `true` leaks `env_vars_static` instead of being turned away. Pin both directions. The doc comment tried to account for every job-token guard and which route exercised it. It was wrong three times running: the count, the call sites of `forbid_elevated_job_token`, and the claim that the CUSTOM_INSTANCE_DB case covers `is_super_admin_authed` when that path tests `job_id` inline. A table that has to be rederived from six crates to stay true does not belong in a comment, so it now states only why these calls are direct. * docs: name the right is_instance_admin caller The comment credited "the concurrency-group listing" with obfuscating rows. The second caller is `prune_concurrency_group`, which returns PermissionDenied; the obfuscating one is `list_worker_groups`. Keep the claim to that single caller, which is what makes this guard fail by leaking `env_vars_static` rather than by admitting a request. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>