diff --git a/packages/core/hooks/use-file-upload.ts b/packages/core/hooks/use-file-upload.ts index d48c90875..8e5273518 100644 --- a/packages/core/hooks/use-file-upload.ts +++ b/packages/core/hooks/use-file-upload.ts @@ -3,12 +3,21 @@ import { useState, useCallback } from "react"; import type { ApiClient } from "../api/client"; import type { Attachment } from "../types"; +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 kept as an -// alias for `url` because many callers persist it into Markdown / avatar -// fields by that name. +// (`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 }; export interface UploadContext { @@ -36,7 +45,12 @@ export function useFileUpload( commentId: ctx?.commentId, chatSessionId: ctx?.chatSessionId, }); - return { ...att, link: att.url }; + // 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 }; } finally { setUploading(false); } diff --git a/packages/core/types/attachment-url.test.ts b/packages/core/types/attachment-url.test.ts new file mode 100644 index 000000000..0ed40f88f --- /dev/null +++ b/packages/core/types/attachment-url.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "vitest"; +import { + attachmentDownloadPath, + attachmentIdFromDownloadURL, + contentReferencesAttachment, +} from "./attachment-url"; + +const ID = "11111111-2222-3333-4444-555555555555"; + +describe("attachmentDownloadPath", () => { + it("returns the stable per-attachment download path", () => { + expect(attachmentDownloadPath(ID)).toBe(`/api/attachments/${ID}/download`); + }); +}); + +describe("attachmentIdFromDownloadURL", () => { + it("extracts the id from a site-relative path", () => { + expect(attachmentIdFromDownloadURL(`/api/attachments/${ID}/download`)).toBe(ID); + }); + + it("strips a query string before matching", () => { + expect( + attachmentIdFromDownloadURL(`/api/attachments/${ID}/download?cache=1`), + ).toBe(ID); + }); + + it("strips a fragment before matching", () => { + expect(attachmentIdFromDownloadURL(`/api/attachments/${ID}/download#frag`)).toBe(ID); + }); + + it("accepts an absolute https URL", () => { + expect( + attachmentIdFromDownloadURL(`https://app.example.com/api/attachments/${ID}/download`), + ).toBe(ID); + }); + + it("rejects URLs that do not start with /api/attachments/", () => { + expect(attachmentIdFromDownloadURL(`/uploads/${ID}.png`)).toBeUndefined(); + expect( + attachmentIdFromDownloadURL("https://cdn.example.com/photo.png"), + ).toBeUndefined(); + }); + + it("rejects URLs missing the /download suffix", () => { + expect(attachmentIdFromDownloadURL(`/api/attachments/${ID}`)).toBeUndefined(); + expect( + attachmentIdFromDownloadURL(`/api/attachments/${ID}/content`), + ).toBeUndefined(); + }); + + it("rejects when the segment between the prefix and suffix is not a UUID literal", () => { + expect( + attachmentIdFromDownloadURL("/api/attachments/not-a-uuid/download"), + ).toBeUndefined(); + expect( + attachmentIdFromDownloadURL("/api/attachments//download"), + ).toBeUndefined(); + }); + + it("rejects malformed absolute URLs", () => { + expect(attachmentIdFromDownloadURL("https://")).toBeUndefined(); + }); + + it("returns undefined for empty input", () => { + expect(attachmentIdFromDownloadURL("")).toBeUndefined(); + }); +}); + +describe("contentReferencesAttachment", () => { + const att = { id: ID, url: "/uploads/workspaces/ws/legacy.png" }; + + it("matches when the markdown uses the stable download path", () => { + const md = `body\n\n![file](${attachmentDownloadPath(ID)})\n`; + expect(contentReferencesAttachment(md, att)).toBe(true); + }); + + it("matches when the markdown uses the legacy storage URL", () => { + const md = `body\n\n![file](${att.url})\n`; + expect(contentReferencesAttachment(md, att)).toBe(true); + }); + + it("matches when both shapes are present (rollout-window mixed content)", () => { + const md = `before\n\n![a](${attachmentDownloadPath(ID)})\n\n![b](${att.url})\n`; + expect(contentReferencesAttachment(md, att)).toBe(true); + }); + + it("returns false when neither URL shape appears in the body", () => { + expect(contentReferencesAttachment("plain text", att)).toBe(false); + }); + + it("returns false on empty content", () => { + expect(contentReferencesAttachment("", att)).toBe(false); + }); + + it("does not crash when the attachment has no legacy url", () => { + const md = `![file](${attachmentDownloadPath(ID)})`; + expect(contentReferencesAttachment(md, { id: ID, url: "" })).toBe(true); + }); +}); diff --git a/packages/core/types/attachment-url.ts b/packages/core/types/attachment-url.ts new file mode 100644 index 000000000..e209ffa5e --- /dev/null +++ b/packages/core/types/attachment-url.ts @@ -0,0 +1,118 @@ +/** + * Stable URL helpers for attachment references that get persisted into + * markdown bodies (issue descriptions, comment bodies, chat messages). + * + * Background — MUL-3130: + * + * The original upload flow returned `att.url` as the link to embed in + * markdown. After PR #3903 (MUL-3132), `att.url` for the LocalStorage + * backend became a 30-minute HMAC-signed `/uploads/?exp&sig` URL. + * That URL is short-lived by design — it's the in-memory token that + * keeps native browser /