Skip to content
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

Add option to override transaction cookie name #414

Merged
merged 4 commits into from
Nov 9, 2022

Conversation

MatthewBacalakis
Copy link
Member

… clients on same protocol and domain.

The key("auth_verification") of the cookie storing nonce/state is problematic in cases where multiple applications share a common protocol and domain. In that case multiple concurrent logins can conflict with the login for one app clearing the auth_verification cookie intended for the other app. I recalled the SPA SDK used to have a similar problem and for this reason clientId was added to the key of the nonce/state storage.

To solve this the same way I've appended .CLIENT_ID to the key of the cookie storing nonce.

@MatthewBacalakis MatthewBacalakis requested a review from a team as a code owner November 3, 2022 22:59
@adamjmcgrath
Copy link
Contributor

Thanks for raising this @MatthewBacalakis!

I know spa-js solves it this way, but I'd like to solve this by allowing the user to override the transaction cookie name themselves.

Users can already set a custom session cookie name with session.name, so this would be consistent with the rest of the API. I'd also like to keep the default name the same and offer some flexibility to users that might want to partition these cookies by audience/scope/something else.

We already have transactionCookie config, so it would be a case of adding a name option to it eg.

app.use(auth({ transactionCookie: { name: 'state.client1' } }));
app.use(auth({ transactionCookie: { name: 'state.client2' } }));

@MatthewBacalakis
Copy link
Member Author

Updated this so that the cookie can optionally be given any name instead of appending the client id. If transactionCookie.name is set in config that name is used. If not the current auth_verification name is used.

Copy link
Contributor

@adamjmcgrath adamjmcgrath left a comment

Choose a reason for hiding this comment

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

lgtm 👍

Could you also update the type defs https://github.com/auth0/express-openid-connect/blob/master/index.d.ts#L480

Something like

transactionCookie?: Pick<CookieConfigParams, 'sameSite'> & { name?: string };

test/callback.tests.js Outdated Show resolved Hide resolved
@adamjmcgrath adamjmcgrath merged commit 2330a68 into auth0:master Nov 9, 2022
@adamjmcgrath adamjmcgrath mentioned this pull request Nov 11, 2022
@adamjmcgrath adamjmcgrath changed the title Adding client id to _auth_verification cookie key to support multiple… Add option to override transaction cookie name Nov 11, 2022
@adamjmcgrath
Copy link
Contributor

This got released in 2.10.0 - thanks for your contribution @MatthewBacalakis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants