Repository navigation
Security auth hardening: per-event reauth, file access tightening, HMAC cron auth - #500
Merged
Merged
Conversation
Adds withPerEventAuth() composable middleware that re-verifies page permissions on sensitive write events. Wires document_update handler as defense-in-depth pattern for future write event handlers. Fixes #422 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Files linked to pages via filePages now require canUserViewPage for at least one linked page. Unlinked files fall back to drive membership. Closes the gap where restricted page attachments were accessible to any drive member via direct URL. Fixes #421 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace static Bearer token auth with HMAC-SHA256 signed requests for all cron endpoints. Requests now include timestamp, nonce, and signature headers with anti-replay protection (5-min window, nonce deduplication). Docker cron entrypoint generates signed requests via cron-curl helper. Fixes #419 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds HMAC-signed, nonce-protected cron request validation; implements per-event Socket.IO re-authorization and exposes its API; and introduces page-scoped file access checks that fall back to drive membership, plus related tests and Docker cron helper changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CronWorker as Cron Worker
participant CronCurl as cron-curl Helper
participant WebService as Web Service
participant ValidateSigned as validateSignedCronRequest
participant NonceStore as Nonce Store / DB
CronWorker->>CronCurl: Invoke cron-curl (method,path)
CronCurl->>CronCurl: Generate timestamp & nonce
CronCurl->>CronCurl: Compute HMAC-SHA256(timestamp:nonce:METHOD:path)
CronCurl->>WebService: Send request with X-Cron-* headers
WebService->>ValidateSigned: validateSignedCronRequest(request)
ValidateSigned->>ValidateSigned: Verify headers & timestamp freshness
ValidateSigned->>NonceStore: checkAndRecordNonce(nonce)
NonceStore-->>ValidateSigned: nonce accepted/recorded
ValidateSigned->>ValidateSigned: Recompute signature & timingSafeEqual
ValidateSigned-->>WebService: validation result (allow/deny)
WebService-->>CronWorker: 200 / 403
sequenceDiagram
participant Client as Socket Client
participant SocketHandler as withPerEventAuth wrapper
participant AuthService as reauthorizePageAccess
participant PagePerm as canUserViewPage / Permission DB
Client->>SocketHandler: emit sensitive event (payload)
SocketHandler->>SocketHandler: extract pageId via extractor
SocketHandler->>SocketHandler: isSensitiveEvent? (yes)
SocketHandler->>AuthService: reauthorizePageAccess(user, pageId, requiredLevel)
AuthService->>PagePerm: query current access level
PagePerm-->>AuthService: access result
AuthService-->>SocketHandler: authorized / denied
alt authorized
SocketHandler->>Client: invoke original handler
else denied
SocketHandler->>Client: emit error
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/app/api/memory/cron/route.ts (1)
17-19:⚠️ Potential issue | 🟡 MinorStale documentation: still references old auth mechanism.
Lines 17–18 mention "CRON_SECRET Bearer token" and
curl -H "Authorization: Bearer $CRON_SECRET", which no longer matches the HMAC-signed request flow.Proposed update
- * Security: CRON_SECRET Bearer token + internal network origin check - * Trigger via: curl -H "Authorization: Bearer $CRON_SECRET" http://localhost:3000/api/memory/cron + * Security: HMAC-signed cron requests (via cron-curl) + internal network origin check + * Trigger via: cron-curl POST http://web:3000/api/memory/cronapps/web/src/app/api/cron/calendar-sync/route.ts (1)
14-18:⚠️ Potential issue | 🟡 MinorStale documentation: curl example still references Bearer token auth.
The comment on line 18 still shows
curl -H "Authorization: Bearer $CRON_SECRET"but the route now uses HMAC-signed requests viavalidateSignedCronRequest. Update the example to reflect the new signing mechanism (or reference the cron-curl helper).apps/web/src/app/api/cron/cleanup-tokens/route.ts (1)
13-18:⚠️ Potential issue | 🟡 MinorStale documentation: auth description and curl example still reference Bearer token.
Lines 13-14 describe "CRON_SECRET Bearer token (timing-safe comparison)" and line 18 shows the old
curl -H "Authorization: Bearer $CRON_SECRET"pattern. Both should be updated to reflect HMAC-signed request authentication.
🤖 Fix all issues with AI agents
In `@apps/realtime/src/__tests__/per-event-auth.test.ts`:
- Around line 186-197: Tests currently call the wrapper with (socket, payload)
but Socket.IO invokes listeners as (data, ack?) — add an integration-style test
that simulates the real invocation by creating the mock socket with
createMockSocket, wrapping the handler with withPerEventAuth('cursor_move',
handler, { pageIdExtractor }), then invoking the returned listener as Socket.IO
would by binding the socket and passing only the payload (e.g.
wrapped.call(socket, payload)) and assert the inner handler was called with the
socket and payload and that mockedGetUserAccessLevel behavior is exercised
accordingly.
In `@apps/realtime/src/index.ts`:
- Around line 861-871: The handler passed to withPerEventAuth currently expects
(sock, payload) but after fixing per-event-auth to close over the socket the
handler must accept only the payload; update the document_update registration to
use a single-argument handler (e.g., async (data) => { ... }) and inside the
body reference the socket object that the wrapper supplies via its closure (use
the AuthSocket provided by the wrapper instead of expecting sock as the first
argument), keep using pageIdExtractor and the same log/emit logic but replace
references to the removed first parameter accordingly.
In `@apps/realtime/src/per-event-auth.ts`:
- Around line 199-241: The wrapper currently expects (socket, payload) but
Socket.IO invokes listeners with only the client payload, so change
withPerEventAuth to return a single-argument listener that closes over the
socket via the function receiver (use this as AuthSocket) or otherwise read the
socket from the listener context; specifically, modify the returned async
function signature from (socket: AuthSocket, payload: unknown) => Promise<void>
to (payload: unknown) => Promise<void> and inside cast this to AuthSocket (e.g.
const socket = this as AuthSocket) before using socket.data, socket.emit, etc.,
update the function's TypeScript typings accordingly (WithPerEventAuth return
type and any tests that invoked the old signature) so Socket.IO will call it
correctly and the handler (document_update etc.) executes.
In `@apps/web/src/lib/auth/__tests__/file-access.test.ts`:
- Around line 61-76: Tests currently recreate the canUserAccessFile logic inline
instead of exercising the real function, so import the production
canUserAccessFile from `@pagespace/lib/permissions/file-access` in
file-access.test.ts and remove the hand-rolled implementation; then mock only
the leaf helpers (canUserViewPage and isUserDriveMember) that canUserAccessFile
calls—using vi.mock or a barrel-export mock for the module that exports those
functions—so the orchestration logic in canUserAccessFile is exercised while
controlling the dependencies' behavior.
In `@apps/web/src/lib/auth/cron-auth.ts`:
- Around line 146-181: The current blind usedNonces.clear() in
checkAndRecordNonce can evict still-valid nonces; change usedNonces from
Set<string> to Map<string, number> (nonce -> timestampMs), record Date.now()
when adding, and replace the coarse clear logic (lastNonceCleanup) with
timestamp-aware pruning that removes only entries older than
TIMESTAMP_MAX_AGE_SECONDS * 1000 before checking; keep computeCronSignature
unchanged but update checkAndRecordNonce to iterate usedNonces and delete stale
entries, then reject if map.has(nonce) else set(nonce, now) and return true.
In `@docker/cron/entrypoint.sh`:
- Around line 51-52: The cron-curl wrapper currently gets world-readable
permissions after sed + chmod +x which exposes the embedded CRON_SECRET; update
the entrypoint logic to remove the world-readable bit and restrict execution to
the intended owner (e.g., use chmod 700 or chmod 500 and chown to the service
user) so the file /usr/local/bin/cron-curl is not readable by other
users/processes, while ensuring the target runtime user (cron or a specific
non-root user) still has execute permission; reference the cron-curl filename
and CRON_SECRET substitution flow and ensure permissions are set immediately
after the sed replacement.
- Line 51: The sed substitution line sed -i "s|__CRON_SECRET__|${CRON_SECRET}|g"
in entrypoint.sh can be broken by sed-special chars in CRON_SECRET; replace this
with a literal-safe injection (e.g., run an awk or Python one-liner that reads
/usr/local/bin/cron-curl and uses a literal gsub/replace of the token
"__CRON_SECRET__" with the CRON_SECRET environment value, or alternatively
export CRON_SECRET and modify cron-curl to read the secret from the environment
at runtime) so that __CRON_SECRET__ is replaced without interpreting &, \, | or
other metacharacters.
🧹 Nitpick comments (4)
apps/realtime/src/per-event-auth.ts (1)
172-180: DuplicateAuthSocketinterface — also defined inindex.ts(Line 371).Both files define identical
AuthSocketinterfaces. Export this type fromper-event-auth.ts(or a shared types module) and import it inindex.tsto avoid drift.apps/realtime/src/__tests__/per-event-auth.test.ts (1)
175-184:as anycast and(p: any)in test helpers violate the no-anyguideline.While pragmatic in tests, the
as anyon Line 181 and multiple(p: any)usages (Lines 188, 208, etc.) could be replaced with minimal type stubs. As per coding guidelines, "Never useanytypes - always use proper TypeScript types."Example: typed mock and extractor
- function createMockSocket(userId?: string) { + function createMockSocket(userId?: string): { socket: { data: { user?: { id: string; name: string; avatarUrl: string | null } }; emit: ReturnType<typeof vi.fn> }; emitFn: ReturnType<typeof vi.fn> } { const emitFn = vi.fn(); return { socket: { data: { user: userId ? { id: userId, name: 'Test', avatarUrl: null } : undefined }, emit: emitFn, - } as any, + } as unknown as AuthSocket, emitFn, }; }For the extractor, cast the payload specifically:
-pageIdExtractor: (p: any) => p.pageId +pageIdExtractor: (p: unknown) => (p as { pageId?: string })?.pageIdapps/web/src/lib/auth/cron-auth.ts (1)
238-245:>=might be more appropriate than>for timestamp boundary.Line 240 uses
Math.abs(now - requestTime) > TIMESTAMP_MAX_AGE_SECONDS, meaning a request exactly 300 seconds old is accepted. Using>=would be strictly safer (reject at exactly the boundary), matching the nonce cleanup window edge case mentioned above.This is a very minor point — the practical impact is negligible.
packages/lib/src/permissions/file-access.ts (1)
33-38: Consider: parallel page-access checks for marginal latency improvement.The sequential loop works correctly but could be parallelized if a file is ever linked to many pages. Low priority since file-page cardinality is typically small.
♻️ Optional: parallelize page access checks
if (linkedPages.length > 0) { - for (const { pageId } of linkedPages) { - const hasAccess = await canUserViewPage(userId, pageId); - if (hasAccess) return true; - } - return false; + const results = await Promise.all( + linkedPages.map(({ pageId }) => canUserViewPage(userId, pageId)) + ); + return results.some(Boolean); }
Critical fixes: - Fix withPerEventAuth Socket.IO signature mismatch: wrapper now accepts socket as first arg and returns single-arg listener via closure, matching how Socket.IO actually invokes event listeners (data, ack?) - Add integration-style test verifying Socket.IO calling convention Security improvements: - Replace sed with awk for literal-safe CRON_SECRET injection in entrypoint - Restrict cron-curl permissions from 755 to 700 - Implement timestamp-aware nonce pruning instead of blanket clear() - Tighten timestamp boundary check from > to >= (reject at exactly 300s) Code quality: - Export AuthSocket from per-event-auth.ts, remove duplicate in index.ts - Replace all `as any` / `(p: any)` in tests with proper types - Add structural sync assertion for file-access test - Update stale Bearer token docs in 3 cron routes to reference HMAC/cron-curl Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
All review feedback addressed in 5b42150Actionable comments — all resolved:
Outside-diff stale docs — all updated:
Nitpick comments — all addressed:
All targeted tests pass locally (21/21 per-event-auth, 7/7 file-access, 39/39 cron-auth). |
4 tasks done
1 of 8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses three security gaps identified in audit:
Per-event Socket.IO reauth ([Security] Integrate per-event realtime reauthorization into live Socket.IO handlers #422): Wires existing dead-code
shouldReauthorize()andreauthorizePageAccess()into the realtime server via a composablewithPerEventAuth()middleware. Applied to a newdocument_updatehandler as defense-in-depth pattern for future write events.Tighten attachment auth ([Security] Clarify and tighten attachment authorization beyond drive-wide membership #421): Files linked to pages via
filePagesnow require page-level access (canUserViewPage) instead of only checking drive membership. Unlinked files fall back toisUserDriveMember. NewcanUserAccessFile()utility in@pagespace/lib/permissions.HMAC-signed cron requests ([Security] Replace cron host/header trust with authenticated service identity #419): Replaces static Bearer token auth with HMAC-SHA256 signed requests including timestamp (5-min window), nonce (anti-replay deduplication), and route-bound signatures. Docker cron entrypoint generates signed requests via
cron-curlhelper usingopenssl dgst.Fixes #419, Fixes #421, Fixes #422
Test plan
withPerEventAuthmiddleware)🤖 Generated with Claude Code
Summary by CodeRabbit
Security
New Features
Tests