From 01fc4f1568c2010af7c23929ff50108b5e1fb635 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 22 Aug 2026 11:50:30 +0200 Subject: [PATCH] fix: name the requested storage when a workspace storage lookup finds nothing (#10803) * fix: name the requested storage when a workspace storage lookup finds nothing * chore: point ee-repo-ref at the merged ee commit --- backend/ee-repo-ref.txt | 2 +- backend/tests/sign_s3_objects_authz.rs | 53 ++++++ backend/tests/workspace_storage_by_name.rs | 152 ++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 14 ++ backend/windmill-api/src/apps.rs | 25 ++- backend/windmill-api/src/job_helpers_oss.rs | 11 ++ .../windmill-api/src/triggers/http/handler.rs | 8 +- backend/windmill-object-store/src/lib.rs | 60 ++++++- backend/windmill-types/src/s3.rs | 6 + backend/windmill-worker/src/common.rs | 6 +- .../src/lib/components/S3FilePreview.svelte | 12 +- 11 files changed, 333 insertions(+), 16 deletions(-) create mode 100644 backend/tests/workspace_storage_by_name.rs diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 459c750af1..8f96f52924 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -ac02c4696ea0be6a8b8ae154ddd7521bc1c3bbc0 +d6aef91c0f7ba556befbf4addeb7674d4a9dd819 \ No newline at end of file diff --git a/backend/tests/sign_s3_objects_authz.rs b/backend/tests/sign_s3_objects_authz.rs index 9acf4352c3..a2baf9d449 100644 --- a/backend/tests/sign_s3_objects_authz.rs +++ b/backend/tests/sign_s3_objects_authz.rs @@ -11,6 +11,12 @@ //! - the non-admin CAN sign `allowed/*` (authorized), and the minted signature //! validates end-to-end through the presigned s3_proxy fetch route; //! - the non-admin CANNOT sign `secret/*` (bypass closed); +//! +//! How `storage` enters the signature is pinned in the same test function rather than its own: +//! `s3_proxy_ee.rs`'s `S3_RESOURCE_CACHE` is process-global and keyed by (workspace, storage), +//! so two test functions sharing the one fixture workspace serve each other's stale — by then +//! deleted — filesystem root. +//! //! Advanced S3 permissions are an enterprise feature, so this test requires the //! `enterprise` + `private` + `parquet` features. #![cfg(all(feature = "enterprise", feature = "private", feature = "parquet"))] @@ -120,5 +126,52 @@ async fn test_sign_s3_objects_enforces_read_authz(db: Pool) -> anyhow: "signed fetch must stream the authorized object's bytes" ); + // ---- The same object signed with an explicit `_default_` must redeem through the same URL. + let resp = authed( + client().post(format!("{base}/apps/sign_s3_objects")), + "SECRET_TOKEN", + ) + .json(&json!({ "s3_objects": [{ "s3": "allowed/file.txt", "storage": "_default_" }] })) + .send() + .await?; + let status = resp.status(); + let signed: serde_json::Value = resp.json().await?; + assert!( + status.is_success(), + "signing on the explicitly-named default storage must succeed: {status} {signed}" + ); + let presigned = signed[0]["presigned"] + .as_str() + .expect("sign must return a presigned string") + .to_string(); + + let resp = client() + .get(format!( + "{base}/s3_proxy/_default_/allowed/file.txt?{presigned}" + )) + .send() + .await?; + let status = resp.status(); + let body = resp.bytes().await?; + assert!( + status.is_success(), + "a signature minted for `_default_` must verify on redemption: {status} {:?}", + String::from_utf8_lossy(&body) + ); + assert_eq!(body.as_ref(), b"authorized payload"); + + // ---- A storage that resolves to nothing must be refused, not signed unchecked. + let resp = authed( + client().post(format!("{base}/apps/sign_s3_objects")), + "SECRET_TOKEN", + ) + .json(&json!({ "s3_objects": [{ "s3": "allowed/file.txt", "storage": "nope" }] })) + .send() + .await?; + assert!( + !resp.status().is_success(), + "an unresolvable storage must be refused, not signed unauthorized" + ); + Ok(()) } diff --git a/backend/tests/workspace_storage_by_name.rs b/backend/tests/workspace_storage_by_name.rs new file mode 100644 index 0000000000..795d3c67ae --- /dev/null +++ b/backend/tests/workspace_storage_by_name.rs @@ -0,0 +1,152 @@ +//! Regression test for resolving a workspace file storage by name: `_default_` resolves to the +//! primary storage (see `get_large_file_storage`), and a name with no storage behind it is +//! reported with the name rather than as a workspace with no storage at all. +//! +//! Pinned against a FilesystemStorage LFS so the test needs no object store. +#![cfg(all(feature = "private", feature = "parquet"))] + +use serde_json::json; +use sqlx::{Pool, Postgres}; +use windmill_test_utils::*; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + builder.header("Authorization", format!("Bearer {}", token)) +} + +/// Configure the workspace's primary storage as a filesystem store, with no secondary +/// storage at all — so `_default_` can only work by naming the primary one. +async fn configure_primary_lfs(db: &Pool, root_path: &str) -> anyhow::Result<()> { + let lfs_config = json!({ + "type": "FilesystemStorage", + "root_path": root_path, + "public_resource": null, + "advanced_permissions": null + }); + sqlx::query!( + "UPDATE workspace_settings SET large_file_storage = $1 WHERE workspace_id = $2", + lfs_config, + "test-workspace" + ) + .execute(db) + .await?; + Ok(()) +} + +#[sqlx::test(fixtures("base"))] +async fn test_workspace_storage_resolves_by_name(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace", + server.addr.port() + ); + + let storage_dir = tempfile::tempdir()?; + configure_primary_lfs(&db, &storage_dir.path().to_string_lossy()).await?; + std::fs::write(storage_dir.path().join("file.txt"), b"primary payload")?; + + let resp = authed( + client().get(format!( + "{base}/job_helpers/download_s3_file?file_key=file.txt&storage=_default_" + )), + "SECRET_TOKEN", + ) + .send() + .await?; + let status = resp.status(); + let body = resp.bytes().await?; + assert!( + status.is_success(), + "`_default_` must resolve to the primary storage: {status} {}", + String::from_utf8_lossy(&body) + ); + assert_eq!(body.as_ref(), b"primary payload"); + + let resp = authed( + client().get(format!( + "{base}/job_helpers/download_s3_file?file_key=file.txt&storage=nope" + )), + "SECRET_TOKEN", + ) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert!( + body.contains("nope"), + "the error for an unknown storage must name it: {body}" + ); + // Must stay a 400: the message echoes back a caller-supplied name, so one like + // `archive not found` reaches the asset previewer's "not found" substring test, and only + // the 400 stops it rendering as the "object not written yet" empty state instead + // (`S3FilePreview.svelte`, `isNotFoundError`). + assert_eq!(status, 400, "got {status}: {body}"); + + Ok(()) +} + +/// `_default_` resolving to the primary storage is only safe because no secondary storage can +/// carry that name: one that did would be shadowed, silently moving the workspace's reads and +/// writes to another bucket. The reservation is what makes the resolution above sound. +#[sqlx::test(fixtures("base"))] +async fn test_default_is_reserved_as_a_secondary_storage_name( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace", + server.addr.port() + ); + let storage_dir = tempfile::tempdir()?; + let root = storage_dir.path().to_string_lossy().to_string(); + + let secondary = |name: &str| { + json!({ + "large_file_storage": { + "type": "FilesystemStorage", + "root_path": root, + "advanced_permissions": null, + "secondary_storage": { + name: { + "type": "FilesystemStorage", + "root_path": root, + "advanced_permissions": null + } + } + } + }) + }; + + let resp = authed( + client().post(format!("{base}/workspaces/edit_large_file_storage_config")), + "SECRET_TOKEN", + ) + .json(&secondary("_default_")) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "`_default_` must be refused as a secondary storage name: {}", + resp.text().await? + ); + + // Any other name still saves, so the check is not blanket-rejecting secondary storages. + let resp = authed( + client().post(format!("{base}/workspaces/edit_large_file_storage_config")), + "SECRET_TOKEN", + ) + .json(&secondary("archive")) + .send() + .await?; + assert!(resp.status().is_success(), "{}", resp.text().await?); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index d21591cb42..420a38353e 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -1956,6 +1956,20 @@ async fn edit_large_file_storage_config( .await?; if let Some(lfs_config) = new_config.large_file_storage { + // `_default_` names the primary storage everywhere else — `get_secondary_storage_names` + // hands it out, the s3-proxy URL carries it, the clients fall back to it — so a secondary + // storage of that name is unreachable by design and would shadow the primary for anything + // that resolves the name. Reject it at the only route that creates one. + if lfs_config + .secondary_storage + .contains_key(windmill_types::s3::DEFAULT_STORAGE) + { + return Err(Error::BadRequest(format!( + "`{}` is reserved for the primary storage and cannot name a secondary one", + windmill_types::s3::DEFAULT_STORAGE + ))); + } + let serialized_lfs_config = serde_json::to_value::(lfs_config) .map_err(|err| Error::internal_err(err.to_string()))?; diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 4a581443ad..c6b53f84b6 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -25,7 +25,7 @@ use crate::{ #[cfg(feature = "parquet")] use crate::{ job_helpers_oss::{ - download_s3_file_internal, get_random_file_name, get_s3_resource, + download_s3_file_internal, get_large_file_storage, get_random_file_name, get_s3_resource, get_workspace_s3_resource_and_check_paths, upload_file_from_req, DownloadFileQuery, LoadCountQuery, LoadFileMetadataQuery, LoadFilePreviewQuery, LoadPreviewQuery, }, @@ -4085,6 +4085,19 @@ async fn sign_s3_objects( // Authorize the CALLER's own read permission before signing — otherwise any workspace // member (operators included) could mint a signature for any key and bypass the advanced // S3 permission rules. This is the fix; do NOT move the check to validation time. + // ...and refuse outright when the storage does not resolve, because + // `get_workspace_s3_resource_and_check_paths` returns early in that case, *before* the + // permission loop below runs at all. Checked here rather than on its `None` return: that + // is also `None` when the resource resolved but was denied (an RLS miss, or an instance + // bucket restriction), and those ran the loop and must keep their existing behaviour. + if get_large_file_storage(&db, &w_id, s3_object.storage.clone()) + .await? + .is_none() + { + return Err(windmill_object_store::workspace_storage_not_found( + s3_object.storage.as_deref(), + )); + } let db_with_opt_authed = DbWithOptAuthed::from_authed(&authed, db.clone(), None); get_workspace_s3_resource_and_check_paths( &db_with_opt_authed, @@ -4097,10 +4110,12 @@ async fn sign_s3_objects( .await?; let exp = (chrono::Utc::now() + chrono::Duration::hours(12)).timestamp(); - let mut message = format!("file_key={}&exp={}", s3_object.s3.clone(), exp); - if let Some(ref storage) = s3_object.storage { - message = format!("{}&storage={}", message, storage); - } + let message = format!( + "file_key={}&exp={}{}", + s3_object.s3.clone(), + exp, + windmill_object_store::s3_signature_storage_fragment(s3_object.storage.as_deref()) + ); let mut max = HmacSha256::new_from_slice(workspace_key.as_bytes()) .map_err(|err| Error::internal_err(format!("Failed to create hmac: {}", err)))?; diff --git a/backend/windmill-api/src/job_helpers_oss.rs b/backend/windmill-api/src/job_helpers_oss.rs index 6cb7a4c769..eb8986a986 100644 --- a/backend/windmill-api/src/job_helpers_oss.rs +++ b/backend/windmill-api/src/job_helpers_oss.rs @@ -346,6 +346,17 @@ mod app_s3_display_stubs { #[cfg(all(feature = "parquet", not(feature = "private")))] pub use app_s3_display_stubs::*; +#[cfg(not(feature = "private"))] +pub async fn get_large_file_storage( + _db: &DB, + _w_id: &str, + _storage: Option, +) -> windmill_common::error::Result> { + Err(windmill_common::error::Error::internal_err( + "Not implemented in Windmill's Open Source repository".to_string(), + )) +} + #[cfg(not(feature = "private"))] pub async fn get_workspace_s3_resource_and_check_paths<'c>( _db_with_opt_authed: &DbWithOptAuthed<'c, ApiAuthed>, diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index 44cff6566a..37442c8111 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -352,12 +352,12 @@ async fn route_job( &db, None, &trigger.workspace_id, - config.storage, + config.storage.clone(), ) .await?; - let s3_resource = s3_resource_opt.ok_or(Error::internal_err( - "No files storage resource defined at the workspace level".to_string(), - ))?; + let s3_resource = s3_resource_opt.ok_or_else(|| { + windmill_object_store::workspace_storage_misconfigured(config.storage.as_deref()) + })?; let s3_client = build_object_store_client(&s3_resource).await?; let path = if trigger.is_static_website { diff --git a/backend/windmill-object-store/src/lib.rs b/backend/windmill-object-store/src/lib.rs index 1481830f2a..0f919f1048 100644 --- a/backend/windmill-object-store/src/lib.rs +++ b/backend/windmill-object-store/src/lib.rs @@ -1009,7 +1009,65 @@ pub fn check_az_account_name_workspace_restriction( Ok(()) } -pub const DEFAULT_STORAGE: &str = "_default_"; +/// The `&storage=` fragment of a presigned s3 signature's HMAC message. `_default_` and an unset +/// storage name the same storage and must fold to the same fragment: a signature minted for one +/// is redeemed through a URL carrying the other, and a disagreement between signer and validator +/// surfaces only as `Invalid signature`. Both must build the message through this. +pub fn s3_signature_storage_fragment(storage: Option<&str>) -> String { + match storage.filter(|s| *s != DEFAULT_STORAGE) { + Some(name) => format!("&storage={name}"), + None => String::new(), + } +} + +/// Error for a workspace file-storage lookup that resolved to nothing. Naming the requested +/// storage separates the two causes — a name this workspace has no storage for, versus a +/// workspace with no storage configured at all — which the reader cannot otherwise tell apart. +/// The wording stays neutral about where the name came from: callers pass an s3 object's +/// `storage`, a request field, or a trigger's stored config. +/// +/// The asset previewer renders "this object has not been written yet" for a 404, and for any +/// other non-400 whose body contains "not found" (`S3FilePreview.svelte`, `isNotFoundError`). +/// So the named variant must stay a **400** — its message echoes a caller-supplied name, which +/// may itself contain "not found" — and the unnamed one must keep a message that does not. +pub fn workspace_storage_not_found(storage: Option<&str>) -> error::Error { + match workspace_storage_not_found_message(storage) { + Some(msg) => error::Error::BadRequest(msg), + None => error::Error::InternalErr( + "No files storage resource defined at the workspace level".to_string(), + ), + } +} + +/// [`workspace_storage_not_found`] for a caller whose storage name comes from stored +/// configuration rather than from the request — a trigger's static-asset config, say. Same +/// message, but a server-side class: the requester cannot correct a name they never supplied. +/// +/// Not for any route the asset previewer reads: this is the 500-with-an-interpolated-name shape +/// that `isNotFoundError` falls through to its "not found" substring test for, so a storage +/// named `archive not found` would render there as "asset not yet materialized". +#[track_caller] +pub fn workspace_storage_misconfigured(storage: Option<&str>) -> error::Error { + // `internal_err` on both arms: it is `#[track_caller]`, so the `@file:line` stamp lands on + // the handler that misconfigured the storage rather than on this helper. + error::Error::internal_err( + workspace_storage_not_found_message(storage).unwrap_or_else(|| { + "No files storage resource defined at the workspace level".to_string() + }), + ) +} + +/// `None` when the request named no storage (or named the primary one), so the caller reports +/// the workspace as having no storage configured at all. +fn workspace_storage_not_found_message(storage: Option<&str>) -> Option { + storage.filter(|s| *s != DEFAULT_STORAGE).map(|name| { + format!( + "No files storage named '{name}' is defined at the workspace level. A storage name \ + must be one of this workspace's secondary storages, or `{DEFAULT_STORAGE}` \ + (equivalently, nothing at all) for the primary one." + ) + }) +} pub fn bundle(w_id: &str, hash: &str) -> String { format!("script_bundle/{}/{}", w_id, hash) diff --git a/backend/windmill-types/src/s3.rs b/backend/windmill-types/src/s3.rs index 5a7634043c..3af4b7f99a 100644 --- a/backend/windmill-types/src/s3.rs +++ b/backend/windmill-types/src/s3.rs @@ -3,6 +3,12 @@ use serde::de::Visitor; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use std::fmt; +/// The name of a workspace's PRIMARY file storage. Reserved: a secondary storage may not carry +/// it (`edit_large_file_storage_config` rejects that), because everything that resolves a +/// storage name — the s3-proxy URL, the clients' fallback, `get_secondary_storage_names` — reads +/// it as the primary one. +pub const DEFAULT_STORAGE: &str = "_default_"; + #[derive(Debug, Deserialize, Serialize, Clone)] pub struct FilesystemSettings { pub root_path: String, diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index 3380d0abc3..6ee22fd882 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -1584,7 +1584,11 @@ pub(crate) async fn get_workspace_s3_resource_path( use windmill_object_store::job_s3_helpers_oss::get_s3_resource_internal; use windmill_types::s3::StorageResourceType; - let raw_lfs_opt = if let Some(storage) = storage { + // `_default_` names the primary storage, never a secondary one (see `DEFAULT_STORAGE`), so + // it resolves below rather than being looked up among storages that cannot carry the name. + let raw_lfs_opt = if let Some(storage) = + storage.filter(|s| s.as_str() != windmill_types::s3::DEFAULT_STORAGE) + { sqlx::query_scalar!( "SELECT large_file_storage->'secondary_storage'->$2 FROM workspace_settings WHERE workspace_id = $1", workspace_id, diff --git a/frontend/src/lib/components/S3FilePreview.svelte b/frontend/src/lib/components/S3FilePreview.svelte index a9c168d68c..62c95581e7 100644 --- a/frontend/src/lib/components/S3FilePreview.svelte +++ b/frontend/src/lib/components/S3FilePreview.svelte @@ -97,13 +97,17 @@ function isNotFoundError(err: any): boolean { // HelpersService surfaces backend errors as ApiError with a `status` - // field plus a serialized body. We accept either a 404 status or a - // "not found" substring (case-insensitive) to be robust against - // future error wrapping changes. + // field plus a serialized body. A missing object arrives as a 500 that + // merely *says* "not found" (`load_file_metadata` wraps the object-store + // error), so the substring test carries this and cannot be dropped. 400 + // must short-circuit ahead of it: those messages echo back a + // caller-supplied storage name, and one like `archive not found` would + // otherwise read as a missing object and hide the diagnostic. const status = err?.status ?? err?.response?.status if (status === 404) return true + if (status === 400) return false const body = String(err?.body ?? err?.message ?? err ?? '').toLowerCase() - return body.includes('not found') || body.includes('404') + return body.includes('not found') } // Reload whenever the file key, workspace, or external refreshKey