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

@@ -1 +1 @@
-ac02c4696ea0be6a8b8ae154ddd7521bc1c3bbc0
+d6aef91c0f7ba556befbf4addeb7674d4a9dd819

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<Postgres>) -> 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(())
 }

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<Postgres>, 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<Postgres>) -> 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<Postgres>,
+) -> 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(())
+}

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::<LargeFileStorageWithSecondary>(lfs_config)
                 .map_err(|err| Error::internal_err(err.to_string()))?;

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)))?;

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<String>,
+) -> windmill_common::error::Result<Option<windmill_types::s3::LargeFileStorage>> {
+    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>,

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 {

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<String> {
+    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)

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,

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,

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