From f329edaf981e401bf6ba557505fcec273b123deb Mon Sep 17 00:00:00 2001 From: hugocasa Date: Wed, 12 Aug 2026 19:14:27 +0200 Subject: [PATCH] fix: re-read folder membership on hash-addressed script reads `authed.folders` is a snapshot cached alongside the caller's token, so a grant made after it was cached denies a script the caller can legitimately read: the folder created by the deploy they are being redirected from, or one an admin has just added them to. Enforcing RLS on the by-hash reads without accounting for that would reintroduce the staleness the privileged-pool shortcut was papering over. Resolve the path first and refresh group and folder membership from the database before running the read under RLS. The existence probe the miss path already did now comes from that same lookup, so the denial path is a query cheaper. Co-Authored-By: Claude Opus 5 --- ...89648ca885dc87ff1b105a35ba23572598504.json | 23 ------- ...d0d2df192a8ac63372190187039af5a7ac491.json | 23 +++++++ .../tests/scripts.rs | 18 ++++++ backend/windmill-api-scripts/src/scripts.rs | 64 ++++++++++++------- 4 files changed, 82 insertions(+), 46 deletions(-) delete mode 100644 backend/.sqlx/query-9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504.json create mode 100644 backend/.sqlx/query-9c18d50a5efbf986621b0b56107d0d2df192a8ac63372190187039af5a7ac491.json diff --git a/backend/.sqlx/query-9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504.json b/backend/.sqlx/query-9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504.json deleted file mode 100644 index 16ff07308d..0000000000 --- a/backend/.sqlx/query-9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM script WHERE hash = $1 AND workspace_id = $2)", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "exists", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Int8", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504" -} diff --git a/backend/.sqlx/query-9c18d50a5efbf986621b0b56107d0d2df192a8ac63372190187039af5a7ac491.json b/backend/.sqlx/query-9c18d50a5efbf986621b0b56107d0d2df192a8ac63372190187039af5a7ac491.json new file mode 100644 index 0000000000..d08ece8a5a --- /dev/null +++ b/backend/.sqlx/query-9c18d50a5efbf986621b0b56107d0d2df192a8ac63372190187039af5a7ac491.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT path FROM script WHERE hash = $1 AND workspace_id = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Int8", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "9c18d50a5efbf986621b0b56107d0d2df192a8ac63372190187039af5a7ac491" +} diff --git a/backend/windmill-api-integration-tests/tests/scripts.rs b/backend/windmill-api-integration-tests/tests/scripts.rs index 05c9c92d3a..bc381a027e 100644 --- a/backend/windmill-api-integration-tests/tests/scripts.rs +++ b/backend/windmill-api-integration-tests/tests/scripts.rs @@ -678,5 +678,23 @@ async fn test_by_hash_endpoints_enforce_acls(db: Pool) -> anyhow::Resu } } + // A grant made after the caller's folder list was cached must not deny the read: + // the requests above cached it, and writing `extra_perms` directly leaves that + // entry stale exactly as an admin granting access to someone else's session does. + sqlx::query( + "UPDATE folder SET extra_perms = extra_perms || '{\"u/test-user-2\": false}'::jsonb + WHERE name = 'private' AND workspace_id = 'test-workspace'", + ) + .execute(&db) + .await?; + let resp = get_as(port, "SECRET_TOKEN_2", "get/h", private).await; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 200, + "a freshly granted folder must be read from the database, not the cached authed \ + (got {status}: {body})" + ); + Ok(()) } diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 8767f2eccc..7d6973ac95 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -3219,35 +3219,50 @@ async fn get_script_by_hash_internal<'c>( Ok(script) } +/// Resolves what a hash-addressed read may see, before the RLS query runs. +/// +/// Returns the script's path when the hash names one at all, regardless of the caller's +/// grants, together with the authed the read should then run as. +/// +/// `authed.folders` is a snapshot cached alongside the caller's token, so a grant made +/// after it was cached (the folder created by the deploy the caller is being redirected +/// from, or one an admin has just added them to) would otherwise deny a script they can +/// legitimately read. `maybe_refresh_folders` re-reads the memberships from the database +/// and needs the path, hence the privileged lookup. +async fn resolve_hash_read( + authed: ApiAuthed, + db: &DB, + w_id: &str, + hash: &ScriptHash, +) -> Result<(Option, ApiAuthed)> { + let path = sqlx::query_scalar!( + "SELECT path FROM script WHERE hash = $1 AND workspace_id = $2", + hash.0, + w_id, + ) + .fetch_optional(db) + .await?; + + let authed = match path.as_deref() { + Some(path) => maybe_refresh_folders(path, w_id, authed, db).await, + None => authed, + }; + Ok((path, authed)) +} + /// A hash-addressed read that misses under RLS is ambiguous: the hash may name no /// script, or one in a folder the caller has no grant on. Legitimate callers land here /// (a flow step, an app component job, or a shared run can all reference a script the /// viewer cannot read), so report which of the two it is rather than a bare "not found". /// Disclosing existence costs nothing a caller who already holds the hash does not have, /// and mirrors what `raw/p` reports for paths. -async fn explain_hash_miss( - err: Error, - db: &DB, - w_id: &str, - hash: &ScriptHash, - authed: &ApiAuthed, -) -> Error { - if !matches!(err, Error::NotFound(_)) { - return err; - } - let exists = sqlx::query_scalar!( - "SELECT EXISTS(SELECT 1 FROM script WHERE hash = $1 AND workspace_id = $2)", - hash.0, - w_id, - ) - .fetch_one(db) - .await; - match exists { - Ok(Some(true)) => Error::NotFound(format!( +fn explain_hash_miss(err: Error, exists: bool, hash: &ScriptHash, authed: &ApiAuthed) -> Error { + match err { + Error::NotFound(_) if exists => Error::NotFound(format!( "Script with hash {hash} exists but {} does not have permissions to access it", authed.username )), - _ => err, + e => e, } } @@ -3261,6 +3276,7 @@ async fn get_script_by_hash( // Addressing a script by hash must not be a way around the folder ACLs that // `get/p` enforces: the fetch always goes through RLS, and `scripts:read:` // is checked once the row (hence the path) is known. + let (path, authed) = resolve_hash_read(authed, &db, &w_id, &hash).await?; let mut tx = user_db.begin(&authed).await?; let r = match get_script_by_hash_internal( &mut tx, @@ -3277,7 +3293,7 @@ async fn get_script_by_hash( .await { Ok(r) => r, - Err(e) => return Err(explain_hash_miss(e, &db, &w_id, &hash, &authed).await), + Err(e) => return Err(explain_hash_miss(e, path.is_some(), &hash, &authed)), }; check_scopes(&authed, || format!("scripts:read:{}", &r.script.path))?; @@ -3298,10 +3314,11 @@ async fn raw_script_by_hash( let hash = ScriptHash(to_i64(hash_str.strip_suffix(".ts").ok_or_else(|| { Error::BadRequest("Raw script path must end with .ts".to_string()) })?)?); + let (path, authed) = resolve_hash_read(authed, &db, &w_id, &hash).await?; let mut tx = user_db.begin(&authed).await?; let r = match get_script_by_hash_internal(&mut tx, &w_id, &hash, None).await { Ok(r) => r, - Err(e) => return Err(explain_hash_miss(e, &db, &w_id, &hash, &authed).await), + Err(e) => return Err(explain_hash_miss(e, path.is_some(), &hash, &authed)), }; tx.commit().await?; @@ -3322,6 +3339,7 @@ async fn get_deployment_status( Extension(user_db): Extension, Path((w_id, hash)): Path<(String, ScriptHash)>, ) -> JsonResult { + let (path, authed) = resolve_hash_read(authed, &db, &w_id, &hash).await?; let mut tx = user_db.begin(&authed).await?; let status_o = sqlx::query!( "SELECT s.path, s.lock, s.lock_error_logs, dm.job_id @@ -3336,7 +3354,7 @@ async fn get_deployment_status( let status = match not_found_if_none(status_o, "DeploymentStatus", hash.to_string()) { Ok(s) => s, - Err(e) => return Err(explain_hash_miss(e, &db, &w_id, &hash, &authed).await), + Err(e) => return Err(explain_hash_miss(e, path.is_some(), &hash, &authed)), }; tx.commit().await?;