Repository navigation
feat(security): wire SecurityAuditService into comms routes - #876
Conversation
Add audit logging for channel messages (read/write), reactions (write/delete), and file uploads (write). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add audit logging for DM messages (read/write/mark_read), conversations (read/write), and message threads (read). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add audit logging for notifications (read/delete), push token creation (logTokenCreated), and email unsubscribe preferences (write). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add audit logging for inbox read access. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis pull request adds non-blocking audit logging to 11 API route handlers across channels, inbox, messages, and notifications services. Each handler now imports Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/app/api/inbox/route.ts (1)
19-21: Move this audit emission to successful response paths.Right now this runs before data retrieval, so failed requests can still be logged as successful inbox reads. Prefer emitting right before each successful
NextResponse.json(...)return.♻️ Proposed adjustment
- securityAudit.logDataAccess(userId, 'read', 'inbox', userId).catch((error) => { - loggers.security.warn('[Inbox] audit log failed', { error: error instanceof Error ? error.message : String(error), userId }); - }); ... - return NextResponse.json({ + securityAudit.logDataAccess(userId, 'read', 'inbox', userId).catch((error) => { + loggers.security.warn('[Inbox] audit log failed', { error: error instanceof Error ? error.message : String(error), userId }); + }); + return NextResponse.json({ items: paginatedItems, pagination: { hasMore, nextCursor, }, } satisfies InboxResponse); ... - return NextResponse.json({ + securityAudit.logDataAccess(userId, 'read', 'inbox', userId).catch((error) => { + loggers.security.warn('[Inbox] audit log failed', { error: error instanceof Error ? error.message : String(error), userId }); + }); + return NextResponse.json({ items: paginatedItems, pagination: { hasMore, nextCursor, }, } satisfies InboxResponse);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/inbox/route.ts` around lines 19 - 21, The current securityAudit.logDataAccess(userId, 'read', 'inbox', userId) call must be moved out of the pre-fetch area and invoked immediately before each successful NextResponse.json(...) return so only successful inbox reads are audited; remove the existing early call, and in the handler(s) that return NextResponse.json (e.g., the GET/default export), call securityAudit.logDataAccess(...) right before constructing the successful NextResponse.json response and chain .catch(err => loggers.security.warn('[Inbox] audit log failed', { error: err instanceof Error ? err.message : String(err), userId })) so failures in logging are still non-blocking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/messages/conversations/route.ts`:
- Around line 236-238: The successful early return that returns
existingConversation bypasses the audit call; ensure every successful branch
logs the write to the conversation audit. Add a
securityAudit.logDataAccess(userId, 'write', 'conversation',
existingConversation.id) (matching the existing newConversation call) before
returning existingConversation in the code path that currently returns early, or
consolidate audit logging so both branches (existingConversation and
newConversation) invoke securityAudit.logDataAccess and handle errors the same
way (using loggers.security.warn).
---
Nitpick comments:
In `@apps/web/src/app/api/inbox/route.ts`:
- Around line 19-21: The current securityAudit.logDataAccess(userId, 'read',
'inbox', userId) call must be moved out of the pre-fetch area and invoked
immediately before each successful NextResponse.json(...) return so only
successful inbox reads are audited; remove the existing early call, and in the
handler(s) that return NextResponse.json (e.g., the GET/default export), call
securityAudit.logDataAccess(...) right before constructing the successful
NextResponse.json response and chain .catch(err =>
loggers.security.warn('[Inbox] audit log failed', { error: err instanceof Error
? err.message : String(err), userId })) so failures in logging are still
non-blocking.
🪄 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: b33bee88-a525-4ea4-8826-f0f5e91a4ad9
📒 Files selected for processing (12)
apps/web/src/app/api/channels/[pageId]/messages/[messageId]/reactions/route.tsapps/web/src/app/api/channels/[pageId]/messages/route.tsapps/web/src/app/api/channels/[pageId]/upload/route.tsapps/web/src/app/api/inbox/route.tsapps/web/src/app/api/messages/[conversationId]/route.tsapps/web/src/app/api/messages/conversations/[conversationId]/route.tsapps/web/src/app/api/messages/conversations/route.tsapps/web/src/app/api/messages/threads/route.tsapps/web/src/app/api/notifications/[id]/route.tsapps/web/src/app/api/notifications/push-tokens/route.tsapps/web/src/app/api/notifications/route.tsapps/web/src/app/api/notifications/unsubscribe/[token]/route.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 386acdbb5d
ℹ️ 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".
| ? `${page[0].createdAt.toISOString()}|${page[0].id}` | ||
| : null; | ||
|
|
||
| securityAudit.logDataAccess(userId, 'read', 'channel_message', pageId).catch((error) => { |
There was a problem hiding this comment.
This new call makes every GET /api/channels/[pageId]/messages request enqueue a security-audit DB write, and SecurityAuditService.logEvent() serializes all writes behind a global pg_advisory_xact_lock transaction (packages/lib/src/audit/security-audit.ts). On high-traffic channel reads (initial loads, reconnections, polling bursts), this creates a lock queue and shared pool pressure that can spill over into user-facing query latency/timeouts, so this read endpoint should not directly emit per-request chain-locked writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. The advisory lock serialization is a pre-existing architectural property of SecurityAuditService.logEvent(), not introduced by this PR. Since the audit call is fire-and-forget (.catch()), it won't affect response latency — the advisory lock queue only affects audit write throughput, not the user-facing request path.
For high-volume read endpoints, a future optimization could batch audit writes or use a write-behind queue, but that's out of scope for this wiring PR. The current pattern matches how audit calls are wired in all other routes (auth, export, account, drives, pages).
Log a read audit event when POST /conversations returns an existing conversation instead of creating a new one. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace userId with 'self' in 5 audit calls where userId was passed as resourceId. Since resourceId is included in the hash chain computation, it cannot be anonymized under GDPR right-to-erasure without breaking tamper-evident integrity. The userId column (excluded from hash) already records the actor. Add tests documenting the invariant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…vocation audit Move inbox audit call from pre-fetch to right before each successful response, so failed requests are not logged as successful reads. Add missing logTokenRevoked audit call to push-token DELETE handler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addressing Review FeedbackCodeRabbit nitpick: Inbox audit placement (inbox/route.ts:19)Fixed in c8a0ac8. Moved the audit call from pre-fetch (where it would log even on failed requests) to right before each successful CodeRabbit: Unaudited existingConversation branch (conversations/route.ts)Already fixed in 3f407d8 (prior commit on this branch). Codex: Advisory lock serialization on hot read path (channels/messages/route.ts)Acknowledged. Fire-and-forget Additional fixes in this push:
|
This branch was previously deployed
1 participant
Summary
securityAudit.logDataAccessandlogTokenCreatedinto 12 communication route files (channels, messages, notifications, inbox).catch()pattern consistent with existing audit wiring in auth/export routes'self'asresourceIdfor list/self endpoints to avoid leakinguserIdinto hash-protected fieldslogTokenRevokedaudit for push-token DELETE (token revocation)Routes Covered
GDPR Compliance
resourceIdis included in the tamper-evident hash chain and cannot be anonymized under right-to-erasure'self'instead ofuserIdasresourceIdsecurity-audit.test.tsTest plan
pnpm typecheckpasses@pagespace/lib/server🤖 Generated with Claude Code