-
Notifications
You must be signed in to change notification settings - Fork 222
fix: enforce back-channel logout on subsequent requests by rehydrating session state #852
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
555ab9f
d348bd4
4223039
b17aeec
e61432d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1141,6 +1141,56 @@ public function defer( | |
| expect($credentials)->toBeNull(); | ||
| }); | ||
|
|
||
| test('getCredentials() enforces a queued backchannel logout on a subsequent request', function(): void { | ||
| $issuer = 'https://' . $this->configuration['domain'] . '/'; | ||
| $sid = uniqid(); | ||
|
|
||
| $token = (new TokenGenerator())->withHs256([ | ||
| 'sid' => $sid, | ||
| 'iss' => $issuer, | ||
| ]); | ||
|
|
||
| $pool = new ArrayAdapter(); | ||
|
|
||
| // Shared session storage so a subsequent instance sees the persisted session. | ||
| $config = array_merge($this->configuration, [ | ||
| 'tokenAlgorithm' => 'HS256', | ||
| 'backchannelLogoutCache' => $pool, | ||
| 'sessionStorage' => new SessionStore(new SdkConfiguration($this->configuration), 'auth0_session'), | ||
| ]); | ||
|
|
||
| $auth0 = new Auth0($config); | ||
| $auth0->authentication()->getHttpClient()->mockResponses([ | ||
| HttpResponseGenerator::create('{"access_token":"1.2.3","id_token":"' . $token . '","refresh_token":"4.5.6","scope":"test:part1,test:part2","expires_in":300}'), | ||
| HttpResponseGenerator::create('{"sub":"__test_sub__"}'), | ||
| ]); | ||
|
|
||
| $_GET['code'] = uniqid(); | ||
| $_GET['state'] = '__test_state__'; | ||
| $auth0->configuration()->getTransientStorage()->set('state', '__test_state__'); | ||
| $auth0->configuration()->getTransientStorage()->set('nonce', '__test_nonce__'); | ||
| $auth0->configuration()->getTransientStorage()->set('code_verifier', '__test_code_verifier__'); | ||
|
|
||
| expect($auth0->exchange())->toBeTrue(); | ||
|
|
||
| $logoutToken = TokenGenerator::create( | ||
| tokenType: TokenGenerator::TOKEN_LOGOUT, | ||
| algorithm: TokenGenerator::ALG_HS256, | ||
| claims: [ | ||
| 'sub' => '__test_sub__', | ||
| 'iss' => $issuer, | ||
| 'sid' => $sid, | ||
| ], | ||
| ); | ||
|
|
||
| $auth0->handleBackchannelLogout($logoutToken->token); | ||
|
|
||
| // A subsequent request: a fresh instance must rehydrate the backchannel key | ||
| // from session storage for the queued logout to be enforced. | ||
| $fresh = new Auth0($config); | ||
| expect($fresh->getCredentials())->toBeNull(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion is right and it does fail without the fix, which I checked. Two suggestions to make it harder to pass by accident. Right now $fresh = new Auth0($config);
expect($fresh->getCredentials())->not->toBeNull(); // rehydration works
// then queue the logout and assert null on another fresh instanceSecond, could we add a |
||
| }); | ||
|
|
||
| test('setIdToken() properly stores data', function(): void { | ||
| $token = (new TokenGenerator())->withHs256(); | ||
| $auth0 = new Auth0($this->configuration + [ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three things about this setup line.
The ticket says all storage backends are affected, but this only exercises
SessionStore. The default isCookieStore, so the configuration most people actually run is the one with no coverage. The bug was ingetState(), which is backend agnostic, so aCookieStorecase should pass without any extra production changes and would cover the default path.new SdkConfiguration($this->configuration)builds a second configuration object just to hand to the store, separate from the one theAuth0instance ends up creating from$config. It works, sinceSessionStoreonly uses the config for cookie params duringstart(), but it reads like it might be intentional when I think it is just awkward. Worth a comment or a restructure.Last one:
'auth0_session'is a fixed namespace and nothing resets$_SESSION.beforeEachonly clears$_GETand$_COOKIE, and there are other tests in this file usingSessionStoreat 374 and 821.pest:ciruns with--order-by=randomand--fail-on-risky, so this is the kind of leak that turns into a confusing failure on some unrelated PR later. Auniqid()namespace, or$_SESSION = [];inbeforeEach, would sort it.