Closed Bug 1342178 Opened 8 years ago Closed 3 years ago

HTTPS Strict Transport Security incorrectly shows as disabled when using 'private browser mode' or 'private window'

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Version:
54 Branch
Type:
defect

Tracking

(firefox51 unaffected, firefox52 unaffected, firefox53 unaffected, firefox54 fix-optional)

Status:
RESOLVED WORKSFORME
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 --- fix-optional

People

(Reporter: buggyz, Unassigned)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

Screenshot showing the problem
371.42 KB, image/png
Details
Bug 1342178 - Pass origin attributes to isSecureURI in devtools and browser.
59 bytes, text/x-review-board-request
past
: review+
keeler
: review+
Details
Bug 1342178 - Regression tests for HSTS/HPKP information in private browsing window.
59 bytes, text/x-review-board-request
Honza
: review+
keeler
: review+
Details
Bug 1342178 - Combine test_network_security-{hsts/hpkp}.html.
59 bytes, text/x-review-board-request
keeler
: review+
Honza
: review+
Details
Bug 1342178 - Rename test_network_security-hpkp.html.
59 bytes, text/x-review-board-request
keeler
: review+
Honza
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170223110219

Steps to reproduce:

I Used Firefox 54.0a1 (2017-02-23) (64-bit) [Nightly] to:

1. Open the the developer tools using F12 then switch to the Network Monitor 
2. Surf to https://www.bonnieradvocaten.nl 
3. Select any of the HTTPS responses (HTML, CSS JS, IMG) by clicking on it, they all contain 
    the HSTS-header
4. In the right pane, that appeared after clicking, select the "Security" tab 

Notice that Firefox 54 shows: "HTTP Strict Transport Security: Disabled" 





Actual results:

Firefox 54  shows, HSTS to be disabled:
"HTTP Strict Transport Security: Disabled"

(Even though the HSTS header is set correctly)


Expected results:

Firefox Nightly 54.0a1 should have shown: "HTTP Strict Transport Security: Enabled"
Since I suspect I might have been to vague in my problem description i'm going to attach  a screenshot that clearly depicts the problem.

Furthermore i would like to add that Firefox 51.0.1+build2-0ubuntu0.16.10.2 does correctly show HSTS as enabled and so does Firefox Developer Edition 53.0a2 (2017-02-23) (64-bit).

Also, i've tried the Firefox 54 Nightly build of (2017-02-11) (it was on another one of my systems,
that browser didn't yet auto update to current build) and it did correctly show HSTS as enabled for https://www.bonnieradvocaten.nl
This screenshot shows the problem, the problem area has been marked with a red framework
Okay, so this problem only occurs when using Firefox 54.0a1 Nightly when you're using Private Browsing mode. 

I've tried both the Windows X64 and 32-bit of Firefox Nightly 54.0a1 and they're also affected if you use private-browsing-mode. 

Firefox stable (51.0.1) under Windows was not affected by this problem.
Hardware: Unspecified → All
Summary: HTTPS Strict Transport Security incorrectly shows as disabled → HTTPS Strict Transport Security incorrectly shows as disabled when using 'private browser mode' or 'private window'
OS: Unspecified → All
Blocks: 1323644
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Netmonitor
Ever confirmed: true
Keywords: regression
It looks like what's happening is when the backend processes the header, it uses both the NO_PERMANENT_STORAGE (i.e. private browsing) flag as well as origin attributes that specify a privateBrowsingId of 1. When the frontend looks up the property ( https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/devtools/shared/webconsole/network-helper.js#647 ) it uses only the flag and not any origin attributes, leading it to not find any relevant stored state.

