1303775 - Captive portal tab may stay open after login

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

I just logged into a captive portal, based I believe on a new tab popup, and when I said yes to the two screens worth of t's and c's it redirected me to the success.txt url. Not exactly sure how that happened, but clearly its a bad UX. we could probably just change success.txt to be a short bit of html with a meta redirect on it or something to a nicer landing page if that's going to be a normal thing.

--

The UI is supposed to close that tab once you log in, but either the closing the tab is racy, or the detector doesn't notice that we logged in.
We should probably update the page to look a little nicer in case that happens.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I think we should make sure that page never appears. Making it look good would increase the network request response time for not much benefit, as this isn't supposed to happen.

Comment 2

[Patrick McManus [:mcmanus]]

belts _and_ suspenders :)

response time under 1500 bytes is essentially constant - driven by packet count rather than bytes. you can jam a redirect in there in case it is shown :)

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

True, but since a redirect would require a switch from success.txt to success.html, wouldn't the common case be impacted by the presumably more expensive HTML parsing code path in XHR?

Comment 4

[Patrick McManus [:mcmanus]]

I thought the common case was you never displayed the success content? I think that's the way it is now and is right.. the thought here is just that if it does get displayed for whatever reason its something better than just "success". I agree we don't want to chase a redirect in the common case.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Valentin Gosu [:valentin] from comment #0)
> I just logged into a captive portal, based I believe on a new tab popup, and
> when I said yes to the two screens worth of t's and c's it redirected me to
> the success.txt url. Not exactly sure how that happened, but clearly its a
> bad UX. we could probably just change success.txt to be a short bit of html
> with a meta redirect on it or something to a nicer landing page if that's
> going to be a normal thing.
> 
> --
> 
> The UI is supposed to close that tab once you log in, but either the closing
> the tab is racy, or the detector doesn't notice that we logged in.
> We should probably update the page to look a little nicer in case that
> happens.

There's actually a check already. The tab should be closed if the portal redirects to the canonical URL after login. The check happens before the redirect though - unfortunately, I wrote a test to make sure it's left open, but not to check if it gets closed.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

That said, I'm not opposed to your ideas for changing success.txt even if we fix this race - just in case it pops up in some other edge case we didn't think of :P

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

I think a good way to fix this bug is to change the logic here: https://dxr.mozilla.org/mozilla-central/rev/944cb0fd05526894fcd90fbe7d1e625ee53cd73d/browser/modules/CaptivePortalWatcher.jsm#174.

We should replace lines 174-182 with code that does the following:
a) If the tab has redirected to the canonical URL, close it and return.
b) Listen for the first origin change in the tab. If it takes us to the canonical URL, close the tab. Otherwise, do nothing.

Does this make sense?

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #7)
> b) Listen for the first origin change in the tab. If it takes us to the
> canonical URL, close the tab. Otherwise, do nothing.

The origin change thing will work much better once bug 1313577 is also fixed.

Comment 9

[Matthew N. [:MattN]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #7)
> I think a good way to fix this bug is to change the logic here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 944cb0fd05526894fcd90fbe7d1e625ee53cd73d/browser/modules/
> CaptivePortalWatcher.jsm#174.

Correct me if I'm wrong but we use the canonicalURL for two purposes:
a) For captivedetect.js (the service) to see if there is a portal interfering with requests.
b) To use as a way to get redirected to the login page (since the canonicalURL is plain http and therefore can be intercepted without cert. warnings)

> We should replace lines 174-182 with code that does the following:

> a) If the tab has redirected to the canonical URL, close it and return.

Define "redirected". Some valid captive portal could use `window.location = destinationURL;` where destinationURL is our portal URL which they got from an initial redirect to a login page.

To clarify, are you talking about handling a redirect but not an initial load? I agree that being redirect here in a portal tab is a sign we need to close the tab but I don't think it's sufficient for the above reason (not all portals will use a HTTP redirect).

> b) Listen for the first origin change in the tab. If it takes us to the
> canonical URL, close the tab. Otherwise, do nothing.

I may be confused about when you're starting your counting of origin changes. When you say "first" are you talking about the change from about:blank to the canonicalURL? If so, the first origin change should always be the canonical URL but not necessarily the canonical content if it's intercepted. Are you talking about checking the content?

