Skip to content

fix(oauth2): prevent profile redirect during OIDC consent flow #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smarcet wants to merge 1 commit into
base: main
Choose a base branch
from
+853 −5
Open

fix(oauth2): prevent profile redirect during OIDC consent flow #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/pull_request_unit_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ jobs:
SSL_ENABLED: false
SESSION_DRIVER: redis
PHP_VERSION: 8.3
OTEL_SDK_DISABLED: true
OTEL_SERVICE_ENABLED: false
services:
mysql:
image: mysql:8.0
Expand Down
6 changes: 4 additions & 2 deletions app/Strategies/OAuth2LoginStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,10 @@ public function getLogin()
{
Log::debug("OAuth2LoginStrategy::getLogin");

if (!Auth::guest())
return Redirect::action("UserController@getProfile");
if (!Auth::guest()) {
Log::debug("OAuth2LoginStrategy::getLogin user already authenticated, redirecting to auth endpoint");
return Redirect::action("OAuth2\OAuth2ProviderController@auth");
}

$this->login_hint_process_strategy->process();

Expand Down
28 changes: 25 additions & 3 deletions app/libs/OAuth2/GrantTypes/InteractiveGrantType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,16 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client
$token_hint = $request->getIdTokenHint();
$otp_login_hint = $request->getOTPLoginHint();

Log::debug(sprintf("InteractiveGrant::processUserHint request %s client %s", $request->__toString(), $client->getId()));
Log::debug
(
sprintf
(
"InteractiveGrant::processUserHint request %s client %s",
$request->__toString(),
$client->getId()
)
);
Comment on lines +464 to +472
Copy link

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't log raw OIDC request payloads here.

OAuth2Request::__toString() serializes the full message, so these new debug lines will write login_hint, id_token_hint, OTP hints, and other auth parameters into logs. That's sensitive user/token material on a hot path; keep the trace to booleans, client ID, and processed-flag state instead.

Also applies to: 702-710

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/libs/OAuth2/GrantTypes/InteractiveGrantType.php` around lines 464 - 472,
The new Log::debug call in InteractiveGrant::processUserHint currently logs the
entire OAuth2Request via OAuth2Request::__toString(), which may leak sensitive
fields (login_hint, id_token_hint, OTPs); replace these full-request traces with
a minimal debug message that includes only non-sensitive info: the client ID
(from $client->getId()), boolean flags indicating which hints were
present/handled (e.g., hasLoginHint, hasIdTokenHint, processed flag), and any
processing outcome. Apply the same change to the other debug statements in
processUserHint (the block around the later Log::debug calls referenced) so no
raw request serialization is written to logs.


// process login hint
$user = null;
if (!empty($otp_login_hint) && !empty ($login_hint)
Expand All @@ -484,7 +493,7 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client
$user = $this->auth_service->getUserById($user_id);
}
$request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_LoginHint);
} else if(!empty($token_hint)) {
} else if(!empty($token_hint) && !$request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint)) {
Log::debug("InteractiveGrant::processUserHint processing Token hint...");

$jwt = BasicJWTFactory::build($token_hint);
Expand Down Expand Up @@ -544,6 +553,7 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client

$this->auth_service->reloadSession($jti->getValue());

$request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint);
}
if(!is_null($user))
{
Expand Down Expand Up @@ -689,6 +699,15 @@ protected function shouldForceReLogin(OAuth2AuthorizationRequest $request, IClie
*/
protected function mustAuthenticateUser(OAuth2AuthorizationRequest $request, Client $client)
{
Log::debug
(
sprintf
(
"InteractiveGrant::mustAuthenticateUser: request %s client %s",
$request->__toString(),
$client->getClientId()
)
);

if ($request instanceof OAuth2AuthenticationRequest) {
try {
Expand All @@ -702,19 +721,22 @@ protected function mustAuthenticateUser(OAuth2AuthorizationRequest $request, Cli
throw $ex;
}
catch (Exception $ex){
Log::warning($ex);
Log::warning("InteractiveGrant::mustAuthenticateUser processUserHint generic error", [ 'error' => $ex]);
$this->auth_service->logout(false);
return true;
Comment on lines 723 to 726
Copy link

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if there's a narrow catch for protocol exceptions before the generic catch
ast-grep --pattern $'catch (InvalidLoginHint $$$) {
  $$$
}'

# Also search for any catch blocks in mustAuthenticateUser
rg -n -A5 'catch.*Exception' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | head -40

Repository: OpenStackweb/openstackid

Length of output: 1379

🏁 Script executed:

#!/bin/bash
# Find the processUserHint method to understand what exceptions it can throw
rg -n -B 20 'catch (JWSInvalidJWKException' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | head -80

Repository: OpenStackweb/openstackid

Length of output: 154

🏁 Script executed:

#!/bin/bash
# Search for InvalidLoginHint and InteractionRequiredException in the codebase
rg -r 'InvalidLoginHint|InteractionRequiredException' --type php | head -20

Repository: OpenStackweb/openstackid

Length of output: 464

🏁 Script executed:

#!/bin/bash
# Look for the processUserHint method definition to see what it can throw
rg -n -A 30 'function processUserHint' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | head -60

Repository: OpenStackweb/openstackid

Length of output: 1802

🏁 Script executed:

#!/bin/bash
# Search for InvalidLoginHint and InteractionRequiredException definitions
rg 'InvalidLoginHint|InteractionRequiredException' --type php -n

Repository: OpenStackweb/openstackid

Length of output: 1791

🏁 Script executed:

#!/bin/bash
# Get broader context of the try-catch block starting from line 458
sed -n '458,730p' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | cat -n

Repository: OpenStackweb/openstackid

Length of output: 12696

🏁 Script executed:

#!/bin/bash
# Check the end of processUserHint method to see where try starts
rg -n 'try' app/libs/OAuth2/GrantTypes/InteractiveGrantType.php | grep -A2 -B2 "70[0-9]:"

Repository: OpenStackweb/openstackid

Length of output: 50

Add narrow catch for protocol exceptions before the generic Exception catch.

The processUserHint() method throws InvalidLoginHint (for malformed hints) and InteractionRequiredException (for interaction requirements). Both are documented in the method's @throws declarations and thrown at multiple points within the method. These protocol exceptions should propagate to the client as OAuth2 errors, not trigger user logout.

The current code catches all Exception types generically at line 266-269, which incorrectly logs out users for protocol errors like bad auth URLs. Add a narrow catch for InvalidLoginHint | InteractionRequiredException before the generic Exception catch to re-throw these exceptions without logout:

Suggested structure 
catch (InvalidLoginHint | InteractionRequiredException $ex) {
    throw $ex;
}
catch (Exception $ex){
    Log::warning("InteractiveGrant::mustAuthenticateUser processUserHint generic error", [ 'error' => $ex]);
    $this->auth_service->logout(false);
    return true;
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/libs/OAuth2/GrantTypes/InteractiveGrantType.php` around lines 723 - 726,
Add a narrow catch for the protocol exceptions thrown by processUserHint so they
propagate instead of triggering logout: inside
InteractiveGrant::mustAuthenticateUser, insert a catch block for
InvalidLoginHint | InteractionRequiredException that simply re-throws the
exception before the existing generic catch(Exception $ex) block; keep the
existing generic catch to Log::warning, call $this->auth_service->logout(false)
and return true for non-protocol exceptions so only unexpected errors cause
logout.

}
}

if($this->shouldPromptLogin($request))
{
Log::warning("InteractiveGrant::mustAuthenticateUser: shouldPromptLogin");
$this->auth_service->logout(false);
return true;
}

if($this->shouldForceReLogin($request, $client))
{
Log::warning("InteractiveGrant::mustAuthenticateUser: shouldForceReLogin");
$this->auth_service->logout(false);
return true;
}
Expand Down
Loading
Loading