feat: operationalize SecurityAuditService across core auth flows#724
feat: operationalize SecurityAuditService across core auth flows#7242witstudios merged 5 commits intomasterfrom
Conversation
Wire the tamper-evident SecurityAuditService into login, logout, and admin auth flows. Create an adapter bridging existing logAuthEvent/ logSecurityEvent to securityAudit.logEvent() with fire-and-forget semantics so audit failures never block auth flows. 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. |
|
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. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a security-audit adapter and integrates non-blocking securityAudit calls into login, logout, and admin auth failure paths; expands server mock surface and re-exports audit functions from the library. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Route as Auth Route
participant Logic as Auth Logic
participant Adapter as Security Audit Adapter
participant AuditSvc as SecurityAuditService
participant DB as Audit DB
Client->>Route: POST /auth/login (credentials, headers)
Route->>Logic: validate credentials, create session
alt login success
Logic->>Adapter: auditAuthEvent(login_success, userId, email, ip)
Adapter->>AuditSvc: securityAudit.logEvent({eventType: "auth.login.success", userId, details})
AuditSvc->>DB: persist security_audit_log entry
AuditSvc-->>Adapter: ack
Adapter-->>Logic: (fire-and-forget)
else login failure
Logic->>Adapter: auditAuthEvent(login_failure, maybeEmail, ip, reason)
Adapter->>AuditSvc: securityAudit.logEvent({eventType: "auth.login.failure", details})
AuditSvc->>DB: persist security_audit_log entry
AuditSvc-->>Adapter: ack
Adapter-->>Logic: (fire-and-forget)
end
Route-->>Client: response (redirect / token)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
CI Fix: Missing
|
The login route now imports securityAudit from @pagespace/lib/server, but login-redirect.test.ts was missing the mock, causing all 3 tests to crash with 500 instead of 200. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2witstudios
force-pushed
the
ppg/operationalize-audit-service
branch
from
f0e70a8 to
aadb476
Compare
Merge two separate export lines from './audit' into one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allows manual triggering of the test suite workflow for cases where the pull_request event doesn't fire automatically. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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)
packages/lib/src/audit/security-audit-adapter.ts (1)
76-76: Email masking pattern preserves 2 characters; elsewhere 3 characters are preserved.The masking pattern
/(.{2}).*(@.*)/, '$1***$2'preserves the first 2 characters of the email local part. However, based on learnings fromsignup-passkey/route.ts, the established pattern masks email as "first 3 chars + '***@' + domain".Consider aligning with the existing pattern for consistency:
♻️ Proposed fix for consistent masking
- const maskedEmail = email ? email.replace(/(.{2}).*(@.*)/, '$1***$2') : undefined; + const maskedEmail = email ? email.replace(/(.{3}).*(@.*)/, '$1***$2') : undefined;Based on learnings: "email masked as first 3 chars + '***@' + domain" in
signup-passkey/route.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/audit/security-audit-adapter.ts` at line 76, The email masking currently assigned to maskedEmail uses the regex /(.{2}).*(@.*)/ and preserves 2 characters; update the pattern to preserve 3 characters to match the established convention (first 3 chars + '***' + domain) by changing the regex used in the maskedEmail assignment (variable maskedEmail in security-audit-adapter.ts) so it captures three leading chars (e.g., /(.{3}).*(@.*)/) and keeps the rest of the masking logic unchanged for consistency with signup-passkey/route.ts.
🤖 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/auth/login/route.ts`:
- Line 152: The login route is sending raw PII to audit/tracking calls; before
calling securityAudit.logAuthFailure, logAuthEvent, and trackAuthEvent replace
email with a masked version using the same pattern as signup-passkey (e.g., take
email.substring(0,3) + '***@' + (email.split('@')[1] || '***') into a
maskedEmail variable) and pass maskedEmail instead of email to
securityAudit.logAuthFailure, logAuthEvent and trackAuthEvent to comply with PII
retention policies.
---
Nitpick comments:
In `@packages/lib/src/audit/security-audit-adapter.ts`:
- Line 76: The email masking currently assigned to maskedEmail uses the regex
/(.{2}).*(@.*)/ and preserves 2 characters; update the pattern to preserve 3
characters to match the established convention (first 3 chars + '***' + domain)
by changing the regex used in the maskedEmail assignment (variable maskedEmail
in security-audit-adapter.ts) so it captures three leading chars (e.g.,
/(.{3}).*(@.*)/) and keeps the rest of the masking logic unchanged for
consistency with signup-passkey/route.ts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/src/app/api/auth/__tests__/login-redirect.test.tsapps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/app/api/auth/__tests__/logout.test.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/lib/auth/__tests__/auth.test.tsapps/web/src/lib/auth/auth.tspackages/lib/src/audit/__tests__/security-audit-adapter.test.tspackages/lib/src/audit/index.tspackages/lib/src/audit/security-audit-adapter.tspackages/lib/src/server.ts
| const reason = !user ? 'invalid_email' : 'invalid_password'; | ||
| logAuthEvent('failed', user?.id, email, clientIP, reason === 'invalid_email' ? 'Invalid email' : 'Invalid password'); | ||
| trackAuthEvent(user?.id, 'failed_login', { reason, email, ip: clientIP }); | ||
| securityAudit.logAuthFailure(email, clientIP, reason).catch(() => {}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the security audit service implementation
find . -type f -name "*security*audit*" -o -name "*audit*" | grep -E "\.(ts|tsx)$" | head -20Repository: 2witstudios/PageSpace
Length of output: 591
🏁 Script executed:
# Search for logAuthFailure import and implementation
rg -n "logAuthFailure" --type ts -A 2Repository: 2witstudios/PageSpace
Length of output: 2312
🏁 Script executed:
# Check the `@pagespace/lib/server` imports to find where securityAudit comes from
rg -n "from.*@pagespace/lib/server" --type tsRepository: 2witstudios/PageSpace
Length of output: 45704
🏁 Script executed:
# Read the logAuthFailure implementation from security-audit.ts
sed -n '223,260p' packages/lib/src/audit/security-audit.tsRepository: 2witstudios/PageSpace
Length of output: 807
🏁 Script executed:
# Search for email masking logic in the security audit service
rg -n "mask.*email|email.*mask|slice.*replace" packages/lib/src/audit/security-audit.tsRepository: 2witstudios/PageSpace
Length of output: 47
🏁 Script executed:
# Check the full logAuthFailure method and surrounding context
sed -n '220,280p' packages/lib/src/audit/security-audit.tsRepository: 2witstudios/PageSpace
Length of output: 1262
🏁 Script executed:
# Find the logEvent implementation
sed -n '100,200p' packages/lib/src/audit/security-audit.ts | head -60Repository: 2witstudios/PageSpace
Length of output: 1969
🏁 Script executed:
# Search for any email masking logic in the entire security-audit.ts file
rg -n "slice|mask|replace|PII" packages/lib/src/audit/security-audit.ts | head -20Repository: 2witstudios/PageSpace
Length of output: 47
🏁 Script executed:
# Check how other routes handle email masking before calling securityAudit methods
rg -B3 "securityAudit.logAuth" apps/web/src/app/api/auth/ --type ts | head -40Repository: 2witstudios/PageSpace
Length of output: 853
🏁 Script executed:
# Check logAuthSuccess implementation and how email is handled there
sed -n '200,250p' packages/lib/src/audit/security-audit.tsRepository: 2witstudios/PageSpace
Length of output: 1119
🏁 Script executed:
# Check if there's any email masking done in the login route before the calls
sed -n '140,160p' apps/web/src/app/api/auth/login/route.tsRepository: 2witstudios/PageSpace
Length of output: 1049
🏁 Script executed:
# Compare with other auth routes to see the masking pattern
rg -B5 "securityAudit\.|trackAuthEvent" apps/web/src/app/api/auth/signup-passkey/route.ts | head -30Repository: 2witstudios/PageSpace
Length of output: 810
Mask email before passing to securityAudit.logAuthFailure to comply with PII retention policies.
The email is passed directly to logAuthFailure without masking. Unlike the signup-passkey route which masks PII before logging (using the pattern email.substring(0, 3) + '***@' + domain), the login route passes the full email unmasked. This is inconsistent and fails to comply with data retention policies.
Apply the same masking pattern used in signup-passkey/route.ts before the call:
const maskedEmail = email.substring(0, 3) + '***@' + (email.split('@')[1] || '***');
securityAudit.logAuthFailure(maskedEmail, clientIP, reason).catch(() => {});Note: Also review logAuthEvent and trackAuthEvent calls on the same line block, as they also receive the unmasked email.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/auth/login/route.ts` at line 152, The login route is
sending raw PII to audit/tracking calls; before calling
securityAudit.logAuthFailure, logAuthEvent, and trackAuthEvent replace email
with a masked version using the same pattern as signup-passkey (e.g., take
email.substring(0,3) + '***@' + (email.split('@')[1] || '***') into a
maskedEmail variable) and pass maskedEmail instead of email to
securityAudit.logAuthFailure, logAuthEvent and trackAuthEvent to comply with PII
retention policies.
Master moved securityAudit/maskEmail imports to @pagespace/lib/audit. Resolved duplicate import in login/route.ts and updated login-redirect.test.ts mock to match new import paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Closes #535
Summary
logAuthEvent/logSecurityEventtosecurityAudit.logEvent()securityAuditcalls into login, logout, and admin auth flowsChanges
New files
auditAuthEvent()andauditSecurityEvent()that fan out tosecurityAudit.logEvent(). Maps auth/security event types to SecurityEventType enums. Uses fire-and-forget pattern (.catch(() => {})) to avoid blocking auth flows.Modified files
auditAuthEventandauditSecurityEventsecurityAudit,auditAuthEvent,auditSecurityEventto server barrel exportssecurityAudit.logAuthFailure()on failed login,securityAudit.logAuthSuccess()andsecurityAudit.logTokenCreated()on successful loginsecurityAudit.logLogout()andsecurityAudit.logTokenRevoked()on logoutsecurityAudit.logAccessDenied()on CSRF validation failure and admin role version mismatch inverifyAdminAuthTest mock updates (backwards compatibility)
securityAuditmocksecurityAuditmocksecurityAuditmockDesign decisions
.catch(() => {})) so audit failures never block auth flowslogSecurityEventand don't need hash-chain audit entries.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit