diff --git a/backend/.sqlx/query-513ed713afdbafb587026d1536c47a9bbaa6967e36777454746b8817d68219a5.json b/backend/.sqlx/query-3a85abf71e6b3008f92528bd43b9bb8113b163a635b5480911b7aab625d892b9.json similarity index 53% rename from backend/.sqlx/query-513ed713afdbafb587026d1536c47a9bbaa6967e36777454746b8817d68219a5.json rename to backend/.sqlx/query-3a85abf71e6b3008f92528bd43b9bb8113b163a635b5480911b7aab625d892b9.json index 31187af03a..590e610cc6 100644 --- a/backend/.sqlx/query-513ed713afdbafb587026d1536c47a9bbaa6967e36777454746b8817d68219a5.json +++ b/backend/.sqlx/query-3a85abf71e6b3008f92528bd43b9bb8113b163a635b5480911b7aab625d892b9.json @@ -1,20 +1,25 @@ { "db_name": "PostgreSQL", - "query": "SELECT s.lock, s.lock_error_logs, dm.job_id\n FROM script s\n LEFT JOIN deployment_metadata dm ON s.hash = dm.script_hash AND s.workspace_id = dm.workspace_id\n WHERE s.hash = $1 AND s.workspace_id = $2", + "query": "SELECT s.path, s.lock, s.lock_error_logs, dm.job_id\n FROM script s\n LEFT JOIN deployment_metadata dm ON s.hash = dm.script_hash AND s.workspace_id = dm.workspace_id\n WHERE s.hash = $1 AND s.workspace_id = $2", "describe": { "columns": [ { "ordinal": 0, + "name": "path", + "type_info": "Varchar" + }, + { + "ordinal": 1, "name": "lock", "type_info": "Text" }, { - "ordinal": 1, + "ordinal": 2, "name": "lock_error_logs", "type_info": "Text" }, { - "ordinal": 2, + "ordinal": 3, "name": "job_id", "type_info": "Uuid" } @@ -26,10 +31,11 @@ ] }, "nullable": [ + false, true, true, true ] }, - "hash": "513ed713afdbafb587026d1536c47a9bbaa6967e36777454746b8817d68219a5" + "hash": "3a85abf71e6b3008f92528bd43b9bb8113b163a635b5480911b7aab625d892b9" } diff --git a/backend/.sqlx/query-9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504.json b/backend/.sqlx/query-9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504.json new file mode 100644 index 0000000000..16ff07308d --- /dev/null +++ b/backend/.sqlx/query-9b35fc71fa5994aa7db3b190c7389648ca885dc87ff1b105a35ba23572598504.json @@ -0,0 +1,23 @@ +{ + "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/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index 71b98b22bd..7a485f1fef 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -1554,11 +1554,13 @@ struct DeploymentStatus { job_id: Option, } async fn get_deployment_status( - Extension(db): Extension, + authed: ApiAuthed, + Extension(user_db): Extension, Path((w_id, path)): Path<(String, StripPath)>, ) -> JsonResult { let path = path.to_path(); - let mut tx = db.begin().await?; + check_scopes(&authed, || format!("flows:read:{}", path))?; + let mut tx = user_db.begin(&authed).await?; let status_o = sqlx::query!( "SELECT f.lock_error_logs, dm.job_id FROM flow f diff --git a/backend/windmill-api-integration-tests/tests/scripts.rs b/backend/windmill-api-integration-tests/tests/scripts.rs index e5f6cb8aa8..05c9c92d3a 100644 --- a/backend/windmill-api-integration-tests/tests/scripts.rs +++ b/backend/windmill-api-integration-tests/tests/scripts.rs @@ -1,5 +1,6 @@ use serde_json::json; use sqlx::{Pool, Postgres}; +use std::collections::HashMap; use windmill_test_utils::*; @@ -561,3 +562,121 @@ async fn test_list_search_scope_filtering(db: Pool) -> anyhow::Result< Ok(()) } + +/// The hash-addressed script endpoints must enforce the same folder ACLs and token +/// scopes as their path-addressed counterparts: a hash is an alternate address for a +/// script, never a way around its permissions. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_by_hash_endpoints_enforce_acls(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/scripts"); + + // `allowed` grants test-user-2 read; `private` grants it nothing. + for (folder, extra_perms) in [ + ("allowed", json!({ "u/test-user-2": false })), + ("private", json!({})), + ] { + let resp = authed(client().post(format!( + "http://localhost:{port}/api/w/test-workspace/folders/create" + ))) + .json(&json!({ "name": folder, "extra_perms": extra_perms })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200, "create folder: {}", resp.text().await?); + } + + let mut hashes = HashMap::new(); + for (path, content) in [ + ( + "f/allowed/foo", + "export async function main() { return 'allowed'; }", + ), + ( + "f/private/bar", + "export async function main() { return 'secret'; }", + ), + ] { + let resp = authed(client().post(format!("{base}/create"))) + .json(&new_script(path, "summary", content)) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 201, "create {path}: {}", resp.text().await?); + let resp = authed_get(port, "get/p", path).await; + assert_eq!(resp.status(), 200); + let body = resp.json::().await?; + hashes.insert(path, body["hash"].as_str().unwrap().to_string()); + } + + // A token scoped to `f/allowed/*` for the same super-admin user: RLS lets it through, + // only the per-path scope check can stop it. + sqlx::query( + "INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin, scopes) VALUES + (encode(sha256('SCOPED_TOKEN'::bytea), 'hex'), 'SCOPED_TOK', 'SCOPED_TOKEN', 'test@windmill.dev', 'scoped', true, ARRAY['scripts:read:f/allowed/*'])", + ) + .execute(&db) + .await?; + + async fn get_as(port: u16, token: &str, endpoint: &str, path: &str) -> reqwest::Response { + client() + .get(script_url(port, endpoint, path)) + .header("Authorization", format!("Bearer {token}")) + .send() + .await + .unwrap() + } + + let private = &hashes["f/private/bar"]; + let allowed = &hashes["f/allowed/foo"]; + + for token in ["SECRET_TOKEN_2", "SCOPED_TOKEN"] { + for (endpoint, hash) in [ + ("get/h", private.clone()), + ("raw/h", format!("{private}.ts")), + ("deployment_status/h", private.clone()), + ] { + let resp = get_as(port, token, endpoint, &hash).await; + let status = resp.status(); + let body = resp.text().await?; + assert!( + !status.is_success(), + "{token} must not reach {endpoint} of f/private/bar (got {status}: {body})" + ); + assert!( + !body.contains("return 'secret'"), + "{token} must not read f/private/bar source via {endpoint}: {body}" + ); + } + } + + // Denying an RLS-invisible script must not read as "this script does not exist": + // legitimate viewers reach these endpoints for scripts they cannot read. + let resp = get_as(port, "SECRET_TOKEN_2", "raw/h", &format!("{private}.ts")).await; + let body = resp.text().await?; + assert!( + body.contains("does not have permissions to access it"), + "denial must name the permission problem, got: {body}" + ); + + // The grant/scope that does cover `f/allowed/*` must keep working by hash. + for token in ["SECRET_TOKEN_2", "SCOPED_TOKEN"] { + for (endpoint, hash) in [ + ("get/h", allowed.clone()), + ("raw/h", format!("{allowed}.ts")), + ("deployment_status/h", allowed.clone()), + ] { + let resp = get_as(port, token, endpoint, &hash).await; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 200, + "{token} must still reach {endpoint} of f/allowed/foo (got {status}: {body})" + ); + } + } + + Ok(()) +} diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 2433502537..8767f2eccc 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -3219,24 +3219,50 @@ async fn get_script_by_hash_internal<'c>( Ok(script) } -#[derive(Deserialize)] -struct GetScriptByHashQuery { - authed: Option, +/// 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!( + "Script with hash {hash} exists but {} does not have permissions to access it", + authed.username + )), + _ => err, + } } + async fn get_script_by_hash( Extension(db): Extension, Extension(user_db): Extension, Path((w_id, hash)): Path<(String, ScriptHash)>, Query(query): Query, - Query(query_auth): Query, Extension(authed): Extension, ) -> JsonResult> { - let mut tx = if query_auth.authed.is_some_and(|x| x) { - user_db.begin(&authed).await? - } else { - db.begin().await? - }; - let r = get_script_by_hash_internal( + // 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 mut tx = user_db.begin(&authed).await?; + let r = match get_script_by_hash_internal( &mut tx, &w_id, &hash, @@ -3248,7 +3274,11 @@ async fn get_script_by_hash( } }), ) - .await?; + .await + { + Ok(r) => r, + Err(e) => return Err(explain_hash_miss(e, &db, &w_id, &hash, &authed).await), + }; check_scopes(&authed, || format!("scripts:read:{}", &r.script.path))?; @@ -3260,16 +3290,23 @@ async fn get_script_by_hash( } async fn raw_script_by_hash( + authed: ApiAuthed, Extension(db): Extension, + Extension(user_db): Extension, Path((w_id, hash_str)): Path<(String, String)>, ) -> Result { - let mut tx = db.begin().await?; 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 r = get_script_by_hash_internal(&mut tx, &w_id, &hash, None).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), + }; tx.commit().await?; + check_scopes(&authed, || format!("scripts:read:{}", &r.script.path))?; + Ok(r.script.content) } @@ -3280,12 +3317,14 @@ struct DeploymentStatus { job_id: Option, } async fn get_deployment_status( + authed: ApiAuthed, Extension(db): Extension, + Extension(user_db): Extension, Path((w_id, hash)): Path<(String, ScriptHash)>, ) -> JsonResult { - let mut tx = db.begin().await?; + let mut tx = user_db.begin(&authed).await?; let status_o = sqlx::query!( - "SELECT s.lock, s.lock_error_logs, dm.job_id + "SELECT s.path, s.lock, s.lock_error_logs, dm.job_id FROM script s LEFT JOIN deployment_metadata dm ON s.hash = dm.script_hash AND s.workspace_id = dm.workspace_id WHERE s.hash = $1 AND s.workspace_id = $2", @@ -3295,7 +3334,13 @@ async fn get_deployment_status( .fetch_optional(&mut *tx) .await?; - let status = not_found_if_none(status_o, "DeploymentStatus", hash.to_string())?; + 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), + }; + tx.commit().await?; + + check_scopes(&authed, || format!("scripts:read:{}", &status.path))?; let deployment_status = DeploymentStatus { lock: status.lock, @@ -3303,7 +3348,6 @@ async fn get_deployment_status( job_id: status.job_id, }; - tx.commit().await?; Ok(Json(deployment_status)) } diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 11beacd294..01f2d7463c 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -9813,10 +9813,6 @@ paths: in: query schema: type: boolean - - name: authed - in: query - schema: - type: boolean responses: "200": description: script details diff --git a/frontend/src/lib/components/flows/content/FlowModuleScript.svelte b/frontend/src/lib/components/flows/content/FlowModuleScript.svelte index 021437d20a..c1a7dc9087 100644 --- a/frontend/src/lib/components/flows/content/FlowModuleScript.svelte +++ b/frontend/src/lib/components/flows/content/FlowModuleScript.svelte @@ -53,6 +53,7 @@ let lock: string | undefined = $state(undefined) let date: string | undefined = $state(undefined) let notFound = $state(false) + let notFoundReason: string | undefined = $state(undefined) function getCachedKey(path: string, hash: string | undefined) { return `${opWs}-${path}-${hash ?? ''}` @@ -66,6 +67,7 @@ previousCode = cachedValues[key]?.previousCode tag = cachedValues[key]?.tag notFound = cachedValues[key]?.notFound ?? false + notFoundReason = undefined } getCachedValues( @@ -97,6 +99,7 @@ async function loadCode(path: string, hash: string | undefined) { try { notFound = false + notFoundReason = undefined const script = path.startsWith('hub/') ? await getScriptByPath(path!) : hash @@ -119,6 +122,9 @@ } } catch (e) { notFound = true + // A step can reference a script the viewer has no read grant on, so show the + // server's reason rather than always claiming the script is missing. + notFoundReason = e?.body console.error(e) } } @@ -140,7 +146,9 @@
tag: {tag}
{/if} {#if notFound} -
script not found at {path} in workspace {opWs}
+
+ {notFoundReason ?? `script not found at ${path} in workspace ${opWs}`} +
{:else if showAllCode} {#if showDiff} {#key (previousCode ?? '') + (code ?? '')} diff --git a/frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte b/frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte index 6894a69549..673ddc1334 100644 --- a/frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte @@ -299,11 +299,13 @@ job.script_hash && persistentScriptDefinition === undefined ) { + // A job can be visible to someone with no read permission on its script, in + // which case the persistent-script banner is simply not shown. const script = await ScriptService.getScriptByHash({ workspace: $workspaceStore!, hash: job.script_hash - }) - if (script.restart_unless_cancelled ?? false) { + }).catch(() => undefined) + if (script?.restart_unless_cancelled ?? false) { persistentScriptDefinition = script } } diff --git a/frontend/src/routes/(root)/(logged)/scripts/get/[...hash]/+page.svelte b/frontend/src/routes/(root)/(logged)/scripts/get/[...hash]/+page.svelte index 04c16c0acb..d57ced8a1b 100644 --- a/frontend/src/routes/(root)/(logged)/scripts/get/[...hash]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/scripts/get/[...hash]/+page.svelte @@ -246,8 +246,7 @@ script = await ScriptService.getScriptByHash({ workspace: $workspaceStore!, hash, - withStarredInfo: true, - authed: true + withStarredInfo: true }) } catch { try {