fix(auth): preserve redirect target through Google, Keyclaok and OAuth2 login - #1583
Open
belcirelk wants to merge 4 commits into
Open
fix(auth): preserve redirect target through Google, Keyclaok and OAuth2 login#1583belcirelk wants to merge 4 commits into
belcirelk wants to merge 4 commits into
Conversation
Same issue as the Microsoft OAuth fix: the "Sign in with Google" link never carried the original ?redirect= query param through the OAuth round trip, so users who followed a deep link into a protected page were sent to their default homepage after authenticating with Google instead of the page they originally requested. - LoginPresenter::GetGoogleUrl() now accepts an optional $state parameter and passes it to the Google client via setState(). PageLoad() passes the page's resume URL into it via GetResumeUrl(). - Web/google-auth.php now reads `state` back from Google's callback params and forwards it to external-auth.php as redirect=, which ExternalAuthLoginPage::GetResumeUrl() already reads via QueryStringKeys::REDIRECT. The forwarded value still passes through RedirectUrlSanitizer::Sanitize() in LoginRedirector::Redirect() before use, same as the plain-login ?redirect= flow, so open-redirect protection is unchanged. Refs: LibreBooking#1559 Assisted-by: Claude:claude-sonnet-4-6
Same issue as the Microsoft and Google OAuth fixes: the "Sign in
with Keycloak" and "Sign in with {oauth2 name}" links never carried
the original ?redirect= query param through the OAuth round trip,
so users who followed a deep link into a protected page were sent
to their default homepage after authenticating instead of the page
they originally requested.
- LoginPresenter::GetKeycloakUrl() and GetOauth2Url() now accept an
optional $state parameter and include it as the OAuth `state`
param on their respective authorize URLs. PageLoad() passes the
page's resume URL into both via GetResumeUrl().
- Web/keycloak-auth.php and Web/oauth2-auth.php now read `state`
back from their callback params and forward it to
external-auth.php as redirect=, which
ExternalAuthLoginPage::GetResumeUrl() already reads via
QueryStringKeys::REDIRECT.
The forwarded value still passes through
RedirectUrlSanitizer::Sanitize() in LoginRedirector::Redirect()
before use, same as the plain-login ?redirect= flow, so open-redirect
protection is unchanged.
Refs: LibreBooking#1559
Assisted-by: Claude:claude-sonnet-4-6
Collaborator
Author
|
These are all the same fix for the 3 methods. I tested the first commit with Google and replicated for the 2 others. I did not touch the Facebook login, the fix would be different. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the “deep link lost after OAuth login” behavior by propagating the original ?redirect= target through the OAuth round-trip for Google, Keycloak, and generic OAuth2 logins (matching the already-fixed Microsoft flow).
Changes:
- Passes the login resume URL into Google/Keycloak/OAuth2 authorize URLs via the OAuth
stateparameter. - Reads
statefrom the OAuth callback and forwards it toexternal-auth.phpasredirect=, allowing existing redirect handling/sanitization to apply. - Ensures the callback scripts URL-encode the forwarded values.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Web/oauth2-auth.php | Forwards OAuth2 callback state to external-auth.php as redirect=. |
| Web/keycloak-auth.php | Forwards Keycloak callback state to external-auth.php as redirect=. |
| Web/google-auth.php | Forwards Google callback state to external-auth.php as redirect= and normalizes header formatting. |
| Presenters/LoginPresenter.php | Adds optional $state support to Google/Keycloak/OAuth2 authorize URL builders and passes GetResumeUrl() through from PageLoad(). |
Comment on lines
+8
to
+15
| if (isset($_GET['code'])) { | ||
| $code = filter_input(INPUT_GET, 'code'); | ||
| header('Location: '.ROOT_DIR.'Web/external-auth.php?type=google&code='.$code); | ||
| $url = ROOT_DIR . 'Web/external-auth.php?type=google&code=' . urlencode($code); | ||
|
|
||
| $state = filter_input(INPUT_GET, 'state'); | ||
| if (!empty($state)) { | ||
| $url .= '&redirect=' . urlencode($state); | ||
| } |
Comment on lines
+8
to
+15
| if (isset($_GET['code'])) { | ||
| $code = filter_input(INPUT_GET, 'code'); | ||
| header('Location: ' . ROOT_DIR . 'Web/external-auth.php?type=keycloak&code=' . $code); | ||
| $url = ROOT_DIR . 'Web/external-auth.php?type=keycloak&code=' . urlencode($code); | ||
|
|
||
| $state = filter_input(INPUT_GET, 'state'); | ||
| if (!empty($state)) { | ||
| $url .= '&redirect=' . urlencode($state); | ||
| } |
Comment on lines
+8
to
+15
| if (isset($_GET['code'])) { | ||
| $code = filter_input(INPUT_GET, 'code'); | ||
| header('Location: ' . ROOT_DIR . 'Web/external-auth.php?type=oauth2&code=' . $code); | ||
| $url = ROOT_DIR . 'Web/external-auth.php?type=oauth2&code=' . urlencode($code); | ||
|
|
||
| $state = filter_input(INPUT_GET, 'state'); | ||
| if (!empty($state)) { | ||
| $url .= '&redirect=' . urlencode($state); | ||
| } |
Comment on lines
+303
to
+305
| if (!empty($state)) { | ||
| $client->setState($state); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This needs to be fixed, and it's a good catch. This PR make it a little bit worse with the redirect, but the issue was already there. Should it be done in a follow up PR? It also affects the MS auth commit.
Collaborator
Author
There was a problem hiding this comment.
What I want to say is that these redirect PRs don't create a new bug.
Comment on lines
+140
to
+144
| $this->_page->SetGoogleUrl($googleEnabled ? $this->GetGoogleUrl($this->_page->GetResumeUrl()) : null); | ||
| $this->_page->SetMicrosoftUrl($microsoftEnabled ? $this->GetMicrosoftUrl($this->_page->GetResumeUrl()) : null); | ||
| $this->_page->SetFacebookUrl($facebookEnabled ? $this->GetFacebookUrl() : null); | ||
| $this->_page->SetKeycloakUrl($keycloakEnabled ? $this->GetKeycloakUrl() : null); | ||
| $this->_page->SetOauth2Url($oauth2Enabled ? $this->GetOauth2Url() : null); | ||
| $this->_page->SetKeycloakUrl($keycloakEnabled ? $this->GetKeycloakUrl($this->_page->GetResumeUrl()) : null); | ||
| $this->_page->SetOauth2Url($oauth2Enabled ? $this->GetOauth2Url($this->_page->GetResumeUrl()) : null); |
Web/google-auth.php, Web/keycloak-auth.php, and Web/oauth2-auth.php used isset($_GET['code']) to gate the redirect to external-auth.php. isset() is true even when code is an array (e.g. ?code[]=x), in which case filter_input() returns null and the script would redirect with an empty code= param, producing a confusing downstream auth failure instead of failing fast. - Require code to be a non-empty string via filter_input(INPUT_GET, 'code', FILTER_UNSAFE_RAW) combined with is_string()/!== '' checks, rejecting arrays and null explicitly. - Apply the same scalar check to state before forwarding it. - Build the redirect query string with http_build_query($params, '', '&', PHP_QUERY_RFC3986) instead of manual string concatenation, matching the encoding already used to build the outgoing authorize URLs in LoginPresenter::GetGoogleUrl()/GetKeycloakUrl()/GetOauth2Url(). This pattern already existed in these three scripts before this change (predates the OAuth state/redirect work in this PR); MicrosoftOAuthCallback was already unaffected since it validates code as a string explicitly. Refs: LibreBooking#1559 Assisted-by: Claude:claude-sonnet-4-6
State was used instead of redirect by error in the previous version.
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
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.
fix(auth): preserve redirect target through Google, Keycloak and OAuth2 login
Same issue as the Microsoft fix: the "Sign in
with ... links never carried
the original ?redirect= query param through the OAuth round trip,
so users who followed a deep link into a protected page were sent
to their default homepage after authenticating instead of the page
they originally requested.
optional $state parameter and include it as the OAuth
stateparam on their respective authorize URLs. PageLoad() passes the
page's resume URL into both via GetResumeUrl().
stateback from their callback params and forward it to
external-auth.php as redirect=, which
ExternalAuthLoginPage::GetResumeUrl() already reads via
QueryStringKeys::REDIRECT.
The forwarded value still passes through
RedirectUrlSanitizer::Sanitize() in LoginRedirector::Redirect()
before use, same as the plain-login ?redirect= flow, so open-redirect
protection is unchanged.
Refs: #1559
Assisted-by: Claude:claude-sonnet-4-6