From 9ffd24cc06486c062c2aa4046adad8413d6bd904 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 25 Sep 2026 18:56:42 +0200 Subject: [PATCH] fix: drop an oauth account link the caller may not hold instead of failing Co-Authored-By: Claude Opus 5.5 --- .../tests/variables.rs | 88 +++++++++++++++++++ backend/windmill-store/src/variables.rs | 65 ++++++++------ 2 files changed, 125 insertions(+), 28 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/variables.rs b/backend/windmill-api-integration-tests/tests/variables.rs index e5018f4f97..464a837287 100644 --- a/backend/windmill-api-integration-tests/tests/variables.rs +++ b/backend/windmill-api-integration-tests/tests/variables.rs @@ -338,3 +338,91 @@ async fn test_variables_are_cached(db: Pool) -> anyhow::Result<()> { Ok(()) } + +/// Reading a variable linked to an OAuth account refreshes through that account, so a variable +/// may only link an account its caller connected or can already reach through a readable +/// variable. Any other requested link is dropped, and the variable is still created. +#[sqlx::test(migrations = "../migrations", fixtures("base", "permissions_test"))] +async fn test_oauth_account_link_requires_ownership(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace/variables", + server.addr.port() + ); + + let alice_account: i32 = sqlx::query_scalar( + "INSERT INTO account (workspace_id, client, expires_at, refresh_token, created_by) + VALUES ('test-workspace', 'github', now(), 'alice-refresh', 'alice') RETURNING id", + ) + .fetch_one(&db) + .await?; + let missing_account = alice_account + 1000; + + let create = |token: &'static str, path: &'static str, account: i32| { + let url = format!("{base}/create"); + async move { + client() + .post(url) + .header("Authorization", format!("Bearer {token}")) + .json(&json!({ "path": path, "value": "x", "is_secret": false, + "description": "", "account": account })) + .send() + .await + .unwrap() + .status() + } + }; + let linked = |path: &'static str| { + let db = db.clone(); + async move { + sqlx::query_scalar::<_, Option>( + "SELECT account FROM variable WHERE workspace_id = 'test-workspace' AND path = $1", + ) + .bind(path) + .fetch_one(&db) + .await + .unwrap() + } + }; + + assert_eq!( + create("ALICE_TOKEN_TEST", "u/alice/conn", alice_account).await, + 201 + ); + assert_eq!(linked("u/alice/conn").await, Some(alice_account)); + + assert_eq!( + create("BOB_TOKEN_TEST12", "u/bob/alias", alice_account).await, + 201 + ); + assert_eq!(linked("u/bob/alias").await, None, "not bob's account"); + + assert_eq!( + create("BOB_TOKEN_TEST12", "u/bob/preclaim", missing_account).await, + 201 + ); + assert_eq!( + linked("u/bob/preclaim").await, + None, + "an id nobody connected yet" + ); + + sqlx::query( + "UPDATE variable SET extra_perms = '{\"u/bob\": false}' + WHERE workspace_id = 'test-workspace' AND path = 'u/alice/conn'", + ) + .execute(&db) + .await?; + assert_eq!( + create("BOB_TOKEN_TEST12", "u/bob/shared", alice_account).await, + 201 + ); + assert_eq!( + linked("u/bob/shared").await, + Some(alice_account), + "bob can read a variable linking it" + ); + + Ok(()) +} diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 419baca228..b0d5c724f3 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -55,7 +55,7 @@ use crate::var_resource_cache::{ }; use lazy_static::lazy_static; use serde::Deserialize; -use sqlx::{Acquire, PgConnection, Postgres, Transaction}; +use sqlx::{Acquire, Postgres, Transaction}; use windmill_common::variables::encrypt; use windmill_git_sync::{handle_deployment_metadata, DeployedObject}; @@ -586,18 +586,22 @@ fn validate_already_encrypted_secret(path: &str, value: &str) -> Result<()> { Ok(()) } -/// Reading a variable linked to an OAuth account refreshes through that account and hands the -/// fresh token to the reader. So only the account's creator may link it, or a caller who can -/// already read a variable that does (`tx` is the caller's transaction, so RLS decides that). -async fn require_account_link_allowed( +/// The OAuth account a variable may link, of the one requested. Reading a variable linked to an +/// account refreshes through it and hands the fresh token to the reader, so only the account's +/// creator may link it, or a caller who can already read a variable that does (checked in the +/// caller's own transaction, so RLS decides). +/// +/// Anything else is dropped rather than refused: account ids are local to a workspace, so a +/// variable synced from another one carries an id that means nothing here, and must still land. +async fn permitted_account_link( db: &DB, - tx: &mut PgConnection, + user_db: &UserDB, + authed: &ApiAuthed, w_id: &str, account: Option, - username: &str, -) -> Result<()> { +) -> Result> { let Some(account) = account else { - return Ok(()); + return Ok(None); }; let created_by = sqlx::query_scalar!( "SELECT created_by FROM account WHERE workspace_id = $1 AND id = $2", @@ -607,9 +611,10 @@ async fn require_account_link_allowed( .fetch_optional(db) .await? .flatten(); - if created_by.as_deref() == Some(username) { - return Ok(()); + if created_by.as_deref() == Some(authed.username.as_str()) { + return Ok(Some(account)); } + let mut tx = user_db.clone().begin(authed).await?; let readable = sqlx::query_scalar!( "SELECT EXISTS(SELECT 1 FROM variable WHERE workspace_id = $1 AND account = $2)", w_id, @@ -618,12 +623,15 @@ async fn require_account_link_allowed( .fetch_one(&mut *tx) .await? .unwrap_or(false); + tx.commit().await?; if readable { - Ok(()) + Ok(Some(account)) } else { - Err(Error::NotAuthorized(format!( - "OAuth account {account} was connected by someone else" - ))) + tracing::warn!( + "Not linking OAuth account {account} in {w_id}: {} neither connected it nor can read a variable linking it", + authed.username + ); + Ok(None) } } @@ -686,9 +694,9 @@ async fn create_variable( variable.value }; - let mut tx = user_db.begin(&authed).await?; + let account = permitted_account_link(&db, &user_db, &authed, &w_id, variable.account).await?; - require_account_link_allowed(&db, &mut tx, &w_id, variable.account, &authed.username).await?; + let mut tx = user_db.begin(&authed).await?; sqlx::query!( "INSERT INTO variable @@ -699,7 +707,7 @@ async fn create_variable( value, variable.is_secret, variable.description, - variable.account, + account, variable.is_oauth.unwrap_or(false), variable.expires_at, variable.labels.as_deref() as Option<&[String]>, @@ -1200,11 +1208,6 @@ async fn update_variable( has_sql_updates = true; } - if let Some(account_id) = ns.account { - sqlb.set_str("account", account_id); - has_sql_updates = true; - } - if let Some(nbool) = ns.is_secret { let old_secret = sqlx::query_scalar!( "SELECT is_secret from variable WHERE path = $1 AND workspace_id = $2", @@ -1237,12 +1240,18 @@ async fn update_variable( None }; - let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; - - if ns.account != old_account_id { - require_account_link_allowed(&db, &mut tx, &w_id, ns.account, &authed.username).await?; + let account = if ns.account.is_some() && ns.account != old_account_id { + permitted_account_link(&db, &user_db, &authed, &w_id, ns.account).await? + } else { + ns.account + }; + if let Some(account_id) = account { + sqlb.set_str("account", account_id); + has_sql_updates = true; } + let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; + if let Some(npath) = ns.path.clone() { if npath != path { check_path_conflict(&db, &w_id, &npath).await?; @@ -1416,7 +1425,7 @@ async fn update_variable( // Clean up old account if it's no longer referenced and different from new account if let Some(old_acc_id) = old_account_id { - if ns.account.is_some() && ns.account != Some(old_acc_id) { + if account.is_some() && account != Some(old_acc_id) { // Check if old account is still referenced by other variables or resources let account_still_used = sqlx::query_scalar!( "SELECT EXISTS(SELECT 1 FROM variable WHERE account = $1 AND workspace_id = $2)",