From a740f7a35e77f52b2609aef253416afa960c4593 Mon Sep 17 00:00:00 2001
From: Eve <eve@multica-ai.local>
Date: Tue, 9 Jun 2026 13:37:47 +0800
Subject: [PATCH] MUL-3130: persist a stable attachment download URL in comment
 markdown
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Comment image attachments rendered as broken placeholders ~30 minutes
after upload because the editor was persisting a short-lived
HMAC-signed URL into the comment body. After PR #3903 (MUL-3132)
hardened /uploads/* with auth, `attachmentToResponse` started signing
`attachment.url` as `/uploads/<key>?exp=<unix>&sig=<HMAC>` for
LocalStorage so token-auth clients could keep loading inline images.
The signature has a 30-min TTL by design — but `useFileUpload` was
returning that signed value as `link` and the editor was writing
`![file](signed-url)` straight into the markdown, so the comment
permanently captured a URL that stopped working as soon as the
signature expired.

The fix is to persist a stable per-attachment URL that the server can
re-sign on every request:

* `useFileUpload` now returns `link = /api/attachments/<id>/download`
  (avatar uploads without an id still fall back to `att.url` so the
  pre-attachment-row code paths keep working).
* `DownloadAttachment` self-resolves the workspace from the attachment
  row instead of reading X-Workspace-Slug / X-Workspace-ID headers,
  and the route is registered under the auth-only group so a native
  browser <img>/<video> resource load (which cannot attach those
  headers) succeeds. Membership is checked inside the handler with
  a 404 deny shape so the route does not act as an IDOR oracle.
* A new `GetAttachmentByIDOnly` SQL query supports the workspace-
  derivation step.
* `AttachmentDownloadProvider` now extracts the attachment id from
  the stable URL when matching markdown refs to attachment records,
  with a fallback to the existing url-equality check for legacy
  comments (and S3/CloudFront markdown that points straight at the
  CDN).
* `contentReferencesAttachment` covers both URL shapes for the
  composer / standalone-list dedup paths so an attachment uploaded
  before the fix and one uploaded after both deduplicate cleanly.

Tests:
- New unit tests for the URL helpers (16 tests, packages/core).
- Backend regression test: bare `<img src>`-style request without
  workspace headers now succeeds for a member (200) and 404s for a
  non-member, replacing the previous "400 without workspace context"
  contract.
- Existing TestDownload*, TestServeLocalUpload*, TestAttachmentTo
  Response* and the 1220 frontend views tests all pass.

Refs: MUL-3130, GitHub issue #3891
Co-authored-by: multica-agent <github@multica.ai>
---
 packages/core/hooks/use-file-upload.ts        |  22 +++-
 packages/core/types/attachment-url.test.ts    |  99 +++++++++++++++
 packages/core/types/attachment-url.ts         | 118 ++++++++++++++++++
 packages/core/types/index.ts                  |   1 +
 .../editor/attachment-download-context.tsx    |  56 ++++++---
 .../views/issues/components/comment-card.tsx  |  16 ++-
 .../views/issues/components/comment-input.tsx |   8 +-
 .../views/issues/components/reply-input.tsx   |   7 +-
 server/cmd/server/router.go                   |  18 ++-
 server/internal/handler/file.go               |  68 +++++++++-
 server/internal/handler/file_test.go          | 110 ++++++++++++++--
 server/pkg/db/generated/attachment.sql.go     |  32 +++++
 server/pkg/db/queries/attachment.sql          |  10 ++
 13 files changed, 523 insertions(+), 42 deletions(-)
 create mode 100644 packages/core/types/attachment-url.test.ts
 create mode 100644 packages/core/types/attachment-url.ts

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/<id>/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/<key>?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 <img>
+// 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/<key>?exp&sig` URL.
+ *   That URL is short-lived by design — it's the in-memory token that
+ *   keeps native browser <img>/<video> resource loads working without an
+ *   Authorization header. Persisting it into a comment body means the
+ *   comment's images break the moment the signature expires (default
+ *   30 min). The same problem applies to CloudFront-mode `download_url`
+ *   values, which are also signed redirects.
+ *
+ *   The fix is to persist a STABLE path that contains only the
+ *   attachment id. The server's /api/attachments/{id}/download endpoint
+ *   self-resolves the workspace from the attachment row and re-signs (or
+ *   proxies) on every request, so the URL is correct forever — until
+ *   the attachment row itself is deleted.
+ */
+
+const DOWNLOAD_PREFIX = "/api/attachments/";
+const DOWNLOAD_SUFFIX = "/download";
+
+/**
+ * UUID literal regex (RFC 4122 form). Used to extract an attachment id
+ * out of a stable download URL when matching markdown image refs back
+ * to their attachment record. Anchored to the segment between
+ * `/api/attachments/` and `/download` so we never accept a non-uuid
+ * path component as an id.
+ */
+const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
+
+/**
+ * Build the stable per-attachment download URL — the only attachment
+ * URL shape that is safe to persist into markdown bodies.
+ *
+ * Returns a site-relative path (no host) so the same value works in
+ * SSR, CSR, Electron, and the mobile webview where the host differs.
+ *
+ * Callers SHOULD use this when emitting markdown that references an
+ * attachment (image src, file-card href, attachment_ids tracker keys).
+ * For ad-hoc one-off URLs that don't outlive the current session
+ * (e.g. a download click handler that re-signs the URL on every press)
+ * the server-returned `download_url` is still appropriate.
+ */
+export function attachmentDownloadPath(attachmentId: string): string {
+  return `${DOWNLOAD_PREFIX}${attachmentId}${DOWNLOAD_SUFFIX}`;
+}
+
+/**
+ * Extract the attachment id from a `/api/attachments/<uuid>/download`
+ * URL (with or without query/host). Returns `undefined` if the URL is
+ * not a download URL, or if the captured segment is not a valid UUID
+ * literal.
+ *
+ * The renderer uses this to map a markdown image ref back to its
+ * attachment record so it can swap in CloudFront-signed URLs for
+ * faster fetches when configured, surface preview metadata, and keep
+ * the standalone-attachment dedup logic working when the markdown
+ * uses a stable path instead of the storage URL.
+ */
+export function attachmentIdFromDownloadURL(rawURL: string): string | undefined {
+  if (!rawURL) return undefined;
+
+  // Strip query string + fragment so a `?exp=...` (legacy signed URL
+  // accidentally embedded by an old client) does not poison the match.
+  let path = rawURL;
+  const qi = path.indexOf("?");
+  if (qi >= 0) path = path.slice(0, qi);
+  const hi = path.indexOf("#");
+  if (hi >= 0) path = path.slice(0, hi);
+
+  // Allow absolute URLs (electron, host-bearing CDN URL); pull just the
+  // pathname. Skip the protocol-aware parser for site-relative paths so
+  // SSR (no document) and webview environments behave identically.
+  if (/^https?:\/\//i.test(path)) {
+    try {
+      path = new URL(path).pathname;
+    } catch {
+      return undefined;
+    }
+  }
+
+  if (!path.startsWith(DOWNLOAD_PREFIX)) return undefined;
+  if (!path.endsWith(DOWNLOAD_SUFFIX)) return undefined;
+
+  const id = path.slice(DOWNLOAD_PREFIX.length, path.length - DOWNLOAD_SUFFIX.length);
+  if (!UUID_RE.test(id)) return undefined;
+  return id;
+}
+
+/**
+ * True when `content` contains a markdown reference to `attachment` —
+ * either the new stable `/api/attachments/<id>/download` shape OR the
+ * legacy `att.url` storage path. Used by the comment composer and the
+ * standalone-attachment list to decide whether an attachment is "in
+ * the body" (and therefore should be tracked as a comment attachment
+ * but not rendered as a card below).
+ *
+ * The two checks must both run because:
+ *   - new uploads from a post-MUL-3130 client write the stable URL
+ *   - edits to a pre-MUL-3130 comment may still have the legacy URL
+ *     in `entry.content`
+ *   - mixed content (one image uploaded before the fix, one after)
+ *     is possible during the rollout window
+ */
+export function contentReferencesAttachment(
+  content: string,
+  attachment: { id: string; url: string },
+): boolean {
+  if (!content) return false;
+  if (content.includes(attachmentDownloadPath(attachment.id))) return true;
+  if (attachment.url && content.includes(attachment.url)) return true;
+  return false;
+}
diff --git a/packages/core/types/index.ts b/packages/core/types/index.ts
index 3e5d76f1d..083dd88d8 100644
--- a/packages/core/types/index.ts
+++ b/packages/core/types/index.ts
@@ -65,6 +65,7 @@ export type { IssueSubscriber } from "./subscriber";
 export type * from "./events";
 export type * from "./api";
 export type { Attachment } from "./attachment";
