mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-04 16:03:06 +00:00
fix: cap the admin-or-devops gate at workspace admin for job tokens
require_admin_or_devops (the EE critical-alerts endpoints) grants when the caller is a workspace admin OR an instance devops. is_devops_email is true for superadmins, so a WM_TOKEN running on-behalf of a superadmin who is not a member of the target workspace could clear the devops branch and read/ack that workspace's critical alerts (GHSA-hfh4-cx4h-3fcr). This gate takes a bare email, not an ApiAuthed, so the token-layer cap could not see it. Thread the caller's job-token provenance and reject the devops branch for job tokens, matching require_devops_role. The workspace-admin branch stays allowed — that is the cap ceiling. Adds an enterprise-gated regression proving the bypass is closed and a real superadmin token still clears the gate. Found while auditing the PR for bare-email gates the choke-point cap misses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -364,3 +364,51 @@ async fn test_wm_token_rejected_by_require_devops_role(db: Pool<Postgres>) -> an
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A job token must not clear an *admin-or-devops* gate via the devops branch.
|
||||
/// `require_admin_or_devops` (the EE critical-alerts endpoints) grants when the
|
||||
/// caller is a workspace admin OR an instance `devops`; since `is_devops_email`
|
||||
/// is true for superadmins, a WM_TOKEN running on-behalf of a superadmin who is
|
||||
/// NOT a member of the target workspace would otherwise gain workspace-scoped
|
||||
/// devops access to a workspace it has no admin rights in (GHSA-hfh4-cx4h-3fcr).
|
||||
/// The workspace-admin branch stays allowed — that is the cap ceiling.
|
||||
#[cfg(feature = "enterprise")]
|
||||
#[sqlx::test(fixtures("preserve_on_behalf_of"))]
|
||||
async fn test_wm_token_rejected_by_admin_or_devops_gate(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
set_jwt_secret().await;
|
||||
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
let base = format!("http://localhost:{port}/api/w/test-workspace/workspaces");
|
||||
|
||||
// superadmin-external is a superadmin but not a member of test-workspace, so
|
||||
// its workspace-level is_admin is false — the exact exploit precondition.
|
||||
let sa_wm = wm_token("superadmin-external@windmill.dev", false).await;
|
||||
let resp = authed(client().get(format!("{base}/critical_alerts")), &sa_wm)
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
403,
|
||||
"superadmin WM_TOKEN must not clear the admin-or-devops gate on a workspace it isn't admin of: {}",
|
||||
resp.text().await?
|
||||
);
|
||||
|
||||
// No false positive: the same superadmin's real API token (not a job token)
|
||||
// still clears the gate via the devops branch.
|
||||
let resp = authed(
|
||||
client().get(format!("{base}/critical_alerts")),
|
||||
"EXTERNAL_SUPERADMIN_TOKEN",
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
assert_ne!(
|
||||
resp.status(),
|
||||
403,
|
||||
"a real superadmin token must still clear the admin-or-devops gate: {}",
|
||||
resp.text().await?
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -214,7 +214,7 @@ pub async fn get_critical_alerts(
|
||||
authed: ApiAuthed,
|
||||
Query(params): Query<crate::utils::AlertQueryParams>,
|
||||
) -> JsonResult<serde_json::Value> {
|
||||
require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, &db).await?;
|
||||
require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, authed.job_id.is_some(), &db).await?;
|
||||
|
||||
crate::utils::get_critical_alerts(db, params, Some(w_id)).await
|
||||
}
|
||||
@@ -230,7 +230,7 @@ pub async fn acknowledge_critical_alert(
|
||||
Path((w_id, id)): Path<(String, i32)>,
|
||||
authed: ApiAuthed,
|
||||
) -> Result<String> {
|
||||
require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, &db).await?;
|
||||
require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, authed.job_id.is_some(), &db).await?;
|
||||
crate::utils::acknowledge_critical_alert(db, Some(w_id), id).await
|
||||
}
|
||||
|
||||
|
||||
@@ -246,10 +246,16 @@ pub async fn require_admin_or_devops(
|
||||
is_admin: bool,
|
||||
username: &str,
|
||||
email: &str,
|
||||
// True when the caller is a job token (`$WM_TOKEN`). `devops` is instance-level
|
||||
// and `is_devops_email` is true for superadmins, so a job token whose on_behalf_of
|
||||
// a `wm_deployers` member pointed at a superadmin would otherwise clear the devops
|
||||
// branch on a workspace it isn't admin of (GHSA-hfh4-cx4h-3fcr). Workspace admin
|
||||
// (`is_admin`) stays allowed — that is the cap ceiling.
|
||||
is_job_token: bool,
|
||||
db: &DB,
|
||||
) -> Result<()> {
|
||||
if !is_admin {
|
||||
if !is_devops_email(db, email).await? {
|
||||
if is_job_token || !is_devops_email(db, email).await? {
|
||||
return Err(Error::RequireAdmin(username.to_string()));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user