1265340 - User is still able to manage the Firefox account, even after logging out

Status: VERIFIED FIXED

(Cloud Services :: Server: Firefox Accounts, defect)

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[Affected versions]:
- 48.0a1 (2016-04-17)
- 47.0a2 (2016-04-18)
- 46.0b11 build1 (20160414152344)
- 45.0.2 build1 (20160407164938)
- 44.0 build3 (20160123151951)
- 29.0

[Affected platforms]:
- Windows 10 x64 
- Mac OS X 10.11 
- Ubuntu 14.04 x86 

[Steps to reproduce]:
1. Launch Firefox with a new profile.
2. Go to Hamburger Menu > Sign in to Sync and click the Sign In button from the Connect with a Firefox Account section.
3. In the Firefox Sync page, enter the credentials for a valid Firefox account and click the Sign In button.
4. In the Firefox Account section from the same page (Options), click the Manage Account link. 
5. Move to the Options page in order to access again the Firefox Account section then click the Disconnect and the Continue buttons.
6. Go to the last opened tab (Firefox Accounts page) and try to modify the account information (Account picture, Display name, etc.) by accessing the corresponding buttons.

[Expected result]:
- After step 5, the user is successfully disconnected from the Firefox account on every active Firefox tab/window.

[Actual result]:
- The user is still able to manage the Firefox account, even after logging out.

[Regression range]:
- This issue is _not a regression_, as it's reproducible all the way back to Fx29.0.
- Please note that I'm setting this bug's severity as [major] because it might be considered as a privacy issue as well.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

There's definitely some weirdness going on here. I've been able to reproduce:

* Signing out of Sync did leave accounts.firefox.com logged in - with no obvious way to log out - the only way I found was to clear all "recent history" and refresh the page.

* Sign in to Sync successfully, then hitting the "manage" button, to then be taken to accounts.firefox.com where it tells me my session has expired and I should again enter my password.

