fix(editor): download attachments without blank web tab (#3752)

* fix(editor): download attachments without blank web tab Co-authored-by: multica-agent <github@multica.ai> * fix(attachments): preserve workspace in web download URLs Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: multica-agent <github@multica.ai>
2026-06-16 19:29:26 +02:00 · 2026-06-04 15:45:42 +08:00
parent 7fdec9e6e4
commit 569b43136c
10 changed files with 481 additions and 65 deletions
--- a/packages/views/editor/use-download-attachment.test.tsx
+++ b/packages/views/editor/use-download-attachment.test.tsx
@@ -5,11 +5,18 @@ import { act, renderHook, waitFor } from "@testing-library/react";
 // outside-of-scope vars, but vi.hoisted runs before the import graph.
 const getAttachmentMock = vi.hoisted(() => vi.fn());
 const getBaseUrlMock = vi.hoisted(() => vi.fn(() => ""));
+const useWorkspaceSlugMock = vi.hoisted(() =>
+  vi.fn<() => string | null>(() => "acme"),
+);

 vi.mock("@multica/core/api", () => ({
  api: { getAttachment: getAttachmentMock, getBaseUrl: getBaseUrlMock },
 }));

+vi.mock("@multica/core/paths", () => ({
+  useWorkspaceSlug: useWorkspaceSlugMock,
+}));
+
 vi.mock("sonner", () => ({
  toast: { error: vi.fn(), success: vi.fn() },
 }));
@@ -30,6 +37,7 @@ beforeEach(() => {
  // a non-empty base (desktop standalone, server-relative download URL)
  // overrides via getBaseUrlMock.mockReturnValue(...).
  getBaseUrlMock.mockReturnValue("");
+  useWorkspaceSlugMock.mockReturnValue("acme");
 });

 afterEach(() => {
@@ -39,16 +47,94 @@ afterEach(() => {
 });

 describe("useDownloadAttachment (web)", () => {
-  it("opens a placeholder tab synchronously, then navigates it to the freshly signed URL", async () => {
+  it("clicks the unified download endpoint without opening a blank tab or buffering a Blob", async () => {
+    getAttachmentMock.mockResolvedValueOnce({
+      id: "att-1",
+      url: "https://static.example.test/file.md",
+      // CloudFront mode may still return a signed CDN URL from metadata;
+      // Web download must ignore it and enter through the same-origin
+      // endpoint so the server owns cloudfront/presign/proxy selection.
+      download_url: SIGNED_URL,
+      filename: "file.md",
+    });
+
+    const openSpy = vi.spyOn(window, "open");
+    const clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, "click")
+      .mockImplementation(() => {});
+    const appendSpy = vi.spyOn(document.body, "appendChild");
+
+    const { result } = renderHook(() => useDownloadAttachment());
+
+    await act(async () => {
+      await result.current("att-1");
+    });
+
+    expect(getAttachmentMock).toHaveBeenCalledWith("att-1");
+    expect(openSpy).not.toHaveBeenCalled();
+    expect(clickSpy).toHaveBeenCalledOnce();
+
+    const anchor = appendSpy.mock.calls
+      .map(([node]) => node)
+      .find((node): node is HTMLAnchorElement =>
+        node instanceof HTMLAnchorElement,
+      );
+    expect(anchor).toBeDefined();
+    expect(anchor!.getAttribute("href")).toBe(
+      "/api/attachments/att-1/download?workspace_slug=acme",
+    );
+    expect(anchor!.href).toBe(
+      "http://localhost:3000/api/attachments/att-1/download?workspace_slug=acme",
+    );
+    // Empty download attribute intentionally defers the final filename to the
+    // endpoint / redirected object Content-Disposition header.
+    expect(anchor!.getAttribute("download")).toBe("");
+    expect(anchor!.isConnected).toBe(false);
+  });
+
+  it("resolves the unified download endpoint against a configured API base", async () => {
+    getBaseUrlMock.mockReturnValue("https://api.example.test/");
+    getAttachmentMock.mockResolvedValueOnce({
+      id: "att 1/slash",
+      url: "https://static.example.test/file.md",
+      download_url: SIGNED_URL,
+      filename: "file.md",
+    });
+    const clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, "click")
+      .mockImplementation(() => {});
+    const appendSpy = vi.spyOn(document.body, "appendChild");
+
+    const { result } = renderHook(() => useDownloadAttachment());
+
+    await act(async () => {
+      await result.current("att 1/slash");
+    });
+
+    expect(clickSpy).toHaveBeenCalledOnce();
+    const anchor = appendSpy.mock.calls
+      .map(([node]) => node)
+      .find((node): node is HTMLAnchorElement =>
+        node instanceof HTMLAnchorElement,
+      );
+    expect(anchor).toBeDefined();
+    expect(anchor!.href).toBe(
+      "https://api.example.test/api/attachments/att%201%2Fslash/download?workspace_slug=acme",
+    );
+  });
+
+  it("encodes the workspace slug into the bare navigation URL instead of relying on custom headers", async () => {
+    useWorkspaceSlugMock.mockReturnValueOnce("team/space");
    getAttachmentMock.mockResolvedValueOnce({
      id: "att-1",
      url: "https://static.example.test/file.md",
      download_url: SIGNED_URL,
      filename: "file.md",
    });
-
-    const placeholder = { opener: window, location: { href: "about:blank" }, close: vi.fn() };
-    const openSpy = vi.spyOn(window, "open").mockReturnValue(placeholder as unknown as Window);
+    const clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, "click")
+      .mockImplementation(() => {});
+    const appendSpy = vi.spyOn(document.body, "appendChild");

    const { result } = renderHook(() => useDownloadAttachment());

@@ -56,19 +142,29 @@ describe("useDownloadAttachment (web)", () => {
      await result.current("att-1");
    });

-    // Placeholder MUST be opened synchronously during the click — otherwise
-    // popup blockers won't honour the gesture.
-    expect(openSpy).toHaveBeenCalledWith("about:blank", "_blank");
-    expect(getAttachmentMock).toHaveBeenCalledWith("att-1");
-    // Disown the opener and redirect to the signed URL.
-    expect(placeholder.opener).toBeNull();
-    expect(placeholder.location.href).toBe(SIGNED_URL);
+    expect(clickSpy).toHaveBeenCalledOnce();
+    const anchor = appendSpy.mock.calls
+      .map(([node]) => node)
+      .find((node): node is HTMLAnchorElement =>
+        node instanceof HTMLAnchorElement,
+      );
+    expect(anchor).toBeDefined();
+    expect(anchor!.getAttribute("href")).toBe(
+      "/api/attachments/att-1/download?workspace_slug=team%2Fspace",
+    );
  });

-  it("closes the placeholder and shows a toast when the fetch fails", async () => {
-    getAttachmentMock.mockRejectedValueOnce(new Error("boom"));
-    const placeholder = { opener: window, location: { href: "about:blank" }, close: vi.fn() };
-    vi.spyOn(window, "open").mockReturnValue(placeholder as unknown as Window);
+  it("shows a toast and does not click a download link when the workspace slug is missing", async () => {
+    useWorkspaceSlugMock.mockReturnValueOnce(null);
+    getAttachmentMock.mockResolvedValueOnce({
+      id: "att-1",
+      url: "https://static.example.test/file.md",
+      download_url: SIGNED_URL,
+      filename: "file.md",
+    });
+    const clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, "click")
+      .mockImplementation(() => {});

    const { result } = renderHook(() => useDownloadAttachment());

@@ -76,7 +172,25 @@ describe("useDownloadAttachment (web)", () => {
      await result.current("att-1");
    });

-    expect(placeholder.close).toHaveBeenCalled();
+    expect(clickSpy).not.toHaveBeenCalled();
+    await waitFor(() => expect(toast.error).toHaveBeenCalled());
+  });
+
+  it("shows a toast and does not click a download link when the metadata preflight fails", async () => {
+    getAttachmentMock.mockRejectedValueOnce(new Error("boom"));
+    const openSpy = vi.spyOn(window, "open");
+    const clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, "click")
+      .mockImplementation(() => {});
+
+    const { result } = renderHook(() => useDownloadAttachment());
+
+    await act(async () => {
+      await result.current("att-1");
+    });
+
+    expect(openSpy).not.toHaveBeenCalled();
+    expect(clickSpy).not.toHaveBeenCalled();
    await waitFor(() => expect(toast.error).toHaveBeenCalled());
  });
 });
--- a/packages/views/editor/use-download-attachment.ts
+++ b/packages/views/editor/use-download-attachment.ts
@@ -3,6 +3,7 @@
 import { useCallback } from "react";
 import { toast } from "sonner";
 import { api } from "@multica/core/api";
+import { useWorkspaceSlug } from "@multica/core/paths";
 import { resolvePublicFileUrl } from "@multica/core/workspace/avatar-url";
 import { useT } from "../i18n";

@@ -10,6 +11,33 @@ interface DesktopBridge {
  downloadURL?: (u: string) => Promise<void> | void;
 }

+function attachmentDownloadEndpoint(
+  attachmentId: string,
+  workspaceSlug: string,
+): string {
+  const params = new URLSearchParams({ workspace_slug: workspaceSlug });
+  const path = `/api/attachments/${encodeURIComponent(attachmentId)}/download`;
+  const endpoint = `${path}?${params.toString()}`;
+  return resolvePublicFileUrl(endpoint) ?? endpoint;
+}
+
+function triggerBrowserDownload(url: string): void {
+  const anchor = document.createElement("a");
+  anchor.href = url;
+  // Keep the click in the current browsing context. For same-origin API
+  // downloads this hint lets Chromium/Safari use Content-Disposition's
+  // filename without opening a blank tab. If the endpoint later 302s to
+  // CloudFront/S3, the server signs that redirect with an attachment
+  // disposition; the browser follows it natively without buffering the file
+  // into JS memory.
+  anchor.download = "";
+  anchor.rel = "noopener";
+  anchor.style.display = "none";
+  document.body.appendChild(anchor);
+  anchor.click();
+  anchor.remove();
+}
+
 // Detected at call time, not module load — the bridge is injected by the
 // Electron preload after `window` exists, and reading it lazily lets the
 // same hook work in both renderers without a build-time fork.
@@ -20,20 +48,18 @@ function hasDesktopDownloadBridge(): boolean {
 }

 /**
- * Returns a callback that downloads an attachment by ID through a freshly
- * signed CloudFront URL. The server re-signs `download_url` on every
- * `GET /api/attachments/{id}` call, so this flow sidesteps stale signatures
- * cached in TanStack Query / inlined in markdown.
+ * Returns a callback that downloads an attachment by ID. The Web path uses
+ * the unified server endpoint directly instead of opening a blank tab or
+ * materializing the file as a Blob in renderer memory.
 *
 * Two execution shapes, picked at call time:
 *
- * - **Web**: open a same-origin `about:blank` tab *synchronously* inside
- *   the click handler — popup blockers (Safari especially) only consider
- *   the gesture frame, not the later async settle. The placeholder tab
- *   keeps the user activation receipt; after the fetch resolves we navigate
- *   it. We can NOT pass `"noopener"` to `window.open` because the HTML
- *   spec (`dom-open` step 17) makes that return `null`, which would leave
- *   us nothing to navigate. We disown the opener manually after the fetch.
+ * - **Web**: first refreshes attachment metadata for the existing error
+ *   feedback path, then clicks a temporary same-origin
+ *   `/api/attachments/{id}/download?workspace_slug=...` anchor. The backend
+ *   endpoint owns CloudFront / S3 presign / proxy selection and download
+ *   Content-Disposition, so large files stay in the browser's native download
+ *   pipeline.
 *
 * - **Desktop**: uses `desktopAPI.downloadURL()` which invokes Electron's
 *   native `webContents.downloadURL()`, showing a save dialog and saving
@@ -43,6 +69,7 @@ function hasDesktopDownloadBridge(): boolean {
 */
 export function useDownloadAttachment(): (attachmentId: string) => Promise<void> {
  const { t } = useT("editor");
+  const workspaceSlug = useWorkspaceSlug();
  return useCallback(
    async (attachmentId: string) => {
      const failed = () => toast.error(t(($) => $.attachment.download_failed));
@@ -73,41 +100,28 @@ export function useDownloadAttachment(): (attachmentId: string) => Promise<void>
        return;
      }

-      // Web: claim the popup permission synchronously, then hydrate the URL.
-      // `window.open` here returns a WindowProxy because we deliberately
-      // omit `noopener`; we revoke the back-channel ourselves once we have
-      // the real URL.
-      const placeholder = typeof window !== "undefined"
-        ? window.open("about:blank", "_blank")
-        : null;
      try {
-        const fresh = await api.getAttachment(attachmentId);
-        // Same relative-URL handling as desktop above. For web the
-        // `apiBaseUrl` is typically empty (same-origin), so `resolvePublicFileUrl`
-        // returns the path unchanged and the browser resolves it against
-        // the page's origin — the existing same-origin behaviour. When a
-        // non-empty base is configured (e.g. tauri/standalone shells that
-        // bundle the SPA but talk to a remote API), the resolver yields an
-        // absolute URL, which works for both `placeholder.location.href`
-        // and the last-resort `window.location.href` redirect.
-        const downloadUrl = resolvePublicFileUrl(fresh.download_url);
-        if (!downloadUrl) {
-          placeholder?.close();
+        // Keep the preflight metadata request so permission/API failures still
+        // produce the existing toast instead of a silent failed navigation. Do
+        // not use `download_url` here: in CloudFront mode it may already be a
+        // signed CDN URL, while the unified endpoint is the stable browser
+        // entry point that chooses cloudfront / presign / proxy server-side.
+        await api.getAttachment(attachmentId);
+        if (typeof document === "undefined") {
          failed();
          return;
        }
-        if (placeholder) {
-          placeholder.opener = null;
-          placeholder.location.href = downloadUrl;
-        } else if (typeof window !== "undefined") {
-          // Popup blocked outright — last-resort navigate the current tab.
-          window.location.href = downloadUrl;
+        if (!workspaceSlug) {
+          failed();
+          return;
        }
+        triggerBrowserDownload(
+          attachmentDownloadEndpoint(attachmentId, workspaceSlug),
+        );
      } catch {
-        placeholder?.close();
        failed();
      }
    },
-    [t],
+    [t, workspaceSlug],
  );
 }
--- a/server/internal/auth/cloudfront.go
+++ b/server/internal/auth/cloudfront.go
@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"log/slog"
 	"net/http"
+	"net/url"
 	"os"
 	"strings"
 	"time"
@@ -195,6 +196,20 @@ func (s *CloudFrontSigner) SignedURL(rawURL string, expiry time.Time) string {
 	return fmt.Sprintf("%s%sPolicy=%s&Signature=%s&Key-Pair-Id=%s", rawURL, separator, encodedPolicy, encodedSig, s.keyPairID)
 }

+func (s *CloudFrontSigner) SignedURLWithContentDisposition(rawURL string, contentDisposition string, expiry time.Time) string {
+	if contentDisposition == "" {
+		return s.SignedURL(rawURL, expiry)
+	}
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return s.SignedURL(rawURL, expiry)
+	}
+	q := u.Query()
+	q.Set("response-content-disposition", contentDisposition)
+	u.RawQuery = q.Encode()
+	return s.SignedURL(u.String(), expiry)
+}
+
 // cfBase64Encode applies CloudFront's URL-safe base64 encoding.
 func cfBase64Encode(data []byte) string {
 	encoded := base64.StdEncoding.EncodeToString(data)
--- a/server/internal/auth/cloudfront_test.go
+++ b/server/internal/auth/cloudfront_test.go
@@ -0,0 +1,57 @@
+package auth
+
+import (
+	"crypto/rand"
+	"crypto/rsa"
+	"encoding/base64"
+	"net/url"
+	"strings"
+	"testing"
+	"time"
+)
+
+func decodeCloudFrontBase64(t *testing.T, encoded string) string {
+	t.Helper()
+	standard := strings.NewReplacer("-", "+", "_", "=", "~", "/").Replace(encoded)
+	decoded, err := base64.StdEncoding.DecodeString(standard)
+	if err != nil {
+		t.Fatalf("decode CloudFront base64: %v", err)
+	}
+	return string(decoded)
+}
+
+func TestCloudFrontSignedURLWithContentDisposition(t *testing.T) {
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	signer := &CloudFrontSigner{
+		keyPairID:  "K123",
+		privateKey: key,
+	}
+
+	got := signer.SignedURLWithContentDisposition(
+		"https://static.example.test/uploads/report.md?existing=1",
+		`attachment; filename="report.md"`,
+		time.Unix(1893456000, 0),
+	)
+	u, err := url.Parse(got)
+	if err != nil {
+		t.Fatalf("parse signed URL: %v", err)
+	}
+	q := u.Query()
+	if got := q.Get("response-content-disposition"); got != `attachment; filename="report.md"` {
+		t.Fatalf("response-content-disposition = %q", got)
+	}
+	if got := q.Get("Key-Pair-Id"); got != "K123" {
+		t.Fatalf("Key-Pair-Id = %q", got)
+	}
+	if q.Get("Signature") == "" {
+		t.Fatalf("missing Signature in %q", got)
+	}
+
+	policy := decodeCloudFrontBase64(t, q.Get("Policy"))
+	if !strings.Contains(policy, "response-content-disposition=attachment%3B+filename%3D%22report.md%22") {
+		t.Fatalf("policy did not include signed response-content-disposition: %s", policy)
+	}
+}
--- a/server/internal/handler/file.go
+++ b/server/internal/handler/file.go
@@ -442,14 +442,28 @@ func (h *Handler) DownloadAttachment(w http.ResponseWriter, r *http.Request) {
 			writeError(w, http.StatusInternalServerError, "cloudfront attachment downloads are not configured")
 			return
 		}
-		http.Redirect(w, r, h.CFSigner.SignedURL(att.Url, time.Now().Add(h.attachmentDownloadURLTTL())), http.StatusFound)
+		http.Redirect(
+			w,
+			r,
+			h.CFSigner.SignedURLWithContentDisposition(
+				att.Url,
+				storage.AttachmentContentDisposition(att.Filename),
+				time.Now().Add(h.attachmentDownloadURLTTL()),
+			),
+			http.StatusFound,
+		)
 	case attachmentDownloadModePresign:
-		presigner, ok := h.Storage.(storage.Presigner)
+		presigner, ok := h.Storage.(storage.DownloadPresigner)
 		if !ok {
 			writeError(w, http.StatusInternalServerError, "attachment storage does not support presigned downloads")
 			return
 		}
-		signedURL, err := presigner.PresignGet(r.Context(), key, h.attachmentDownloadURLTTL())
+		signedURL, err := presigner.PresignGetWithContentDisposition(
+			r.Context(),
+			key,
+			h.attachmentDownloadURLTTL(),
+			storage.AttachmentContentDisposition(att.Filename),
+		)
 		if err != nil {
 			slog.Error("failed to presign attachment download", "id", uuidToString(att.ID), "key", key, "error", err)
 			writeError(w, http.StatusBadGateway, "failed to create download URL")
@@ -478,7 +492,7 @@ func (h *Handler) resolveAttachmentDownloadMode(rawURL string) attachmentDownloa
 	if shouldProxyAttachmentURL(rawURL) {
 		return attachmentDownloadModeProxy
 	}
-	if _, ok := h.Storage.(storage.Presigner); ok {
+	if _, ok := h.Storage.(storage.DownloadPresigner); ok {
 		return attachmentDownloadModePresign
 	}
 	return attachmentDownloadModeProxy
@@ -547,8 +561,9 @@ func (h *Handler) proxyAttachmentDownload(w http.ResponseWriter, r *http.Request
 // Exists to (a) bypass CloudFront CORS (not configured) and (b) bypass
 // Content-Disposition: attachment which Chromium honors for iframe document
 // loads. Media types (image/video/audio/pdf) intentionally use download_url
-// instead; /download signs or streams them with Content-Disposition: inline
-// using the same media-type policy as storage uploads.
+// instead. Metadata download_url keeps CloudFront/S3's media preview behavior;
+// the explicit /download route signs redirects as attachment downloads and
+// proxy mode streams with the same media-type policy as storage uploads.
 //
 // Hard cap: 2 MB. Larger files return 413. Anything outside the text
 // whitelist returns 415.
--- a/server/internal/handler/file_test.go
+++ b/server/internal/handler/file_test.go
@@ -3,18 +3,26 @@ package handler
 import (
 	"bytes"
 	"context"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"encoding/base64"
 	"encoding/json"
+	"encoding/pem"
 	"fmt"
 	"io"
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"sync"
 	"testing"
 	"time"

 	"github.com/go-chi/chi/v5"
+	"github.com/multica-ai/multica/server/internal/auth"
+	"github.com/multica-ai/multica/server/internal/middleware"
 	db "github.com/multica-ai/multica/server/pkg/db/generated"
 )

@@ -43,9 +51,10 @@ func createHandlerTestChatSession(t *testing.T, agentID string) string {
 // strips the synthetic CDN host so consumers can pass either the URL or the
 // raw key.
 type mockStorage struct {
-	mu           sync.Mutex
-	files        map[string][]byte
-	presignCalls []string
+	mu                  sync.Mutex
+	files               map[string][]byte
+	presignCalls        []string
+	presignDispositions []string
 }

 func (m *mockStorage) Upload(_ context.Context, key string, data []byte, _ string, _ string) (string, error) {
@@ -91,6 +100,24 @@ func (m *mockStorage) PresignGet(_ context.Context, key string, _ time.Duration)
 	m.presignCalls = append(m.presignCalls, key)
 	return "https://signed.example.com/" + key + "?X-Amz-Signature=mock", nil
 }
+func (m *mockStorage) PresignGetWithContentDisposition(_ context.Context, key string, _ time.Duration, contentDisposition string) (string, error) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	m.presignCalls = append(m.presignCalls, key)
+	m.presignDispositions = append(m.presignDispositions, contentDisposition)
+	u := url.URL{
+		Scheme: "https",
+		Host:   "signed.example.com",
+		Path:   "/" + key,
+	}
+	q := u.Query()
+	q.Set("X-Amz-Signature", "mock")
+	if contentDisposition != "" {
+		q.Set("response-content-disposition", contentDisposition)
+	}
+	u.RawQuery = q.Encode()
+	return u.String(), nil
+}
 func (m *mockStorage) put(key string, data []byte) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
@@ -407,6 +434,35 @@ func newDownloadRequest(t *testing.T, attachmentID, workspaceID string) (*http.R
 	return req, httptest.NewRecorder()
 }

+func newDownloadRouter() http.Handler {
+	r := chi.NewRouter()
+	r.Use(middleware.RequireWorkspaceMember(testHandler.Queries))
+	r.Get("/api/attachments/{id}/download", testHandler.DownloadAttachment)
+	return r
+}
+
+func testCloudFrontSigner(t *testing.T) *auth.CloudFrontSigner {
+	t.Helper()
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("generate CloudFront test key: %v", err)
+	}
+	pemBytes := pem.EncodeToMemory(&pem.Block{
+		Type:  "RSA PRIVATE KEY",
+		Bytes: x509.MarshalPKCS1PrivateKey(key),
+	})
+	t.Setenv("CLOUDFRONT_KEY_PAIR_ID", "KTEST")
+	t.Setenv("CLOUDFRONT_DOMAIN", "static.example.test")
+	t.Setenv("COOKIE_DOMAIN", ".example.test")
+	t.Setenv("CLOUDFRONT_PRIVATE_KEY", base64.StdEncoding.EncodeToString(pemBytes))
+	t.Setenv("CLOUDFRONT_PRIVATE_KEY_SECRET", "")
+	signer := auth.NewCloudFrontSignerFromEnv()
+	if signer == nil {
+		t.Fatal("expected CloudFront signer")
+	}
+	return signer
+}
+
 func TestAttachmentToResponse_NonCloudFrontUsesDownloadEndpoint(t *testing.T) {
 	origSigner := testHandler.CFSigner
 	testHandler.CFSigner = nil
@@ -430,6 +486,94 @@ func TestAttachmentToResponse_NonCloudFrontUsesDownloadEndpoint(t *testing.T) {
 	}
 }

+func TestDownloadAttachment_CloudFrontRedirectSignsAttachmentDisposition(t *testing.T) {
+	origStorage := testHandler.Storage
+	origCfg := testHandler.cfg
+	origSigner := testHandler.CFSigner
+	testHandler.Storage = &mockStorage{}
+	testHandler.cfg.AttachmentDownloadMode = "cloudfront"
+	testHandler.CFSigner = testCloudFrontSigner(t)
+	t.Cleanup(func() {
+		testHandler.Storage = origStorage
+		testHandler.cfg = origCfg
+		testHandler.CFSigner = origSigner
+	})
+
+	id := seedAttachmentURL(t, "https://static.example.test/downloads/cloudfront.md", "cloud front.md", "text/markdown", 10)
+
+	req, w := newDownloadRequest(t, id, testWorkspaceID)
+	testHandler.DownloadAttachment(w, req)
+
+	if w.Code != http.StatusFound {
+		t.Fatalf("status = %d, want 302; body=%s", w.Code, w.Body.String())
+	}
+	loc := w.Header().Get("Location")
+	parsed, err := url.Parse(loc)
+	if err != nil {
+		t.Fatalf("parse Location: %v", err)
+	}
+	if got := parsed.Query().Get("response-content-disposition"); got != `attachment; filename="cloud front.md"` {
+		t.Fatalf("response-content-disposition = %q", got)
+	}
+	if got := parsed.Query().Get("Key-Pair-Id"); got != "KTEST" {
+		t.Fatalf("Key-Pair-Id = %q", got)
+	}
+}
+
+func TestDownloadAttachment_BareNavigationWithWorkspaceSlugQueryPassesMiddleware(t *testing.T) {
+	store := &mockStorage{}
+	origStorage := testHandler.Storage
+	origCfg := testHandler.cfg
+	origSigner := testHandler.CFSigner
+	testHandler.Storage = store
+	testHandler.cfg.AttachmentDownloadMode = "proxy"
+	testHandler.CFSigner = nil
+	t.Cleanup(func() {
+		testHandler.Storage = origStorage
+		testHandler.cfg = origCfg
+		testHandler.CFSigner = origSigner
+	})
+
+	key := "downloads/bare-nav.txt"
+	body := []byte("download body")
+	store.put(key, body)
+	id := seedAttachmentURL(t, "https://s3.example.com/test-bucket/"+key, "bare-nav.txt", "text/plain", int64(len(body)))
+
+	req := httptest.NewRequest("GET", "/api/attachments/"+id+"/download?workspace_slug="+url.QueryEscape(handlerTestWorkspaceSlug), nil)
+	req.Header.Set("X-User-ID", testUserID)
+	w := httptest.NewRecorder()
+
+	newDownloadRouter().ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200; body=%s", w.Code, w.Body.String())
+	}
+	if got := w.Body.String(); got != string(body) {
+		t.Fatalf("body = %q, want %q", got, body)
+	}
+	if req.Header.Get("X-Workspace-ID") != "" || req.Header.Get("X-Workspace-Slug") != "" {
+		t.Fatalf("bare navigation test must not set custom workspace headers")
+	}
+}
+
+func TestDownloadAttachment_BareNavigationWithoutWorkspaceQueryFailsMiddleware(t *testing.T) {
+	req := httptest.NewRequest("GET", "/api/attachments/00000000-0000-0000-0000-000000000001/download", nil)
+	req.Header.Set("X-User-ID", testUserID)
+	w := httptest.NewRecorder()
+
+	newDownloadRouter().ServeHTTP(w, req)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400; body=%s", w.Code, w.Body.String())
+	}
+	if !strings.Contains(w.Body.String(), "workspace_id or workspace_slug is required") {
+		t.Fatalf("body = %q, want workspace identifier error", w.Body.String())
+	}
+	if req.Header.Get("X-Workspace-ID") != "" || req.Header.Get("X-Workspace-Slug") != "" {
+		t.Fatalf("bare navigation test must not set custom workspace headers")
+	}
+}
+
 func TestDownloadAttachment_AutoInternalEndpointProxies(t *testing.T) {
 	store := &mockStorage{}
 	origStorage := testHandler.Storage
@@ -502,9 +646,19 @@ func TestDownloadAttachment_AutoPublicEndpointPresigns(t *testing.T) {
 	if !strings.Contains(loc, "X-Amz-Signature=mock") {
 		t.Fatalf("Location = %q, want fake S3 signature", loc)
 	}
+	parsed, err := url.Parse(loc)
+	if err != nil {
+		t.Fatalf("parse Location: %v", err)
+	}
+	if got := parsed.Query().Get("response-content-disposition"); got != `attachment; filename="public.txt"` {
+		t.Fatalf("response-content-disposition = %q", got)
+	}
 	if len(store.presignCalls) != 1 || store.presignCalls[0] != key {
 		t.Fatalf("presign calls = %v, want [%s]", store.presignCalls, key)
 	}
+	if len(store.presignDispositions) != 1 || store.presignDispositions[0] != `attachment; filename="public.txt"` {
+		t.Fatalf("presign dispositions = %v", store.presignDispositions)
+	}
 }

 func TestDownloadAttachment_ExplicitProxyStreamsPublicEndpoint(t *testing.T) {
--- a/server/internal/storage/s3.go
+++ b/server/internal/storage/s3.go
@@ -174,16 +174,24 @@ func (s *S3Storage) GetReader(ctx context.Context, key string) (io.ReadCloser, e
 }

 func (s *S3Storage) PresignGet(ctx context.Context, key string, ttl time.Duration) (string, error) {
+	return s.PresignGetWithContentDisposition(ctx, key, ttl, "")
+}
+
+func (s *S3Storage) PresignGetWithContentDisposition(ctx context.Context, key string, ttl time.Duration, contentDisposition string) (string, error) {
 	if key == "" {
 		return "", fmt.Errorf("s3 PresignGet: empty key")
 	}
 	if ttl <= 0 {
 		ttl = 30 * time.Minute
 	}
-	out, err := s3.NewPresignClient(s.client).PresignGetObject(ctx, &s3.GetObjectInput{
+	input := &s3.GetObjectInput{
 		Bucket: aws.String(s.bucket),
 		Key:    aws.String(key),
-	}, func(opts *s3.PresignOptions) {
+	}
+	if contentDisposition != "" {
+		input.ResponseContentDisposition = aws.String(contentDisposition)
+	}
+	out, err := s3.NewPresignClient(s.client).PresignGetObject(ctx, input, func(opts *s3.PresignOptions) {
 		opts.Expires = ttl
 	})
 	if err != nil {
--- a/server/internal/storage/s3_test.go
+++ b/server/internal/storage/s3_test.go
@@ -2,6 +2,7 @@ package storage

 import (
 	"context"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
@@ -48,6 +49,36 @@ func TestS3StoragePresignGet(t *testing.T) {
 	}
 }

+func TestS3StoragePresignGetWithContentDisposition(t *testing.T) {
+	store := &S3Storage{
+		client: s3.New(s3.Options{
+			Region:      "us-east-1",
+			Credentials: aws.NewCredentialsCache(credentials.NewStaticCredentialsProvider("AKID", "SECRET", "")),
+		}),
+		bucket: "test-bucket",
+	}
+
+	got, err := store.PresignGetWithContentDisposition(
+		context.Background(),
+		"uploads/abc/file.txt",
+		5*time.Minute,
+		`attachment; filename="report.txt"`,
+	)
+	if err != nil {
+		t.Fatalf("PresignGetWithContentDisposition: %v", err)
+	}
+	u, err := url.Parse(got)
+	if err != nil {
+		t.Fatalf("parse presigned URL: %v", err)
+	}
+	if got := u.Query().Get("response-content-disposition"); got != `attachment; filename="report.txt"` {
+		t.Fatalf("response-content-disposition = %q", got)
+	}
+	if sig := u.Query().Get("X-Amz-Signature"); sig == "" {
+		t.Fatalf("missing X-Amz-Signature in %q", got)
+	}
+}
+
 func TestS3StorageKeyFromURL_CustomEndpointWithTrailingSlash(t *testing.T) {
 	s := &S3Storage{
 		bucket:      "test-bucket",
--- a/server/internal/storage/storage.go
+++ b/server/internal/storage/storage.go
@@ -22,3 +22,7 @@ type Storage interface {
 type Presigner interface {
 	PresignGet(ctx context.Context, key string, ttl time.Duration) (string, error)
 }
+
+type DownloadPresigner interface {
+	PresignGetWithContentDisposition(ctx context.Context, key string, ttl time.Duration, contentDisposition string) (string, error)
+}
--- a/server/internal/storage/util.go
+++ b/server/internal/storage/util.go
@@ -27,6 +27,10 @@ func ContentDisposition(contentType, filename string) string {
 	return disposition + `; filename="` + sanitizeFilename(filename) + `"`
 }

+func AttachmentContentDisposition(filename string) string {
+	return `attachment; filename="` + sanitizeFilename(filename) + `"`
+}
+
 // isInlineContentType returns true for media types that browsers should
 // display inline (images, video, audio, PDF). Everything else triggers a
 // download via Content-Disposition: attachment.