fix(attachments): harden local sidecar serving and tighten Upload gate

Follow-ups to #2444:

- ServeFile refuses keys ending in .meta.json so the sidecar JSON isn't
  a stable read API. Sits before any disk work so a crafted
  .meta.json sibling can't trigger an out-of-tree read.
- ServeFile rejects paths that resolve outside uploadDir (via
  filepath.Rel) before readLocalMeta runs. http.ServeFile's own ..
  guard fires later on r.URL.Path, but readLocalMeta would otherwise
  do a stray disk read on <some-path>.meta.json before the 400 lands.
- Upload only writes a sidecar when filename is non-empty. ServeFile
  only reads the filename anyway, so a content-type-only sidecar was
  dead disk weight.
- Drop the dead json.Marshal error branch — marshaling two strings
  cannot fail.

Three new tests cover sidecar suffix rejection, the traversal guard,
and the no-filename Upload short-circuit.

Co-authored-by: multica-agent <github@multica.ai>

server/internal/storage/local.go

@@ -114,12 +114,12 @@ func (s *LocalStorage) Upload(ctx context.Context, key string, data []byte, cont
 	// Best-effort sidecar so ServeFile can restore the original filename in
 	// Content-Disposition. A failure here is logged but does not fail the
 	// upload — the file is still usable, just without the human-readable
-	// download name.
-	if filename != "" || contentType != "" {
-		body, err := json.Marshal(localMeta{Filename: filename, ContentType: contentType})
-		if err != nil {
-			slog.Error("local storage meta marshal failed", "key", key, "error", err)
-		} else if err := os.WriteFile(dest+metaSuffix, body, 0644); err != nil {
+	// download name. Skip when there's no filename to preserve: a sidecar
+	// without a filename is dead weight, since ServeFile only reads it for
+	// that field.
+	if filename != "" {
+		body, _ := json.Marshal(localMeta{Filename: filename, ContentType: contentType})
+		if err := os.WriteFile(dest+metaSuffix, body, 0644); err != nil {
 			slog.Error("local storage meta write failed", "key", key, "error", err)
 		}
 	}
@@ -135,7 +135,25 @@ func (s *LocalStorage) GetFilePath(key string) string {
 }
 
 func (s *LocalStorage) ServeFile(w http.ResponseWriter, r *http.Request, filename string) {
+	// The sidecar is an implementation detail of the local backend; refuse
+	// to serve it directly so /uploads/<key>.meta.json doesn't become a
+	// stable read API. Comes before any disk work so a path-traversal
+	// attempt at a .meta.json sibling can't trigger an out-of-tree read.
+	if strings.HasSuffix(filename, metaSuffix) {
+		http.NotFound(w, r)
+		return
+	}
+
 	filePath := filepath.Join(s.uploadDir, filename)
+	// filepath.Join cleans the path but doesn't enforce containment, so a
+	// caller passing "../etc/passwd" lands outside uploadDir. http.ServeFile
+	// rejects such requests on r.URL.Path, but readLocalMeta runs first —
+	// without this guard a crafted path could trigger a stray disk read on
+	// an arbitrary <some-path>.meta.json before the 400 lands.
+	if !isUnder(s.uploadDir, filePath) {
+		http.NotFound(w, r)
+		return
+	}
 	slog.Info("serving file", "filename", filename, "filepath", filePath)
 
 	// Mirror the S3 Upload path: when sidecar metadata exists for this key,
@@ -158,6 +176,17 @@ func (s *LocalStorage) ServeFile(w http.ResponseWriter, r *http.Request, filenam
 	http.ServeFile(w, r, filePath)
 }
 
+// isUnder reports whether target resolves to a path inside dir (or equal to
+// it). Both inputs are passed through filepath.Clean so trailing slashes and
+// "." segments don't fool the comparison.
+func isUnder(dir, target string) bool {
+	rel, err := filepath.Rel(filepath.Clean(dir), filepath.Clean(target))
+	if err != nil {
+		return false
+	}
+	return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))
+}
+
 func readLocalMeta(filePath string) (localMeta, bool) {
 	body, err := os.ReadFile(filePath + metaSuffix)
 	if err != nil {

server/internal/storage/local_test.go

@@ -306,6 +306,100 @@ func TestLocalStorage_ServeFile_NoSidecarFallback(t *testing.T) {
 	}
 }
 
+// TestLocalStorage_ServeFile_RejectsSidecarSuffix verifies that the sidecar
+// JSON itself is not addressable via /uploads/*. The sidecar is an
+// implementation detail; exposing it would turn the filename + content-type
+// pair into a stable read API and make any future ACL change leakier than
+// the data file it sits next to.
+func TestLocalStorage_ServeFile_RejectsSidecarSuffix(t *testing.T) {
+	tmpDir := t.TempDir()
+	t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
+
+	store := NewLocalStorageFromEnv()
+	if store == nil {
+		t.Fatal("NewLocalStorageFromEnv returned nil")
+	}
+
+	ctx := context.Background()
+	if _, err := store.Upload(ctx, "abc.xlsx", []byte("body"), "text/plain", "real.xlsx"); err != nil {
+		t.Fatalf("Upload failed: %v", err)
+	}
+
+	sidecarKey := "abc.xlsx" + metaSuffix
+	req := httptest.NewRequest(http.MethodGet, "/uploads/"+sidecarKey, nil)
+	rec := httptest.NewRecorder()
+	store.ServeFile(rec, req, sidecarKey)
+
+	if rec.Code != http.StatusNotFound {
+		t.Errorf("status = %d, want 404", rec.Code)
+	}
+	if got := rec.Header().Get("Content-Disposition"); got != "" {
+		t.Errorf("Content-Disposition = %q, want empty", got)
+	}
+}
+
+// TestLocalStorage_ServeFile_RejectsPathTraversal documents that a key
+// pointing outside uploadDir is rejected before any sidecar read. Without
+// this guard, readLocalMeta would attempt a disk read at <some-path>.meta.json
+// before http.ServeFile's own ".." check fires on r.URL.Path.
+func TestLocalStorage_ServeFile_RejectsPathTraversal(t *testing.T) {
+	tmpDir := t.TempDir()
+	t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
+
+	store := NewLocalStorageFromEnv()
+	if store == nil {
+		t.Fatal("NewLocalStorageFromEnv returned nil")
+	}
+
+	// Seed a sidecar OUTSIDE uploadDir so we'd notice if it were read: the
+	// header would carry "leaked.xlsx". Locating the sibling inside the
+	// per-test TempDir keeps the test self-contained — no real /etc reads.
+	parentDir := filepath.Dir(tmpDir)
+	leakedBase := filepath.Join(parentDir, "leaked-target")
+	if err := os.WriteFile(leakedBase+metaSuffix, []byte(`{"filename":"leaked.xlsx","content_type":"text/plain"}`), 0644); err != nil {
+		t.Fatalf("seed leaked sidecar failed: %v", err)
+	}
+	t.Cleanup(func() {
+		os.Remove(leakedBase + metaSuffix)
+	})
+
+	traversal := "../" + filepath.Base(leakedBase)
+	req := httptest.NewRequest(http.MethodGet, "/uploads/"+traversal, nil)
+	rec := httptest.NewRecorder()
+	store.ServeFile(rec, req, traversal)
+
+	if rec.Code != http.StatusNotFound {
+		t.Errorf("status = %d, want 404", rec.Code)
+	}
+	if got := rec.Header().Get("Content-Disposition"); got != "" {
+		t.Errorf("Content-Disposition = %q, want empty (sidecar must not leak)", got)
+	}
+}
+
+// TestLocalStorage_Upload_SkipsSidecarWhenFilenameEmpty verifies the tighter
+// Upload gate: a write with no filename has nothing useful to preserve, so
+// we shouldn't litter the upload directory with content-type-only sidecars
+// that ServeFile would ignore anyway.
+func TestLocalStorage_Upload_SkipsSidecarWhenFilenameEmpty(t *testing.T) {
+	tmpDir := t.TempDir()
+	t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
+
+	store := NewLocalStorageFromEnv()
+	if store == nil {
+		t.Fatal("NewLocalStorageFromEnv returned nil")
+	}
+
+	ctx := context.Background()
+	key := "no-filename.bin"
+	if _, err := store.Upload(ctx, key, []byte("body"), "application/octet-stream", ""); err != nil {
+		t.Fatalf("Upload failed: %v", err)
+	}
+
+	if _, err := os.Stat(filepath.Join(tmpDir, key+metaSuffix)); !os.IsNotExist(err) {
+		t.Errorf("sidecar should not exist when filename is empty, got err=%v", err)
+	}
+}
+
 // TestLocalStorage_Delete_RemovesSidecar verifies the cleanup half of the
 // fix: when the upload is deleted, its sidecar metadata file disappears too.
 // Otherwise the upload directory grows orphan .meta.json files forever.