search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
128 stars 41 forks source link

Logged out users still appear logged in when navigating out of and back into AMO frontend #12288

Closed AlexandraMoga closed 6 years ago

AlexandraMoga commented 6 years ago

STR:

  1. Load AMO - https://addons.mozilla.org/en-US/firefox/
  2. Log in
  3. Log out
  4. Click on Developer Hub in the header
  5. Go back to AMO

Actual result: The user still appears to be logged in. Navigation through pages that require a logged in state is possible (see user collections page)

Expected result: User is logged out

Notes:

log out issue

bobsilverberg commented 6 years ago

I've been able to reproduce this locally, and it seems like a pretty important bug to fix, so I've made it a P2. I don't necessarily expect it to make it into this week's tag, but aiming to fix it before next week's push seems like a good idea.

Thanks for catching it @AlexandraMoga!

willdurand commented 6 years ago

The issue is reproducible on Chrome, which serves the page from the disk cache when hitting the back button. I guess that's the same issue with Firefox. There is no issue related to our app as far as I can see, because cookies get removed and state reinitialized.

A potential fix would be to open this developer hub link to a new window, which is suggested by the icon on the right anyway.

bobsilverberg commented 6 years ago

When I tested this I found that following the STR above, I was able to perform actions as a logged-in user, so it seems like a fairly serious issue. For example, I could successfully edit my profile, including saving changes, so the app still thinks I'm authenticated, even though I logged out.

Unless I am misunderstanding your explanation, this does seem like a problem with the app.

willdurand commented 6 years ago

When I tested this I found that following the STR above, I was able to perform actions as a logged-in user, so it seems like a fairly serious issue. For example, I could successfully edit my profile, including saving changes, so the app still thinks I'm authenticated, even though I logged out.

Longer explanation:

(a) The redux state is serialized and put into the HTML sent by the server. This state contains the authentication token used to perform actions, like querying the API with elevated privileges (auth: true).

(b) The deauthentication is a 2-step process: 1. it calls the server to delete the session, and 2. it dispatches a LOG_OUT redux action, which restes the redux state. That's what happens at step 3 of the STR in the OP.

(c) Going to Developer Hub does not open a new tab.

(d) In Chrome (and I think that's similar in FF), hitting the back button from there does NOT call the server, the HTML sent previously in (a) is cached on disk and served. That's what happens at step 5 of the STR in the OP, on Chrome.

(e) Because the HTML contains the serialized redux state of the previous page in (a), it contains a valid authentication token. The client-side application loads just fine, reuses this serialized state and therefore thinks the user is authenticated (since there is a token in the redux state). API calls work fine too, because of the token (the API is a stateless HTTP API).

I believe it is a caching issue because of the following sentence in the OP:

the logged out state is set after the page is reloaded

I am still wondering why the token is still valid after we delete the session though. I would have expected one-time use tokens, but that's probably not the root cause here, since the frontend has no way to know that anyway (i.e. the app will still think the user is logged in, no matter if the token is valid or not).

willdurand commented 6 years ago

That's also why I think opening the link to a new tab should fix the issue (not the root cause though).

bobsilverberg commented 6 years ago

I am still wondering why the token is still valid after we delete the session though.

Yes, that was my main concern too. Even if the app thinks we're still authenticated, the API calls should fail, as we've logged out.

willdurand commented 6 years ago

I re-prioritized this issue as I really don't think it is a major security issue. I can't find any issue with our code. Just learnt about the p2 being high priority p3, so re-adding p2.

kumar303 commented 6 years ago

I am still wondering why the token is still valid after we delete the session though.

It's because authentication happens with a token (in a signed cookie and also in Redux state). There is no way to invalidate the token on the AMO server -- we simply delete the cookie and hope for the best. One reason it's like this is because using database sessions puts a lot of strain on the database. Maybe it's something we should revisit, though.

This old issue has some insight but there are others that I can't find right now: https://github.com/mozilla/addons/issues/4038 (moving from JWT to cookie auth).

I merged https://github.com/mozilla/addons-frontend/pull/6195 because I think it no longer makes Firefox serve cached content in the back button. I could be wrong, though.

@AlexandraMoga after this change, if you can find any other way to get back to the signed in page, let us know.

kumar303 commented 6 years ago

...if you can find any other way to get back to the signed in page, let us know.

I just found a way but I have an idea for how to fix it. https://github.com/mozilla/addons/issues/12332

AlexandraMoga commented 6 years ago

@willdurand @kumar303

While testing this again I realized that opening the Developer Hub link from the header in a different window will only partially fix the issue. If I click on the Developer Hub link from the footer (as @kumar303 observed), the page will open in the same tab, which means that when I go back to AMO, I will still be logged in. Moreover, I think that having two ways of opening the same page is a bit inconsistent.

I've also tested the fix proposed by @kumar303 in https://github.com/mozilla/addons/issues/12332 and that appears to have solved the issue.

My suggestion is to wait until the next milestone when https://github.com/mozilla/addons/issues/12332 will land, rather than pushing a partial fix this week. I would much more prefer to keep opening Dev Hub in the same tab, not a new window.

What is your opinion?

willdurand commented 6 years ago

The dev hub link in the header must be opened in a new tab IMO, otherwise the icon next to it is misleading.

AlexandraMoga commented 6 years ago

I'm going to mark this as verified fixed on the basis that it covers the original STR. A complete fix to cover all scenarios is going to land next week with mozilla/addons-server#6217

dev hub new window

eviljeff commented 6 years ago

I'm a bit confused about the fix here - the "Developer Hub" link is only one way to navigate away from the frontend rendered pages to django served pages so all the other links in the header still exhibit the same behaviour as before (e.g. Submit an Addon, Manage my submissions, Reviewer tools), right?

AlexandraMoga commented 6 years ago

In addition to that, there is another link to Developer Hub in the page footer, which will open dev hub in the same tab, not a new window.

kumar303 commented 6 years ago

I'm a bit confused about the fix here

It wasn't really a fix. It was a workaround but, really, I think this link should have always opened in a new tab.

The real fix to the bug was in https://github.com/mozilla/addons-frontend/pull/6218

eviljeff commented 6 years ago

I think this link should have always opened in a new tab.

By why should this one link to developer hub open in a new tab but not the other links to developer hub?