1105470 - contentLocation in webconsole.js isn't updated after a redirect

Status: NEW

(DevTools :: Console, defect, P3)

Description

[Ken Lyon]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141106120505

Steps to reproduce:

Our website attempts to redirect page requests from https to http unless they specifically need to be served over https (such as login pages). We do this by redirecting the request on the server side and returning a 301 status code.


Actual results:

All resources loaded via http are listed in the Developer Tools console with red text and the [Mixed Content] warning link.

While I've not investigated further, I suspect scripts etc. could be disabled as a result of being flagged as "Mixed Content" too.


Expected results:

I would expect the resources to be listed as normal "GET" requests without being interpreted as "Mixed Content".

I would also expect all scripts etc. to load as normal.

I do not expect this scenario to be considered as "mixed content" since the ENTIRE RESPONSE is being served over http. I could understand if this was an iframe within an https page or something like that, or if this was a form submission. It seems like a bug to me, though, that this is considered mixed.

If the page and all its content are retrieved over http, then what exactly is "mixed"? As far as I can see it, it's only the fact that the original request was made using https.

If this is indeed the intended behaviour, please advise how we can safely redirect a user from https to http without causing security issues.

Comment 1

[Tanvi Vyas[:tanvi]]

Hi Ken,

Can I have a link to your website so I can take a closer look at this?  Also, we've made some changes to our mixed content redirect detection in Firefox 36 via bug 418354.  Perhaps the issue is fixed now.  Firefox 36 is targeted to hit stable users in February (https://wiki.mozilla.org/RapidRelease/Calendar).

Thanks!

Comment 2

[Ken Lyon]

Hi Taniv,

One of our websites that exhibits the behaviour is www.workbc.ca/CareerCompass.

Repro steps:

