From cedd6dc901918d3001229fa95a5fe86e369b7bc4 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 28 Sep 2026 18:34:55 +0200 Subject: [PATCH] fix: hold interpolated references and captures to the token path scopes (#11391) * fix: hold interpolated references and captures to the token path scopes Co-Authored-By: Claude Opus 5.5 (1M context) * fix: let a resource read cover its own linked secret variable Co-Authored-By: Claude Opus 5.5 (1M context) * fix: resolve policy-granted app upload resources on the viewer's rls Co-Authored-By: Claude Opus 5.5 (1M context) * fix: cover multi-secret linked variables and keep capture paths out of refusals Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) --- ...fd18ae25e0ee4810f1c9638e4400ab0c914a7.json | 23 ++++ .../tests/resources.rs | 107 ++++++++++++++++++ backend/windmill-api/src/apps.rs | 14 ++- backend/windmill-api/src/capture.rs | 42 ++++++- backend/windmill-store/src/resources.rs | 27 +++++ 5 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 backend/.sqlx/query-2da4b7cf1549494945c9761885dfd18ae25e0ee4810f1c9638e4400ab0c914a7.json diff --git a/backend/.sqlx/query-2da4b7cf1549494945c9761885dfd18ae25e0ee4810f1c9638e4400ab0c914a7.json b/backend/.sqlx/query-2da4b7cf1549494945c9761885dfd18ae25e0ee4810f1c9638e4400ab0c914a7.json new file mode 100644 index 0000000000..88b7f6411b --- /dev/null +++ b/backend/.sqlx/query-2da4b7cf1549494945c9761885dfd18ae25e0ee4810f1c9638e4400ab0c914a7.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT path FROM capture WHERE id = $1 AND workspace_id = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Int8", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "2da4b7cf1549494945c9761885dfd18ae25e0ee4810f1c9638e4400ab0c914a7" +} diff --git a/backend/windmill-api-integration-tests/tests/resources.rs b/backend/windmill-api-integration-tests/tests/resources.rs index 53648ec759..9c167ef70c 100644 --- a/backend/windmill-api-integration-tests/tests/resources.rs +++ b/backend/windmill-api-integration-tests/tests/resources.rs @@ -566,6 +566,113 @@ async fn test_resource_value_cache_is_identity_scoped(db: Pool) -> any Ok(()) } +/// A token scoped to one resource must not read, through the `$var:`/`$res:` references it +/// writes into that resource, what its scopes would refuse to read directly. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_interpolated_references_need_the_token_scopes( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + + let resp = authed(client().post(format!("{base}/variables/create"))) + .json(&json!({"path": "u/test-user/secret", "value": "CANARY", "is_secret": true, "description": ""})) + .send() + .await?; + assert_eq!(resp.status(), 201); + let resp = authed(client().post(format!("{base}/resources/create"))) + .json(&json!({"path": "u/test-user/other", "value": {"pw": "OTHER"}, "resource_type": "object", "description": ""})) + .send() + .await?; + assert_eq!(resp.status(), 201); + + let mint = |scopes: serde_json::Value| async move { + let resp = + authed(client().post(format!("http://localhost:{port}/api/users/tokens/create"))) + .json( + &json!({"label": "scoped", "scopes": scopes, "workspace_id": "test-workspace"}), + ) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 201); + resp.text().await.unwrap() + }; + let probe_only = mint(json!(["resources:write:u/test-user/probe"])).await; + let with_var = mint(json!([ + "resources:read:u/test-user/probe", + "variables:read:u/test-user/secret" + ])) + .await; + + let read_probe = |value: serde_json::Value, token: String| { + let base = base.clone(); + let probe_only = probe_only.clone(); + async move { + let resp = client() + .post(format!("{base}/resources/create?update_if_exists=true")) + .bearer_auth(&probe_only) + .json(&json!({"path": "u/test-user/probe", "value": value, "resource_type": "object", "description": ""})) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 201); + let resp = client() + .get(format!( + "{base}/resources/get_value_interpolated/u/test-user/probe" + )) + .bearer_auth(&token) + .send() + .await + .unwrap(); + (resp.status().as_u16(), resp.text().await.unwrap()) + } + }; + + for value in [ + json!({"v": "$var:u/test-user/secret"}), + json!({"v": "$jsonvar:u/test-user/secret"}), + json!({"v": ["$res:u/test-user/other"]}), + ] { + let (status, body) = read_probe(value.clone(), probe_only.clone()).await; + assert_eq!(status, 403, "{value}: {body}"); + assert!( + !body.contains("CANARY") && !body.contains("OTHER"), + "{body}" + ); + } + + let (status, body) = + read_probe(json!({"v": "$var:u/test-user/secret"}), with_var.clone()).await; + assert_eq!((status, body.as_str()), (200, r#"{"v":"CANARY"}"#)); + + // The resource's own linked secrets (at its path, or `_`) are part of it. + for (path, value) in [ + ("u/test-user/probe", "LINKED"), + ("u/test-user/probe_key", "KEY"), + ] { + let resp = authed(client().post(format!("{base}/variables/create"))) + .json(&json!({"path": path, "value": value, "is_secret": true, "description": ""})) + .send() + .await?; + assert_eq!(resp.status(), 201); + } + let (status, body) = read_probe( + json!({"pw": "$var:u/test-user/probe", "key": "$var:u/test-user/probe_key"}), + probe_only.clone(), + ) + .await; + assert_eq!(status, 200, "{body}"); + assert_eq!( + serde_json::from_str::(&body)?, + json!({"pw": "LINKED", "key": "KEY"}) + ); + + Ok(()) +} + /// A resource whose value contains a `$WM_*` contextual variable (e.g. `$WM_TOKEN`) is /// job-dependent and must NEVER be cached — even when first read WITHOUT a `job_id`, where the /// placeholder is left unresolved (caching that would serve a stale placeholder to a later job diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 080f3c8431..ef48927676 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -4812,8 +4812,9 @@ async fn upload_s3_file_from_app( if let Some(ref s3_resource_path) = query.s3_resource_path { if matched_input.allow_user_resources { if let Some(authed) = opt_authed { + let viewer = policy_granted_viewer(&authed); let db_with_opt_authed = DbWithOptAuthed::from_authed( - &authed, + &viewer, db.clone(), Some(user_db.clone()), ); @@ -5780,6 +5781,14 @@ async fn exists_app( Ok(Json(exists)) } +/// The viewer as whom a resource an app policy lets the viewer pick (`allow_user_resources`) +/// is resolved. The policy, not the token, grants that resource, and app tokens are minted +/// with a fixed scope set that never names variables, so the references inside it resolve on +/// the viewer's RLS alone. +fn policy_granted_viewer(authed: &ApiAuthed) -> ApiAuthed { + ApiAuthed { scopes: None, ..authed.clone() } +} + async fn build_args( policy: &Policy, PolicyTriggerableInputs { @@ -5806,8 +5815,9 @@ async fn build_args( key.and_then(|x| x.clone().strip_prefix("$res:").map(|x| x.to_string())) { if let Some(authed) = authed { + let viewer = policy_granted_viewer(authed); let db_with_opt_authed = - DbWithOptAuthed::from_authed(authed, db.clone(), Some(user_db.clone())); + DbWithOptAuthed::from_authed(&viewer, db.clone(), Some(user_db.clone())); let res = get_resource_value_interpolated_internal( &db_with_opt_authed, w_id, diff --git a/backend/windmill-api/src/capture.rs b/backend/windmill-api/src/capture.rs index 6255b5eb13..8e9fb4a823 100644 --- a/backend/windmill-api/src/capture.rs +++ b/backend/windmill-api/src/capture.rs @@ -66,6 +66,7 @@ use crate::{ args::RawWebhookArgs, db::{ApiAuthed, DB}, users::fetch_api_authed, + utils::check_scopes, }; use axum::{ @@ -319,6 +320,7 @@ async fn get_configs( Extension(user_db): Extension, Path((w_id, runnable_kind, path)): Path<(String, RunnableKind, StripPath)>, ) -> JsonResult> { + check_scopes(&authed, || format!("capture:read:{}", path.to_path()))?; let mut tx = user_db.begin(&authed).await?; let configs = sqlx::query_as!( @@ -557,6 +559,7 @@ async fn set_config( Path(w_id): Path, Json(nc): Json, ) -> JsonResult> { + check_scopes(&authed, || format!("capture:write:{}", nc.path))?; let nc = match nc.trigger_kind { TriggerKind::Postgres => { set_postgres_trigger_config(&w_id, authed.clone(), &db, user_db.clone(), nc).await? @@ -616,6 +619,7 @@ async fn ping_config( StripPath, )>, ) -> Result<()> { + check_scopes(&authed, || format!("capture:write:{}", path.to_path()))?; let mut tx = user_db.begin(&authed).await?; if matches!(trigger_kind, TriggerKind::Postgres) { windmill_common::datatable_roles::lock_datatable_streams(&mut *tx, false).await?; @@ -667,6 +671,7 @@ async fn list_captures( Path((w_id, runnable_kind, path)): Path<(String, RunnableKind, StripPath)>, Query(query): Query, ) -> JsonResult> { + check_scopes(&authed, || format!("capture:read:{}", path.to_path()))?; let mut tx = user_db.begin(&authed).await?; let (per_page, offset) = paginate(Pagination { page: query.page, per_page: query.per_page }); @@ -713,12 +718,44 @@ async fn list_captures( Ok(Json(captures)) } +/// A capture addressed by id carries no path in the route, so its path scope can only be +/// checked against the row. Looked up under the caller's RLS so the check never reveals +/// whether an id the caller cannot see exists, and the refusal omits the path, which the +/// caller named only by id. +async fn check_capture_id_scope( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + authed: &ApiAuthed, + w_id: &str, + id: i64, + action: &str, +) -> Result<()> { + if authed.scopes.is_none() { + return Ok(()); + } + let path = sqlx::query_scalar!( + "SELECT path FROM capture WHERE id = $1 AND workspace_id = $2", + id, + w_id, + ) + .fetch_optional(&mut **tx) + .await?; + if let Some(path) = path { + check_scopes(authed, || format!("capture:{action}:{path}")).map_err(|_| { + windmill_common::error::Error::PermissionDenied(format!( + "This token's capture:{action} scope does not cover capture {id}" + )) + })?; + } + Ok(()) +} + async fn get_capture( authed: ApiAuthed, Extension(user_db): Extension, Path((w_id, id)): Path<(String, i64)>, ) -> JsonResult { let mut tx = user_db.begin(&authed).await?; + check_capture_id_scope(&mut tx, &authed, &w_id, id, "read").await?; let capture = sqlx::query_as!( Capture, @@ -751,6 +788,7 @@ async fn delete_capture( Path((w_id, id)): Path<(String, i64)>, ) -> Result<()> { let mut tx = user_db.begin(&authed).await?; + check_capture_id_scope(&mut tx, &authed, &w_id, id, "write").await?; // capture RLS only keys on the path segment, so without workspace_id an id from // another workspace whose path collides with the caller's grants would be deleted. sqlx::query!( @@ -781,8 +819,10 @@ async fn move_captures_and_configs( Path((w_id, runnable_kind, old_path)): Path<(String, RunnableKind, StripPath)>, Json(body): Json, ) -> Result<()> { - let mut tx = user_db.begin(&authed).await?; let old_path = old_path.to_path(); + check_scopes(&authed, || format!("capture:write:{}", old_path))?; + check_scopes(&authed, || format!("capture:write:{}", body.new_path))?; + let mut tx = user_db.begin(&authed).await?; sqlx::query!( r#" diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 71e94b57f3..48d6b19181 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -812,6 +812,7 @@ pub async fn get_resource_value_interpolated_internal<'a>( token_for_context, 0, &used_job_context, + Some(path), ) .await?; if let Some(identity) = cache_identity.as_deref() { @@ -851,10 +852,29 @@ pub async fn transform_json_value( token, depth, &used_job_context, + None, ) .await } +/// RLS resolves a reference as the token's user, not as the token: a token scoped to one +/// resource would otherwise read, through that resource, every variable or resource its +/// user can. Job tokens carry no scopes, so what a runnable resolves is unaffected. +/// `resource_path` is the resource being expanded: its own linked secrets +/// ([`is_owned_linked_var`]) are covered by the read of it. +fn check_interpolation_scope( + db_with_opt_authed: &DbWithOptAuthed<'_, ApiAuthed>, + domain: &str, + path: &str, + resource_path: Option<&str>, +) -> Result<()> { + match db_with_opt_authed.authed() { + Some(_) if resource_path.is_some_and(|r| is_owned_linked_var(r, path)) => Ok(()), + Some(authed) => check_scopes(authed, || format!("{domain}:read:{path}")), + None => Ok(()), + } +} + /// Like [`transform_json_value`], but records into `used_job_context` whether the value /// contains a `$WM_*` contextual variable (resolved from `job_id`/`token`). A value that did /// not is job-independent and safe to cache; one that did must not be cached or shared across @@ -868,6 +888,7 @@ pub async fn transform_json_value_tracked( token: Option<&str>, depth: u8, used_job_context: &std::sync::atomic::AtomicBool, + resource_path: Option<&str>, ) -> Result { if depth >= MAX_RESOURCE_INTERPOLATION_DEPTH { return Err(Error::internal_err(format!( @@ -877,6 +898,7 @@ pub async fn transform_json_value_tracked( match v { Value::String(y) if y.starts_with("$var:") => { let path = y.strip_prefix("$var:").unwrap(); + check_interpolation_scope(db_with_opt_authed, "variables", path, resource_path)?; let v = crate::variables::get_value_internal(&db_with_opt_authed, workspace, path, false) @@ -885,6 +907,7 @@ pub async fn transform_json_value_tracked( } Value::String(y) if y.starts_with("$jsonvar:") => { let path = y.strip_prefix("$jsonvar:").unwrap(); + check_interpolation_scope(db_with_opt_authed, "variables", path, resource_path)?; let v = crate::variables::get_value_internal(&db_with_opt_authed, workspace, path, false) @@ -900,6 +923,7 @@ pub async fn transform_json_value_tracked( "Invalid resource path: {path}" ))); } + check_interpolation_scope(db_with_opt_authed, "resources", path, None)?; let mut tx: Transaction<'_, Postgres> = db_with_opt_authed.begin().await?; let v = sqlx::query_scalar!( "SELECT value from resource WHERE path = $1 AND workspace_id = $2", @@ -919,6 +943,7 @@ pub async fn transform_json_value_tracked( token, depth + 1, used_job_context, + Some(path), ) .await } else { @@ -1018,6 +1043,7 @@ pub async fn transform_json_value_tracked( token, depth + 1, used_job_context, + resource_path, ) .await?; } @@ -1042,6 +1068,7 @@ pub async fn transform_json_value_tracked( token, depth + 1, used_job_context, + resource_path, ) .await?; m.insert(a.clone(), v);