feat(uploads): direct-to-S3 channel/DM attachments; retire processor upload router

[2witstudios]

What & why

Migrates channel and DM attachment uploads off the processor's multipart POST /api/upload/single onto the same presign → PUT(Tigris) → complete flow page files already use (#1460). Bytes now go browser → S3 directly; the only server-side touch is the processor re-hashing the stored object before the file row is linked.

This started from the merge note on #1460 asking whether the sibling /api/upload/multiple should be moved too. Ground-truth audit: /multiple was dead code (zero callers), and /single was the last thing keeping the processor in the upload-bytes business — its only caller was the channel/DM pipeline. So the answer became the full migration + cleanup.

Hard cutover — the attachment direct-to-S3 surface was unreleased, so there is no dual path.

Architecture (pure decisions, effects at the edges)

• packages/lib attachment-upload-core.ts — pure validation, file-row/result builders, slotTargetMatches. Now the single source of AttachmentTarget / FileRecordInput (re-exported from attachment-upload.ts, which is reduced to createAttachmentUploadServiceToken).
• apps/web lib/upload/ — attachment-direct (presign/complete/cancel orchestration), attachment-verify-effect (web→processor verify), attachment-client (client 3-step), attachment-route-helpers/-handlers. Six thin routes under {channels/[pageId]|messages/[conversationId]}/upload/{presign,complete,cancel}.
• useAttachmentUpload hook does presign → PUT → complete per file — same public FileAttachment API and uploadUrl base (sub-routes derived), so consumers are untouched.
• UploadSlotMetadata gains an optional attachmentTarget binding, so a presign jobId can't be replayed against a different conversation/page.

Integrity (preserved, not regressed)

New processor POST /api/verify (verify.ts + pure verify-core.ts), mounted with files:write scope and without requirePageBinding so conversation tokens work. It:

