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

Redirect loop after refresh fetch caused by cookie path #69

Open
russau opened this issue Jun 14, 2023 · 6 comments
Open

Redirect loop after refresh fetch caused by cookie path #69

russau opened this issue Jun 14, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@russau
Copy link

russau commented Jun 14, 2023

What would you like to be added:

I'm writing the issue here with the hope it'll help someone else stuck in the same situation. Maybe a documentation update could help others. Or maybe a change to the cookie logic.

Why is this needed:

I'm seeing my application go into a redirect loop when the id token expires and we fetch tokens from refreshToken. If this happens in a deep link the browser stores the cookie with a path. The easy fix was to change my code to use cookiePath: '/'.

Now I have two idtoken cookies one with path=/ and another with path=/deep/link/path. Both get submitted by the browser (see https://stackoverflow.com/a/24214538/109102). This loop uses the last cookie as the idtoken:

for (const {name, value} of cookies){
if (name.startsWith(tokenCookieNamePrefix) && name.endsWith(idTokenCookieNamePostfix)) {
tokens.idToken = value;
}
if (name.startsWith(tokenCookieNamePrefix) && name.endsWith(refreshTokenCookieNamePostfix)) {
tokens.refreshToken = value;

Maybe this is worth a mention in the README. Or maybe rework the cookie logic to check every idtoken cookie until there is a success? Hmm, this does sound a bit clumsy.

@jeandek
Copy link
Contributor

jeandek commented Jun 15, 2023

Hi Russ,

Thanks for raising this issue. I don't recall it ever being reported before. If I understand correctly, the issue is that the refreshed token does not replace the expired one, and that the latter has precedence when Cognito@Edge verifies the token's validity due to its shorter path. Correct?

The issue can be avoided using the cookiePath parameter when creating the Authenticator instance in your function, but I wonder if it would make sense to default it to /.

@russau
Copy link
Author

russau commented Jun 16, 2023

Yes - I end up with two id cookies on different paths. The linked stackoverflow talks about how browsers deal with cookies in this situation. I do see Cognito@Edge consistently get the shorter matching path.

Setting cookiePath: '/' did fix the issue for me.

I see other frameworks that default to / -> https://flask-login.readthedocs.io/en/latest/#cookie-settings This could break compatibility if anyone is relying on the current behaviour (probably unlikely).

@jeandek jeandek added the bug Something isn't working label Jun 23, 2023
@BredoGen
Copy link

Got the same problem (redirect loop for some clients) on a test stage after ~1 week of rare usage without changes.

Unfortunately had no chance to collect debug logs, but it seems to be the same issue.

cookiePath: '/' fixed the problem immediately.

Seems to be an optimal default value (/).

@johnbeech
Copy link

Confirmed this issue today after spotting it a few days ago; the fix seems obvious - we were puzzled as to why the authorizer was setting multiple cookies on different paths, until we caught and traced the redirect loop in our browser.

@ckifer
Copy link
Contributor

ckifer commented Jan 23, 2024

also have the same issue. Might be worth solving in the library...

@JanKoppe
Copy link

JanKoppe commented Mar 1, 2024

We're having the same issue! This behaviour is also super hard to re-create if you don't know what you're looking for.

Setting the cookiePath:"/" fixed it for us immediately.

Would appreciate this being highlighted in the README, or even set as the default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants