diff --git a/packages/core/hooks/use-file-upload.ts b/packages/core/hooks/use-file-upload.ts index 8e5273518..baaa31291 100644 --- a/packages/core/hooks/use-file-upload.ts +++ b/packages/core/hooks/use-file-upload.ts @@ -7,18 +7,41 @@ import { attachmentDownloadPath } from "../types/attachment-url"; import { MAX_FILE_SIZE } from "../constants/upload"; // Carries the full Attachment so editors that need preview metadata -// (`content_type`, `download_url`) get it directly; `link` is the -// stable persisted URL — `/api/attachments//download` — that the -// editor writes into the markdown body. The previous design used -// `att.url`, but on the LocalStorage backend that value became a -// 30-min HMAC-signed `/uploads/?exp&sig` URL after MUL-3132. -// Persisting a short-lived signature into a permanent comment body -// broke every image the moment the signature expired (MUL-3130). -// `attachmentDownloadPath(att.id)` re-signs at request time, never -// stops working until the attachment row is deleted, and resolves -// the workspace from the row itself so it loads as a native -// src without any X-Workspace-* headers. -export type UploadResult = Attachment & { link: string }; +// (`content_type`, `download_url`) get it directly. Two URL fields are +// surfaced because they have different lifetimes: +// +// `link` — the same value as `att.url`. Short-lived for the +// LocalStorage backend (HMAC-signed `/uploads/`) +// and a long-lived CDN URL on S3 / CloudFront. This +// is what avatar / logo callers persist into +// `avatar_url` style fields, and what URL-only +// consumers (Markdown renderers without a record +// in hand) get to load directly. Keeping it +// semantically equal to `att.url` preserves the +// pre-MUL-3130 contract for non-markdown callers +// so avatar uploads do not get rerouted through +// the workspace-membership-gated download endpoint. +// +// `markdownLink` — the stable per-attachment URL +// `/api/attachments//download`. Safe to embed +// in markdown bodies that outlive the current +// session because the server self-resolves the +// workspace from the attachment row and re-signs / +// proxies on every request. Empty when the upload +// has no attachment-row id (the no-workspace +// avatar branch of UploadFile, which falls back +// to `link`). Only the editor's markdown-persisting +// flow should use it; everything else uses `link`. +// +// MUL-3130 introduced the persisted-image regression by collapsing +// these two semantics into a single `link` field. Keeping them +// separate is what lets new comments embed a stable URL while +// avatar / logo callers continue to write `att.url` into long-lived +// fields without picking up workspace-membership gating. +export type UploadResult = Attachment & { + link: string; + markdownLink: string; +}; export interface UploadContext { issueId?: string; @@ -45,12 +68,14 @@ export function useFileUpload( commentId: ctx?.commentId, chatSessionId: ctx?.chatSessionId, }); - // Avatar uploads (no workspace context) come back without an id; - // fall back to the storage URL so legacy avatar pickers that - // persist `att.url` directly keep working. Comment / issue - // uploads always carry an id. - const link = att.id ? attachmentDownloadPath(att.id) : att.url; - return { ...att, link }; + // Avatar / no-workspace uploads come back without an + // attachment-row id (UploadFile's no-workspace branch returns + // {id, url, filename} that fails the AttachmentResponseSchema + // and degrades to the empty record). In that case the stable + // markdown URL doesn't exist — markdown callers fall back to + // `link` which mirrors `att.url`. + const markdownLink = att.id ? attachmentDownloadPath(att.id) : att.url; + return { ...att, link: att.url, markdownLink }; } finally { setUploading(false); } diff --git a/packages/views/chat/components/chat-input.test.tsx b/packages/views/chat/components/chat-input.test.tsx index 580c09934..2845b3095 100644 --- a/packages/views/chat/components/chat-input.test.tsx +++ b/packages/views/chat/components/chat-input.test.tsx @@ -20,6 +20,11 @@ function makeUpload(overrides: Partial & { id: string; link: strin content_type: "image/png", size_bytes: 1, created_at: new Date(0).toISOString(), + // markdownLink defaults to the same value as `link` so legacy + // tests assert the previous URL shape unless they pass an + // explicit override. Real callers always set it to the stable + // /api/attachments//download path via useFileUpload. + markdownLink: overrides.link, ...overrides, }; } diff --git a/packages/views/editor/attachment.test.tsx b/packages/views/editor/attachment.test.tsx index 6eb543962..6215b6b26 100644 --- a/packages/views/editor/attachment.test.tsx +++ b/packages/views/editor/attachment.test.tsx @@ -136,7 +136,12 @@ describe("Attachment — image dispatch", () => { renderWithQuery(); const img = document.querySelector("img"); expect(img).toBeTruthy(); - expect(img?.getAttribute("src")).toBe(att.url); + // The rendered src is the freshly-loadable URL — when the record + // carries a signed download_url (CloudFront / S3 presign style) it + // wins over the raw stored url so token-mode loads work + // without an Authorization header. See pickInlineMediaURL in + // attachment.tsx for the rationale (MUL-3130 review follow-up). + expect(img?.getAttribute("src")).toBe(att.download_url); expect(img?.getAttribute("alt")).toBe("shot.png"); expect(screen.getByTitle("View")).toBeTruthy(); expect(screen.getByTitle("Download")).toBeTruthy(); @@ -176,7 +181,12 @@ describe("Attachment — image dispatch", () => { />, ); const img = document.querySelector("img"); - expect(img?.getAttribute("src")).toBe(att.url); + // Once the URL resolves to a record, the rendered src swaps to + // the record's signed download_url so the image is loadable in + // token-mode clients that can't attach Authorization headers + // (MUL-3130 review). The raw stored url is the fallback for + // unresolved markdown only. + expect(img?.getAttribute("src")).toBe(att.download_url); fireEvent.click(screen.getByTitle("Download")); expect(downloadMock).toHaveBeenCalledWith("att-1"); }); @@ -229,6 +239,30 @@ describe("Attachment — image dispatch", () => { expect(screen.queryByTitle("View")).toBeNull(); expect(screen.queryByTitle("Download")).toBeNull(); }); + + it("stable /api/attachments//download download_url falls through to the freshly-signed record.url for token-mode loadability (MUL-3130)", () => { + // Pinned behavior: when an attachment record carries the default + // proxy-mode download_url (a bare /api/attachments//download + // path, which does NOT load as a native /