diff --git a/backend/.sqlx/query-abb56dec024600d0d7a63212f63c3296196898003707f2cb3a890690173a51a5.json b/backend/.sqlx/query-a462d8661fee58129ec52952ee08a33fcc2e22b47d352030d55e1375c885cd36.json similarity index 51% rename from backend/.sqlx/query-abb56dec024600d0d7a63212f63c3296196898003707f2cb3a890690173a51a5.json rename to backend/.sqlx/query-a462d8661fee58129ec52952ee08a33fcc2e22b47d352030d55e1375c885cd36.json index df1b7fab40..59b9eb9889 100644 --- a/backend/.sqlx/query-abb56dec024600d0d7a63212f63c3296196898003707f2cb3a890690173a51a5.json +++ b/backend/.sqlx/query-a462d8661fee58129ec52952ee08a33fcc2e22b47d352030d55e1375c885cd36.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT path FROM app WHERE id = $1", + "query": "SELECT a.path FROM app a JOIN app_version av ON av.app_id = a.id\n WHERE a.id = $1 AND a.workspace_id = $2 AND av.id = $3", "describe": { "columns": [ { @@ -11,6 +11,8 @@ ], "parameters": { "Left": [ + "Int8", + "Text", "Int8" ] }, @@ -18,5 +20,5 @@ false ] }, - "hash": "abb56dec024600d0d7a63212f63c3296196898003707f2cb3a890690173a51a5" + "hash": "a462d8661fee58129ec52952ee08a33fcc2e22b47d352030d55e1375c885cd36" } diff --git a/backend/.sqlx/query-d02c1cfefc7d5f87ca4555a092f5fbc777a91271b80d0e488a8cae79afd56d8f.json b/backend/.sqlx/query-b29cf43a194f4a61a67ce95a5485dc287df1b1d79bf5040216ee48583405cd8b.json similarity index 75% rename from backend/.sqlx/query-d02c1cfefc7d5f87ca4555a092f5fbc777a91271b80d0e488a8cae79afd56d8f.json rename to backend/.sqlx/query-b29cf43a194f4a61a67ce95a5485dc287df1b1d79bf5040216ee48583405cd8b.json index f90af3cca7..8b35679943 100644 --- a/backend/.sqlx/query-d02c1cfefc7d5f87ca4555a092f5fbc777a91271b80d0e488a8cae79afd56d8f.json +++ b/backend/.sqlx/query-b29cf43a194f4a61a67ce95a5485dc287df1b1d79bf5040216ee48583405cd8b.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT a.id as app_id, av.id as version_id, dm.deployment_msg as deployment_msg\n FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version\n WHERE a.workspace_id = $1 AND a.path = $2\n ORDER BY created_at DESC", + "query": "SELECT a.id as app_id, av.id as version_id, dm.deployment_msg as deployment_msg\n FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version AND dm.workspace_id = a.workspace_id\n WHERE a.workspace_id = $1 AND a.path = $2\n ORDER BY created_at DESC", "describe": { "columns": [ { @@ -31,5 +31,5 @@ true ] }, - "hash": "d02c1cfefc7d5f87ca4555a092f5fbc777a91271b80d0e488a8cae79afd56d8f" + "hash": "b29cf43a194f4a61a67ce95a5485dc287df1b1d79bf5040216ee48583405cd8b" } diff --git a/backend/windmill-api-integration-tests/tests/app_history_cross_workspace.rs b/backend/windmill-api-integration-tests/tests/app_history_cross_workspace.rs new file mode 100644 index 0000000000..a75e9bee98 --- /dev/null +++ b/backend/windmill-api-integration-tests/tests/app_history_cross_workspace.rs @@ -0,0 +1,146 @@ +//! `app_version.id` is a global sequence, so an app-history deployment message +//! addressed by `(app_id, app_version)` alone crosses workspaces. This pins both +//! halves: the write refuses a version that is not the workspace's app's, and the +//! read only joins deployment metadata owned by the workspace. + +use serde_json::json; +use sqlx::{Pool, Postgres}; +use windmill_test_utils::*; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(builder: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + builder.header("Authorization", "Bearer SECRET_TOKEN") +} + +fn new_app(path: &str) -> serde_json::Value { + json!({ + "path": path, + "summary": "Test app", + "value": { "type": "rawapp", "inline_script": null }, + "policy": { "execution_mode": "anonymous", "triggerables": {} } + }) +} + +/// Creates the app and returns its `(app_id, version_id)`. +async fn create_app(port: u16, workspace: &str, path: &str) -> anyhow::Result<(i64, i64)> { + let base = format!("http://localhost:{port}/api/w/{workspace}/apps"); + let resp = authed(client().post(format!("{base}/create"))) + .json(&new_app(path)) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 201, "create {path} in {workspace}: {body}"); + + let latest = authed(client().get(format!("{base}/get_latest_version/{path}"))) + .send() + .await? + .json::() + .await?; + Ok(( + latest["app_id"].as_i64().unwrap(), + latest["version"].as_i64().unwrap(), + )) +} + +async fn update_history( + port: u16, + workspace: &str, + app_id: i64, + version: i64, + msg: &str, +) -> anyhow::Result<(u16, String)> { + let resp = authed(client().post(format!( + "http://localhost:{port}/api/w/{workspace}/apps/history_update/a/{app_id}/v/{version}" + ))) + .json(&json!({ "deployment_msg": msg })) + .send() + .await?; + let status = resp.status().as_u16(); + Ok((status, resp.text().await?)) +} + +async fn history(port: u16, workspace: &str, path: &str) -> anyhow::Result { + Ok(authed(client().get(format!( + "http://localhost:{port}/api/w/{workspace}/apps/history/p/{path}" + ))) + .send() + .await? + .json() + .await?) +} + +#[sqlx::test( + migrations = "../migrations", + fixtures("base", "app_history_second_workspace") +)] +async fn test_app_history_is_workspace_scoped(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let victim_path = "u/test-user/victim_app"; + let attacker_path = "u/test-user/attacker_app"; + let (victim_app, victim_version) = create_app(port, "test-workspace", victim_path).await?; + let (attacker_app, attacker_version) = + create_app(port, "test-workspace-2", attacker_path).await?; + + // Own app, another workspace's version: the version must belong to the app. + let (status, body) = update_history( + port, + "test-workspace-2", + attacker_app, + victim_version, + "injected", + ) + .await?; + assert_eq!(status, 404, "foreign version must be rejected: {body}"); + + // Another workspace's app entirely: the app must belong to the workspace. + let (status, body) = update_history( + port, + "test-workspace-2", + victim_app, + victim_version, + "injected", + ) + .await?; + assert_eq!(status, 404, "foreign app must be rejected: {body}"); + + // The legitimate update still lands, and only in its own workspace. + let (status, body) = update_history( + port, + "test-workspace-2", + attacker_app, + attacker_version, + "deployed by owner", + ) + .await?; + assert_eq!(status, 200, "own app and version must be accepted: {body}"); + assert_eq!( + history(port, "test-workspace-2", attacker_path).await?[0]["deployment_msg"], + "deployed by owner" + ); + + // A deployment_metadata row owned by another workspace but pointing at this + // app's version must not surface — the read joins on workspace too. + sqlx::query( + "INSERT INTO deployment_metadata (workspace_id, path, app_version, deployment_msg) + VALUES ('test-workspace-2', $1, $2, 'injected')", + ) + .bind(victim_path) + .bind(victim_version) + .execute(&db) + .await?; + + assert_eq!( + history(port, "test-workspace", victim_path).await?[0]["deployment_msg"], + serde_json::Value::Null, + "another workspace's deployment metadata must not surface" + ); + + Ok(()) +} diff --git a/backend/windmill-api-integration-tests/tests/fixtures/app_history_second_workspace.sql b/backend/windmill-api-integration-tests/tests/fixtures/app_history_second_workspace.sql new file mode 100644 index 0000000000..db8e287494 --- /dev/null +++ b/backend/windmill-api-integration-tests/tests/fixtures/app_history_second_workspace.sql @@ -0,0 +1,17 @@ +-- Additive fixture: a second workspace sharing the `base` fixture's admin user, +-- so the same caller can hold an app in each workspace. + +INSERT INTO workspace (id, name, owner) VALUES + ('test-workspace-2', 'test-workspace-2', 'test-user'); + +INSERT INTO usr(workspace_id, email, username, is_admin, role) VALUES + ('test-workspace-2', 'test@windmill.dev', 'test-user', true, 'Admin'); + +INSERT INTO workspace_key(workspace_id, kind, key) VALUES + ('test-workspace-2', 'cloud', 'test-key-2'); + +INSERT INTO workspace_settings (workspace_id) VALUES + ('test-workspace-2'); + +INSERT INTO group_ (workspace_id, name, summary, extra_perms) VALUES + ('test-workspace-2', 'all', 'All users', '{}'); diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 0e64d1cba2..32591284ef 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -546,7 +546,7 @@ async fn list_apps( if lq.with_deployment_msg.unwrap_or(false) { sqlb.join("deployment_metadata dm") .left() - .on("dm.app_version = app.versions[array_upper(app.versions, 1)]") + .on("dm.app_version = app.versions[array_upper(app.versions, 1)] AND dm.workspace_id = app.workspace_id") .fields(&["dm.deployment_msg"]); } @@ -1055,7 +1055,7 @@ async fn get_app_history( let mut tx = user_db.begin(&authed).await?; let query_result = sqlx::query!( "SELECT a.id as app_id, av.id as version_id, dm.deployment_msg as deployment_msg - FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version + FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version AND dm.workspace_id = a.workspace_id WHERE a.workspace_id = $1 AND a.path = $2 ORDER BY created_at DESC", w_id, @@ -1084,7 +1084,7 @@ async fn get_latest_version( let mut tx = user_db.begin(&authed).await?; let row = sqlx::query!( "SELECT a.id as app_id, av.id as version_id, dm.deployment_msg as deployment_msg - FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version + FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version AND dm.workspace_id = a.workspace_id WHERE a.workspace_id = $1 AND a.path = $2 ORDER BY created_at DESC", w_id, @@ -1112,15 +1112,24 @@ async fn update_app_history( Json(app_history_update): Json, ) -> Result<()> { let mut tx = user_db.begin(&authed).await?; - let app_path = sqlx::query_scalar!("SELECT path FROM app WHERE id = $1", app_id) - .fetch_optional(&mut *tx) - .await?; + // `app_version.id` is a global sequence, so the version has to be tied back to the app and + // the app to the workspace: without both, the row this writes lands on another workspace's + // deployment history. + let app_path = sqlx::query_scalar!( + "SELECT a.path FROM app a JOIN app_version av ON av.app_id = a.id + WHERE a.id = $1 AND a.workspace_id = $2 AND av.id = $3", + app_id, + w_id, + app_version, + ) + .fetch_optional(&mut *tx) + .await?; let Some(app_path) = app_path else { tx.commit().await?; - return Err(Error::NotFound( - format!("App with ID {app_id} not found").to_string(), - )); + return Err(Error::NotFound(format!( + "App with ID {app_id} and version {app_version} not found" + ))); }; check_scopes(&authed, || format!("apps:write:{}", &app_path))?;