Repository navigation
feat(security): implement P1-P4 security hardening fixes - #277
Conversation
This PR implements the security hardening plan priorities 1-4: **P1: User Account Suspension Check** - Add suspendedAt/suspendedReason fields to users schema - Update user-validator.ts to check suspension status - Update session-service.ts to auto-revoke sessions for suspended users **P2: WebSocket Origin Validation Blocking** - Change validateAndLogWebSocketOrigin to return boolean and reject invalid origins - Fail closed in production when no allowed origins configured - Add proper logging for rejected connections **P3: SSRF DNS Rebinding Mitigation** - Add buildIPDirectURL helper to connect directly to validated IPs - Modify safeFetch to use resolved IP with Host header preservation - Prevents TOCTOU attacks where DNS rebinds between validation and fetch **P4: Service Token Audit Logging** - Add structured security logging to processor auth middleware - Log successful validations, failures, and scope assertion failures - Include request context (IP, endpoint, user agent) in all logs **P5: Legacy Cleanup** - Remove SERVICE_JWT_SECRET from .env.example and docker-compose.yml - Mark vulnerabilities #1, #2, #11 as RESOLVED in security docs - Update zero-trust-architecture.md to reflect opaque token migration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds account suspension tracking and enforcement, replaces SERVICE_JWT_SECRET with database-backed opaque tokens, strengthens auth middleware logging, rejects disallowed WebSocket origins, and hardens SSRF/DNS-rebinding handling via IP-direct requests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant RealtimeServer as Realtime Server
participant SessionService as Session Service / DB
participant Logger
Client->>RealtimeServer: Open WS connection (with Origin + token)
RealtimeServer->>Logger: log incoming connection (origin, ip)
RealtimeServer->>RealtimeServer: normalize & validate Origin
alt origin allowed
RealtimeServer->>SessionService: validate opaque token (session lookup)
SessionService-->>RealtimeServer: session data (user, suspendedAt, scopes, sessionId)
alt user suspended or session revoked
RealtimeServer->>Logger: security.warn/revoke (tokenPrefix, userId, reason)
RealtimeServer-->>Client: Reject connection (401/403)
else valid session
RealtimeServer->>Logger: security.info (sessionId, userId, scopes)
RealtimeServer-->>Client: Accept connection
end
else origin disallowed
RealtimeServer->>Logger: security.warn (origin rejected)
RealtimeServer-->>Client: Reject connection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/lib/src/security/url-validator.ts`:
- Around line 270-279: buildIPDirectURL causes HTTPS fetches to fail TLS
validation because SNI is set to the IP, not the original hostname; fix by not
using IP-direct URLs for https OR ensure the fetch uses an undici Agent with a
custom connector that sets servername to originalUrl.hostname; update the call
sites that consume buildIPDirectURL (and/or change buildIPDirectURL behavior)
to: for https requests either leave the URL host as the original hostname or, if
you must connect to the resolved IP, supply a dispatcher/Agent built with
buildConnector() and a connect override that passes
servername=originalUrl.hostname so SNI matches the certificate (refer to
buildIPDirectURL and the fetch/dispatcher logic that uses its return value).
🧹 Nitpick comments (2)
apps/processor/src/middleware/auth.ts (1)
100-103: Verify error type cast is safe.The error is cast to
Errorfor logging. If a non-Error object is thrown, this could result in incomplete log data.🔧 Optional: Safer error handling
} catch (error) { - loggers.security.error('Processor auth: validation error', error as Error, { + loggers.security.error('Processor auth: validation error', error instanceof Error ? error : new Error(String(error)), { ...requestContext, tokenPrefix, });apps/realtime/src/index.ts (1)
101-108: Consider validating ADDITIONAL_ALLOWED_ORIGINS format.The parsing silently ignores invalid URLs (empty strings after normalization), which is safe but could mask configuration errors.
💡 Optional: Log warning for invalid entries
const additionalOrigins = process.env.ADDITIONAL_ALLOWED_ORIGINS; if (additionalOrigins) { - const parsed = additionalOrigins + const entries = additionalOrigins.split(',').map((o) => o.trim()); + const parsed = entries - .split(',') - .map((o) => normalizeOrigin(o.trim())) + .map((o) => normalizeOrigin(o)) .filter((o) => o.length > 0); + const invalidCount = entries.length - parsed.length; + if (invalidCount > 0) { + loggers.realtime.warn('ADDITIONAL_ALLOWED_ORIGINS contains invalid entries', { + totalEntries: entries.length, + validEntries: parsed.length, + }); + } origins.push(...parsed); }
- Fix logger import path: @pagespace/lib/logger-config -> @pagespace/lib/logging/logger-config - Fix SSRF DNS rebinding for HTTPS: Skip IP-direct URLs for HTTPS because TLS/SNI requires hostname - Add safer error type casting in auth middleware (error instanceof Error check) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…ion-guard fix(ai): settle CodeQL alert #277 on the consult route without weakening its check
Summary
This PR implements the security hardening plan priorities 1-4, addressing critical security concerns identified in the security analysis.
P1: User Account Suspension Check
suspendedAtandsuspendedReasonfields to users schemauser-validator.tsto check suspension status and returnuser_suspendedfailure reasonsession-service.tsto auto-revoke sessions when user is suspendedpackages/db/src/schema/auth.ts,apps/processor/src/services/user-validator.ts,packages/lib/src/auth/session-service.tsP2: WebSocket Origin Validation Blocking
validateAndLogWebSocketOriginto return boolean and reject invalid origins (not just log)apps/realtime/src/index.tsP3: SSRF DNS Rebinding Mitigation
buildIPDirectURLhelper to connect directly to validated IPssafeFetchto use resolved IP with Host header preservationpackages/lib/src/security/url-validator.tsP4: Service Token Audit Logging
loggers.securityapps/processor/src/middleware/auth.tsP5: Legacy Cleanup
SERVICE_JWT_SECRETfrom.env.exampleanddocker-compose.ymlzero-trust-architecture.mdto reflect opaque token migration.env.example,docker-compose.yml,docs/3.0-guides-and-tools/cloud-security-analysis.md,docs/security/zero-trust-architecture.mdDatabase Migration
0053_even_slayback.sqlto addsuspendedAtandsuspendedReasoncolumns to users tableTest plan
pnpm db:migrateto apply migration🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security Improvements
✏️ Tip: You can customize this high-level summary in your review settings.