Maybe the right thing to do here is to drop the flags parameters from nsISiteSecurityService functions and make the origin attributes parameters mandatory, and check for a non-zero privateBrowsingId to see if we're supposed to not persist the state?
David do we know who can fix this regression?
Flags: needinfo?(dkeeler)
I caused that regression, so I'll take this bug.
Assignee: nobody → jhao
Flags: needinfo?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> It looks like what's happening is when the backend processes the header, it
> uses both the NO_PERMANENT_STORAGE (i.e. private browsing) flag as well as
> origin attributes that specify a privateBrowsingId of 1. When the frontend
> looks up the property (
> https://dxr.mozilla.org/mozilla-central/rev/
> 5069348353f8fc1121e632e3208da33900627214/devtools/shared/webconsole/network-
> helper.js#647 ) it uses only the flag and not any origin attributes, leading
> it to not find any relevant stored state.
> 
> Maybe the right thing to do here is to drop the flags parameters from
> nsISiteSecurityService functions and make the origin attributes parameters
> mandatory, and check for a non-zero privateBrowsingId to see if we're
> supposed to not persist the state?

I agree this might be the best solution, but it will break some add-ons, such as HTTPS Everywhere.  How do we usually deal with add-on compatibility issue in this kind of situations?
(In reply to Jonathan Hao [:jhao] from comment #8)
> Created attachment 8842763 [details]
> Bug 1342178 - Pass origin attributes to isSecureURI in devtools.
> 
> Review commit: https://reviewboard.mozilla.org/r/116536/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/116536/

Hi Panos and David, could you take a look at this patch?  It passes the origin attributes in the security info to isSecureURI().

This should be able to fix the regression.  I also opened bug 1343806 to track what David proposed in comment 4.
Comment on attachment 8842763 [details]
Bug 1342178 - Pass origin attributes to isSecureURI in devtools and browser.

https://reviewboard.mozilla.org/r/116536/#review118336

We should probably add some tests to ensure that this functionality doesn't regress again.
In general, this should work (although see the issue about the serialization), but I'm wary of expanding the nsITransportSecurityInfo API. I think if we can get the origin attributes from another source that already exposes it, that would probably be best (unless it introduces too much complexity). What do you think?

::: browser/base/content/browser.js:3381
(Diff revision 1)
>    let flags = PrivateBrowsingUtils.isWindowPrivate(window) ?
>                Ci.nsISocketProvider.NO_PERMANENT_STORAGE : 0;
>  
>    let uri = Services.io.newURI(location);
>  
> -  let hasHSTS = sss.isSecureURI(sss.HEADER_HSTS, uri, flags);
> +  const originAttributes = securityInfo.originAttributes;

From what I can tell, securityInfo comes from a serialized nsITransportSecurityInfo, and TransportSecurityInfo doesn't include its mOriginAttributes in the serialization. One way to address this would be to include it in the serialization. Another option would be to send the origin attributes from the failed channel where we got the security info in the first place as well (from here, I think: https://dxr.mozilla.org/mozilla-central/rev/e91de6fb2b3dce9c932428265b0fdb630ea470d7/browser/base/content/content.js#255 ).

::: devtools/shared/webconsole/network-helper.js:647
(Diff revision 1)
>            // isSecureURI only cares about the host, not the scheme.
>            let host = httpActivity.hostname;
>            uri = Services.io.newURI("https://" + host);
>          }
>  
> -        info.hsts = sss.isSecureURI(sss.HEADER_HSTS, uri, flags);
> +        const originAttributes = securityInfo.originAttributes;

Am I correct in that we could also get the origin attributes via httpActivity.channel.loadInfo.loadingPrincipal.originAttributes?

::: netwerk/socket/nsITransportSecurityInfo.idl:25
(Diff revision 1)
>       * If verification succeeded, this will be null.
>       */
>      readonly attribute nsIX509CertList failedCertChain;
> +
> +    [implicit_jscontext]
> +    readonly attribute jsval originAttributes;

My initial reaction to expanding this API is if we can get the origin attributes from elsewhere (e.g. someChannel.loadInfo.loadingPrincipal.originAttributes) then we should probably do that rather than this. That does introduce complexity elsewhere, so I'm not sure what would be best, in the end.
Attachment #8842763 - Flags: review?(dkeeler)
Hi David,

Yes, you're right.  We can get the origin attributes from the channel.  I used the securityInfo in browser.js because I couldn't find any channels in there, and I used it in devtools just for consistency.  Your second option seems better.

Thanks.
Attachment #8842763 - Flags: review?(past)
Comment on attachment 8842763 [details]
Bug 1342178 - Pass origin attributes to isSecureURI in devtools and browser.

https://reviewboard.mozilla.org/r/116536/#review119768

Cool - this is the sort of thing I was thinking of. I don't know the exact details, but you should be able to open a new private browser window using some test utility function that takes a callback (or returns a promise). Using that, you should be able to re-run that test in that context (it might take some refactoring, however).

::: commit-message-e91de:1
(Diff revision 2)
> +Bug 1342178 - Pass origin attributes to isSecureURI in devtools. r=past,keeler

Well, and browser too, right?

::: devtools/client/debugger/new/debugger.js:8975
(Diff revision 2)
>                    // isSecureURI only cares about the host, not the scheme.
>                    let host = httpActivity.hostname;
>                    uri = Services.io.newURI("https://" + host);
>                  }
>  
> -	        info.hsts = sss.isSecureURI(sss.HEADER_HSTS, uri, flags);
> +	        const originAttributes = securityInfo.originAttributes;

