mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-06 00:02:13 +00:00
fix: require admin on workspace tarball settings export (#10817)
* fix: require admin on workspace tarball settings export Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: name the refused flag in the settings export error Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
665f83e1f4
commit
46c363ffa4
@@ -1,6 +1,6 @@
|
||||
use sqlx::postgres::Postgres;
|
||||
use sqlx::Pool;
|
||||
use windmill_test_utils::{initialize_tracing, ApiServer};
|
||||
use windmill_test_utils::{initialize_tracing, set_jwt_secret, ApiServer};
|
||||
|
||||
/// Integration test: exercises every explicit-column query in `tarball_workspace`.
|
||||
///
|
||||
@@ -287,3 +287,77 @@ async fn test_tarball_export_gates_values_on_item_scopes(db: Pool<Postgres>) ->
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// `settings.json` carries the admin-managed integration config that `get_settings`
|
||||
/// is admin-only for (the webhook URL, ai_config, git_sync, handler extra_args), so
|
||||
/// `include_settings` takes the same admin check as `get_settings` rather than
|
||||
/// riding on the route's `workspaces:read`. Git sync exports settings through the
|
||||
/// same route, so the gate must still admit its system identity.
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn test_tarball_export_settings_are_admin_only(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
set_jwt_secret().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let base_url = format!("http://localhost:{}", server.addr.port());
|
||||
|
||||
sqlx::query(
|
||||
r#"UPDATE workspace_settings
|
||||
SET webhook = 'https://hook.example/?token=WEBHOOK_SECRET',
|
||||
ai_config = '{"providers":{"openai":{"api_key":"AI_CONFIG_SECRET"}}}'::jsonb
|
||||
WHERE workspace_id = 'test-workspace'"#,
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
let export = async |token: &str| -> anyhow::Result<(u16, String)> {
|
||||
let resp = reqwest::Client::new()
|
||||
.get(format!(
|
||||
"{base_url}/api/w/test-workspace/workspaces/tarball?include_settings=true&settings_version=v2"
|
||||
))
|
||||
.bearer_auth(token)
|
||||
.send()
|
||||
.await?;
|
||||
let status = resp.status().as_u16();
|
||||
// Lossy: a successful export is a tar, not UTF-8. Only the values matter here.
|
||||
Ok((
|
||||
status,
|
||||
String::from_utf8_lossy(&resp.bytes().await?).into_owned(),
|
||||
))
|
||||
};
|
||||
|
||||
// SECRET_TOKEN_2 belongs to test-user-2, a non-admin member of test-workspace.
|
||||
let (status, body) = export("SECRET_TOKEN_2").await?;
|
||||
assert_eq!(status, 403, "non-admin exported settings: {body}");
|
||||
|
||||
let (status, body) = export("SECRET_TOKEN").await?;
|
||||
assert_eq!(status, 200, "admin denied settings: {body}");
|
||||
assert!(
|
||||
body.contains("WEBHOOK_SECRET") && body.contains("AI_CONFIG_SECRET"),
|
||||
"admin got no settings"
|
||||
);
|
||||
|
||||
// Git sync pushes the workspace to the repo by exporting it under
|
||||
// `superadmin_sync@windmill.dev`, which belongs to no workspace: the job token
|
||||
// it runs with is the export's only admin claim.
|
||||
let sync_email = windmill_common::users::SUPERADMIN_SYNC_EMAIL;
|
||||
let sync_token = windmill_common::auth::create_token_for_owner(
|
||||
&db,
|
||||
"test-workspace",
|
||||
sync_email,
|
||||
"git-sync",
|
||||
300,
|
||||
sync_email,
|
||||
&uuid::Uuid::new_v4(),
|
||||
None,
|
||||
None,
|
||||
)
|
||||
.await?;
|
||||
let (status, body) = export(&sync_token).await?;
|
||||
assert_eq!(status, 200, "git-sync identity denied settings: {body}");
|
||||
assert!(
|
||||
body.contains("WEBHOOK_SECRET"),
|
||||
"git-sync identity got no settings"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -643,6 +643,16 @@ pub(crate) async fn tarball_workspace(
|
||||
windmill_api_auth::forbid_scoped_token_workspace_key(&authed)?;
|
||||
}
|
||||
|
||||
// settings.json carries the admin-managed integration config that `get_settings`
|
||||
// is admin-only for (ai_config, the webhook URL, git_sync, handler extra_args),
|
||||
// so it takes the same check. Not a per-field redaction: fields silently dropped
|
||||
// from settings.json come back as null on the next `wmill sync push`.
|
||||
if include_settings.unwrap_or(false) && !authed.is_admin {
|
||||
return Err(Error::PermissionDenied(
|
||||
"include_settings requires workspace admin".to_string(),
|
||||
));
|
||||
}
|
||||
|
||||
// The route is gated by workspaces:read, but the tarball also carries the item
|
||||
// values that the per-item routes gate on their own domain (get_resource_value,
|
||||
// get_variable). A whole-workspace export cannot be confined to a path, so it
|
||||
@@ -1626,13 +1636,7 @@ pub(crate) async fn tarball_workspace(
|
||||
slack_name: row.slack_name.clone(),
|
||||
slack_command_script: row.slack_command_script.clone(),
|
||||
slack_oauth_client_id: row.slack_oauth_client_id.clone(),
|
||||
// Mirror the non-admin redaction in `get_settings`: the OAuth
|
||||
// client secret is admin-only and must not leak via tarball.
|
||||
slack_oauth_client_secret: if authed.is_admin {
|
||||
row.slack_oauth_client_secret.clone()
|
||||
} else {
|
||||
None
|
||||
},
|
||||
slack_oauth_client_secret: row.slack_oauth_client_secret.clone(),
|
||||
};
|
||||
serde_json::to_value(settings)
|
||||
.map(|v| serde_json::to_string_pretty(&v).ok())
|
||||
|
||||
Reference in New Issue
Block a user