1) In Firefox, open the address https://www.workbc.ca/CareerCompass.
2) The page automatically redirects to regular HTTP. (The actual URL you see will be http://www.workbc.ca/CareerCompass/CMSPages/PortalTemplate.aspx?aliaspath=/Home)

Expected results:

The console should show the GET entries for all the constituent parts without any "Mixed Content" warnings.

Actual results:

The only item in the console that does NOT have a "Mixed Content" warning is the 301 redirect from "https://www.workbc.ca/CareerCompass" to "http://www.workbc.ca/CareerCompass/CMSPages/PortalTemplate.aspx?aliaspath=/Home".

Comment 3

[Tanvi Vyas[:tanvi]]

Hi Ken,

Thanks for your report!  The content is inaccurately getting flagged as "Mixed Content" by the Net panel (tab) in the Web Console.  We actually also report Mixed Content warnings in the Security panel of the webconsole.  The security panel is the source of truth here and gets its information from our security checking code (nsMixedContentBlocker).  The Net panel just does a naive check for http from an https page.  My guess is that it uses the original location and doesn't account for top level redirects (to be investigated).

When we implemented the Security panel, we debated whether or not to leave the naive checks in the Net panel and decided to keep them there.  Given this bug, perhaps that was the wrong decision.  Note that no content is actually getting blocked on the page, since the security checking code accounts for the redirect and knows that we are on an insecure page.  Nevertheless, we are giving erroneous reports in the Net panel and that should be fixed.

I will take a closer look to see what's going on and if we should a) remove mixed content warnings from the net panel, or if we should b) keep the mixed content warnings in the net panel and the security panel, but replace the naive checks in the net panel with the correct logic from nsMixedContentBlocker.

need-info'ing myself so I don't lose track of this.

Comment 4

[Ken Lyon]

Does anyone have an update on this? I'm wondering if there are plans to fix it and if so how soon?

Comment 5

[Tanvi Vyas[:tanvi]]

Hi Ken,

I haven't had a chance to look into this issue yet.  If you or anyone else would like to take the bug, go for it and I would be happy to advise.

Comment 6

[Ken Lyon]

I just tested this on Firefox 36 (Windows 7) and it's still an issue.

The page I originally referred you to now no longer redirects to http so it's not a good example any more.

Instead, you can use https://www.workbc.ca/WorkBC-Centres.aspx as an example now. It redirects to http and you can see from the console that there are still a lot of "Mixed Content" warnings.

Comment 7

[Tanvi Vyas[:tanvi]]

I'm not able to fix this issue right now, but perhaps someone from the devtools team can look into it?

Comment 8

[Ken Lyon]

This is still an issue in Firefox 36.0.1. (Tested on Windows 7.)

Comment 9

[Tanvi Vyas[:tanvi]]

Example: https://people.mozilla.org/~tvyas/https_302_http.html

The root of the problem seems to be that contentLocation here http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#1526 is referring to the previous page before the redirect.  Is that a bug or expected behavior?

The check in webconsole is pretty naive and was implemented before nsMixedContentBlocker existed:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/utils.js#446
Trying to call MixedContentBlocker here to determine if the network load is really mixed is probably overkill.  The accurate Mixed Content messages are already in the Security pane.


Options:
* fix contentLocation so that it reflects the top level document load instead of the previous page (assuming this doesn't break other things).
* close this bug won't fix
* remove the mixed content check from the net panel

I leave it up to the devtool team to decide how they would like to proceed.  Not really sure who on the team to needinfo here; have cc'ed a few of you.

Comment 10

[Tanvi Vyas[:tanvi]]

Flagging Mihai based on hg blame for webconsole.js.  Mihai, can you take a look at comment 9?

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I believe Mihai is unavailable, so let's give Panos a try.

Comment 12

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

I've looked into this briefly, but I will look more closely once I've cleared a few things from my todo list.

Comment 13

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

I've looked at this, and I agree that there is probably a bug in the way contentLocation is updated. But beyond that, I'm interested in either using the proper way to identify mixed content requests or removing that check, assuming there is a similar warning in the web console. Tanvi, I don't see any related messages when the Security filter is enabled, so I assume that by "Security pane" you mean the Security tab in the Page Info dialog, correct?

In that case, I would prefer to keep the (correct!) warning in the console, as I find people tend to look for such messages in the web console, first and foremost. Would using the following check be the modern way to accomplish this?

netutil.URIChainHasFlags(uri, Ci.nsIProtocolHandler.URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)

Also, I don't see any shield icon in my nightly when I visit your example URL and follow along the prompts, why is that?

Comment 14

[Tanvi Vyas[:tanvi]]

Attachment: Mixed content warning messages on an HTTP page.png

Hi Panos!  Thanks for looking into this.

The proper way of identifying mixed content requests requires using nsMixedContentBlocker.cpp.  That code runs on every load.  When we find mixed content, we show a warning in the red security tab in the webconsole itself by calling ReportToConsole().  If a developer has that tab turned on, they will see the error messages.  If they have it turned off, they won't.

Since some developers have it turned off and since every request that goes out is in the Net tab anyway, we decided to duplicate the message.  So if a resource load in the Net tab is http and the page it is in is https, we will use the color red for the Net tab entry instead of the color black.  We do a naive check here to get this information comparing the subresources scheme and the contentLocation's scheme.  The bug here is because the contentLocation's scheme is not accurate after a redirect.  If we tried to put the full logic of nsMixedContentBlocker.cpp in the Net panel of webconsole, it would make the code overly complicated for something that is probably not worth the complexity or the implementation time.

So here are our options:
* Fix the contentLocation bug so that is updated after a redirect.  There may be other bugs besides this one in the console because of the inaccurate contentLocation.
* Stop changing the color of the text in the Net panel when it thinks mixed content is present.  Instead, just let developers rely on the Security tab in the webconsole.

In this example https://people.mozilla.org/~tvyas/https_302_http.html, we have an https page redirect to an http page.  The http page does a subresource load of an http script.  Since we are now at an http page, there is no mixed content.  The user shouldn't see a shield or any messages related to mixed content in the webconsole.  But because of this bug, we see two red entries in the Net Panel that should be black (see screenshot).  You can compare this to https://people.mozilla.org/~tvyas/mixedboth.html, where you will get the Shield icon and a bunch of mixed content warnings in the webconsole.

> netutil.URIChainHasFlags(uri, Ci.nsIProtocolHandler.URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)
>
This check will not work, since we still need to know if the parent is http or https.  The full set of flags we check on subresources in nsMixedContentBlocker is here, but again, implementing this in webconsole is probably overkill:
netutil.URIChainHasFlags(uri, Ci.nsIProtocolHandler.URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)

Comment 15

[Tanvi Vyas[:tanvi]]

Panos, do you know who in the devtools team works on the webconsole?

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #15)
> Panos, do you know who in the devtools team works on the webconsole?

:bgrins and :linclark are the current owners.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: redirection-rdp-traffic.json

OK, so contentLocation doesn't seem to be updated because it's tied to the "navigate" event on the Target which doesn't get fired until after the net messages are received: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/target.js#486.  I've attached an RDP trace (which can be visualized with the RDP Inspector addon)

For reference: tabNavigated is fired by the backend by _willNavigate and _navigate as controlled by an nsIWebProgressListener here: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#2276.  _willNavigate sends a tabNavigated packet with "start" and _navigate sends one with "stop".  Then the target waits for the "stop" one before firing the event that the webconsole is using to determine contentLocation.  I guess the issue is that the progress listener state of `isWindow && isStop` is firing later than the HTTP subresource requests that are happening?  But not sure if:

1) we should be special casing this state in webconsole (i.e. wait to print net traffic until tab is done navigating)
2) maybe there's a better set of onStateChange flags we could use to detect the 'finished' state
3) we should add a new packet type for a redirection starting along with the new url so we can do better comparisons

Since the net requests are coming through before that event I could definitely see this causing issues in other tools relying on the tab navigation events, so doing a CSS webconsole workaround seems like it's not the right solution.

Tanvi, maybe we can chat at some point about the sequence of events the nsIWebProgressListener is going through that's leading us into this state?