Looks like this one needs to be updated as well.
Attachment #8842763 - Flags: review?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #14)
> Comment on attachment 8842763 [details]
> Bug 1342178 - Pass origin attributes to isSecureURI in devtools and browser.
> 
> https://reviewboard.mozilla.org/r/116536/#review119768
> 
> Cool - this is the sort of thing I was thinking of. I don't know the exact
> details, but you should be able to open a new private browser window using
> some test utility function that takes a callback (or returns a promise).
> Using that, you should be able to re-run that test in that context (it might
> take some refactoring, however).

In test_network_security-hsts.html, it listens to the networkUpdateEvent[1].  When I opened a new private browsing window like in test_sts_privatebrowsing_perwindowpb.html[2], I couldn't get the networkUpdateEvent from that window.  Maybe I did something wrong?

[1]: http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/devtools/shared/webconsole/test/test_network_security-hsts.html#62
[2]: http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_privatebrowsing_perwindowpb.html
 
> ::: commit-message-e91de:1
> (Diff revision 2)
> > +Bug 1342178 - Pass origin attributes to isSecureURI in devtools. r=past,keeler
> 
> Well, and browser too, right?
> 
> ::: devtools/client/debugger/new/debugger.js:8975
> (Diff revision 2)
> >                    // isSecureURI only cares about the host, not the scheme.
> >                    let host = httpActivity.hostname;
> >                    uri = Services.io.newURI("https://" + host);
> >                  }
> >  
> > -	        info.hsts = sss.isSecureURI(sss.HEADER_HSTS, uri, flags);
> > +	        const originAttributes = securityInfo.originAttributes;
> 
> Looks like this one needs to be updated as well.

Thanks, please take a look again.
Comment on attachment 8842763 [details]
Bug 1342178 - Pass origin attributes to isSecureURI in devtools and browser.

https://reviewboard.mozilla.org/r/116536/#review120174

LGTM!
Attachment #8842763 - Flags: review?(dkeeler) → review+
Comment on attachment 8842763 [details]
Bug 1342178 - Pass origin attributes to isSecureURI in devtools and browser.

https://reviewboard.mozilla.org/r/116536/#review120450

Looks good, thanks.
Attachment #8842763 - Flags: review?(past) → review+
Comment on attachment 8844733 [details] [diff] [review]
This is my WIP patch for the test.  As I mentioned in the previous comment, I can't make this patch work.  I couldn't get the networkUpdateEvent, so this test won't finish.

Review of attachment 8844733 [details] [diff] [review]:
-----------------------------------------------------------------