I thought we'd taken steps to fix some of these, but TBH I can't even remember how it is supposed to work currently (eg, I know the different local and remote states was expected some time ago, but I thought we'd recently taken steps to fix that).

Comment 2

[Ryan Kelly [:rfkelly]]

Ugh.  I believe this is a token-caching issue.

When you load the page at accounts.firefox.com, it uses the sessionToken to generate itself a signed certificate, and uses that to get some oauth tokens with various scopes.  These are cached locally in memory by the page.

When you sign out of Firefox, it destroys the sessionToken, but the page at accounts.firefox.com doesn't know to discard its cached certs and tokens.  So if you continue interacting with the page, you can still access many of its features that use those tokens for authentication.

If you try to do something that requires a sessionToken, or if you reload the page to clear the cached tokens, then you get the "session expired" error that :markh notes in Comment 1.

Shane, do you recall if we're supposed to receive a "logout" notification from the browser when the user disconnects through the preferences UI?

Comment 3

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Ryan Kelly [:rfkelly] from comment #2)
> When you sign out of Firefox, it destroys the sessionToken, but the page at
> accounts.firefox.com doesn't know to discard its cached certs and tokens. 
> So if you continue interacting with the page, you can still access many of
> its features that use those tokens for authentication.
Yes, indeed, at accounts.firefox.com the user is able to interact with the page and access its features, but the fact that the changes are also successfully saved (not just temporarily, but for the next log in sessions, too) is the most important, in my opinion.

Comment 4

[Shane Tomlinson [:stomlinson]]

> Shane, do you recall if we're supposed to receive a "logout" notification from the browser when the user disconnects through the preferences UI?

I've never heard a mention of such a notification, so I claim ignorance. Does one exist? `fxAccounts.signOut` is called in [1], but no message is sent.

For a WebChannel message to be properly handled, an open page to the content server is required. In the STRs this is the case, the content server page is still open so this would work. If the content server is not open and the user attempts to open settings, the sessionToken will be checked and will be found to be invalid.

[1] - https://dxr.mozilla.org/mozilla-central/rev/ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/browser/components/preferences/in-content/sync.js#665

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #4)
> For a WebChannel message to be properly handled, an open page to the content
> server is required.

IIUC, this case is about the client telling the auth server about the logout, and we don't have a facility for sending unsolicited webchannel messages in that direction, so I don't think that's particularly simple (probably doable though)

> If the content server is not open and the
> user attempts to open settings, the sessionToken will be checked and will be
> found to be invalid.

Excuse my ignorance, but does that mean the server has a copy of the same session token (in localstorage or whatever) that the client just logged out from? And that token is checked only upon *open* of settings and not subsequent operations in there? I guess that must be for perf reasons (or more likely the above is just plain wrong ;)?

Comment 6

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #5)
> (In reply to Shane Tomlinson [:stomlinson] from comment #4)
> > For a WebChannel message to be properly handled, an open page to the content
> > server is required.
> 
> IIUC, this case is about the client telling the auth server about the
> logout, and we don't have a facility for sending unsolicited webchannel
> messages in that direction, so I don't think that's particularly simple
> (probably doable though)

I once had an idea of tracking when content server pages open and close so that it would be possible to send unsolicited WebChannel messages.

 
> Excuse my ignorance, but does that mean the server has a copy of the same
> session token (in localstorage or whatever) that the client just logged out
> from? And that token is checked only upon *open* of settings and not
> subsequent operations in there? I guess that must be for perf reasons (or
> more likely the above is just plain wrong ;)?


Yes, the content server keeps a copy of that token. It uses that token to perform its own operations. The token is checked for validity as soon as the settings page, AND the auth server checks token validity on every interaction - otherwise we'd have a blatant security hole. Once the settings page is loaded and the initial token check occurs, the content server performs no additional manual checks on the token and assumes the token remains valid until an operation fails.

Comment 7

[Ryan Kelly [:rfkelly]]

> And that token is checked only upon *open* of settings and not subsequent operations
> in there? I guess that must be for perf reasons

:markh, you can think of this as equivalent to the cert-caching problem that we see in the browser code, where resetting the password doesn't log you out of sync straight away because your browser has a signed cert that's good for 6 hours.  The content-server has used that sessionToken to generate oauth tokens that remain valid even after the sessionToken itself has been destroyed.

> IIUC, this case is about the client telling the auth server about the logout

We already tell the auth-server about it when we destroy the sessionToken; (part of) this problem is telling the content-server so that it can throw away any oauth tokens it has generated for itself.

Comment 8

[Ryan Kelly [:rfkelly]]

I harp on about OpenID Connect a lot, but they really have tread this path before us.  There's a (draft, optional) spec for a mechanism by with an OIDC relying party can request an OIDC provider to log out, which is entirely analogous to the situation we're in here - the browser (relying party) has processed a logout event with should propagate to the provider (FxA).

It's dirt simple, the RP just redirects to a "logout" endpoint on the IdP:

  http://openid.net/specs/openid-connect-session-1_0.html#RPLogout

Rather than mucking around with webchannel events and hoping there's a page open to receive them, we could just open one (maybe in a hidden iframe) and force it to process the logout.

Comment 9

[Shane Tomlinson [:stomlinson]]

(In reply to Ryan Kelly [:rfkelly] from comment #8)
> I harp on about OpenID Connect a lot, but they really have tread this path
> before us.  There's a (draft, optional) spec for a mechanism by with an OIDC
> relying party can request an OIDC provider to log out, which is entirely
> analogous to the situation we're in here - the browser (relying party) has
> processed a logout event with should propagate to the provider (FxA).
> 
> It's dirt simple, the RP just redirects to a "logout" endpoint on the IdP:
> 
>   http://openid.net/specs/openid-connect-session-1_0.html#RPLogout
> 
> Rather than mucking around with webchannel events and hoping there's a page
> open to receive them, we could just open one (maybe in a hidden iframe) and
> force it to process the logout.


The hidden iframe scheme could work if there are no other content server pages already open.

We have a content server endpoint that may already suit the purpose: /clear

/clear is used by functional tests to clear localStorage and sessionStorage between test runs.

The reason I say the hidden scheme could work if there are no other content server pages open is that if a page is open, it'll have some view of the world already loaded into memory. The hidden iframe to /clear will clear localStorage, and then the open visible page could re-write its view of the world to localStorage.

We could get around that by listening for some BroadcastChannel message that informs open tabs that everything has been cleared and they should stop whatever they are doing, right now.

Comment 10

[Ryan Kelly [:rfkelly]]

> We could get around that by listening for some BroadcastChannel message that informs
> open tabs that everything has been cleared and they should stop whatever they are doing, right now.

I think we already have something close to this, because if I have two tabs open and signed into FxA, and I sign out in one of them, the other one updates itself automatically.

> We have a content server endpoint that may already suit the purpose: /clear

I wonder, is there any risk with exposing this endpoint on our production instance?  It means that e.g. any page on the internet could force you to log out of FxA just by loading a hidden iframe.  We could reduce the scope of abuse by making a specific page like:

   /logout?uid=ABCDEF1234567890

That you can only use to log someone out if you already know who they're logged in as.  This page could also trigger the broadcast logout message.

:markh what do you think of the approach of opening a hidden iframe to a "logout" URL rather than doing it via WebChannel?  It would be ugly, but effective.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

HiddenFrame.jsm could help us here, but we'd need a way to know for-sure that the page has done what it needs to do and the iframe can be destroyed, while also being resilient to bizarre network behaviours. It also isn't going to work if the user happens to be offline when they disconnect, which is an edge-case, but one worth considering. So I think we should explore other options, but this seems viable if we decide this has the right tradeoffs we want to make.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #11)
> HiddenFrame.jsm could help us here,

Although that doesn't seem e10s friendly - if I'm reading it correctly, the frame would always be in the parent process, and I suspect it is non-trivial to support a hidden frame in the content process. That could well be problematic if the content wants to sendMessage between itself. I feel I need to know more about how the server finds existing open tabs to talk to.

Maybe we could work out how to have a hidden remote iframe attached to sync prefs rather than the "hidden dom window".

Comment 13

[Ryan Kelly [:rfkelly]]

> I feel I need to know more about how the server finds existing open tabs to talk to.

The open tabs talk to each other via BroadcastChannel [1] or a localStorage-based fallback

[1] https://developer.mozilla.org/en-US/docs/Web/API/BroadcastChannel

Comment 14

[Shane Tomlinson [:stomlinson]]

(In reply to Ryan Kelly [:rfkelly] from comment #10)
> > We could get around that by listening for some BroadcastChannel message that informs
> > open tabs that everything has been cleared and they should stop whatever they are doing, right now.
> 
> I think we already have something close to this, because if I have two tabs
> open and signed into FxA, and I sign out in one of them, the other one
> updates itself automatically.

The /clear page does not trigger the message you are thinking of, that message is only broadcast if the user signs out. /clear is a hammer to destroy local content-server state, it does not contact the auth server.


> 
> > We have a content server endpoint that may already suit the purpose: /clear
> 
> I wonder, is there any risk with exposing this endpoint on our production
> instance?  It means that e.g. any page on the internet could force you to
> log out of FxA just by loading a hidden iframe.  


No danger AFAICT, we don't allow iframing except under strict conditions. The browser being the browser is free to ignore x-frame-options for trusted content.

Comment 15

[Shane Tomlinson [:stomlinson]]

(In reply to Ryan Kelly [:rfkelly] from comment #13)
> > I feel I need to know more about how the server finds existing open tabs to talk to.
> 
> The open tabs talk to each other via BroadcastChannel [1] or a
> localStorage-based fallback
> 
> [1] https://developer.mozilla.org/en-US/docs/Web/API/BroadcastChannel

Remember, I have a strong desire to make the localStorage based fallback go away.

Comment 16

[Ryan Kelly [:rfkelly]]

> It also isn't going to work if the user happens to be offline when they disconnect,
> which is an edge-case, but one worth considering.

How do we currently handling the offline case?  IIUC the page on accounts.firefox.com only detects that it's been "logged out" because it goes to use the sessionToken and finds it is invalid, which only happens because the browser explicitly destroys the sessionToken as part of the logout process.  If the user is offline, how do we handle this destroy-the-sessionToken step currently?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ryan Kelly [:rfkelly] from comment #16)
> How do we currently handling the offline case?

We just discard it - we don't explicitly handle offline as a special case, we just ignore the error attempting to destroy it cleanly.

> IIUC the page on
> accounts.firefox.com only detects that it's been "logged out" because it
> goes to use the sessionToken and finds it is invalid, which only happens
> because the browser explicitly destroys the sessionToken as part of the
> logout process.

Yeah - I was trying to explore a path where we also discard the token copy on behalf of the server - ie, by pretending the user did "Clear Recent History -> Everything" but only for the specified domain. I certainly understand objections to this idea though.

Comment 18

[Shane Tomlinson [:stomlinson]]

I have a content-server only fix that has just been merged in [1]. This should roll into production in about 2 weeks with train-66.

The fix is to check the user's session any time there is a window.focus event. Because the user signs out via the about:preferences#sync page, they will have to re-focus the window with the settings page, which causes a window focus event. 

This solution still leaves open the possibility of the user clicking "Disconnect" on another device from the devices view, and the user on the disconnected device can still interact with the settings page, for the same reasons as this bug - the OAuth tokens are still good even though the sessionToken is not.

There are a couple of ways to get around this additional problem:

1. When the push notification is sent to the browser that it has been disconnected, the browser notifies any open content server windows.
2. The content server could check sessionToken validity before any settings page interaction.

Long term I'd rather see the first because the second adds an additional XHR request to every settings page interaction.

Opened [2] with this newly found issue and STRs.

[1] - https://github.com/mozilla/fxa-content-server/pull/3924
[2] - https://github.com/mozilla/fxa-content-server/issues/3928

Closing this particular bug as fixed, since for the given STRs the problem *is* fixed. I would like to continue discussing with :markh to figure out a good solution to [2].

Comment 19

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I confirm that this issue is not reproducible anymore on 
- latest Nightly 51.0a1 (2016-09-13) 
- 49.0 build3 (20160912134115)
Setting the corresponding flags.