+export { attachmentDownloadPath, attachmentIdFromDownloadURL, contentReferencesAttachment } from "./attachment-url";
 export type { ChatSession, ChatMessage, ChatMessagesPage, ChatPendingTask, PendingChatTaskItem, PendingChatTasksResponse, SendChatMessageResponse } from "./chat";
 export type { StorageAdapter } from "./storage";
 export type {
diff --git a/packages/views/editor/attachment-download-context.tsx b/packages/views/editor/attachment-download-context.tsx
index 5972cdb2b..ac88db0b3 100644
--- a/packages/views/editor/attachment-download-context.tsx
+++ b/packages/views/editor/attachment-download-context.tsx
@@ -2,6 +2,7 @@
 
 import { createContext, use, useMemo, type ReactNode } from "react";
 import type { Attachment } from "@multica/core/types";
+import { attachmentIdFromDownloadURL } from "@multica/core/types/attachment-url";
 import { openExternal } from "../platform";
 import { useDownloadAttachment } from "./use-download-attachment";
 
@@ -31,30 +32,49 @@ interface ProviderProps {
  * Provides a click-time download handler to Tiptap NodeViews mounted inside
  * `ContentEditor`. Without a provider the consumer falls back to opening the
  * raw URL via `openExternal` — same behaviour as before this hook existed.
+ *
+ * URL → attachment matching has two fallbacks (in order). New comments
+ * (post-MUL-3130) persist the stable `/api/attachments/<id>/download`
+ * shape; legacy comments persist whatever was in `att.url` at upload
+ * time, including the short-lived `/uploads/<key>?exp&sig` pattern that
+ * triggered MUL-3130. The id-from-URL extractor handles new content;
+ * exact-url equality covers legacy and S3/CloudFront markdown that
+ * never got the new shape.
  */
 export function AttachmentDownloadProvider({ attachments, children }: ProviderProps) {
   const download = useDownloadAttachment();
   const value = useMemo<ResolvedDownload>(
-    () => ({
-      resolveAttachmentId: (url) => {
+    () => {
+      const lookup = (url: string): Attachment | undefined => {
         if (!url || !attachments?.length) return undefined;
-        return attachments.find((a) => a.url === url)?.id;
-      },
-      resolveAttachment: (url) => {
-        if (!url || !attachments?.length) return undefined;
-        return attachments.find((a) => a.url === url);
-      },
-      openByUrl: (url) => {
-        const att = url && attachments?.length
-          ? attachments.find((a) => a.url === url)
-          : undefined;
-        if (att) {
-          download(att.id);
-          return;
+        // Preferred path: stable `/api/attachments/<id>/download` URL.
+        // Match by id so the lookup survives a host swap (Electron vs
+        // web vs SSR) and any incidental query/fragment.
+        const idFromUrl = attachmentIdFromDownloadURL(url);
+        if (idFromUrl) {
+          const byId = attachments.find((a) => a.id === idFromUrl);
+          if (byId) return byId;
         }
-        if (url) openExternal(url);
-      },
-    }),
+        // Legacy path: full URL equality. Covers comments persisted
+        // before MUL-3130, S3/CloudFront markdown that points
+        // straight at the CDN, and anything else where
+        // `attachments[i].url` was the literal value embedded in
+        // markdown.
+        return attachments.find((a) => a.url === url);
+      };
+      return {
+        resolveAttachmentId: (url) => lookup(url)?.id,
+        resolveAttachment: lookup,
+        openByUrl: (url) => {
+          const att = lookup(url);
+          if (att) {
+            download(att.id);
+            return;
+          }
+          if (url) openExternal(url);
+        },
+      };
+    },
     [attachments, download],
   );
   return (
diff --git a/packages/views/issues/components/comment-card.tsx b/packages/views/issues/components/comment-card.tsx
index 8cd36e0f4..ae1a761cd 100644
--- a/packages/views/issues/components/comment-card.tsx
+++ b/packages/views/issues/components/comment-card.tsx
@@ -37,6 +37,7 @@ import { useFileUpload } from "@multica/core/hooks/use-file-upload";
 import { api } from "@multica/core/api";
 import { ReplyInput } from "./reply-input";
 import type { TimelineEntry, Attachment } from "@multica/core/types";
+import { contentReferencesAttachment } from "@multica/core/types";
 import { useCommentCollapseStore, useCommentDraftStore } from "@multica/core/issues/stores";
 import { useT } from "../../i18n";
 
@@ -134,11 +135,14 @@ export function AttachmentList({
   onRemove?: (attachmentId: string) => void;
 }) {
   if (!attachments?.length) return null;
-  // Skip attachments whose URL is already referenced in the markdown content,
-  // and duplicates of the same file (same name/type/size) that are referenced.
+  // Skip attachments whose URL (stable or legacy) is already referenced
+  // in the markdown content, and duplicates of the same file (same
+  // name/type/size) that are referenced. The dual-shape match is the
+  // MUL-3130 follow-through — a comment can mix the new
+  // /api/attachments/<id>/download URL and the legacy att.url shape.
   const standalone = content
     ? attachments.filter((a) => {
-        if (content.includes(a.url)) return false;
+        if (contentReferencesAttachment(content, a)) return false;
         // Dedup: if another attachment with the same file identity is already
         // inline in the content, this is a duplicate upload — skip it.
         const hasSiblingInContent = attachments.some(
@@ -147,7 +151,7 @@ export function AttachmentList({
             other.filename === a.filename &&
             other.content_type === a.content_type &&
             other.size_bytes === a.size_bytes &&
-            content.includes(other.url),
+            contentReferencesAttachment(content, other),
         );
         if (hasSiblingInContent) return false;
         return true;
@@ -178,7 +182,7 @@ function collectActiveAttachmentIds(
 ): string[] {
   const ids = new Set<string>();
   for (const attachment of attachments) {
-    if (content.includes(attachment.url)) ids.add(attachment.id);
+    if (contentReferencesAttachment(content, attachment)) ids.add(attachment.id);
   }
   for (const id of retainedStandaloneIds ?? []) ids.add(id);
   return [...ids];
@@ -194,7 +198,7 @@ function initialStandaloneAttachmentIds(entry: TimelineEntry): Set<string> {
   const content = entry.content ?? "";
   return new Set(
     (entry.attachments ?? [])
-      .filter((attachment) => !content.includes(attachment.url))
+      .filter((attachment) => !contentReferencesAttachment(content, attachment))
       .map((attachment) => attachment.id),
   );
 }
diff --git a/packages/views/issues/components/comment-input.tsx b/packages/views/issues/components/comment-input.tsx
index fd54d4883..73a07b2c6 100644
--- a/packages/views/issues/components/comment-input.tsx
+++ b/packages/views/issues/components/comment-input.tsx
@@ -7,6 +7,7 @@ import { SubmitButton } from "@multica/ui/components/common/submit-button";
 import { useFileUpload } from "@multica/core/hooks/use-file-upload";
 import { api } from "@multica/core/api";
 import type { Attachment } from "@multica/core/types";
+import { contentReferencesAttachment } from "@multica/core/types";
 import { enterKey, formatShortcut, modKey } from "@multica/core/platform";
 import { useCommentDraftStore } from "@multica/core/issues/stores";
 import { useT } from "../../i18n";
@@ -68,9 +69,12 @@ function CommentInput({ issueId, onSubmit }: CommentInputProps) {
   const handleSubmit = async () => {
     const content = editorRef.current?.getMarkdown()?.replace(/(\n\s*)+$/, "").trim();
     if (!content || submitting) return;
-    // Only send attachment IDs for uploads still present in the content.
+    // Track every attachment whose stable download URL OR legacy
+    // storage URL is referenced in the markdown body. Both shapes
+    // can appear in the same comment during the MUL-3130 rollout —
+    // see contentReferencesAttachment for the rationale.
     const activeIds = pendingAttachments
-      .filter((a) => content.includes(a.url))
+      .filter((a) => contentReferencesAttachment(content, a))
       .map((a) => a.id);
     setSubmitting(true);
     try {
diff --git a/packages/views/issues/components/reply-input.tsx b/packages/views/issues/components/reply-input.tsx
index 72feb68fa..caaa909ae 100644
--- a/packages/views/issues/components/reply-input.tsx
+++ b/packages/views/issues/components/reply-input.tsx
@@ -8,6 +8,7 @@ import { ActorAvatar } from "../../common/actor-avatar";
 import { useFileUpload } from "@multica/core/hooks/use-file-upload";
 import { api } from "@multica/core/api";
 import type { Attachment } from "@multica/core/types";
+import { contentReferencesAttachment } from "@multica/core/types";
 import { useCommentDraftStore, type CommentDraftKey } from "@multica/core/issues/stores";
 import { cn } from "@multica/ui/lib/utils";
 import { useT } from "../../i18n";
@@ -89,9 +90,11 @@ function ReplyInput({
   const handleSubmit = async () => {
     const content = editorRef.current?.getMarkdown()?.replace(/(\n\s*)+$/, "").trim();
     if (!content || submitting) return;
-    // Only send attachment IDs for uploads still present in the content.
+    // Track every attachment whose stable download URL OR legacy
+    // storage URL is referenced in the markdown body. Both shapes
+    // can appear in the same comment during the MUL-3130 rollout.
     const activeIds = pendingAttachments
-      .filter((a) => content.includes(a.url))
+      .filter((a) => contentReferencesAttachment(content, a))
       .map((a) => a.id);
     setSubmitting(true);
     try {
diff --git a/server/cmd/server/router.go b/server/cmd/server/router.go
index f701c7d10..406860bf1 100644
--- a/server/cmd/server/router.go
+++ b/server/cmd/server/router.go
@@ -578,6 +578,18 @@ func NewRouterWithOptions(pool *pgxpool.Pool, hub *realtime.Hub, bus *events.Bus
 		r.Post("/api/upload-file", h.UploadFile)
 		r.Post("/api/feedback", h.CreateFeedback)
 
+		// Attachment download — user-scoped (auth-only), NOT
+		// workspace-scoped. The handler self-resolves the workspace
+		// from the attachment row and enforces membership inside, so
+		// this route is callable as a native browser <img>/<video>
+		// src that cannot attach X-Workspace-Slug / X-Workspace-ID
+		// headers. Persisting `/api/attachments/<id>/download` into
+		// comment markdown depends on this — see MUL-3130. The
+		// metadata / delete endpoints below stay workspace-scoped
+		// because they are JSON-API consumers that always have
+		// workspace context.
+		r.Get("/api/attachments/{id}/download", h.DownloadAttachment)
+
 		r.Route("/api/workspaces", func(r chi.Router) {
 			r.Get("/", h.ListWorkspaces)
 			r.Post("/", h.CreateWorkspace)
@@ -831,7 +843,11 @@ func NewRouterWithOptions(pool *pgxpool.Pool, hub *realtime.Hub, bus *events.Bus
 
 			// Attachments
 			r.Get("/api/attachments/{id}", h.GetAttachmentByID)
-			r.Get("/api/attachments/{id}/download", h.DownloadAttachment)
+			// /api/attachments/{id}/download is registered in the
+			// outer Auth-only group above so it can be loaded as a
+			// native <img>/<video> src without workspace headers
+			// (MUL-3130). The handler self-resolves the workspace
+			// from the attachment row.
 			r.Get("/api/attachments/{id}/content", h.GetAttachmentContent)
 			r.Delete("/api/attachments/{id}", h.DeleteAttachment)
 
diff --git a/server/internal/handler/file.go b/server/internal/handler/file.go
index 776cc723b..d5b12f1c9 100644
--- a/server/internal/handler/file.go
+++ b/server/internal/handler/file.go
@@ -516,12 +516,78 @@ func (h *Handler) loadAttachmentForRequest(w http.ResponseWriter, r *http.Reques
 	return att, true
 }
 
+// loadAttachmentForDownload is a workspace-self-resolving variant used by the
+// /api/attachments/{id}/download endpoint. It looks the attachment up by ID
+// alone, then enforces that the authenticated user is a member of the
+// attachment's workspace.
+//
+// Why a separate code path: a native browser <img>/<video> resource load on
+// /api/attachments/{id}/download cannot attach the X-Workspace-Slug /
+// X-Workspace-ID headers that loadAttachmentForRequest relies on. Putting
+// the workspace into the URL (?workspace_slug=...) would work mechanically
+// but bakes a non-essential identifier into every persisted comment markdown
+// link — unnecessary because the attachment row already records its
+// workspace. This helper keeps the URL clean (`/api/attachments/{id}/download`)
+// and treats the attachment id + cookie/Bearer auth as sufficient.
+//
+// Membership uses the same 404-on-deny shape as ServeLocalUpload so the
+// route does not act as an IDOR oracle for attachment IDs that happen to
+// belong to a different workspace. The membership cache fast path mirrors
+// canReadWorkspaceUpload exactly.
+func (h *Handler) loadAttachmentForDownload(w http.ResponseWriter, r *http.Request) (db.Attachment, bool) {
+	attachmentID := chi.URLParam(r, "id")
+	attUUID, ok := parseUUIDOrBadRequest(w, attachmentID, "attachment id")
+	if !ok {
+		return db.Attachment{}, false
+	}
+	att, err := h.Queries.GetAttachmentByIDOnly(r.Context(), attUUID)
+	if err != nil {
+		// 404 (not 403/401) so non-member and non-existent look identical
+		// to outside callers. Same shape as ServeLocalUpload's
+		// canReadWorkspaceUpload deny path.
+		writeError(w, http.StatusNotFound, "attachment not found")
+		return db.Attachment{}, false
+	}
+
+	userID, ok := requireUserID(w, r)
+	if !ok {
+		return db.Attachment{}, false
+	}
+
+	workspaceID := uuidToString(att.WorkspaceID)
+	if workspaceID == "" {
+		writeError(w, http.StatusNotFound, "attachment not found")
+		return db.Attachment{}, false
+	}
+	if h.MembershipCache.Get(r.Context(), userID, workspaceID) {
+		return att, true
+	}
+	if _, err := h.getWorkspaceMember(r.Context(), userID, workspaceID); err != nil {
+		writeError(w, http.StatusNotFound, "attachment not found")
+		return db.Attachment{}, false
+	}
+	h.MembershipCache.Set(r.Context(), userID, workspaceID)
+	return att, true
+}
+
 // ---------------------------------------------------------------------------
 // DownloadAttachment — GET /api/attachments/{id}/download
 // ---------------------------------------------------------------------------
+//
+// Workspace context is derived from the attachment row itself, not from
+// X-Workspace-Slug / X-Workspace-ID headers. This is what lets a markdown
+// `<img src="/api/attachments/{id}/download">` work as a native browser
+// resource load: the browser cannot attach those headers to <img>/<video>
+// fetches, so resolving via the attachment row is the only way to keep
+// the URL stable across reloads (the previous design persisted a 30-min
+// signed /uploads URL into the markdown body — that URL stopped working
+// the moment the signature expired).
+//
+// Membership is enforced inside loadAttachmentForDownload with a 404 deny
+// shape so the route doesn't IDOR-leak attachment IDs to non-members.
 
 func (h *Handler) DownloadAttachment(w http.ResponseWriter, r *http.Request) {
-	att, ok := h.loadAttachmentForRequest(w, r)
+	att, ok := h.loadAttachmentForDownload(w, r)
 	if !ok {
 		return
 	}
diff --git a/server/internal/handler/file_test.go b/server/internal/handler/file_test.go
index 83c134c3d..b39a9f675 100644
--- a/server/internal/handler/file_test.go
+++ b/server/internal/handler/file_test.go
@@ -22,7 +22,6 @@ import (
 
 	"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"
 )
 
@@ -435,8 +434,13 @@ func newDownloadRequest(t *testing.T, attachmentID, workspaceID string) (*http.R
 }
 
 func newDownloadRouter() http.Handler {
+	// Mirrors the production router after MUL-3130: the download
+	// route is registered under Auth-only with no
+	// RequireWorkspaceMember wrapper. The handler self-resolves the
+	// workspace from the attachment row and enforces membership
+	// internally, so a native browser <img>/<video> resource load
+	// with no X-Workspace-* headers is the supported call shape.
 	r := chi.NewRouter()
-	r.Use(middleware.RequireWorkspaceMember(testHandler.Queries))
 	r.Get("/api/attachments/{id}/download", testHandler.DownloadAttachment)
 	return r
 }
@@ -556,24 +560,114 @@ func TestDownloadAttachment_BareNavigationWithWorkspaceSlugQueryPassesMiddleware
 	}
 }
 
-func TestDownloadAttachment_BareNavigationWithoutWorkspaceQueryFailsMiddleware(t *testing.T) {
-	req := httptest.NewRequest("GET", "/api/attachments/00000000-0000-0000-0000-000000000001/download", nil)
+// TestDownloadAttachment_BareNavigationServesMemberWithoutWorkspaceHeaders
+// is the regression test for MUL-3130: a markdown image rendered as
+// `<img src="/api/attachments/<id>/download">` produces a native browser
+// resource load that cannot attach X-Workspace-Slug / X-Workspace-ID
+// headers. After the fix the handler self-resolves the workspace from
+// the attachment row, so a bare URL succeeds for a workspace member.
+func TestDownloadAttachment_BareNavigationServesMemberWithoutWorkspaceHeaders(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)))
+
+	// Bare URL — no workspace_slug / workspace_id query, no
+	// X-Workspace-* headers. This is what a browser <img> tag emits
+	// when the markdown stores `/api/attachments/<id>/download`.
+	req := httptest.NewRequest("GET", "/api/attachments/"+id+"/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 w.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200; 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 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")
 	}
 }
 
+// TestDownloadAttachment_BareNavigationDeniesNonMemberWith404 covers the
+// IDOR boundary: a stray attachment ID belonging to a workspace the
+// requester is NOT a member of must return 404, not 200 (would leak
+// bytes) and not 403 (would confirm the ID exists). Mirrors
+// ServeLocalUpload's deny shape.
+func TestDownloadAttachment_BareNavigationDeniesNonMemberWith404(t *testing.T) {
+	if testPool == nil {
+		t.Skip("test database not available")
+	}
+	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
+	})
+
+	// Seed an attachment that lives in a workspace testUserID is NOT
+	// a member of. The workspace row has to exist so the FK on
+	// attachment.workspace_id resolves; we tear both down on
+	// cleanup.
+	ctx := context.Background()
+	var foreignWorkspaceID string
+	if err := testPool.QueryRow(ctx, `
+		INSERT INTO workspace (name, slug, description, issue_prefix)
+		VALUES ('Bare-Nav Foreign', 'bare-nav-foreign', '', 'BNF')
+		RETURNING id::text
+	`).Scan(&foreignWorkspaceID); err != nil {
+		t.Fatalf("seed foreign workspace: %v", err)
+	}
+	t.Cleanup(func() { testPool.Exec(ctx, `DELETE FROM workspace WHERE id = $1`, foreignWorkspaceID) })
+
+	key := "downloads/bare-nav-foreign.txt"
+	store.put(key, []byte("foreign-body"))
+	var id string
+	if err := testPool.QueryRow(ctx, `
+		INSERT INTO attachment (workspace_id, uploader_type, uploader_id, filename, url, content_type, size_bytes)
+		VALUES ($1, 'member', $2, $3, $4, $5, $6)
+		RETURNING id::text
+	`, foreignWorkspaceID, testUserID, "foreign.txt", "https://s3.example.com/test-bucket/"+key, "text/plain", 12).Scan(&id); err != nil {
+		t.Fatalf("seed foreign attachment: %v", err)
+	}
+	t.Cleanup(func() { testPool.Exec(ctx, `DELETE FROM attachment WHERE id = $1`, id) })
+
+	req := httptest.NewRequest("GET", "/api/attachments/"+id+"/download", nil)
+	req.Header.Set("X-User-ID", testUserID)
+	w := httptest.NewRecorder()
+
+	newDownloadRouter().ServeHTTP(w, req)
+
+	if w.Code != http.StatusNotFound {
+		t.Fatalf("status = %d, want 404 for non-member; body=%s", w.Code, w.Body.String())
+	}
+	if strings.Contains(w.Body.String(), "foreign-body") {
+		t.Fatalf("response body leaked file contents: %q", w.Body.String())
+	}
+}
+
 func TestDownloadAttachment_AutoInternalEndpointProxies(t *testing.T) {
 	store := &mockStorage{}
 	origStorage := testHandler.Storage
diff --git a/server/pkg/db/generated/attachment.sql.go b/server/pkg/db/generated/attachment.sql.go
index f52c6b939..91f5204a6 100644
--- a/server/pkg/db/generated/attachment.sql.go
+++ b/server/pkg/db/generated/attachment.sql.go
@@ -115,6 +115,38 @@ func (q *Queries) GetAttachment(ctx context.Context, arg GetAttachmentParams) (A
 	return i, err
 }
 
+const getAttachmentByIDOnly = `-- name: GetAttachmentByIDOnly :one
+SELECT id, workspace_id, issue_id, comment_id, uploader_type, uploader_id, filename, url, content_type, size_bytes, created_at, chat_session_id, chat_message_id FROM attachment
+WHERE id = $1
+`
+
+// Used by the download endpoint, which derives workspace context from the
+// attachment row itself rather than from request headers/query params. The
+// caller still has to verify the requester is a member of the returned
+// workspace_id before serving the bytes — this query is access-neutral on
+// purpose so a self-contained URL like /api/attachments/{id}/download can
+// work as a native <img>/<video> resource load (no header attachment).
+func (q *Queries) GetAttachmentByIDOnly(ctx context.Context, id pgtype.UUID) (Attachment, error) {
+	row := q.db.QueryRow(ctx, getAttachmentByIDOnly, id)
+	var i Attachment
+	err := row.Scan(
+		&i.ID,
+		&i.WorkspaceID,
+		&i.IssueID,
+		&i.CommentID,
+		&i.UploaderType,
+		&i.UploaderID,
+		&i.Filename,
+		&i.Url,
+		&i.ContentType,
+		&i.SizeBytes,
+		&i.CreatedAt,
+		&i.ChatSessionID,
+		&i.ChatMessageID,
+	)
+	return i, err
+}
+
 const linkAttachmentsToChatMessage = `-- name: LinkAttachmentsToChatMessage :exec
 UPDATE attachment
 SET chat_message_id = $1
diff --git a/server/pkg/db/queries/attachment.sql b/server/pkg/db/queries/attachment.sql
index bc97f2f46..758ccb190 100644
--- a/server/pkg/db/queries/attachment.sql
+++ b/server/pkg/db/queries/attachment.sql
@@ -23,6 +23,16 @@ ORDER BY created_at ASC;
 SELECT * FROM attachment
 WHERE id = $1 AND workspace_id = $2;
 
+-- name: GetAttachmentByIDOnly :one
+-- Used by the download endpoint, which derives workspace context from the
+-- attachment row itself rather than from request headers/query params. The
+-- caller still has to verify the requester is a member of the returned
+-- workspace_id before serving the bytes — this query is access-neutral on
+-- purpose so a self-contained URL like /api/attachments/{id}/download can
+-- work as a native <img>/<video> resource load (no header attachment).
+SELECT * FROM attachment
+WHERE id = $1;
+
 -- name: ListAttachmentsByCommentIDs :many
 SELECT * FROM attachment
 WHERE comment_id = ANY($1::uuid[]) AND workspace_id = $2