starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-08-24 08:01:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: require item read scope on workspace tarball export (#10797)

Browse Source 
* fix: require item read scope on workspace tarball export

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix: accept a wildcard path grant for whole-domain scope checks

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix: let a wildcard path grant delegate the unqualified scope

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Ruben Fiszel <ruben@windmill.dev>
This commit is contained in:
hugocasa
2026-08-22 12:06:54 +02:00
committed by GitHub
parent 40f0cab2ad
commit dc27db68de
4 changed files with 153 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/tests/workspace_export.rs
+91
View File
@@ -196,3 +196,94 @@ async fn test_tarball_export_all_tables(db: Pool<Postgres>) -> anyhow::Result<()
Ok(())
}
/// The archive carries every resource's and variable's `value`, which the per-item
/// routes gate on `resources:read:<path>` / `variables:read:<path>`. A token holding
/// only `workspaces:read` (what the route itself needs) must not collect them, and a
/// path-scoped token cannot stand in for the whole workspace either.
#[sqlx::test(fixtures("base"))]
async fn test_tarball_export_gates_values_on_item_scopes(db: Pool<Postgres>) -> anyhow::Result<()> {
initialize_tracing().await;
let server = ApiServer::start(db.clone()).await?;
let base_url = format!("http://localhost:{}", server.addr.port());
sqlx::query(
r#"INSERT INTO resource (workspace_id, path, value, resource_type, created_by)
VALUES ('test-workspace', 'u/test-user/creds',
'{"password": "RESOURCE_VALUE"}'::jsonb, 'postgresql', 'test-user')"#,
)
.execute(&db)
.await?;
sqlx::query(
r#"INSERT INTO variable (workspace_id, path, value, is_secret, description)
VALUES ('test-workspace', 'u/test-user/plain', 'VARIABLE_VALUE', false, '')"#,
)
.execute(&db)
.await?;
sqlx::query(
r#"INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin, scopes) VALUES
(encode(sha256('WS_READ_TOKEN'::bytea), 'hex'), 'WS_READ_TO', 'WS_READ_TOKEN',
'test@windmill.dev', 'workspaces:read only', false, '{workspaces:read}'),
(encode(sha256('PATH_SCOPED_TOKEN'::bytea), 'hex'), 'PATH_SCOPE', 'PATH_SCOPED_TOKEN',
'test@windmill.dev', 'path-scoped item read', false,
'{workspaces:read,resources:read:u/test-user/creds,variables:read:u/test-user/plain}'),
(encode(sha256('ITEM_READ_TOKEN'::bytea), 'hex'), 'ITEM_READ_', 'ITEM_READ_TOKEN',
'test@windmill.dev', 'item read', false,
'{workspaces:read,resources:read,variables:read}'),
(encode(sha256('WILDCARD_TOKEN'::bytea), 'hex'), 'WILDCARD_T', 'WILDCARD_TOKEN',
'test@windmill.dev', 'wildcard item read', false,
'{workspaces:read,resources:read:*,variables:read:*}')"#,
)
.execute(&db)
.await?;
let export = async |token: &str, query: &str| -> anyhow::Result<(u16, String)> {
let resp = reqwest::Client::new()
.get(format!(
"{base_url}/api/w/test-workspace/workspaces/tarball?{query}"
))
.bearer_auth(token)
.send()
.await?;
let status = resp.status().as_u16();
// Lossy: a successful export is a tar, not UTF-8. Only the values matter here.
Ok((
status,
String::from_utf8_lossy(&resp.bytes().await?).into_owned(),
))
};
for token in ["WS_READ_TOKEN", "PATH_SCOPED_TOKEN"] {
let (status, body) = export(token, "").await?;
assert_eq!(status, 403, "{token} exported values: {body}");
assert!(
!body.contains("RESOURCE_VALUE"),
"{token} leaked a resource"
);
let (status, body) = export(token, "skip_resources=true").await?;
assert_eq!(status, 403, "{token} exported variables: {body}");
assert!(
!body.contains("VARIABLE_VALUE"),
"{token} leaked a variable"
);
// Skipping both kinds leaves an export the route's own scope covers.
let (status, body) = export(token, "skip_resources=true&skip_variables=true").await?;
assert_eq!(status, 200, "{token} denied a value-free export: {body}");
}
// `*` is a resource path the scope picker mints, and it spans the whole domain,
// so it must export exactly as the unqualified grant does.
for token in ["ITEM_READ_TOKEN", "WILDCARD_TOKEN"] {
let (status, body) = export(token, "").await?;
assert_eq!(status, 200, "{token} denied: {body}");
assert!(
body.contains("RESOURCE_VALUE") && body.contains("VARIABLE_VALUE"),
"{token} exported no values"
);
}
Ok(())
}
backend/windmill-api-auth/src/lib.rs
+19 -2
View File
@@ -631,8 +631,11 @@ fn scope_contains(caller: &ScopeDefinition, requested: &ScopeDefinition) -> bool
match (&caller.resource, &requested.resource) {
// Caller is unrestricted on resources: covers everything.
(None, _) => true,
// Caller is resource-restricted but the request is not: broader.
(Some(_), None) => false,
// Caller is resource-restricted but the request is not: broader, unless the
// caller lists `*` and so already spans every path. Kept in step with
// `ScopeDefinition::includes`, which accepts that same grant for a
// whole-collection read: what a token may exercise, it may also delegate.
(Some(caller_resources), None) => caller_resources.iter().any(|r| r == "*"),
(Some(caller_resources), Some(requested_resources)) => {
resource_set_contains(caller_resources, requested_resources)
}
@@ -1810,6 +1813,20 @@ mod tests {
opt_scopes(Some(vec!["users:read", "if_jobs:filter_tags:default"])).as_deref()
)
.is_ok());
// A `*` path grant spans the domain, so it may mint the unqualified form a
// whole-collection read requires; a listed path may not.
let wildcard = authed_with_scopes(Some(vec!["resources:read:*"]));
assert!(ensure_scopes_within_caller(
&wildcard,
opt_scopes(Some(vec!["resources:read"])).as_deref()
)
.is_ok());
let path_scoped = authed_with_scopes(Some(vec!["resources:read:f/team/db"]));
assert!(ensure_scopes_within_caller(
&path_scoped,
opt_scopes(Some(vec!["resources:read"])).as_deref()
)
.is_err());
// Apps `write` covers `run`, so an app-editor token can mint the run-only
// credential for the same app — but only within its own resource subtree,
// and the equivalence stays Apps-only.
backend/windmill-api-auth/src/scopes.rs
+23 -1
View File
@@ -147,7 +147,11 @@ impl ScopeDefinition {
(Some(self_resources), Some(other_resources)) => {
resources_match(self_resources, other_resources)
}
(Some(_), None) => false,
// A requirement naming no path is the whole domain, so only a grant that
// itself spans every path satisfies it. `*` is that grant — the scope UI
// accepts it as a resource path and `resources_match` already reads it as
// everything — while any listed path leaves the collection unauthorized.
(Some(self_resources), None) => self_resources.iter().any(|r| r == "*"),
(None, _) => true,
}
}
@@ -1434,6 +1438,24 @@ mod tests {
.is_ok());
}
// Whole-collection reads (the workspace export, `apps:read`, ...) require the
// domain with no path. Only a grant spanning every path may satisfy that.
#[test]
fn test_unqualified_requirement_needs_a_whole_domain_grant() {
let unqualified = ScopeDefinition::new("resources", "read", None, None);
let wildcard = ScopeDefinition::new("resources", "read", None, Some(vec!["*".to_string()]));
assert!(wildcard.includes(&unqualified));
let path_scoped = ScopeDefinition::new(
"resources",
"read",
None,
Some(vec!["f/team/db".to_string(), "u/alice/db".to_string()]),
);
assert!(!path_scoped.includes(&unqualified));
}
#[test]
fn test_resource_array_matching() {
// Test wildcard access
backend/windmill-api/src/workspaces_export.rs
+20 -12
View File
@@ -634,23 +634,27 @@ pub(crate) async fn tarball_workspace(
skip_resources
);
// The route is gated by workspaces:read, but exporting DECRYPTED secrets is a
// variable-read capability beyond workspace metadata. Require variables:read
// only on the plaintext-secret path: ordinary tarball pulls (structure and
// encrypted-only values) keep working with workspaces:read, and the workspace
// key itself takes an admin *and* an unscoped token (include_key), since it
// decrypts those same secrets offline. No-op for unscoped tokens.
let export_plain_secrets = plain_secret.or(plain_secrets).unwrap_or(false)
&& !skip_secrets.unwrap_or(false)
&& !skip_variables.unwrap_or(false);
if export_plain_secrets {
check_scopes(&authed, || "variables:read".to_string())?;
}
// The workspace key decrypts every secret offline, so it takes an admin *and* an
// unscoped token. Checked before the item scopes below so that a scoped token
// asking for the key is told about the key rather than about a scope no token
// holding the key would need anyway.
if include_key.unwrap_or(false) {
require_admin(authed.is_admin, &authed.username)?;
windmill_api_auth::forbid_scoped_token_workspace_key(&authed)?;
}
// The route is gated by workspaces:read, but the tarball also carries the item
// values that the per-item routes gate on their own domain (get_resource_value,
// get_variable). A whole-workspace export cannot be confined to a path, so it
// takes the unrestricted domain scope: a path-scoped token has to skip that kind.
// No-op for unscoped tokens.
if !skip_resources.unwrap_or(false) {
check_scopes(&authed, || "resources:read".to_string())?;
}
if !skip_variables.unwrap_or(false) {
check_scopes(&authed, || "variables:read".to_string())?;
}
// Opt-in behavior for surfacing per-resource ACLs on flow/app rows.
// Folder and group rows have always carried `extra_perms` in source and
// continue to do so unconditionally (`KeepEvenEmpty`) so existing
@@ -695,6 +699,10 @@ pub(crate) async fn tarball_workspace(
Some(t) => Err(Error::BadRequest(format!("Invalid Archive Type {t}"))),
}?;
let export_plain_secrets = plain_secret.or(plain_secrets).unwrap_or(false)
&& !skip_secrets.unwrap_or(false)
&& !skip_variables.unwrap_or(false);
// Record what the export is about to disclose, once nothing left can reject the
// request: an entry written before the gates above would claim a disclosure that
// a 403 or an invalid archive type then prevented. On the pool and before the RLS