fix(files): allow downloading issue attachments

[martian56]

What

Closes #135. Issue/epic attachments could never be downloaded. The file-serving endpoint only accepted object paths under uploads/, but the attachment flow stores objects as attachments/<issueId>/<assetId> and hands the UI URLs like /api/files/attachments/.... Every attachment download was rejected with 400 before any lookup — a High-severity break of a core feature (uploads appeared to succeed but the file was unretrievable).

How

ServeFile now serves both the uploads/ (avatars/covers/logos) and attachments/ (issue attachments) prefixes. Empty paths and path traversal (..) are still rejected. The check is pulled into a small isServableObjectPath helper so it is unit-testable without MinIO. The route stays behind RequireAuth, so the auth model is unchanged.

Testing

• New internal/handler/upload_path_test.go unit-tests isServableObjectPath: uploads/ and attachments/ paths pass; empty, traversal, and other prefixes are rejected.

• Verified against a running server (MinIO up, authenticated request):

  Path	Before	After
  attachments/<uuid>/<uuid>	400	404 (reaches lookup)
  uploads/<...>	404	404
  secrets/key	400	400
  attachments/../secrets/key	400	400

  The 400 -> 404 flip on the attachments path is the exact fix; disallowed prefixes and traversal still 400.

AI assistance

Produced with the help of Claude Code (Claude Opus 4.8). AI-assisted commits carry a Co-Authored-By trailer.

Summary by CodeRabbit

• Bug Fixes
  • Improved file download path validation to only allow safe objects under uploads/ and attachments/, blocking empty and path-traversal requests.
  • Added access control for attachment downloads: unauthorized or missing attachments now result in not found; if attachment authorization isn’t configured, requests return service unavailable.
• Tests
  • Added unit tests for parsing/validating upload and attachment object paths.
  • Added tests covering attachment download authorization behavior.

[strix-security]

Strix Security Review

All previously reported security findings have been resolved.

1 resolved finding

• Missing authorization on attachment downloads via /api/files/*path — resolved in b81a433

Review summary

Re-review of the latest attachment download changes. I verified the new attachment authorization path in ServeFile, the attachment membership check in AuthorizeDownload, and the route wiring for /api/files/*path; I did not find any new security issues in the changed code.

_{Updated for c209368.}

---

Reviewed by Strix
_{Re-run review · Configure security review settings}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 81bdfebf-5412-418f-87fd-531ba4681d30

📥 Commits

Reviewing files that changed from the base of the PR and between b81a433 and c209368.

📒 Files selected for processing (2)

• apps/api/internal/handler/upload.go
• apps/api/internal/service/attachment.go

🚧 Files skipped from review as they are similar to previous changes (2)

• apps/api/internal/service/attachment.go
• apps/api/internal/handler/upload.go

---

📝 Walkthrough

Walkthrough

Attachment file serving now accepts attachments/ object paths, parses attachment IDs from the path, and authorizes downloads through the attachment service. The router wires that service into the upload handler, and new tests cover attachment authorization plus path parsing and allowlisting.

Changes

Attachment download path and authorization flow

Layer / File(s)	Summary
Path validation and ServeFile flow apps/api/internal/handler/upload.go	Adds isServableObjectPath and parseAttachmentPath, then updates ServeFile to allow uploads/ and attachments/ paths and route attachment downloads through attachment-specific checks.
Authorization service, wiring, and tests apps/api/internal/service/attachment.go, apps/api/internal/router/router.go, apps/api/internal/handler/attachment_authz_test.go, apps/api/internal/handler/upload_path_test.go	Adds AttachmentService.AuthorizeDownload, passes Attachments into UploadHandler, and adds tests for attachment authorization plus path parsing and object-path allowlisting.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ServeFile
  participant AttachmentService

  Client->>ServeFile: GET /api/files/attachments/<issueId>/<assetId>
  ServeFile->>ServeFile: validate object path
  ServeFile->>ServeFile: parse attachment IDs
  ServeFile->>AttachmentService: AuthorizeDownload(issueID, assetID, userID)
  AttachmentService-->>ServeFile: allowed / not found / forbidden
  ServeFile-->>Client: stream file or return 404/503

Loading

Poem

A rabbit hopped through folders deep,
Found attachments stuck asleep,
With checks and keys and paths made right,
The files now hop out into sight.
🐇📎

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The path fix is there, but the issue's requested serving hardening (nosniff, Content-Disposition, content-type limits) is missing.	Add the attachment-serving hardening from the issue: set X-Content-Type-Options to nosniff, send Content-Disposition, and constrain attachment content types before serving.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise, conventional, and matches the main change: enabling issue attachment downloads.
Description check	✅ Passed	It covers the problem, approach, and testing, with only non-critical template sections omitted.
Out of Scope Changes check	✅ Passed	The changes stay focused on attachment downloads, path validation, authorization, routing, and tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/attachment-download

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/api/internal/handler/upload.go`:
- Around line 115-124: The ServeFile flow in UploadHandler only validates the
MinIO path and then streams the object, so attachment URLs can bypass
issue/workspace authorization. Update ServeFile to detect attachment paths,
resolve the attachment record first, and enforce the same access checks used by
the attachment APIs before calling MinIO. Use the existing UploadHandler,
ServeFile, and attachment lookup/authorization helpers to locate the record and
verify the caller is allowed to access that issue/project.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eebd55f6-17bf-4d6d-8581-53f97cbd8936

📥 Commits

Reviewing files that changed from the base of the PR and between c4e5094 and 9bd7f89.

📒 Files selected for processing (2)

• apps/api/internal/handler/upload.go
• apps/api/internal/handler/upload_path_test.go

[strix-security]

Strix flagged a new security finding below. See the pinned summary comment for the full PR status.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/api/internal/service/attachment.go`:
- Around line 92-98: In AuthorizeDownload, stop collapsing all failures from
GetAttachmentByAssetID and IsMember into ErrAttachmentNotFound or
ErrProjectForbidden. Update the attachment lookup and membership check handling
so only the genuine nil/not-found and false/not-member cases return those domain
errors, while any non-nil datastore error is returned directly to the caller to
flow into the 5xx path. Keep the fix localized around
s.is.GetAttachmentByAssetID and s.ws.IsMember.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a2645d0a-5aae-49df-86a1-18363541f0c6

📥 Commits

Reviewing files that changed from the base of the PR and between 9bd7f89 and b81a433.

📒 Files selected for processing (5)

• apps/api/internal/handler/attachment_authz_test.go
• apps/api/internal/handler/upload.go
• apps/api/internal/handler/upload_path_test.go
• apps/api/internal/router/router.go
• apps/api/internal/service/attachment.go