fix(uploads): verify processor file hash

[2witstudios]

Summary

• Require attachment upload callers to pass a validated EnforcedAuthContext instead of a raw user id
• Recompute the uploaded file SHA-256 in the web layer and reject processor responses with mismatched contentHash or size before persistence/linkage/accounting
• Apply the same enforced-context upload boundary to the DM upload route now on master
• Document DM attachment scanning depth and content-addressed dedup trust assumptions

Root Cause

processAttachmentUpload previously trusted the processor-returned contentHash when writing the files row and target linkage. A compromised processor could return another tenant's existing content hash and cause the uploader to receive a fresh authorized linkage to unrelated data.

Validation

• pnpm i
• pnpm --filter @pagespace/lib build
• pnpm --filter web exec vitest run 'src/app/api/messages/[conversationId]/upload/__tests__/route.test.ts'
• pnpm --filter web exec vitest run 'src/app/api/channels/[pageId]/upload/__tests__/route.test.ts'
• pnpm --filter web typecheck
• pnpm --filter @pagespace/lib exec vitest run src/services/__tests__/process-attachment-upload.test.ts
• pnpm --filter web lint (passes with existing unrelated QuickCreatePalette.tsx hook dependency warning)
• git diff --check

[coderabbitai]

Warning

Rate limit exceeded

@2witstudios has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 32 minutes and 8 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fa074ace-db03-4b5e-bf4f-b4155ca4c235

📥 Commits

Reviewing files that changed from the base of the PR and between 9de398a and 5200bf3.

📒 Files selected for processing (4)

• apps/web/src/app/api/channels/[pageId]/upload/__tests__/route.test.ts
• apps/web/src/app/api/messages/[conversationId]/upload/__tests__/route.test.ts
• apps/web/src/app/api/messages/[conversationId]/upload/route.ts
• tasks/dm-file-attachments.md

📝 Walkthrough

Walkthrough

This PR implements file upload integrity validation by computing SHA-256 hashes of received bytes, validating processor responses against those hashes before persisting files, and refactoring the upload pipeline to use enforced authentication contexts instead of raw user IDs. Marketing documentation, test coverage, and task specifications are updated accordingly.

Changes

File Upload Integrity & Auth Context

Layer / File(s)	Summary
Auth Context Interface apps/web/src/app/api/channels/[pageId]/upload/route.ts, apps/web/src/app/api/channels/[pageId]/upload/__tests__/route.test.ts	Channel upload route migrates from authenticateRequestWithOptions to authenticateWithEnforcedContext; test mocks updated to return simplified { ctx: { userId } } shape; route passes authContext: ctx to downstream service.
Core Upload Integrity packages/lib/src/services/attachment-upload.ts	processAttachmentUpload now accepts authContext: EnforcedAuthContext instead of raw userId. Pipeline computes SHA-256 hash of received file bytes, validates processor's JSON response (content hash & size), and returns 502 with integrity error if mismatches occur; semaphore slot is released on validation failure.
Service Integration packages/lib/src/services/__tests__/process-attachment-upload.test.ts	Test suite refactored to compute file hashes dynamically, mock processor responses aligned to actual file content, and construct EnforcedAuthContext via helper; new test scenarios validate processor integrity failures (hash/size mismatch → 502, no persistence/linking, semaphore release).
Documentation & Specification apps/marketing/src/app/docs/security/zero-trust/page.tsx, tasks/dm-file-attachments.md	Security documentation adds "File Upload Integrity" section detailing processor response validation, content-addressable dedup, byte-based content detection, and channel vs. DM post-upload handling; task spec updated with stricter processor response and auth context requirements.

Sequence Diagram

sequenceDiagram
    actor Client
    participant Route as Channel Upload<br/>Route
    participant Service as Upload<br/>Service
    participant Processor as External<br/>Processor
    participant Storage as File<br/>Storage

    Client->>Route: POST with file + auth
    Route->>Route: authenticateWithEnforcedContext
    Route->>Service: processAttachmentUpload(request, authContext)
    
    Service->>Service: computeFileSha256(file)<br/>→ expectedHash
    Service->>Processor: POST file bytes
    Processor-->>Service: { contentHash, size, ... }
    
    alt Processor Response Valid
        Service->>Service: validateProcessorResult<br/>(response, expected)
        Service->>Storage: persist file
        Service->>Storage: link to target
        Service-->>Route: { fileId, contentHash, ... }
        Route-->>Client: 200 OK
    else Hash or Size Mismatch
        Service->>Service: release semaphore slot
        Service->>Route: 502 error<br/>(integrity failure)
        Route-->>Client: 502 Bad Gateway
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Poem

