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?;