feat: harden cron auth with zero-trust HMAC validation#686
feat: harden cron auth with zero-trust HMAC validation#6862witstudios merged 4 commits intomasterfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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. 📝 WalkthroughWalkthroughCron authentication replaced legacy secret validation with HMAC-SHA256 signed requests, nonce anti-replay, and timestamp checks. Multiple cron route handlers now call Changes
Sequence DiagramsequenceDiagram
participant Client as Cron Scheduler
participant Route as Cron Route Handler
participant Validator as validateSignedCronRequest
participant HeaderCheck as Header Validation
participant SigVerify as Signature Verification
participant NonceStore as Nonce Store
participant NetworkCheck as Internal Network Check
Client->>Route: GET /api/cron/[endpoint]\n(X-Cron-Timestamp, X-Cron-Nonce, X-Cron-Signature)
Route->>Validator: invoke validateSignedCronRequest(request)
Validator->>HeaderCheck: ensure headers present & timestamp fresh
alt headers/timestamp invalid
HeaderCheck-->>Validator: error
Validator-->>Route: NextResponse (400/403)
else
Validator->>SigVerify: verify HMAC-SHA256 signature (timing-safe)
alt signature invalid
SigVerify-->>Validator: mismatch
Validator-->>Route: NextResponse (403)
else
Validator->>NonceStore: checkAndRecordNonce(nonce)
alt nonce replay
NonceStore-->>Validator: reused
Validator-->>Route: NextResponse (403)
else
Validator->>NetworkCheck: confirm internal-network origin
alt not internal
NetworkCheck-->>Validator: rejected
Validator-->>Route: NextResponse (403)
else
Validator-->>Route: null (authorized)
Route->>Route: proceed with cron task
Route-->>Client: 200 OK
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/web/src/app/api/cron/purge-deleted-messages/route.ts (1)
6-14:⚠️ Potential issue | 🟡 MinorStale docstring still references Bearer token auth.
The JSDoc comment at lines 12–13 says
curl -H "Authorization: Bearer $CRON_SECRET", but the route now uses HMAC-signed headers (x-cron-timestamp,x-cron-nonce,x-cron-signature). Update the example to reflect the new invocation method.apps/web/src/app/api/cron/cleanup-orphaned-files/route.ts (1)
19-24:⚠️ Potential issue | 🟡 MinorStale docstring still references Bearer token auth.
Lines 20–24 describe authentication as "CRON_SECRET Bearer token (timing-safe comparison)" — this no longer matches the HMAC-signed model. Same issue as in
purge-deleted-messages/route.tsand other cron routes; update all to document the newx-cron-*header approach.apps/web/src/app/api/cron/verify-audit-chain/route.ts (1)
11-12:⚠️ Potential issue | 🟡 MinorStale docstring — same Bearer token reference as other routes.
Line 12 still shows the old
curl -H "Authorization: Bearer $CRON_SECRET"invocation. Please update across all cron route docstrings in this PR.apps/web/src/app/api/cron/retention-cleanup/route.ts (1)
14-19:⚠️ Potential issue | 🟡 MinorStale docstring — same Bearer token reference.
Lines 15–16 and 18–19 describe the old auth model. Same fix needed here as the other cron routes.
apps/web/src/app/api/cron/purge-ai-usage-logs/route.ts (1)
14-15:⚠️ Potential issue | 🟡 MinorStale docstring — same Bearer token reference.
Line 15 still references the old
curl -H "Authorization: Bearer $CRON_SECRET"pattern. Same fix as the other cron routes.apps/web/src/lib/auth/cron-auth.ts (1)
69-108:⚠️ Potential issue | 🟠 MajorUse shared Redis for distributed nonce tracking across instances.
The in-memory
Mapdoes not persist across serverless invocations or multiple replicas. An attacker can replay a signed request against a different instance within the 5-minute window, bypassing the replay check. While the HMAC signature + timestamp provide significant protection, this creates a replay vulnerability in distributed deployments.Migrate
checkAndRecordNonce()to usegetSharedRedisClient()from@pagespace/lib/services/shared-rediswith graceful fallback to in-memory for development, following the pattern indistributed-rate-limit.ts.
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/auth/__tests__/cron-auth.test.ts`:
- Line 96: Remove the stray bare semicolon left in the test file
apps/web/src/lib/auth/__tests__/cron-auth.test.ts (it was a leftover from
removed validateCronRequest / hasValidCronSecret tests); delete the lone ";" so
the file contains only valid test code and re-run the test suite to ensure no
syntax errors remain.
🧹 Nitpick comments (1)
apps/web/src/lib/auth/cron-auth.ts (1)
172-205: Nonce is recorded before signature is verified — a minor ordering concern.
checkAndRecordNonceat line 183 records the nonce into the store. If the subsequent signature check (line 200) fails, the nonce has been "burned" — a legitimate request using that same nonce would then be rejected. In practice this is harmless because nonces are random and unique per request, but reordering to verify the signature first and record the nonce only on success would be slightly more robust.♻️ Suggested reorder: verify signature before recording nonce
// Anti-replay: check timestamp freshness const requestTime = parseInt(timestamp, 10); const now = Math.floor(Date.now() / 1000); if (isNaN(requestTime) || Math.abs(now - requestTime) >= TIMESTAMP_MAX_AGE_SECONDS) { return NextResponse.json( { error: 'Forbidden - cron request timestamp expired' }, { status: 403 } ); } - // Anti-replay: check nonce uniqueness - if (!checkAndRecordNonce(nonce)) { - return NextResponse.json( - { error: 'Forbidden - cron request nonce already used' }, - { status: 403 } - ); - } - // Recompute and compare signature const url = new URL(request.url); const method = request.method.toUpperCase(); const path = url.pathname; const expectedSignature = computeCronSignature(cronSecret, timestamp, nonce, method, path); const expectedBuffer = Buffer.from(expectedSignature, 'utf-8'); const providedBuffer = Buffer.from(signature, 'utf-8'); if (expectedBuffer.length !== providedBuffer.length || !timingSafeEqual(expectedBuffer, providedBuffer)) { return NextResponse.json( { error: 'Forbidden - invalid cron signature' }, { status: 403 } ); } + // Anti-replay: check nonce uniqueness (after signature verified) + if (!checkAndRecordNonce(nonce)) { + return NextResponse.json( + { error: 'Forbidden - cron request nonce already used' }, + { status: 403 } + ); + } + // Defense-in-depth: require internal network origin
Migrate all cron endpoints to HMAC-signed validation and make CRON_SECRET required in production (fail-closed security model). Changes: - Migrate 5 routes from legacy Bearer token to signed validation - Add production fail-closed: 403 if CRON_SECRET not configured - Remove legacy validateCronRequest and hasValidCronSecret exports - Update tests for new fail-closed behavior All 7 cron routes now use validateSignedCronRequest with: - HMAC-SHA256 signature verification - Anti-replay protection (timestamp + nonce) - Defense-in-depth internal network checks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove stray semicolon in test file (line 96) - Update stale docstrings in 5 cron routes to reflect HMAC auth model - Reorder validation: verify signature before recording nonce - Update validateSignedCronRequest docstring with correct validation order Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2witstudios
force-pushed
the
feat/zero-trust-cron-hardening
branch
from
da5aa84 to
c43f175
Compare
Addressed CodeRabbit Review FeedbackThank you for the thorough review! I've addressed the following items in commit c43f175: Fixed:
Out of scope (follow-up work):
All tests pass (25/25) and the branch has been rebased on latest master. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/lib/auth/cron-auth.ts (2)
69-70: In-memory nonce store won't prevent replay across multiple server instances.The
usedNoncesMap is process-local. If the app runs on multiple replicas (e.g., serverless functions, multiple pods), a nonce accepted by one instance can be replayed against another. For a single-instance deployment this is fine, but if you scale horizontally, consider an external store (e.g., Redis with TTL) or document the single-instance assumption.
30-56:isInternalRequestis spoofable when used outside a trusted reverse proxy.The
hostheader and absence ofx-forwarded-forcan both be controlled by an attacker connecting directly. This is acceptable as defense-in-depth behind HMAC, but worth a doc note that the infrastructure (reverse proxy/load balancer) must setx-forwarded-foron all inbound requests for this check to be meaningful.apps/web/src/lib/auth/__tests__/cron-auth.test.ts (1)
137-149: Consider resettingcronSecretWarningLoggedbetween tests.The module-level
cronSecretWarningLoggedflag persists across tests. If the "without CRON_SECRET" development-mode test runs first and sets the flag, subsequent tests in the same suite won't trigger the warning log path. This doesn't affect correctness today since no test asserts on the warning, but it could cause confusion if someone adds such an assertion later. A_resetWarningFlag()test helper (similar to_resetNonceStore) would make the test suite more robust.
- Document single-instance assumption for in-memory nonce store - Add infrastructure requirement note for isInternalRequest - Add _resetWarningFlag() test helper for proper test isolation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addressed Remaining Nitpicks (commit 8653372)
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/lib/auth/cron-auth.ts (2)
46-52:host.startsWith('localhost')also matches hostnames likelocalhost.evil.com.Since this is defense-in-depth behind HMAC, it's low risk, but a stricter check would anchor against the port separator or end-of-string:
♻️ Tighter localhost match
- if ( - host.startsWith('localhost') || - host.startsWith('127.0.0.1') || - host.startsWith('[::1]') - ) { + const hostname = host.split(':')[0]; + if (['localhost', '127.0.0.1', '[::1]'].includes(hostname)) {
74-78: In-memory nonce store has no upper-bound on size.Acknowledged as a known single-instance limitation (and Redis is a documented follow-up), but if a burst of requests occurs, the map grows unchecked until the next cleanup. Consider adding a cap (e.g., reject if
usedNonces.size > MAX) as a simple safeguard against memory exhaustion, even in single-instance mode.
- Use regex to anchor localhost match to port separator or end-of-string (prevents localhost.evil.com matching) - Add MAX_NONCES (10,000) limit to prevent memory exhaustion under burst - Add tests for both security improvements Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Additional Security Hardening (commit 42dcf6e)
Both improvements have test coverage (27 tests now passing). |
1 participant
Summary
Addresses #562 - Migrate all cron endpoints to HMAC-signed validation and make
CRON_SECRETrequired in production (fail-closed security model).validateSignedCronRequestCRON_SECRETnot configured whenNODE_ENV === 'production'validateCronRequestandhasValidCronSecretexports (no backward compatibility)Security Model
All 7 cron routes now use
validateSignedCronRequestwith:CRON_SECRETnot setTest plan
cd apps/web && pnpm vitest run src/lib/auth/__tests__/cron-auth.test.ts(25/25 passing)🤖 Generated with Claude Code
Summary by CodeRabbit