fix(auth): close the two open CodeQL alerts in the Vue app - #312
fix(auth): close the two open CodeQL alerts in the Vue app#312rlorenzo wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
==========================================
+ Coverage 41.74% 41.79% +0.04%
==========================================
Files 992 992
Lines 49697 49684 -13
Branches 5854 5851 -3
==========================================
+ Hits 20748 20763 +15
+ Misses 28038 28007 -31
- Partials 911 914 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Bundle ReportChanges will increase total bundle size by 3.18kB (0.14%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
Files in
|
|
@coderabbitai full review |
|
f221f78 to
41d48cf
Compare
|
Warning Review limit reached
Next review available in: 18 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
- replace the always-true fullPath length guard in getLoginUrl, which existed only to register a reactive dependency, with a computed that reads the route it depends on (CodeQL js/user-controlled-bypass) - fold requireLogin's duplicate base-prefixing into withApplicationBase
41d48cf to
a172651
Compare
|
@coderabbitai full review |
|
There was a problem hiding this comment.
Pull request overview
This PR addresses two open CodeQL alerts in the Vue SPA authentication/redirect flow by centralizing login URL generation and hardening the sendBackTo deep-link redirect handling.
Changes:
- Refactors login
ReturnUrlcreation to derive fromroute.fullPathand apply a shared application-base prefix (withApplicationBase). - Moves CTS post-login
sendBackToredirect logic to the sharedhandleSendBackToRedirecthelper and switches query parsing toObject.fromEntriesfor prototype-safety. - Adds focused unit tests covering rejected redirect targets, query parsing,
__proto__handling, and base-prefix behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| VueApp/src/CTS/pages/CtsHome.vue | Uses shared RequireLogin helpers for login redirect and sendBackTo handling. |
| VueApp/src/composables/RequireLogin.ts | Simplifies reactive login URL computation, exports shared helpers, and hardens sendBackTo query parsing. |
| VueApp/src/composables/tests/require-login.test.ts | Adds unit tests validating redirect safety and base-prefixing behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CtsHome carried its own copy of the post-login redirect and login-URL build, and the copy skipped the isValidInternalPath guard that the shared helper applies. - route CtsHome through handleSendBackToRedirect and buildLoginUrl - build the redirect query with Object.fromEntries so a "__proto__" key stays own data instead of reaching the prototype chain (CodeQL js/remote-property-injection) - keep the current path as the login ReturnUrl so a sendBackTo deep link survives the CAS round trip
The guard read useRoute(), the route being navigated away from, so a pending sendBackTo navigation still saw the query that triggered it and pushed the same target again, cancelling its own navigation in a loop (measured 248 cancelled navigations before the URL settled back). - drop the guard call site so the landing page owns the redirect, which CtsHome already does through the shared helper - document that the helper belongs on the landing page, not in a guard
a172651 to
5b681db
Compare
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Closes CodeQL alerts 765 and 768. Alerts 766/767 were dismissed as false positives: only the filter values come from
route.query, the keys are always caller-supplied literals.Changes
765
js/user-controlled-bypass—getLoginUrlwrapped an always-trueroute.fullPath.length >= 0check around two identical branches. It guarded nothing; it existed only to fake a reactive dependency, sincegetCurrentPath()read the non-reactiveglobalThis.location. Now derives the return path fromroute.fullPath, with base prefixing shared viawithApplicationBase().768
js/remote-property-injection—CtsHomecarried its own copy of the post-login redirect, and the copy skipped theisValidInternalPathguard. Now calls the shared helper, which builds the query withObject.fromEntriesso a__proto__key stays own data.Redirect loop (found while testing) — the guard called the redirect helper with
useRoute(), the route being navigated away from, so a pendingsendBackTonavigation re-fired and cancelled itself. Measured onmain: 248 cancelled navigations in ~1.5s. Fixed by dropping the guard call site, since the landing page already owns the redirect.Verified in browser
Emulated user holding
SVMSecure.CTS.Students, loading/CTS/Home?sendBackTo=<target>against each file set in turn:main/CTS/MyAssessments…?tab=epa&view=listhttps://evil.example/steal/CTS/https://evil.example/steal, 404//evil.example/steal//evil.example/steal, 404Not an open redirect on
main:location.hoststayed local in both hostile cases, because vue-router resolves the value as an in-app path, not a URL.For reviewers
useRequireLoginlose the guard-levelsendBackToredirect. It looped for them too, and nothing in VIPER 1 or VIPER 2 generates those links.CtsHome's loginReturnUrlis now the current path, not a hardcoded/CTS/, so a deep link survives the sign-in round trip./CTS/serves the legacy Razor page (CTSController.Indexclaims/[area]), so the SPA deep link only works from/CTS/Home.main, so 765/768 stay open through theDevelopment/TEST leg.Testing
1134 frontend tests pass, 13 new in
require-login.test.ts(rejected targets, query parsing,__proto__, base handling).verify:buildandvue-tscgreen. Also drove the live Vite-served module in the browser: all 8 redirect cases matched the unit tests.