🐰 A hash-check hoppy dance,
Each byte a careful glance,
Processor responses verified true,
Authentication contexts shiny new,
Integrity blooms in every stance! 🌱

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% 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
Title check	✅ Passed	The title accurately summarizes the main change: verifying processor file hash before persistence, which is the core security fix in this changeset.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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/processor-zero-trust-fix

---

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

_{Review rate limit: 0/1 reviews remaining, refill in 32 minutes and 8 seconds.}

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

apps/web/src/app/api/channels/[pageId]/upload/__tests__/route.test.ts (1)

63-65: ⚡ Quick win

Use a real EnforcedAuthContext in test fixtures to prevent contract drift.

The current plain-object ctx keeps tests passing even if the route/service contract later relies on actual EnforcedAuthContext semantics beyond userId.

Suggested test-fixture tightening

+import { EnforcedAuthContext } from '@pagespace/lib/permissions/enforced-context';
+import type { SessionClaims } from '@pagespace/lib/auth/session-service';
...
 function makeAuthSuccess(userId = 'user-1') {
-  return { ctx: { userId } };
+  const claims: SessionClaims = {
+    sessionId: `session-${userId}`,
+    userId,
+    userRole: 'user',
+    tokenVersion: 1,
+    adminRoleVersion: 1,
+    type: 'user',
+    scopes: [],
+    expiresAt: new Date(Date.now() + 60_000),
+  };
+  return { ctx: EnforcedAuthContext.fromSession(claims) };
 }
...
-      authContext: { userId: 'user-1' },
+      authContext: expect.objectContaining({ userId: 'user-1' }),

Also applies to: 94-95

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/app/api/channels/`[pageId]/upload/__tests__/route.test.ts around
lines 63 - 65, Replace the plain-object fixture returned by makeAuthSuccess with
a real EnforcedAuthContext instance to avoid contract drift: import or construct
the EnforcedAuthContext type/object used by the route (e.g., the
EnforcedAuthContext exported from your auth module) and have makeAuthSuccess
return { ctx: <EnforcedAuthContext with userId set> } (or call the helper that
builds one) instead of a raw object; also update the other test fixtures in this
file that mirror lines 94–95 to use the same EnforcedAuthContext-based
construction so tests exercise the real auth contract.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tasks/dm-file-attachments.md`:
- Line 52: The docs still show processAttachmentUpload({ request, target }) but
the implementation now requires a validated auth context; update the earlier
function signature text to reflect this by changing the documented signature to
processAttachmentUpload({ request, target, authContext }) and note that
authContext must be an enforced/validated auth context (not a raw
caller-provided userId); update any nearby explanatory text to state that
authContext is required and validated before calling processAttachmentUpload
(reference: processAttachmentUpload).

---

Nitpick comments:
In `@apps/web/src/app/api/channels/`[pageId]/upload/__tests__/route.test.ts:
- Around line 63-65: Replace the plain-object fixture returned by
makeAuthSuccess with a real EnforcedAuthContext instance to avoid contract
drift: import or construct the EnforcedAuthContext type/object used by the route
(e.g., the EnforcedAuthContext exported from your auth module) and have
makeAuthSuccess return { ctx: <EnforcedAuthContext with userId set> } (or call
the helper that builds one) instead of a raw object; also update the other test
fixtures in this file that mirror lines 94–95 to use the same
EnforcedAuthContext-based construction so tests exercise the real auth contract.

🪄 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: ceba5870-bacf-4743-9e99-9e34705ed7cc

📥 Commits

Reviewing files that changed from the base of the PR and between 572ade9 and 9de398a.

📒 Files selected for processing (6)

• apps/marketing/src/app/docs/security/zero-trust/page.tsx
• apps/web/src/app/api/channels/[pageId]/upload/__tests__/route.test.ts
• apps/web/src/app/api/channels/[pageId]/upload/route.ts
• packages/lib/src/services/__tests__/process-attachment-upload.test.ts
• packages/lib/src/services/attachment-upload.ts
• tasks/dm-file-attachments.md