This is a devtools event and I'm a little rusty on that code. Perhaps the event isn't triggered in private windows? Honza should know more.
Attachment #8844733 - Flags: feedback?(past) → feedback?(odvarko)
Thanks, Panos. FWIW I also tried opening a new non-private window and still couldn't get the event.
Comment on attachment 8844733 [details] [diff] [review]
This is my WIP patch for the test.  As I mentioned in the previous comment, I can't make this patch work.  I couldn't get the networkUpdateEvent, so this test won't finish.

(In reply to Jonathan Hao [:jhao] from comment #21)
> Thanks, Panos. FWIW I also tried opening a new non-private window and still
> couldn't get the event.

You need to attach to the new opened browser tab (in the new private window).

The first (default) browser window is attached in `startTest` by calling:

attachConsoleToTab(["NetworkActivity"], onAttach);

You need to do similar thing for the private window, after:

let win = mainWindow.OpenBrowserWindow({private: true});

Since the console is attached you'll get `networkEventUpdate` events
(you might need to trigger reload or XHR)

Honza
Attachment #8844733 - Flags: feedback?(odvarko)
Thanks, Honza.  Excuse me for some more questions.

How should I attach to that newly created window?  There doesn't seem to be a parameter to specify the window, and I don't think we could use ContentTask.spawn() execute attachConsoleToTab, right?

Is there an example?
Flags: needinfo?(odvarko)
(In reply to Jonathan Hao [:jhao] from comment #23)
> Thanks, Honza.  Excuse me for some more questions.
> 
> How should I attach to that newly created window?  There doesn't seem to be
> a parameter to specify the window, and I don't think we could use
> ContentTask.spawn() execute attachConsoleToTab, right?
> 
> Is there an example?

Sorry for the delay, I've been traveling past two weeks.

My understanding is that `attachConsoleToTab` attaches to the currently selected browser tab. See the implementation of this function here:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/devtools/shared/webconsole/test/common.js#57

Here is the line where the currently selected tab is used.
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/devtools/shared/webconsole/test/common.js#101

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #24)
> My understanding is that `attachConsoleToTab` attaches to the currently
> selected browser tab. See the implementation of this function here:
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/devtools/shared/webconsole/test/
> common.js#57
> 
> Here is the line where the currently selected tab is used.
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/devtools/shared/webconsole/test/
> common.js#101
> 
> Honza

That's what I've been missing.  Thanks a lot!
Attachment #8844733 - Attachment is obsolete: true
Comment on attachment 8853312 [details]
Bug 1342178 - Regression tests for HSTS/HPKP information in private browsing window.

https://reviewboard.mozilla.org/r/125400/#review128174

Looks good to me in general, but I have some comments about the structure of these tests. Clearing review for now.

::: devtools/shared/webconsole/test/test_network_security-hpkp.html:81
(Diff revision 1)
>    runNextCase(state);
>  }
>  
>  function runNextCase(state) {
>    gCurrentTestCase++;
> -  if (gCurrentTestCase === TEST_CASES.length) {
> +  if (gCurrentTestCase === TEST_CASES.length * 2) {

I think using the length of the TEST_CASES array is a bit fragile. How about something like this:

* have two arrays: TEST_CASES and TEST_MODES or something
* in startTest, for each test mode, for each test case, add a synthesized test case to another array (like ALL_TESTS or something) where each entry consists of what mode to run in and the details of the test case
* in runNextCase, pop off the next testcase from ALL_TESTS and run it as appropriate (e.g. if it's a private mode test case, run it in a private window)

::: devtools/shared/webconsole/test/test_network_security-hsts.html:110
(Diff revision 1)
>    if (aPacket.updateType === "securityInfo") {
>      aState.client.getSecurityInfo(aPacket.from, onSecurityInfo);
>    }
>  }
>  
> +function whenDelayedStartupFinished(aWindow, aCallback) {

Seems a bit unfortunate that this file is more or less a duplicate of test_network_security-hpkp.html, but I'm not sure how much work it would be to refactor the common code.
Attachment #8853312 - Flags: review?(dkeeler)
Comment on attachment 8854762 [details]
Bug 1342178 - Combine test_network_security-{hsts/hpkp}.html.

https://reviewboard.mozilla.org/r/126712/#review129658

LGTM with a couple of comments.

::: devtools/shared/webconsole/test/test_network_security-hpkp.html:25
(Diff revision 1)
>  const HPKP_PREF = "security.cert_pinning.process_headers_from_non_builtin_roots";
>  
>  // Static pins tested by unit/test_security-info-static-hpkp.js.
>  const TEST_CASES = [
>    {
> -    desc: "no Public Key Pinning",
> +    desc: "no HSTS",

"no HSTS or HPKP"?

::: devtools/shared/webconsole/test/test_network_security-hpkp.html:32
(Diff revision 1)
> +    usesHSTS: false,
> +    usesPinning: false,
> +  },
> +  {
> +    desc: "HSTS from this response",
> +    url: "https://example.com/"+

nit: space before +

::: devtools/shared/webconsole/test/test_network_security-hpkp.html:54
(Diff revision 1)
>    },
>    {
>      desc: "dynamic Public Key Pinning with previous request",
>      url: "https://include-subdomains.pinning-dynamic.example.com/",
> +    usesHSTS: false,
>      usesPinning: true,

Seems like we should add an instance of this testcase for before we've seen the header, so usesPinning would also be false (analagous to the first HSTS testcase).
Attachment #8854762 - Flags: review?(dkeeler) → review+
Comment on attachment 8854763 [details]
Bug 1342178 - Rename test_network_security-hpkp.html.

https://reviewboard.mozilla.org/r/126714/#review129656

::: devtools/shared/webconsole/test/chrome.ini:35
(Diff revision 1)
>  [test_jsterm_last_result.html]
>  [test_jsterm_queryselector.html]
>  [test_network_get.html]
>  [test_network_longstring.html]
>  [test_network_post.html]
> -[test_network_security-hpkp.html]
> +[test_network_security-sss.html]

Maybe just "test_network_security.html"?
Attachment #8854763 - Flags: review?(dkeeler) → review+
Comment on attachment 8853312 [details]
Bug 1342178 - Regression tests for HSTS/HPKP information in private browsing window.

https://reviewboard.mozilla.org/r/125400/#review129660

Cool - LGTM. Thanks for working on this!

::: devtools/shared/webconsole/test/test_network_security-sss.html:120
(Diff revision 2)
> -  if (gCurrentTestCase === TEST_CASES.length) {
> +  if (gCurrentTestCase === ALL_TESTS.length) {
>      info("Tests ran. Cleaning up.");
>      closeDebugger(state, SimpleTest.finish);
>      return;
>    }
> -
> +  

nit: unnecessary spaces on this line
Attachment #8853312 - Flags: review?(dkeeler) → review+
I am seeing error when running the devtools/shared/webconsole/test/test_network_security.html

Buffered messages finished
0 INFO TEST-START | Shutdown
1 INFO Passed:  21
2 INFO Failed:  3
3 INFO Todo:    0
4 INFO Mode:    non-e10s
5 INFO SimpleTest FINISHED
The following tests failed:
56 INFO TEST-UNEXPECTED-FAIL | devtools/shared/webconsole/test/test_network_security.html | Strict Transport Security detected correctly. - got false, expected true
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:310:5
onSecurityInfo@chrome://mochitests/content/chrome/devtools/shared/webconsole/test/test_network_security.html:142:5
emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:110:7
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:86:38
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1322:29
emitReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1021:29
DevTools RDP*request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:717:21
getSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/client.js:552:12
onNetworkEventUpdate@chrome://mochitests/content/chrome/devtools/shared/webconsole/test/test_network_security.html:151:5
eventSource/proto.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:130:9
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1017:7
send/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:570:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
DevToolsUtils.executeSoon*exports.executeSoon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:42:19
send@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:563:9
send@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1481:5
receiveMessage@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:761:7
57 INFO TEST-UNEXPECTED-FAIL | devtools/shared/webconsole/test/test_network_security.html | Strict Transport Security detected correctly. - got false, expected true
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:310:5
onSecurityInfo@chrome://mochitests/content/chrome/devtools/shared/webconsole/test/test_network_security.html:142:5
emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:110:7
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:86:38
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1322:29
emitReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1021:29
DevTools RDP*request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:717:21
getSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/client.js:552:12
onNetworkEventUpdate@chrome://mochitests/content/chrome/devtools/shared/webconsole/test/test_network_security.html:151:5
eventSource/proto.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:130:9
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1017:7
send/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:570:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
DevToolsUtils.executeSoon*exports.executeSoon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:42:19
send@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:563:9
send@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1481:5
receiveMessage@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:761:7
58 INFO TEST-UNEXPECTED-FAIL | devtools/shared/webconsole/test/test_network_security.html | Public Key Pinning detected correctly. - got true, expected false
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:310:5
onSecurityInfo@chrome://mochitests/content/chrome/devtools/shared/webconsole/test/test_network_security.html:144:5
emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:110:7
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:86:38
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1322:29
emitReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1021:29
DevTools RDP*request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:717:21
getSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/client.js:552:12
onNetworkEventUpdate@chrome://mochitests/content/chrome/devtools/shared/webconsole/test/test_network_security.html:151:5
eventSource/proto.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:130:9
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1017:7
send/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:570:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
DevToolsUtils.executeSoon*exports.executeSoon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:42:19
send@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:563:9
send@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1481:5
receiveMessage@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:761:7
Buffered messages finished
SUITE-END | took 11s
Flags: needinfo?(jhao)
Forgot to add, I am on Win10

Honza
Hi Honza,

I don't have a Windows machine, but it looks fine on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93e03df98ec75ed6c105c7ad3d6b1e123405a440

Are you sure you've applied all the patches?
Flags: needinfo?(jhao)
Comment on attachment 8853312 [details]
Bug 1342178 - Regression tests for HSTS/HPKP information in private browsing window.

https://reviewboard.mozilla.org/r/125400/#review130910
Attachment #8853312 - Flags: review?(odvarko) → review+
Comment on attachment 8854762 [details]
Bug 1342178 - Combine test_network_security-{hsts/hpkp}.html.

https://reviewboard.mozilla.org/r/126712/#review130912
Attachment #8854762 - Flags: review?(odvarko) → review+
Comment on attachment 8854763 [details]
Bug 1342178 - Rename test_network_security-hpkp.html.

https://reviewboard.mozilla.org/r/126714/#review130914

LGTM!

All tests green! (it was my mistake previously)

Honza
Attachment #8854763 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #45)
> Comment on attachment 8854763 [details]
> Bug 1342178 - Rename test_network_security-hpkp.html.
> 
> https://reviewboard.mozilla.org/r/126714/#review130914
> 
> LGTM!
> 
> All tests green! (it was my mistake previously)
> 
> Honza

Thank you!
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db9b64a44012
Combine test_network_security-{hsts/hpkp}.html. r=Honza,keeler
https://hg.mozilla.org/integration/autoland/rev/4afce44bc2b9
Rename test_network_security-hpkp.html. r=Honza,keeler
https://hg.mozilla.org/integration/autoland/rev/f19289f0cfef
Regression tests for HSTS/HPKP information in private browsing window. r=Honza,keeler
https://hg.mozilla.org/integration/autoland/rev/7f9114ffbcae
Pass origin attributes to isSecureURI in devtools and browser. r=keeler,past
I tried to request longer timeout.
Flags: needinfo?(jhao)
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c844ec70bdb4
Combine test_network_security-{hsts/hpkp}.html. r=Honza,keeler
https://hg.mozilla.org/integration/autoland/rev/87912394d7d7
Rename test_network_security-hpkp.html. r=Honza,keeler
https://hg.mozilla.org/integration/autoland/rev/db96edf786de
Regression tests for HSTS/HPKP information in private browsing window. r=Honza,keeler
https://hg.mozilla.org/integration/autoland/rev/bc87945ba833
Pass origin attributes to isSecureURI in devtools and browser. r=keeler,past
Backed out for frequently timing out in test_network_security.html on Linux:

https://hg.mozilla.org/mozilla-central/rev/f914d40a48009c5acd1093e9939cc0ec035696dd
https://hg.mozilla.org/mozilla-central/rev/3d3e5d0a650bed912cadecc228d89976d826f86f
https://hg.mozilla.org/mozilla-central/rev/2e80094cf06e0a067373e52de98598a5da93c389
https://hg.mozilla.org/mozilla-central/rev/3210020c5e498e11fc52b4489cff5ec0ad0ed943

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=90411785&repo=autoland

[task 2017-04-11T09:28:06.379044Z] 09:28:06     INFO - Testing site with HSTS from this response, no Public Key Pinning
[task 2017-04-11T09:28:06.380060Z] 09:28:06     INFO - Cleaning up the previous window.
[task 2017-04-11T09:28:06.380808Z] 09:28:06     INFO - Buffered messages logged at 09:22:36
[task 2017-04-11T09:28:06.381901Z] 09:28:06     INFO - TEST-PASS | devtools/shared/webconsole/test/test_network_security.html | Strict Transport Security detected correctly. 
[task 2017-04-11T09:28:06.383052Z] 09:28:06     INFO - TEST-PASS | devtools/shared/webconsole/test/test_network_security.html | Public Key Pinning detected correctly. 
[task 2017-04-11T09:28:06.383907Z] 09:28:06     INFO - Testing site with stored HSTS from previous response, no Public Key Pinning
[task 2017-04-11T09:28:06.384712Z] 09:28:06     INFO - Cleaning up the previous window.
[task 2017-04-11T09:28:06.385530Z] 09:28:06     INFO - Buffered messages finished
[task 2017-04-11T09:28:06.386378Z] 09:28:06     INFO - TEST-UNEXPECTED-TIMEOUT | devtools/shared/webconsole/test/test_network_security.html | application timed out after 330 seconds with no output
Status: RESOLVED → REOPENED
Flags: needinfo?(jhao)
Resolution: FIXED → ---
It seems that sometimes in test cases we can't get any onNetworkEventUpdate so the test is just blocked.  I can reproduce intermittently on my Linux machine.  I tried cleaning up the network cache but it's not working.
Flags: needinfo?(jhao)
Can we consider landing the fix only first?  I think the intermittent test failures has nothing to do with this bug in devtools and might take a while to solve.
(In reply to Jonathan Hao [:jhao] from comment #63)
> Can we consider landing the fix only first?  I think the intermittent test
> failures has nothing to do with this bug in devtools and might take a while
> to solve.

Who was this question directed to?
Flags: needinfo?(jhao)
Flags: needinfo?(jhao) → needinfo?(dkeeler)
My concern with landing the fix without the test is that other pressing matters will come up and the test will never land. However, if it can be demonstrated that the issue is not with the changes in this test but rather the test framework itself or perhaps the interaction of the network console and private browsing, then it might be alright (i.e. if a completely new test that only opens a private browsing window and then the devtools and then closes them again (or whatever behavior is resulting in the timeouts) has intermittent timeouts, the problem is probably not with the changes here).
Flags: needinfo?(dkeeler)
Assignee: jonathan → nobody
Product: Firefox → DevTools
Severity: normal → --
status-firefox55: fix-optional → ---

Tested on Nightly, in a private Browsing Window and HTTP Strict Transport Security is correctly set to Enabled.
If this still happens for you, please reopen with updated Steps to reproduce.

Status: REOPENED → RESOLVED
Closed: 8 years ago → 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: