awslabs / cognito-at-edge

Serverless authentication solution to protect your website or Amplify application
Apache License 2.0
168 stars 54 forks source link

Redirect loop after refresh fetch caused by cookie path #69

Open russau opened 1 year ago

russau commented 1 year ago

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:

https://github.com/awslabs/cognito-at-edge/blob/77e2f9e322ce7fcc47fd3e83b4d37d923eb7e390/src/index.ts#L244-L249

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 commented 1 year ago

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 commented 1 year ago

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).

BredoGen commented 11 months ago

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 commented 7 months ago

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 commented 5 months ago

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

JanKoppe commented 4 months ago

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.