fix(security): consolidate email regex, shell executor, CSRF token patterns#857
fix(security): consolidate email regex, shell executor, CSRF token patterns#8572witstudios merged 4 commits intomasterfrom
Conversation
…tterns (#F-3d/e, F-advisory, F-5d) Three P3 pattern consolidation fixes from QA audit: - Extract shared isValidEmail() to packages/lib with bounded-quantifier RFC 5322 regex, 254-char length check, and TLD requirement. Replace 4 inline regex usages across web, control-plane, and marketing. - Add execFile() method to shell-executor using child_process.execFile (no shell interpretation). Add slug validation to upgrade-service as defense-in-depth (all other callers already validate). - Remove unused csrfToken from OAuth redirect URLs (Google + Apple callbacks). No client code reads it; CSRF tokens come from the /api/auth/csrf endpoint. Eliminates exposure in browser history, server logs, and Referer headers. Co-Authored-By: Claude Opus 4.6 (1M context) <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
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 12 minutes and 0 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 (3)
📝 WalkthroughWalkthroughThis PR adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ShellExecutor
participant History as History Log
participant ChildProcess as child_process.execFile
rect rgb(100, 150, 200, 0.5)
note over Client,ChildProcess: Real ShellExecutor Flow
Client->>ShellExecutor: execFile(program, args, options)
ShellExecutor->>ChildProcess: execFile(program, args)
alt Command Completes
ChildProcess-->>ShellExecutor: callback(error, stdout, stderr)
ShellExecutor->>History: record {command, args, options}
ShellExecutor-->>Client: {stdout, stderr, exitCode}
else Command Fails
ChildProcess-->>ShellExecutor: callback(error, stdout, stderr)
ShellExecutor->>History: record {command, args, options}
ShellExecutor-->>Client: {stdout, stderr, exitCode: 1}
end
end
rect rgb(200, 100, 100, 0.5)
note over Client,ChildProcess: Mock ShellExecutor Flow (with Timeout)
Client->>ShellExecutor: execFile(program, args, options)
alt Response Delay > Timeout
ShellExecutor->>History: (no record on timeout)
ShellExecutor-->>Client: {stdout: '', stderr: 'command timed out', exitCode: -1}
else Within Timeout
ShellExecutor->>ShellExecutor: await response.delay
ShellExecutor->>History: record {command, args}
ShellExecutor-->>Client: {stdout, stderr, exitCode}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related issues
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 |
# Conflicts: # apps/web/src/app/api/account/route.ts
The @pagespace/lib mock in get-patch-route.test.ts was missing the isValidEmail function after it was added to the import. Also adds deleteMonitoringDataForUser from the master merge. Co-Authored-By: Claude Opus 4.6 (1M context) <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
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/auth/google/callback/__tests__/route.test.ts (1)
778-786:⚠️ Potential issue | 🟡 MinorTest description is now inconsistent with the assertion.
The test is named "redirects to returnUrl with auth success and CSRF token" (line 778), but line 786 now asserts that
csrfTokenis not in the URL. The description should be updated to reflect the new expected behavior.Suggested fix
- it('redirects to returnUrl with auth success and CSRF token', async () => { + it('redirects to returnUrl with auth success without CSRF token in URL', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/google/callback/__tests__/route.test.ts` around lines 778 - 786, The test title is inconsistent with its assertion: update the it(...) description string currently "redirects to returnUrl with auth success and CSRF token" to reflect the expectation that csrfToken is absent (e.g., "redirects to returnUrl with auth success without CSRF token") so the test name matches the assertion expect(location).not.toContain('csrfToken') in the test for the GET handler.apps/web/src/app/api/auth/apple/callback/__tests__/route.test.ts (1)
933-946:⚠️ Potential issue | 🟡 MinorTest description is inconsistent with the updated assertion.
The test name says "redirects to returnUrl with auth success and CSRF token" (line 933), but line 946 now asserts that
csrfTokenis not in the redirect URL. This matches the same issue in the Google OAuth tests; update for consistency.Suggested fix
- it('redirects to returnUrl with auth success and CSRF token', async () => { + it('redirects to returnUrl with auth success without CSRF token in URL', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/apple/callback/__tests__/route.test.ts` around lines 933 - 946, Update the test title in the Apple callback test block so it matches the assertions: change the it(...) description that currently reads "redirects to returnUrl with auth success and CSRF token" to reflect that csrfToken should NOT be present (e.g., "redirects to returnUrl with auth success and no CSRF token"); locate the test in route.test.ts where createSignedState, createCallbackRequest and POST are used and adjust only the string description to match the existing expectations.apps/web/src/app/api/auth/google/__tests__/google-callback-redirect.test.ts (1)
204-236:⚠️ Potential issue | 🟡 MinorTest description is inconsistent with the updated assertion.
The test name says "should create session and redirect with CSRF token" (line 204), but line 236 now asserts that
csrfTokenis not in the redirect URL. Update the description to match the new expected behavior.Suggested fix
- it('given successful OAuth, should create session and redirect with CSRF token', async () => { + it('given successful OAuth, should create session and redirect without CSRF token in URL', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/google/__tests__/google-callback-redirect.test.ts` around lines 204 - 236, Update the test description string in the it(...) block named "should create session and redirect with CSRF token" to reflect the actual assertion (no csrfToken in redirect), e.g. change to "should create session and redirect without CSRF token" so the it() description matches the expectation in the test that location does not contain csrfToken; keep all assertions and mocks (sessionService.createSession, appendSessionCookie, GET call) unchanged.
🧹 Nitpick comments (1)
apps/web/src/app/api/account/__tests__/get-patch-route.test.ts (1)
45-57: Mock implementation duplicates validation logic.The mock replicates the
isValidEmaillogic verbatim. If the real implementation inpackages/lib/src/validators/email.tschanges (e.g., different max length or regex), this mock will diverge, causing tests to pass while production behavior differs.Consider using
vi.importActualfor the email validator to avoid drift:vi.mock('@pagespace/lib', async () => { const actual = await vi.importActual<typeof import('@pagespace/lib')>('@pagespace/lib'); return { isValidEmail: actual.isValidEmail, // use real implementation createUserServiceToken: vi.fn(), deleteAiUsageLogsForUser: vi.fn(), deleteMonitoringDataForUser: vi.fn(), }; });This ensures test validation behavior stays in sync with production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/account/__tests__/get-patch-route.test.ts` around lines 45 - 57, The mock in the vi.mock block duplicates the isValidEmail implementation causing drift; replace the copied validator by importing the real implementation via vi.importActual and return isValidEmail from the actual module while keeping createUserServiceToken, deleteAiUsageLogsForUser, and deleteMonitoringDataForUser as vi.fn() so tests use production validation but stub the side-effect functions. Ensure you reference the existing vi.mock call and the symbols isValidEmail, createUserServiceToken, deleteAiUsageLogsForUser, and deleteMonitoringDataForUser when making the change.
🤖 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/marketing/src/app/api/contact/route.ts`:
- Around line 3-9: This file duplicates the email validator (EMAIL_PATTERN and
isValidEmail); replace the local implementation by importing and using the
shared validator exported from `@pagespace/lib` or `@pagespace/lib/validators`
instead: remove the EMAIL_PATTERN constant and local isValidEmail function, add
an import for the shared isValidEmail (or named export) and update any
references in route.ts to call that imported function so validation is
centralized with the other consumers (apps/web and control-plane).
---
Outside diff comments:
In `@apps/web/src/app/api/auth/apple/callback/__tests__/route.test.ts`:
- Around line 933-946: Update the test title in the Apple callback test block so
it matches the assertions: change the it(...) description that currently reads
"redirects to returnUrl with auth success and CSRF token" to reflect that
csrfToken should NOT be present (e.g., "redirects to returnUrl with auth success
and no CSRF token"); locate the test in route.test.ts where createSignedState,
createCallbackRequest and POST are used and adjust only the string description
to match the existing expectations.
In `@apps/web/src/app/api/auth/google/__tests__/google-callback-redirect.test.ts`:
- Around line 204-236: Update the test description string in the it(...) block
named "should create session and redirect with CSRF token" to reflect the actual
assertion (no csrfToken in redirect), e.g. change to "should create session and
redirect without CSRF token" so the it() description matches the expectation in
the test that location does not contain csrfToken; keep all assertions and mocks
(sessionService.createSession, appendSessionCookie, GET call) unchanged.
In `@apps/web/src/app/api/auth/google/callback/__tests__/route.test.ts`:
- Around line 778-786: The test title is inconsistent with its assertion: update
the it(...) description string currently "redirects to returnUrl with auth
success and CSRF token" to reflect the expectation that csrfToken is absent
(e.g., "redirects to returnUrl with auth success without CSRF token") so the
test name matches the assertion expect(location).not.toContain('csrfToken') in
the test for the GET handler.
---
Nitpick comments:
In `@apps/web/src/app/api/account/__tests__/get-patch-route.test.ts`:
- Around line 45-57: The mock in the vi.mock block duplicates the isValidEmail
implementation causing drift; replace the copied validator by importing the real
implementation via vi.importActual and return isValidEmail from the actual
module while keeping createUserServiceToken, deleteAiUsageLogsForUser, and
deleteMonitoringDataForUser as vi.fn() so tests use production validation but
stub the side-effect functions. Ensure you reference the existing vi.mock call
and the symbols isValidEmail, createUserServiceToken, deleteAiUsageLogsForUser,
and deleteMonitoringDataForUser when making the change.
🪄 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: 285d9103-7a95-4d8b-92c0-2f1858ba9bbb
📒 Files selected for processing (19)
apps/control-plane/src/services/__tests__/shell-executor.test.tsapps/control-plane/src/services/__tests__/upgrade-service.test.tsapps/control-plane/src/services/shell-executor.tsapps/control-plane/src/services/upgrade-service.tsapps/control-plane/src/validation/tenant-validation.tsapps/control-plane/vitest.config.tsapps/marketing/src/app/api/contact/route.tsapps/web/src/app/api/account/__tests__/get-patch-route.test.tsapps/web/src/app/api/account/route.tsapps/web/src/app/api/auth/apple/callback/__tests__/route.test.tsapps/web/src/app/api/auth/apple/callback/route.tsapps/web/src/app/api/auth/google/__tests__/google-callback-redirect.test.tsapps/web/src/app/api/auth/google/__tests__/open-redirect-protection.test.tsapps/web/src/app/api/auth/google/callback/__tests__/route.test.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/internal/contact/route.tspackages/lib/src/validators/__tests__/email.test.tspackages/lib/src/validators/email.tspackages/lib/src/validators/index.ts
💤 Files with no reviewable changes (2)
- apps/web/src/app/api/auth/apple/callback/route.ts
- apps/web/src/app/api/auth/google/callback/route.ts
Test names referenced "CSRF token" in redirect but assertions now verify csrfToken is absent. Updated 3 test descriptions to match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressing CodeRabbit review feedback: Test descriptions (3 files) — Fixed in commit 6fdde79. Updated all test names that still referenced "CSRF token" to say "without CSRF token in URL" to match the Marketing email duplication — Intentional. Mock duplication in |
1 participant
Summary
Three P3 nice-to-have consolidation fixes from independent QA audit. No vulnerabilities — maintenance risk reduction only.
packages/lib/src/validators/email.tswithisValidEmail(). Replaces 4 inconsistent inline regex patterns across web, control-plane, and marketing. Adds 254-char RFC 5321 length check and TLD requirement. 12 unit tests including ReDoS resistance.execFile()method toShellExecutorusingchild_process.execFile(no shell interpretation). Add slug validation toupgrade-service.ts(the only caller that didn't validate). 6 new tests.csrfTokenfrom Google/Apple OAuth redirect URLs. No client code reads it from the URL — CSRF tokens come from/api/auth/csrfendpoint. Eliminates exposure in browser history, server logs, and Referer headers. 5 test assertions updated.Test plan
packages/libvalidator tests pass (46/46) — email + id validatorscontrol-planeaffected tests pass (83/83) — tenant-validation, shell-executor, upgrade-servicewebOAuth tests pass (295/295) — all Google + Apple auth test files🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests