fix(frontend): validate redirect query targets#2158
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (1)
📝 WalkthroughWalkthroughThis PR centralizes redirect-path validation by adding ChangesCentralized Redirect Path Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (2)
src/services/redirects.ts (1)
47-56: ⚡ Quick winApply blocked-path checks to the fallback too.
Right now a blocked
targetfalls back tofallbackPath, butfallbackitself is only checked withisSafeRedirectPath(). That means a future caller can still reintroduce the loop theblockedPaths/blockedPrefixesoption is meant to prevent.♻️ Suggested change
export function getSafeRedirectPath(target: unknown, fallback = defaultRedirectPath, options: SafeRedirectOptions = {}): string { - const fallbackPath = fallback === '' || isSafeRedirectPath(fallback) - ? fallback - : defaultRedirectPath + const fallbackPath = fallback === '' + ? '' + : isSafeRedirectPath(fallback) && !isBlockedRedirectPath(fallback, options) + ? fallback + : defaultRedirectPath if (typeof target !== 'string') return fallbackPath🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/redirects.ts` around lines 47 - 56, The logic in getSafeRedirectPath allows a blocked target to fall back to a fallback that wasn't checked against blocked-path rules; update the function so the computed fallbackPath is validated by both isSafeRedirectPath and isBlockedRedirectPath (using the same options) before returning it — i.e., after computing fallbackPath (allowing ''), if the original target is invalid/blocked then check if fallbackPath isBlockedRedirectPath and if so return defaultRedirectPath; use the existing helpers isSafeRedirectPath and isBlockedRedirectPath and the SafeRedirectOptions parameter to perform this additional check.tests/redirects.unit.test.ts (1)
3-3: ⚡ Quick winUse the
~/alias in this test import.The relative path works, but it breaks the repo-wide TS import convention and is more brittle if the test file moves.
♻️ Suggested change
-import { getSafeRedirectPath, isSafeRedirectPath } from '../src/services/redirects.ts' +import { getSafeRedirectPath, isSafeRedirectPath } from '~/services/redirects'As per coding guidelines,
**/*.{ts,tsx}: use path alias~/to referencesrc/directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/redirects.unit.test.ts` at line 3, Replace the relative import for getSafeRedirectPath and isSafeRedirectPath with the project path alias; update the import statement that currently references ../src/services/redirects.ts to use the '~/services/redirects' alias so the test imports getSafeRedirectPath and isSafeRedirectPath via the repo-wide TS convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/redirects.ts`:
- Around line 47-56: The logic in getSafeRedirectPath allows a blocked target to
fall back to a fallback that wasn't checked against blocked-path rules; update
the function so the computed fallbackPath is validated by both
isSafeRedirectPath and isBlockedRedirectPath (using the same options) before
returning it — i.e., after computing fallbackPath (allowing ''), if the original
target is invalid/blocked then check if fallbackPath isBlockedRedirectPath and
if so return defaultRedirectPath; use the existing helpers isSafeRedirectPath
and isBlockedRedirectPath and the SafeRedirectOptions parameter to perform this
additional check.
In `@tests/redirects.unit.test.ts`:
- Line 3: Replace the relative import for getSafeRedirectPath and
isSafeRedirectPath with the project path alias; update the import statement that
currently references ../src/services/redirects.ts to use the
'~/services/redirects' alias so the test imports getSafeRedirectPath and
isSafeRedirectPath via the repo-wide TS convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7107785-d616-47e3-a441-1beefd9fef3a
📒 Files selected for processing (7)
src/pages/accountDisabled.vuesrc/pages/login.vuesrc/pages/onboarding/organization.vuesrc/pages/resend_email.vuesrc/pages/sso-callback.vuesrc/services/redirects.tstests/redirects.unit.test.ts
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
jooj211
left a comment
jooj211
left a comment
There was a problem hiding this comment.
Approved. I verified the centralized redirect helper rejects external, protocol-relative, scheme-like, whitespace/control-character, and backslash-normalized targets, preserves valid same-origin app paths, and applies blocked-prefix checks to fallbacks as well as requested targets.
Checks run locally:
- bun x vitest run tests/redirects.unit.test.ts tests/sso-enforcement-redirect.unit.test.ts
- bun x eslint src/services/redirects.ts src/pages/accountDisabled.vue src/pages/login.vue src/pages/onboarding/organization.vue src/pages/resend_email.vue src/pages/sso-callback.vue tests/redirects.unit.test.ts
- bun -e redirect edge-case smoke test
- git diff --check HEAD~4..HEAD
|
Closing as AI-generated spam. Part of a 50+ PR wave of duplicate |
3 participants
/claim #1667
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests