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

Cookie Pollution #217

Closed
jakobo opened this issue Sep 18, 2019 · 28 comments · Fixed by #254
Closed

Cookie Pollution #217

jakobo opened this issue Sep 18, 2019 · 28 comments · Fixed by #254

Comments

@jakobo
Copy link

jakobo commented Sep 18, 2019

I appreciate the simpler API compared to the older Auth0.js, but the cookie pollution in spa-js is extreme. If you're proxying frontend requests (to avoid CORS issues), you're sending the auth0.txn and auth0.is.authenticated cookies to your webserver. To make matters worse, because of #26, you'll add another auth0.txn cookie for every single login flow within a 24 hour window. Here's a quick screenshot that shows the absurdity.

Screenshot 2019-09-17 20 17 04

Even without the login testing, the user is going to always send at least two auth0 cookies to the server on every request.

Is there any reason we're using cookies over session/localStorage? Seems like it'd be trivial to rewrite storage.ts and just clean up expired keys on save().

@quangthe
Copy link

I have the same issue.

@igalst
Copy link

igalst commented Sep 23, 2019

We also have same issue that triggered "Bad request" from our Cloudfront. It is quite an urgent issue for us.
image

@mannikf
Copy link

mannikf commented Sep 24, 2019

Also an issue. Followed sample to the letter and getting the same errors as the people who have reported above. Will this issue even get acknowledged?

@luisrudge
Copy link
Contributor

Hi there! Sorry it took us so long to address this. I want to start by apologizing to all of you for all the wait. I understand this is a serious issue and want to shed some light on why we built it like this and ask for feedback on some solutions we're thinking for the future.

Why are we using cookie in the first place?

