Adjust login url handling for authenticated users#3079
Open
Adjust login url handling for authenticated users#3079
Conversation
Users are no longer redirected to a fixed location via the guest middleware, instead they are redirected to the external login route. We don't redirect directly to prevent an open redirect. The redirect via the external_login route is happening via vue router and therefore only allows valid SPA route redirects
Contributor
|
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 44 minutes and 32 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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds typed FormRequest classes, a shared REDIRECT_URL constant, and short-circuits authenticated users on OIDC/Shibboleth redirect endpoints to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3079 +/- ##
=============================================
- Coverage 96.77% 96.70% -0.07%
- Complexity 1924 1935 +11
=============================================
Files 457 461 +4
Lines 13132 13127 -5
Branches 2133 2141 +8
=============================================
- Hits 12708 12695 -13
- Misses 424 432 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
resources/js/views/ExternalLogin.vue (1)
84-88:⚠️ Potential issue | 🟡 MinorConsider validating
route.query.redirectbeforerouter.push(pre-existing, but exposure broadened by this PR).This line is not changed, but the PR widens the set of flows that land here with an attacker-influenceable
redirectquery (authenticated users can now reach/external_login?redirect=…directly). Ifroute.query.redirectis something like//evil.com/x,http://evil.com, or an array, behavior may be unsafe or buggy:
- Protocol-relative (
//host) values may be accepted by some history implementations as external navigations.- Repeated
?redirect=a&redirect=bbecomes an array —router.push(['…','…'])is not a valid argument.A simple guard would make the contract explicit:
🛡️ Suggested guard
- if (route.query.redirect !== undefined) { - router.push(route.query.redirect); + const redirect = route.query.redirect; + if (typeof redirect === "string" && /^\/(?![/\\])/.test(redirect)) { + router.push(redirect); } else { router.push({ name: "rooms.index" }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/views/ExternalLogin.vue` around lines 84 - 88, The redirect handling in ExternalLogin.vue currently pushes route.query.redirect without validation; ensure route.query.redirect is a safe single relative path before calling router.push. Update the logic around route.query.redirect: verify it's a string (if it's an array, pick the first element or ignore), reject values that start with '//' or contain a scheme like 'http:' or 'https:', and only allow paths that begin with a single '/' (or otherwise match an explicit internal-route whitelist); if the value fails validation, fall back to router.push({ name: "rooms.index" }). Use the existing route.query.redirect and router.push symbols when locating and changing the code.
🧹 Nitpick comments (2)
app/Auth/Shibboleth/ShibbolethController.php (1)
28-37: Minor: use lowercaseauth()helper and consider simplifying query construction.Two small points:
Auth()with an uppercaseAis unconventional — Laravel's global helper is lowercasedauth(). PHP function names are case-insensitive so it works, but it's inconsistent with the rest of the codebase and with the OIDC controller (which uses the same casing here). Preferauth()->check()for consistency.- The two-branch construction can be collapsed by building the query array upfront:
♻️ Suggested simplification
- if (Auth()->check()) { - $uri = Uri::of(self::REDIRECT_URL)->withQuery(['no_message' => true]); - if ($request->query('redirect')) { - return redirect($uri - ->withQuery(['redirect' => $request->query('redirect')]) - ->value()); - } - - return redirect($uri->value()); - } + if (auth()->check()) { + $query = ['no_message' => true]; + if ($request->query('redirect')) { + $query['redirect'] = $request->query('redirect'); + } + + return redirect(Uri::of(self::REDIRECT_URL)->withQuery($query)->value()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Auth/Shibboleth/ShibbolethController.php` around lines 28 - 37, Replace the unconventional Auth() call with the lowercase helper auth() in the conditional (use auth()->check()) and simplify the redirect logic in ShibbolethController by building a single query array first (include 'no_message' => true and conditionally add 'redirect' => $request->query('redirect')), then apply that query to Uri::of(self::REDIRECT_URL) and return a single redirect(...) using the uri->value(); this collapses the two-branch return into one and keeps URI construction centralized.app/Auth/OIDC/OIDCController.php (1)
30-40: Duplicated short-circuit logic withShibbolethController.This authenticated-user short-circuit is byte-for-byte identical to the new block in
app/Auth/Shibboleth/ShibbolethController.php(lines 28–37). Consider extracting it into a small trait, a shared helper, or a base controller method to avoid drift (e.g., if theno_messagecontract, URL, or validation rules change later, both places must be updated in lockstep).Same
Auth()casing nit as noted on the Shibboleth side — preferauth()->check().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Auth/OIDC/OIDCController.php` around lines 30 - 40, Extract the duplicated authenticated-user short-circuit into a shared helper (trait or base controller method) and replace the inline block in both OIDCController and ShibbolethController with a call to that helper; specifically, move the logic that checks auth()->check(), constructs Uri::of(self::REDIRECT_URL)->withQuery(['no_message' => true]), handles optional $request->query('redirect') and returns the redirect into a single method (e.g., handleAuthenticatedRedirect(Request $request) in a trait or base class), update both controllers to call that method, and also change Auth()->check() to auth()->check() to fix the casing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Auth/Shibboleth/ShibbolethController.php`:
- Around line 28-39: The redirect query parameter is forwarded unvalidated from
ShibbolethController::redirect (and similarly OIDCController::redirect) to
/external_login and then used by the frontend (ExternalLogin.vue and Login.vue)
in router.push(), creating an open-redirect risk; fix this by validating and
normalizing the redirect before forwarding and before calling router.push(): in
ShibbolethController::redirect (and OIDCController::redirect) reject or remove
any redirect that contains '//' or '/\' or that is not a single('/')-prefixed
path or a recognized SPA route name, and only forward whitelisted values to
/external_login; in ExternalLogin.vue and Login.vue, enforce the same
whitelist/validation on route.query.redirect (or require a route name/object)
and refuse/purge malformed values before calling router.push() to ensure only
known SPA routes or safe single-segment paths are allowed.
---
Outside diff comments:
In `@resources/js/views/ExternalLogin.vue`:
- Around line 84-88: The redirect handling in ExternalLogin.vue currently pushes
route.query.redirect without validation; ensure route.query.redirect is a safe
single relative path before calling router.push. Update the logic around
route.query.redirect: verify it's a string (if it's an array, pick the first
element or ignore), reject values that start with '//' or contain a scheme like
'http:' or 'https:', and only allow paths that begin with a single '/' (or
otherwise match an explicit internal-route whitelist); if the value fails
validation, fall back to router.push({ name: "rooms.index" }). Use the existing
route.query.redirect and router.push symbols when locating and changing the
code.
---
Nitpick comments:
In `@app/Auth/OIDC/OIDCController.php`:
- Around line 30-40: Extract the duplicated authenticated-user short-circuit
into a shared helper (trait or base controller method) and replace the inline
block in both OIDCController and ShibbolethController with a call to that
helper; specifically, move the logic that checks auth()->check(), constructs
Uri::of(self::REDIRECT_URL)->withQuery(['no_message' => true]), handles optional
$request->query('redirect') and returns the redirect into a single method (e.g.,
handleAuthenticatedRedirect(Request $request) in a trait or base class), update
both controllers to call that method, and also change Auth()->check() to
auth()->check() to fix the casing.
In `@app/Auth/Shibboleth/ShibbolethController.php`:
- Around line 28-37: Replace the unconventional Auth() call with the lowercase
helper auth() in the conditional (use auth()->check()) and simplify the redirect
logic in ShibbolethController by building a single query array first (include
'no_message' => true and conditionally add 'redirect' =>
$request->query('redirect')), then apply that query to
Uri::of(self::REDIRECT_URL) and return a single redirect(...) using the
uri->value(); this collapses the two-branch return into one and keeps URI
construction centralized.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e78846f0-7717-4c03-99b3-a04e2b7ecec3
📒 Files selected for processing (7)
app/Auth/OIDC/OIDCController.phpapp/Auth/Shibboleth/ShibbolethController.phpresources/js/router.jsresources/js/views/ExternalLogin.vuetests/Backend/Feature/api/v1/OIDCTest.phptests/Backend/Feature/api/v1/ShibbolethTest.phptests/Frontend/e2e/Login.cy.js
PILOS
 |
||||||||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
3078-auth-redirect-doesnt-work-if-already-logged-in
|
| Run status |
|
| Run duration | 07m 21s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
1
|
 |
0
|
 |
0
|
 |
0
|
 |
626
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
| Test | Artifacts | |
|---|---|---|
| Room Join with lobby settings > Lobby enabled for guests only |
Test Replay
Screenshots
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
resources/js/views/Login.vue (1)
123-145: Handle array and empty-string cases forroute.query.redirectacross all usages.
route.query.redirectis typed asLocationQueryValue | LocationQueryValue[]by Vue Router, so URLs with duplicate keys (e.g.,?redirect=/a&redirect=/b) make it an array. This breaks three locations:
- Line 128 (onMounted):
router.push(route.query.redirect)with array input will fail/warn- Line 149–150 (oidcRedirectUrl):
encodeURIComponent(route.query.redirect)converts array to comma-separated string ("a,b")- Lines 156–157 (shibbolethRedirectUrl): same issue as above
- Line 174 (handleLogin): same pattern as line 128
Additionally, empty-string redirects (
?redirect=) pass the!== undefinedcheck and push an empty path.Extract
route.query.redirectonce as a normalized value and reuse it:♻️ Suggested refactoring
+const normalizedRedirect = computed(() => { + const value = route.query.redirect; + const redirectValue = Array.isArray(value) ? value[0] : value; + return redirectValue || undefined; +}); const oidcRedirectUrl = computed(() => { const url = "/auth/oidc/redirect"; - return route.query.redirect - ? url + "?redirect=" + encodeURIComponent(route.query.redirect) + return normalizedRedirect.value + ? url + "?redirect=" + encodeURIComponent(normalizedRedirect.value) : url; }); const shibbolethRedirectUrl = computed(() => { const url = "/auth/shibboleth/redirect"; - return route.query.redirect - ? url + "?redirect=" + encodeURIComponent(route.query.redirect) + return normalizedRedirect.value + ? url + "?redirect=" + encodeURIComponent(normalizedRedirect.value) : url; }); onMounted(() => { if (authStore.isAuthenticated) { - if (route.query.redirect !== undefined) { - router.push(route.query.redirect); + if (normalizedRedirect.value) { + router.push(normalizedRedirect.value); } else { router.push({ name: "rooms.index" }); } return; } // ... }); async function handleLogin({ data, id }) { // ... if (normalizedRedirect.value) { await router.push(normalizedRedirect.value); } else { await router.push({ name: "rooms.index" }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/views/Login.vue` around lines 123 - 145, Normalize route.query.redirect once (e.g., getNormalizedRedirect) and use that everywhere: in the onMounted redirect logic, in oidcRedirectUrl and shibbolethRedirectUrl construction, and in handleLogin. Specifically, read route.query.redirect, if it's an array take the first non-empty element, if it's a string treat empty string as undefined, and otherwise treat undefined as no redirect; then use the normalized string for router.push and for encodeURIComponent calls so arrays and empty values no longer cause incorrect pushes or comma-joined encodings. Ensure you reference the existing symbols route.query.redirect, onMounted redirect block, oidcRedirectUrl, shibbolethRedirectUrl, and handleLogin when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@resources/js/views/Login.vue`:
- Around line 123-145: Normalize route.query.redirect once (e.g.,
getNormalizedRedirect) and use that everywhere: in the onMounted redirect logic,
in oidcRedirectUrl and shibbolethRedirectUrl construction, and in handleLogin.
Specifically, read route.query.redirect, if it's an array take the first
non-empty element, if it's a string treat empty string as undefined, and
otherwise treat undefined as no redirect; then use the normalized string for
router.push and for encodeURIComponent calls so arrays and empty values no
longer cause incorrect pushes or comma-joined encodings. Ensure you reference
the existing symbols route.query.redirect, onMounted redirect block,
oidcRedirectUrl, shibbolethRedirectUrl, and handleLogin when applying the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fbd445ee-57c9-406a-bca6-1c96a3298847
📒 Files selected for processing (5)
resources/js/router.jsresources/js/views/Login.vuetests/Backend/Feature/api/v1/OIDCTest.phptests/Backend/Feature/api/v1/ShibbolethTest.phptests/Frontend/e2e/Login.cy.js
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Backend/Feature/api/v1/ShibbolethTest.php
- tests/Backend/Feature/api/v1/OIDCTest.php
samuelwei
changed the title
Collaborator
Author
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Auth/OIDC/OpenIDConnectClient.php (1)
181-199:⚠️ Potential issue | 🟡 MinorFix stale PHPDoc return contract for
authenticate().The method now returns
void, but the docblock still describes a boolean return contract. Please update it to avoid misleading IDE/static-analysis hints.✏️ Proposed docblock fix
- * `@return` bool Returns true if authentication is successful, false if the code is missing + * `@return` void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Auth/OIDC/OpenIDConnectClient.php` around lines 181 - 199, Update the PHPDoc for the OpenIDConnectClient::authenticate(Request $request) method to reflect its actual signature: change the `@return` line from "bool Returns true if authentication is successful, false if the code is missing" to "@return void" and adjust the short description accordingly (e.g., remove the boolean return description); also tidy any duplicated `@throws` entries if present so the docblock accurately matches the method signature and exceptions declared in the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/Auth/OIDC/OpenIDConnectClient.php`:
- Around line 181-199: Update the PHPDoc for the
OpenIDConnectClient::authenticate(Request $request) method to reflect its actual
signature: change the `@return` line from "bool Returns true if authentication is
successful, false if the code is missing" to "@return void" and adjust the short
description accordingly (e.g., remove the boolean return description); also tidy
any duplicated `@throws` entries if present so the docblock accurately matches the
method signature and exceptions declared in the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 14498866-054e-45a1-938e-ee95bd881d38
📒 Files selected for processing (11)
CHANGELOG.mdapp/Auth/OIDC/OIDCCallbackRequest.phpapp/Auth/OIDC/OIDCController.phpapp/Auth/OIDC/OIDCProvider.phpapp/Auth/OIDC/OIDCRedirectRequest.phpapp/Auth/OIDC/OpenIDConnectClient.phpapp/Auth/Shibboleth/ShibbolethCallbackRequest.phpapp/Auth/Shibboleth/ShibbolethController.phpapp/Auth/Shibboleth/ShibbolethRedirectRequest.phpresources/js/views/ExternalLogin.vueresources/js/views/Login.vue
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/js/views/ExternalLogin.vue
- resources/js/views/Login.vue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3078
Type
Checklist
Changes
Other information
Summary by CodeRabbit
Improvements
Validation
Tests