Comment 19

[Tanvi Vyas[:tanvi]]

It seems like you need to look for the redirecting state and then fire an event:
http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl#40

This state is set here in AsyncOnChannelRedirect, which gets called for all redirects:
http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#1368

When the event is fired, you can update contentLocation to the new redirected uri.

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: redirect-logging.patch

I added some additional logging for this (see patch) and what I'm seeing is not matching up with the documentation here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebProgressListener.  Specifically, "When a redirect occurs, a new request is generated automatically to process the new request. Expect a corresponding STATE_START event for the new request, and a STATE_STOP for the redirected request."

Here are the state changes I'm seeing:

"ON STATE CHANGE FOR https://people.mozilla.org/~tvyas/https_302_http.html" 
isStart: true 
isStop: false 
isDocument: true 
isWindow: true 
isRedirect: false

"ON STATE CHANGE FOR https://people.mozilla.org/~tvyas/https_302_http.html" 
isStart: false 
isStop: false 
isDocument: true 
isWindow: false 
isRedirect: true <-- Here's the redirect.. now I'm expecting to see a STATE_START event next..

"ON STATE CHANGE FOR http://people.mozilla.org/~tvyas/mixedcontent.html" 
isStart: false  <-- But STATE_START isn't set here
isStop: false 
isDocument: true 
isWindow: false 
isRedirect: false

"ON STATE CHANGE FOR http://people.mozilla.org/~tvyas/mixedcontent.html" 
isStart: false 
isStop: true 
isDocument: true 
isWindow: false 
isRedirect: false

"ON STATE CHANGE FOR http://people.mozilla.org/~tvyas/mixedcontent.html" 
isStart: false 
isStop: true 
isDocument: false 
isWindow: true 
isRedirect: false

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The comment in nsDocLoader.h suggests[1] we might be stuck?  Not sure how you are meant to get the new channel...?

[1]: https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.h#172-174

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #21)
> The comment in nsDocLoader.h suggests[1] we might be stuck?  Not sure how
> you are meant to get the new channel...?
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.
> h#172-174

Jonas, do you happen to know about the nsIWebProgressListener and redirect states, or know someone who might?  Specifically, see Comment 20 in which I'm not seeing a STATE_START event that I'm expecting to based on the documentation.  So we aren't keeping very good track of the current page / navigation state inside devtools in this scenario.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, I've never worked with nsIWebProgressListener so don't know anything about it off the top of my head.

Smaug and bz might?

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

See comment 22.

Comment 25

[Boris Zbarsky [:bzbarsky]]

The debugger progress listener is registered like so:

2159     webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATUS |
2160                                           Ci.nsIWebProgress.NOTIFY_STATE_WINDOW |
2161                                           Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);

That means that it only wants to receive OnStatusChange events, and OnStateChange events that have STATE_IS_WINDOW or STATE_IS_DOCUMENT set.  You can see this clearly in the log from comment 20: all the notifications have either isDocument or isWindow set to true.

There _are_ start/stop OnStateChange notifications for the post-redirect request being generated, in general, as the documentation says there should be.  From the NSPR log for docloader while loading the URI involved:

2094568192[115c62070]: DocLoader:127a15000: Status (http://people.mozilla.org/~tvyas/mixedcontent.html): code: 10001
2094568192[115c62070]: DocLoader:115cd1e30: Status (http://people.mozilla.org/~tvyas/mixedcontent.html): code: 10001

those are STATE_IS_REQUEST|STATE_START notifications.  But this listener explicitly asked to not receive those, so it doesn't.

Now arguably this STATE_START notification should have STATE_IS_DOCUMENT set on it, but I'm pretty wary of changing that given that this stuff is all rather ad-hoc and lots of things have historically depended on its exact behavior... I'm especially wary of it due to this comment in nsDocLoader::OnStartRequest:

      // Only fire the start document load notification for the first
      // document URI...  Do not fire it again for redirections

right before it dispatches the STATE_START|STATE_IS_REQUEST|STATE_IS_DOCUMENT (via nsDocLoader::doStartDocumentLoad).

Past that, it's not clear to me exactly what information this listener is looking for and why it thinks this information is available solely from STATE_IS_DOCUMENT and STATE_IS_WINDOW notifications, but those are definitely documented to only fire at the beginning and end of the load process for the document, I thought...

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Great, thanks :bz!

This progress listener only wants to watch for changes in both top-level and sub-frame document locations.  So, we could add NOTIFY_STATE_REQUEST, but we'll need to filter out inline requests somehow.

Really, NOTIFY_LOCATION is probably a better fit for the problem, except we want both start and stop states to re-create "will navigate" and "navigate", and onLocationChange seems to only get one event.

Comment 27

[Boris Zbarsky [:bzbarsky]]

For what it's worth, you should be able to filter out the non-document loads more or less like so:

  if (aRequest != aWebProgress.QueryInterface(Ci.nsIDocumentLoader).documentChannel) {
    return;
  }