For the past few years, Auth0.js had a few issues (#990, #897, #820, #816 and more) when dealing with local storage. localStorage can fail in unexpected ways when dealing with applications that run inside an iframe, incognito/private mode, storage keys that were never cleaned up etc. The way this all works is unreliable at best. That's why we made the decision to migrate Auth0.js to cookies as well. We totally understand the trade offs and we believe they're more aligned with the experience we want to provide to developers implementing Auth0. The feedback was very good and that's why we decided to keep using it when we created auth0-spa-js.

With that being said, the experience you shared in this issue is not the experience we want you to have and we'd like to better understand in what scenarios are you seeing this happen in production. I'm specifically asking about the production environment because that's where this issue is less likely to appear, considering how the sdk behaves regarding cookies.

Taking a step back, this is how the cookie is handled in the SDK:

  • No cookie is ever created for getting an access token or logging in with a popup
  • The only time a cookie is created is when using loginWithRedirect is called. We need the cookie to create this 'link' between the authentication request and the authentication response
  • When the user is redirected back with an authentication response that has the same state as the request, the cookie is removed automatically
  • If, for any reason, the authentication response is not successful or doesn't have the same state as the request, then the cookie will be automatically expired (excluded) 24h after its creation

What that means is that the only two ways of getting all those cookies created without them being cleaned up are:

  • by redirecting to the authorize page (loginWithRedirect) and clicking the back button. With the work that we did in Reduce transaction cookie size #32, it means that the user would have to do that 28 times in order to be an issue (less times if you use appState)
  • by calling the loginWithRedirect method multiple times at once (in a loop, or without disabling a button that calls this method and clicking that button multiple times before the redirect happens)

Of course, neither of those situations are perfect, but the tradeoff we decided to do is: we provided a solution that is super reliable in production (no localStorage quirks), at the expense of having some of those issues during development. We totally understand that clicking 28 times and pressing the back button is something that can happen in production, but it's such an edge case that we didn't think of addressing it at first.

There's no 100% perfect solution for this. I had an idea to limit the amount of transactions to 15. So, every new transaction after the 14th, will clear the oldest transaction etc. But then, what about folks that use a large appState? There's an edge case there as well.

Now, what I want to hear from you to help move this forward is:

  • How is this issue being surfaced in your application?
  • Does it happen in production or did you caught this only during development?
  • What do you think about the 15 transactions limit? Do you think this would likely solve your issue without having to rely on localStorage?

@mannikf
Copy link

mannikf commented Oct 2, 2019

We haven't even made it to production because of this issue. It occurs randomly when the user has auth'd in before, or after logging out. The only "solution" is to manually delete the cookies, close the browser tab, and then try to log back in. I use quotes because it doesn't even work all of the time. We tried to manually delete every cookie when the user logs out and that also seems to limit the 400 occurrence but its not foolproof.

I'm fine for limiting the transactions if it will solve this issue as it basically makes our application unusable for end users.

@paulfalgout
Copy link
Contributor

For me we were experiencing what @mannikf was but it was because we were using a * wildcard subdomain in the web origin. Previous when we used the implicit flow, this didn't matter as all of the other fields allow for wildcards and web origin isn't a factor. So switching to this library, following the migration guide instantly produced the mass of cookies.. which was hard to trace or fully understand what the issue was because all of the settings looked appropriate. The only way to really understand that wildcards cannot be used is by googling and reading a forum somewhere, which in turn cleared up the cookie loop for us. But at first, it appeared that this issue was the root of the problem and not a side effect of the other.

@luisrudge
Copy link
Contributor

Thank you so much for your patience and following up with this issue.

@mannikf Can you share a step-by-step guide on how to reproduce this in your case? As stated before, the only time we add a new cookie is when loginWithRedirect is called and this cookie is automatically removed when you call handleRedirectCallback (or after 24h), so it's not clear to me what's happening in your case and I'd like to understand it better so we can properly address it.

@paulfalgout I don't fully understand what happened in your scenario. After the migration guide, did something started to fail and that's why you saw a large amount of cookie entries?

@paulfalgout
Copy link
Contributor

Yep so in implicit flow you don't need to set the web origin as it won't be used. In this lib if you setup a wildcard in your web origin, then loginWithRedirect will succeed but getTokenSilently will fail. We don't currently use a landing page or the universal login. We either prompt=none or if that fails we prompt=login and set the connection

@luisrudge
Copy link
Contributor

@paulfalgout Ah, perfect. But once you get redirected back to your application, you still call handleRedirectCallback, right? This should be cleaning up cookies correctly and preventing the cookie pollution issue.

As part of this specific feedback, I'll talk with the dashboard team to see if we can improve the user experience when adding a * in the web origins field. I guess it makes sense to show a warning or even invalidate the request when a wildcard is detected.

Thank you so much for the feedback!

@paulfalgout
Copy link
Contributor

yep we would call that.. FWIW the initial login worked fine, it was always something with getTokenSilently that would set off the loop that would end in cookie death. I don't 100% know exactly what combination of things caused the problem.. If all I do is add the wildcard domains back to our current setup and test against it, it all fails without the loop currently. Essentially early on in migration we hit a loop.. it wasn't clear as to why, but the first obvious issue was this issue with the cookies. Once we figured out the wildcard was the issue we cleaned up the code and whatever changes we made there, avoid the loop even if I add the wildcard issue back.. so... ¯_(ツ)_/¯

That said, the migration change is publicly available RoundingWell/care-ops-frontend@d0e6117 but the process of migration was squashed into the end results commit, so probably not that illuminating.

@paulfalgout
Copy link
Contributor

paulfalgout commented Oct 3, 2019

one additional complication to throw into the mix.. during development we were often already logged in through the implicit grant code, then we'd update the test environment to the migration, which wasn't necessarily as clean as if we had logged out prior to the code update... so I don't know how that may have added to the issue.

@luisrudge
Copy link
Contributor

@paulfalgout maybe that was the case then.. Auth0.js uses cookies even for silent authentication or popup authentication so the pollution would be bigger. spa-js only add cookies for loginWithRedirect. Thanks for the great feedback and context on your issue!

@kevinfmanning
Copy link

I followed the tutorial basically to the letter. We have noticed this happening different ways. One user reported it after logging in, using it for awhile, then not using it till the next day and the cookies were all filled up.

@luisrudge
Copy link
Contributor

@kevinfmanning did you reproduce this by yourself? Do you use loginWithRedirect in lots of places in your app? Or maybe you redirect every time the user gets a 401/403 response from the server? Considering we only create a cookie when you call loginWithRedirect, I'm trying to understand how often this method is being called in order for the cookies to be filled so quickly.

@kevinfmanning
Copy link

per the example loginWithRedirect is called once in the PrivateRoute.js We do have multiple routes that all need secured in our App.js and in the example it only has one PrivateRoute. How do you propose having many routes all be secured and only have loginWithRedirect being called once?

@krisdages
Copy link

krisdages commented Oct 17, 2019

I started using the SDK today, and quickly came across HTTP 400 errors with webpack-dev-server due to this cookie proliferation. With my project, the cookies were not getting removed on logout or when redirecting back.

I looked at the code for logout, and the only cookie it removes is the auth0.is.authenticated.

So I installed es-cookie, and just added this before every call to logout or loginWithRedirect:

//Since I'm not using cookies for anything else.
for (const key of Object.keys(Cookies.getAll())) {
    Cookies.remove(key);
}

Just curious, is there any reason why the solution to this issue couldn't be just looking at the cookie keys and removing the auth0 ones before logging out or executing the actual redirect when logging in?

Edit: Realized I wasn't using handleRedirectCallback since it seemed to be working without me doing anything at all... :) Guess because the session was valid when I tried to get a new access token?
I did fix my code to call it, but the cookie stuck around...

Looking in the code in 1.3.1 for handleRedirectCallback it looks like it has the same issue as logout. The only cookie modified is auth0.is.authenticated. It looks like transactionManager.remove() should be taking care of the temp cookie, but it doesn't seem to actually be removing it for some reason.

💭 I had previously added some code to remove the redirect querystring parameters from my URL using history.replaceState() when handling the redirect; since I'm using hash-based app routing, they would stick around when routing back into my app from the callback page. Is this something that handleRedirectCallback might be able to handle as well?

@stevehobbsdev
Copy link
Contributor

@krisdages Could you tell us a bit more about how you're using the library? I'm interested in things like the workflow, which methods you're calling and at what points. A timeline, if you will.

For everyone else, could you all make sure that you're calling handleRedirectCallback at the point when the authorization server comes back to your app after authentication. It's up to your app but a common way to do this is to have the authorization server call back to your app on some kind of /callback route, where you would call this method before redirecting the user elsewhere in your app (e.g. the home page).

If you are calling this method at the right time and getting this cookie pollution issue, please let us know.

@krisdages
Copy link

krisdages commented Oct 19, 2019

It appears that handleRedirectCallback was throwing an "Invalid state" error. Because I am using hash-based routing, the callback is the root of my app, and the route is being added as a hash.

https://mydomain.com/?code=XXXXXX&state=XXXXXX#/login-callback

When parsing the state off the querystring, the value of state is showing up as

XXXXXX#/login-callback

Looks like it's looking for the querystring in location.href and assuming the hash is not present.
If it used location.search instead that might fix the issue.

As a workaround for now, I am adding an extra parameter to the querystring after state but before the hash, and it seems to be working.

@stevehobbsdev
Copy link
Contributor

@krisdages Can I ask which version of auth0-spa-js you're using? We recently patched an issue with hash-based routing and also a separate issue with hashes appearing in the URL in v1.3.2.

If you're not using the latest version, could you try that and let us know how you get on?

@kevinfmanning
Copy link

Is there any example with this on multiple private routes? The one given includes just one PrivateRoute and we want to have more. I think the loginWithRedirect is getting called for everyone that we have listed in our app.

@krisdages
Copy link

krisdages commented Oct 23, 2019

@stevehobbsdev Will do. I was on 1.3.1

Update - 1.3.2 did fix my issue with state + hash.

@krisdages
Copy link

krisdages commented Oct 23, 2019

Found another situation that could cause the cookie fillup.

If there is an error status in the redirect querystring, handling the callback throws an Error, which is good :)
I had been simply logging this error and re-attempting the login instead of showing an "error during login" screen.