• re-hashes the stored S3 object (zero-trust — deletes poisoned objects on mismatch so they can't become a dedup target), and
• returns the Magika-detected MIME, which /complete persists instead of the client-declared type.

New contentStore.headOriginalSize HEAD-probes size before download (rejects > 1 GiB with 413) and re-throws genuine infra errors — unlike getOriginal, which swallows them — so a transient S3 outage returns a retryable 503 instead of masquerading as a definitive not-found.

Verify contract: 200 = definitive verdict (ok / hash_mismatch / object_not_found), 413 = too large, 503 = retryable infra. Only 5xx is retryable.

Cleanup (the satisfying part)

Deleted the entire processor upload router (/single + /multiple, upload-multer-config, server mount), the legacy web multipart POST routes, and processAttachmentUploads/uploadOneFile — plus their tests. The processor no longer receives upload bytes at all — it only reads from S3 (serve, pull-verify, verify).

Testing

TDD throughout (pure logic isolated from effects). New suites: core (15), processor verify route + pure core (21+10), headOriginalSize (5), web orchestrator (15), verify-effect (7), route helpers (8), client (9).

• ✅ bun run typecheck — 13/13 turbo tasks
• ✅ web next build — typecheck + ESLint + compile clean
• ✅ processor: 963 tests · lib: 4709 tests · new web suites green
• ✅ existing hook consumers (ChannelInput) unaffected

Manual end-to-end check (upload an image in a channel + a DM, dedup, integrity-tamper → 422, oversize → 413) is recommended before merge — the worktree can't run React render/browser tests.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Attachment byte verification with content-hash checking and clear verdicts (match/mismatch/blocked/absent/too-large).
  • New presign / complete / cancel upload flows for channel and direct-message attachments; client-side upload helper and error messages.

• Refactoring

  • Switched from batch multipart uploads to direct-to-S3 presign + complete orchestration.
  • Upload flow reorganized into explicit presign/complete/cancel steps and centralized verification.

---

Review round (convergence)

Codex P2s — both fixed (commit f136cc1) + regression tests + threads resolved:

• Slot leak on verify throw: the verifyAttachmentBytes call in completeAttachment is now wrapped in try/catch — a thrown error (network / AbortSignal.timeout) releases the reserved semaphore slot + active-upload count and returns a retryable 503, instead of leaking the slot until the stale-slot sweep.
• Forged size via dedup: complete now persists and charges the verifier's authoritative byte length (verify.size, what the processor actually re-read from S3) rather than the client-declared presign size — closing the gap where the alreadyExists (dedup) path skips the size-enforcing PUT.

CI fixes (commit f136cc1): rewrote useAttachmentUpload.test.ts for the 3-step flow (mock uploadAttachment); added the six attachment routes to the security-audit-coverage allowlist (audit is emitted by the shared orchestrator/resolver, not inline in the thin routes).

CodeQL alerts on verify.ts — assessed, threads resolved with justification:

• Missing rate limiting: /api/verify is service-to-service (web → processor, service-token auth) and already rate-limited upstream by the per-user upload semaphore (tier concurrency cap) + storage quota; the processor has no per-endpoint limiter on any route by design. js/missing-rate-limiting is a pre-existing accepted pattern (open alerts already on master); this PR net-removes several by deleting the upload router.
• User-controlled bypass: false positive — the gate is the token resource-binding; contentHash is the verification target, and deleteOriginal fires only on a hash mismatch of a content-addressed key the caller themselves populated (presign returns alreadyExists for valid existing objects), so there's no cross-tenant impact.

Synced with master (merge 3426c6cb) — no conflicts; rebuilt + re-validated the combined tree.

Validation (merged tree): next build clean (typecheck + ESLint + compile); lib attachment suites (24), processor verify/content-store/server (182), web upload + audit suites (69) all green; Unit Tests green in CI on the prior commit.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
pagespace-marketing	Ready	Preview, Comment	Jun 3, 2026 5:41pm

[coderabbitai]

Warning

Review limit reached

@2witstudios, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 13 minutes and 3 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c7060b3a-0cb5-4ee8-a076-ec97ce0e3abf

📥 Commits

Reviewing files that changed from the base of the PR and between 24b159a and ea94676.

📒 Files selected for processing (1)

• apps/web/src/hooks/__tests__/useAttachmentUpload.test.ts

📝 Walkthrough

Walkthrough

This PR refactors attachment upload from a unified multipart flow to a direct-to-S3 architecture with processor-side zero-trust byte verification. The processor gains a new /api/verify endpoint for content-hash re-checking and MIME detection. The web app removes old monolithic upload routes and introduces presign/complete/cancel orchestrators with slot-based concurrency management, complemented by client-side orchestration and per-file serial uploads. Core upload logic is extracted into shareable helpers.

Changes

Direct-to-S3 Upload Refactor

Layer / File(s)	Summary
Processor byte-verify endpoint apps/processor/src/api/verify-core.ts, apps/processor/src/api/verify.ts, apps/processor/src/cache/content-store.ts, apps/processor/src/server.ts, apps/processor/src/api/__tests__/verify-core.test.ts, apps/processor/src/api/__tests__/verify.test.ts	Adds /api/verify endpoint that HEAD-probes object size, downloads stored bytes, re-verifies content hash, and detects MIME. Includes size-classification decision core, response mapping logic with retry semantics, and storage HEAD helper. Routes mounted at /api/verify with files:write scope.
Shared attachment upload core packages/lib/src/services/attachment-upload-core.ts, packages/lib/src/services/__tests__/attachment-upload-core.test.ts, packages/lib/src/services/attachment-upload-repository.ts, packages/lib/src/services/attachment-upload.ts, packages/lib/src/services/upload-semaphore.ts, packages/lib/package.json	Extracts deterministic helpers for presign validation, drive derivation from target type, DB record construction, and slot target matching. Updates library exports and semaphore metadata to track attachment target alongside drive ID for cross-target replay prevention.
Presign orchestrator apps/web/src/lib/upload/attachment-direct.ts (presignAttachment), apps/web/src/lib/upload/__tests__/attachment-direct.test.ts (presign cases)	Reserves semaphore slots, validates quota/parameters, checks S3 object existence, and issues presigned PUT URLs with TTL. Releases slot and decrements active uploads on errors.
Complete orchestrator apps/web/src/lib/upload/attachment-direct.ts (completeAttachment), apps/web/src/lib/upload/__tests__/attachment-direct.test.ts (complete cases)	Validates reserved slot target binding, calls processor /api/verify for zero-trust byte verification, persists file record, links to target, and releases slot. Performs best-effort post-commit bookkeeping (quota update, audit, activity logging). Returns 403 on slot mismatch or expiry.
Cancel orchestrator apps/web/src/lib/upload/attachment-direct.ts (cancelAttachment), apps/web/src/lib/upload/__tests__/attachment-direct.test.ts (cancel cases)	Releases reserved slots on client-side PUT failure. Validates slot ownership and returns 403 for invalid/expired jobs. Decrements active uploads best-effort.
Client orchestration apps/web/src/lib/upload/attachment-client.ts, apps/web/src/lib/upload/__tests__/attachment-client.test.ts, apps/web/src/hooks/useAttachmentUpload.ts	New uploadAttachment function computes hash, presigns, conditionally uploads to Tigris, completes, and best-effort cancels on failure. Maps HTTP status codes to user-facing messages. Hook switched to per-file serial loop instead of batch.
Route layer: helpers and handlers apps/web/src/lib/upload/attachment-route-helpers.ts, apps/web/src/lib/upload/__tests__/attachment-route-helpers.test.ts, apps/web/src/lib/upload/attachment-route-handlers.ts, apps/web/src/lib/upload/attachment-verify-effect.ts, apps/web/src/lib/upload/__tests__/attachment-verify-effect.test.ts	Provides authenticateAttachmentRequest, resolveChannelTarget (with edit permission check), and resolveConversationTarget (with email verification check). Route handlers (handlePresign/handleComplete/handleCancel) parse JSON, validate fields, and dispatch to orchestrators. Processor verify response interpreter maps status codes and reasons to client outcomes.
New presign/complete/cancel routes apps/web/src/app/api/channels/[pageId]/upload/presign/route.ts, apps/web/src/app/api/channels/[pageId]/upload/complete/route.ts, apps/web/src/app/api/channels/[pageId]/upload/cancel/route.ts, apps/web/src/app/api/messages/[conversationId]/upload/presign/route.ts, apps/web/src/app/api/messages/[conversationId]/upload/complete/route.ts, apps/web/src/app/api/messages/[conversationId]/upload/cancel/route.ts	Six new routes (3 per context: channel and conversation) that resolve the upload target, delegate to shared handlers, and return orchestrator results with consistent error responses.
Old upload endpoints removed apps/web/src/app/api/channels/[pageId]/upload/route.ts (removed), apps/web/src/app/api/messages/[conversationId]/upload/route.ts (removed), apps/web/src/app/api/channels/[pageId]/upload/__tests__/route.test.ts (removed), apps/web/src/app/api/messages/[conversationId]/upload/__tests__/route.test.ts (removed), apps/processor/src/api/__tests__/upload.test.ts (removed), apps/processor/src/api/__tests__/upload-chunk.test.ts (removed), apps/processor/src/api/__tests__/upload-dedupe.test.ts (removed), apps/processor/src/api/__tests__/upload-multer-config.test.ts (removed)	Removes monolithic unified upload endpoints, multipart test coverage, and processor upload configuration. Old routes handled auth, page/conversation lookup, permission/email checks, and delegation to processAttachmentUploads.

Sequence Diagram(s)

sequenceDiagram
  participant Client as Client
  participant Web as Web API
  participant Processor as Processor
  participant S3 as S3
  Client->>Web: POST /presign (hash, type, size)
  Web->>Web: resolve target, check quota
  Web->>S3: check object exists
  S3-->>Web: exists or not
  alt object exists
    Web-->>Client: {alreadyExists: true}
  else object missing
    Web->>S3: get presigned PUT URL
    S3-->>Web: presigned URL
    Web-->>Client: {status, jobId, uploadUrl}
  end
  Client->>S3: PUT file to uploadUrl
  S3-->>Client: 200
  Client->>Web: POST /complete (jobId)
  Web->>Web: validate slot target
  Web->>Processor: POST /verify (hash)
  Processor->>S3: HEAD + GET object
  S3-->>Processor: bytes
  Processor->>Processor: verify hash, detect MIME
  Processor-->>Web: {ok, detectedMime, size}
  Web->>Web: persist file, link to target
  Web-->>Client: {success, file}

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• 2witstudios/PageSpace#506: Removed processor upload route tests including upload-chunk.test.ts, directly aligned with this PR's removal of unified upload routes and introduction of presign/complete/cancel pattern.
• 2witstudios/PageSpace#887: Modifies AUDIT_EXEMPT_ROUTES allowlist entries; related because this PR updates the same audit exemptions for upload presign/complete/cancel routes.
• 2witstudios/PageSpace#876: Previously added security/audit calls to the old channel upload route which this PR removes and replaces with new sub-routes.

Poem

🐰 In burrows of code the presigns gleam,

Slots held like secrets in a queuing dream.
The processor counts each byte with care,
Web hands the keys, the client lays them there.
Hooray — uploads hop forward, safe and fair.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.16% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(uploads): direct-to-S3 channel/DM attachments; retire processor upload router' directly and clearly summarizes the main change: migrating channel and DM attachment uploads to a direct-to-S3 flow and removing the legacy processor upload router.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pu/upload-limits

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a93157630

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[2witstudios]

Proactive review round (self-review + multi-angle critique)

Ran a high-recall review across correctness / removed-behavior / cross-file / cleanup angles. Fixed (commit 24b159a3):

1. Content-type spoof (security): verify.ts now applies the same isAllowedContentType denylist the page-file ingest path uses — rejecting + deleting browser-executable markup / scripts / native executables based on the actual bytes. presign already blocks these when declared; since /complete now persists the detected MIME (and some files are served inline), this closes the declare image/png → upload SVG/HTML bytes XSS vector. New blocked_type verdict → web surfaces 415.
2. Client never-throws (robustness): uploadAttachment is now genuinely non-throwing — computeContentHash and the presign-body parse are guarded, so a crypto/parse failure returns a result instead of throwing out of the hook's per-file loop and aborting the whole batch (a regression vs the old per-file flow). Every post-presign failure cancels the reserved slot, and the real error (e.g. Tigris 403) is surfaced instead of a blanket message.

Assessed and intentionally deferred (with rationale):

• Dedup tier/per-file-limit bypass: on the alreadyExists path, presign validates only the client-declared size against the tier limit; the real object size is bounded only by the global 1 GiB MAX_VERIFY_BYTES. Exploitability is low (content hashes are unguessable 256-bit, so a caller can only dedup onto an object whose hash they already know), and storage is now charged the verifier-authoritative size (the earlier P2 fix), so accounting stays correct. The page-file flow shares the same dedup gap — best fixed once for both in a focused follow-up rather than diverging here.
• activeUploads counter on slot expiry: if a slot expires before /complete, the DB counter isn't decremented (the in-memory slot is swept). Can't be decremented safely on the 403 path (a bogus jobId is indistinguishable from an expired one, so it'd let a client drive the counter negative). Pre-existing pattern shared with the page-file route; out of scope here.
• headOriginalSize null on missing ContentLength: extremely unlikely (S3/Tigris always return it on a successful HEAD); the 2 GiB streamToBuffer cap is the hard backstop regardless.
• Cleanup/dedup with the page-file flow: the presign/complete effect sequences, the slot-release idiom, and the verify reason↔message contract are duplicated across the new attachment flow and the released page-file routes. Sharing them is worthwhile but touches shipped code; deferring to a dedicated refactor to keep this PR's blast radius contained.

All other paths verified clean: semaphore +1/−1 accounting balanced across success/dedup/verify-fail/throw/cancel; deleted-service behavior (quota, audit, integrity, sanitized filename, email-verify, participant checks) faithfully re-established; hook public contract unchanged.

[coderabbitai]

Actionable comments posted: 2

🤖 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/processor/src/api/__tests__/verify.test.ts`:
- Around line 122-129: The test in 'deletes the object and returns blocked_type
when the detected type is disallowed' still expects HTTP 200 but the new verify
contract returns 415 for blocked_type; update the assertion in this test to
expect response.status toBe(415) (and keep the existing body assertions and
mockDeleteOriginal check) so the test matches the hardened verify contract;
locate the test by the it(...) description and the mocks mockDetectContentType
and mockIsAllowedContentType to change the status expectation.

In `@apps/web/src/hooks/__tests__/useAttachmentUpload.test.ts`:
- Around line 28-37: The test helper reuses the same attachment object causing
tests to be order-dependent; update the ok helper (and the other occurrence at
lines ~131-132) to return a fresh copy instead of the shared attachment
reference by creating and returning a shallow-cloned/new object each call
(preserve fields id, originalName, size, mimeType, contentHash and allow an
optional parameter to override values), so any mutations (e.g., adding
instanceId) do not leak between tests; keep the ok and fail return shapes the
same.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: eda218f2-31d4-4d14-968d-c3f8efe212ad

📥 Commits

Reviewing files that changed from the base of the PR and between 0a93157 and 24b159a.

📒 Files selected for processing (13)

• apps/processor/src/api/__tests__/verify-core.test.ts
• apps/processor/src/api/__tests__/verify.test.ts
• apps/processor/src/api/verify-core.ts
• apps/processor/src/api/verify.ts
• apps/web/src/app/api/__tests__/security-audit-coverage.test.ts
• apps/web/src/hooks/__tests__/useAttachmentUpload.test.ts
• apps/web/src/lib/upload/__tests__/attachment-client.test.ts
• apps/web/src/lib/upload/__tests__/attachment-direct.test.ts
• apps/web/src/lib/upload/__tests__/attachment-verify-effect.test.ts
• apps/web/src/lib/upload/attachment-client.ts
• apps/web/src/lib/upload/attachment-direct.ts
• apps/web/src/lib/upload/attachment-verify-effect.ts
• packages/lib/package.json

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

• apps/processor/src/api/tests/verify-core.test.ts
• apps/web/src/lib/upload/tests/attachment-client.test.ts
• apps/processor/src/api/verify-core.ts
• apps/web/src/lib/upload/attachment-verify-effect.ts
• packages/lib/package.json
• apps/web/src/lib/upload/attachment-client.ts
• apps/web/src/lib/upload/tests/attachment-verify-effect.test.ts
• apps/web/src/lib/upload/tests/attachment-direct.test.ts
• apps/processor/src/api/verify.ts
• apps/web/src/lib/upload/attachment-direct.ts