From 569b43136c8bf2a89b7a85dde566a005473ad897 Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Thu, 4 Jun 2026 15:45:42 +0800 Subject: [PATCH] fix(editor): download attachments without blank web tab (#3752) * fix(editor): download attachments without blank web tab Co-authored-by: multica-agent * fix(attachments): preserve workspace in web download URLs Co-authored-by: multica-agent --------- Co-authored-by: multica-agent --- .../editor/use-download-attachment.test.tsx | 146 ++++++++++++++-- .../views/editor/use-download-attachment.ts | 90 +++++----- server/internal/auth/cloudfront.go | 15 ++ server/internal/auth/cloudfront_test.go | 57 +++++++ server/internal/handler/file.go | 27 ++- server/internal/handler/file_test.go | 160 +++++++++++++++++- server/internal/storage/s3.go | 12 +- server/internal/storage/s3_test.go | 31 ++++ server/internal/storage/storage.go | 4 + server/internal/storage/util.go | 4 + 10 files changed, 481 insertions(+), 65 deletions(-) create mode 100644 server/internal/auth/cloudfront_test.go diff --git a/packages/views/editor/use-download-attachment.test.tsx b/packages/views/editor/use-download-attachment.test.tsx index 649ecb781..623874055 100644 --- 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()); }); }); diff --git a/packages/views/editor/use-download-attachment.ts b/packages/views/editor/use-download-attachment.ts index b82f34877..b3a3d014a 100644 --- 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; } +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 { 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 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], ); } diff --git a/server/internal/auth/cloudfront.go b/server/internal/auth/cloudfront.go index e4cccd6dd..1f86ae317 100644 --- 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) diff --git a/server/internal/auth/cloudfront_test.go b/server/internal/auth/cloudfront_test.go new file mode 100644 index 000000000..cbab24aa9 --- /dev/null +++ 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) + } +} diff --git a/server/internal/handler/file.go b/server/internal/handler/file.go index 0bbfe88c9..2674c3e2c 100644 --- 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. diff --git a/server/internal/handler/file_test.go b/server/internal/handler/file_test.go index b0fd238e0..83c134c3d 100644 --- 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) { diff --git a/server/internal/storage/s3.go b/server/internal/storage/s3.go index 46b28a1a8..d98756b8a 100644 --- 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 { diff --git a/server/internal/storage/s3_test.go b/server/internal/storage/s3_test.go index 5b00a85bc..dd72e761c 100644 --- 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", diff --git a/server/internal/storage/storage.go b/server/internal/storage/storage.go index f8cdb17bf..3ceb115d3 100644 --- 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) +} diff --git a/server/internal/storage/util.go b/server/internal/storage/util.go index a22137508..dab56fc3b 100644 --- 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.