starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-10-17 08:03:06 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: drop an oauth account link the caller may not hold instead of failing

Browse Source 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
hugocasa
2026-10-15 16:32:00 +02:00
co-authored by Claude Opus 5.5
parent d36db20f9f
commit 9ffd24cc06
2 changed files with 125 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/windmill-api-integration-tests/tests/variables.rs
+88
View File
@@ -338,3 +338,91 @@ async fn test_variables_are_cached(db: Pool<Postgres>) -> 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<Postgres>) -> 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<i32>>(
"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(())
}
backend/windmill-store/src/variables.rs
+37 -28
View File
@@ -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<i32>,
username: &str,
) -> Result<()> {
) -> Result<Option<i32>> {
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)",