When the error was persistent (e.g. due to a bad rule), this caused a redirect loop that after about 10-11 rapid cycles resulted in an oversized cookie and a 400 error returned from the dev server.

@luisrudge
Copy link
Contributor

@krisdages you're absolutely right! we're not cleaning the state when there's an error: https://github.com/auth0/auth0-spa-js/blob/master/src/Auth0Client.ts#L248 We'll get this fixed ASAP! Thanks for reporting.

@paulfalgout
Copy link
Contributor

Ah this error issue matches what we were seeing as well. We'd get an error because of the domain origin and immediately try again.

@stevehobbsdev
Copy link
Contributor

👋 We released 1.4.0 to try and alleviate this issue. If anyone who was afflicted by this has any feedback that it has now solved their issue or not, that would be very much appreciated!

@NevenLiang
Copy link

NevenLiang commented Mar 4, 2020

I have tried to manually remove the cookies with name 'a0.spajs.xxx' before calling the method contains loginWithRedirect() as following.

// Clean up the cookies generated by Auth0 client before
document.cookie.split('; ').forEach(cookie => {
  const key = cookie.split('=')[0];

  if (key.includes('a0.spajs.txs.')) {
    removeCookie(key);  // it needs to be implemented by yourself
  }
});

authService.login();  // the method contains `loginWithRedirect()`

@stevehobbsdev
Copy link
Contributor

Thanks @NevenLiang . The feeling at the moment is that we're going to build this into the SDK for you behind an option that you can turn on. I can assume the code snippet you supplied worked for you?

@NevenLiang
Copy link

NevenLiang commented Mar 5, 2020

@stevehobbsdev Yes, it works.

I have sawn the source code. IMO, removeAll() is needed for TransactionManager in the SDK to remove all transaction cookies.

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 a pull request may close this issue.

10 participants