To detect these two location changes faster than waiting for the captive portal service to tell us about state changes, we should use a listener in the front-end e.g. onLocationChange. Milliseconds can matter in whether a user notices success.txt or not and I think having this logic in CaptivePortalWatcher.jsm will reduce/remove the lag.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Matthew N. [:MattN] from comment #9)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #7)
> > I think a good way to fix this bug is to change the logic here:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 944cb0fd05526894fcd90fbe7d1e625ee53cd73d/browser/modules/
> > CaptivePortalWatcher.jsm#174.
> 
> Correct me if I'm wrong but we use the canonicalURL for two purposes:
> a) For captivedetect.js (the service) to see if there is a portal
> interfering with requests.
> b) To use as a way to get redirected to the login page (since the
> canonicalURL is plain http and therefore can be intercepted without cert.
> warnings)

Correct.

> > We should replace lines 174-182 with code that does the following:
> 
> > a) If the tab has redirected to the canonical URL, close it and return.
> 
> Define "redirected". Some valid captive portal could use `window.location =
> destinationURL;` where destinationURL is our portal URL which they got from
> an initial redirect to a login page.

By redirected, I mean if the tab's current URL (when we reach line 174) is the canonicalURL.

> To clarify, are you talking about handling a redirect but not an initial
> load? I agree that being redirect here in a portal tab is a sign we need to
> close the tab but I don't think it's sufficient for the above reason (not
> all portals will use a HTTP redirect).

Right, I didn't necessarily mean a HTTP redirect, sorry for not being clear.

> > b) Listen for the first origin change in the tab. If it takes us to the
> > canonical URL, close the tab. Otherwise, do nothing.
>
> I may be confused about when you're starting your counting of origin
> changes. When you say "first" are you talking about the change from
> about:blank to the canonicalURL? If so, the first origin change should
> always be the canonical URL but not necessarily the canonical content if
> it's intercepted. Are you talking about checking the content?

I mean the first origin change that we detect after we start listening somewhere around line 174. I.e. The first origin change we detect after _captivePortalGone() is called.

> To detect these two location changes faster than waiting for the captive
> portal service to tell us about state changes, we should use a listener in
> the front-end e.g. onLocationChange. Milliseconds can matter in whether a
> user notices success.txt or not and I think having this logic in
> CaptivePortalWatcher.jsm will reduce/remove the lag.

Definitely. This is kinda why I said this bug may be "easier" once bug 1313577 is also fixed - if we add a listener as soon as we open the captive portal tab, that listener can take care of closing the tab and _captivePortalGone() doesn't have to worry about it. While I'm thinking about this, it occurs to me that it might be kind of strange to the user if they see the captive portal tab close on its own in the background without any indication of why it closed (this can totally happen if the portal gets logged into from somewhere other than the portal tab). Maybe _captivePortalGone() should show a new notification (that's not persistent) to tell the user that Firefox has detected that the network is now accessible.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

I'd like to fix this properly by having a landing page, so marking this wontfix for 52 and 53. I'm not sure if we'll be able land/host a landing page for 54 so marking it fix-optional.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Er, I confused this with bug 1322296. If that one doesn't get fixed for 54 this would be nice to have, if we can find an elegant solution that works deterministically (I need to re-investigate).

Comment 13

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 15

[Ethan Tseng [:ethan]]

This bug will be resolved by bug 1313577.

Comment 16

[:prathiksha]

Attachment: Bug 1303775 - Fix race conditions prevalent with closing captive portal tabs that re-direct to the canonicalURL after successful login.

Comment 17

[:prathiksha]

Attachment: Bug 1303775 - Fix race conditions prevalent with closing captive portal tabs that re-direct to the canonicalURL after successful login/abort. r?johannh,nhnt11

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b86d0dfff95
Fix race conditions prevalent with closing captive portal tabs that re-direct to the canonicalURL after successful login/abort. r=johannh,nhnt11

Comment 19

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 6b86d0dfff95 (Bug 1303775) for causing perma bc failures in browser/base/content/test/captivePortal/browser_CaptivePortalWatcher.js CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=229ddddc3a44850a295fa6c8bd0ccfbc8271dde0&selectedJob=294867607

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=294867607&repo=autoland&lineNumber=2931

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9b28758785e
Backed out changeset 6b86d0dfff95 for causing perma bc failures in browser/base/content/test/captivePortal/browser_CaptivePortalWatcher.js CLOSED TREE

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/942cf82e2198
Fix race conditions prevalent with closing captive portal tabs that re-direct to the canonicalURL after successful login/abort. r=johannh,nhnt11

Comment 22

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/942cf82e2198