875456 - Log mixed content messages from the Mixed Content Blocker to the Security Pane in the Web Console

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[Garrett Robinson [:grobinson]]

When mixed content is blocked, an error message is logged to the Security pane in the Web Console due to Bug 837351. Earlier work in Bug 737873 warns web developers about mixed content by appending a [Mixed Content] tag to mixed content resources that are loaded in the Network pane, which links to documentation about mixed content. 737873 achieves this in a hacky way, by examining network events in the webconsole code, comparing their protocol of the requested resource to that of the parent document, and modifying the logged network message if it is mixed content.

Now that we have the security pane (Bug 837351), it makes more sense to log messages about mixed content to it. We will continue to log errors for blocked mixed content, and can now log warnings about mixed content that was not blocked. We can do this directly from inside the Mixed Content Blocker, since it is now on by default.

Two open questions are:

1. Should we continue to emit the [Mixed Content] warning tag in the Network pane? On the one hand, it might give better visibility to mixed content errors. On the other, removing this would allow us to remove all of the hacky code added to the web console for this purpose.
2. Can we still provide an informative link to documentation in the warning message? This may require a custom hook in the web console code (see http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#1294 for the current implementation). This might be a feature that we'd like to extend to other types of security errors as well.

Comment 1

[Garrett Robinson [:grobinson]]

The current solution for mixed content warnings in the Network Pane is in makeMixedContentNode in webconsole.js (currently http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#1368).

Comment 3

[Tanvi Vyas[:tanvi]]

Right now, mixed content loads show up in the Net panel of the webconsole because of a hack implemented before nsMixedContentBlocker existed (in bug 737873) there are some cases that are not caught.

I just came across one such case where the favicon doesn't show up as mixed content in the console on https://www.netaddress.com/tpl/Door/Login?Domain=usa.net

Favicon url: http://www.netaddress.com/favicon.ico

Perhaps favicons don't appear in the Net panel?

Comment 4

[Ivan Alagenchev :ialagenchev]

Attachment: 762593.patch

The patch for 875456 depends on this on

Comment 5

[Ivan Alagenchev :ialagenchev]

Attachment: 846918.patch

this patch depends on the attached patch for 762593

Comment 6

[Ivan Alagenchev :ialagenchev]

Attachment: 875456.patch

Uploaded the wrong file on the previous upload...

This patch depends on the patch for 762593.

Comment 7

[Tanvi Vyas[:tanvi]]

Comment on attachment 782849 [details] [diff] [review]
875456.patch

>From: Ivan Alagenchev <[e-mail]>
>patch for 875456
>
>diff --git a/browser/devtools/webconsole/webconsole.js b/browser/devtools/webconsole/webconsole.js
>index d5f97b4..71cfb10 100644
>--- a/browser/devtools/webconsole/webconsole.js
>+++ b/browser/devtools/webconsole/webconsole.js
>@@ -1463,17 +1433,17 @@ WebConsoleFrame.prototype = {
>     // Create the actual insecure passwords warning node and make it clickable

Remove the words insecure passwords

>     let warningNode = this.document.createElement("label");
>     warningNode.setAttribute("value", moreInfoLabel);
>     warningNode.setAttribute("title", moreInfoLabel);
>     warningNode.classList.add("hud-clickable");
>     warningNode.classList.add("webconsole-learn-more-link");
> 
>     warningNode.addEventListener("click", function(aEvent) {
>-      this.owner.openLink(INSECURE_PASSWORDS_LEARN_MORE);
>+      this.owner.openLink(aURL);
>       aEvent.preventDefault();
>       aEvent.stopPropagation();
>     }.bind(this));
> 
>     linkNode.appendChild(warningNode);
>   },
> 
>   /**



>diff --git a/content/base/src/nsMixedContentBlocker.cpp b/content/base/src/nsMixedContentBlocker.cpp
>index 8b6360f..dc0b0ca 100644
>--- a/content/base/src/nsMixedContentBlocker.cpp
>+++ b/content/base/src/nsMixedContentBlocker.cpp
>@@ -514,16 +534,24 @@ nsMixedContentBlocker::ShouldLoad(uint32_t aContentType,
>           eventSink->OnSecurityChange(aRequestingContext, (State | nsIWebProgressListener::STATE_BLOCKED_MIXED_ACTIVE_CONTENT));
>        }
>        return NS_OK;
>     }
> 
>   } else {
>     // The content is not blocked by the mixed content prefs.
> 
>+    // Check to see if we have mixed content that we aren't blocking
>+    // and log a message that we are loading mixed content.

Change comment; we've already checked to see if we have mixed content.
// Log message indicating that mixed content is being loaded on the page.

>+    if (classification == eMixedDisplay) {
>+      LogMixedContentMessage(classification, aContentLocation, rootDoc, false);
>+    } else if (classification == eMixedScript) {
>+      LogMixedContentMessage(classification, aContentLocation, rootDoc, false);
>+    }
>+

You are passing in the classification, so you don't need to check the classification with the if/else.
LogMixedContentMessage(classification, aContentLocation, rootDoc, false);

>     // Fire the event from a script runner as it is unsafe to run script
>     // from within ShouldLoad
>     nsContentUtils::AddScriptRunner(
>       new nsMixedContentEvent(aRequestingContext, classification));
>     return NS_OK;
>   }
> 
>   *aDecision = REJECT_REQUEST;

You need to call LogMixedContentMessage in a couple more places.  At http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#456 and http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#474 you need:
LogMixedContentMessage(classification, aContentLocation, rootDoc, false);

Comment 8

[Tanvi Vyas[:tanvi]]

Thinking about this more, the mochitest will only test allow Mixed Content messages, since both the mixed active and mixed display prefs are set to false.  Maybe we should have a second test where the prefs are set to true (or toggle the pref between true and false in the same test) and look for mixed blocked messages too.

If we want to really cover all our bases (which may be overkill) we could also test that when a user "disables protection" they get the logged messages.  That would test the two places where I mentioned we need to add a LogMixedContentMessage() call in comment 7.

Comment 9

[Ivan Alagenchev :ialagenchev]

Attachment: 875456.patch

updating the patch to reflect Tanvi's comments

Comment 10

[Ivan Alagenchev :ialagenchev]

(In reply to Tanvi Vyas [:tanvi] from comment #8)
> Thinking about this more, the mochitest will only test allow Mixed Content
> messages, since both the mixed active and mixed display prefs are set to
> false.  Maybe we should have a second test where the prefs are set to true
> (or toggle the pref between true and false in the same test) and look for
> mixed blocked messages too.
> 
> If we want to really cover all our bases (which may be overkill) we could
> also test that when a user "disables protection" they get the logged
> messages.  That would test the two places where I mentioned we need to add a
> LogMixedContentMessage() call in comment 7.

Yes, I think there should be a mochitest for each particular case. I'm adding an uploaded patch that addresses your comments anyways so that Garrett can take a look at this initial version in the meantime.

Comment 11

[Ivan Alagenchev :ialagenchev]

Attachment: 875456.patch

Uploading a new patch with mochitests covering all cases included

Comment 12

[Tanvi Vyas[:tanvi]]

Comment on attachment 783357 [details] [diff] [review]
875456.patch

>From: Ivan Alagenchev <[e-mail]>
>patch for 875456


>diff --git a/browser/devtools/webconsole/test/browser_webconsole_bug_875456_mixedcontent_securityerrors.js b/browser/devtools/webconsole/test/browser_webconsole_bug_875456_mixedcontent_securityerrors.js
>new file mode 100644
>index 0000000..a0d4942
>--- /dev/null
>+++ b/browser/devtools/webconsole/test/browser_webconsole_bug_875456_mixedcontent_securityerrors.js
>@@ -0,0 +1,141 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+/*
>+ * The test loads a web page with mixed active and display content
>+ * on it while the "block mixed content" settins are _off_.
Add a "g" to settings.

>+ * It then checks that the loading mixed content warning messages
>+ * are logged to the console and have the correct "Learn More"
>+ * url appended to them. Once we are done testing the loading of mixed content,
>+ * we turn blocking on and test for blocked messages and for manually
>+ * disabling MCB via the doorhanger click.
>+ */
>+
>+const TEST_URI = "https://example.com/browser/browser/devtools/webconsole/test/test-bug-875456-mixedcontent-securityerrors.html";
>+const LEARN_MORE_URI = "https://developer.mozilla.org/en/Security/MixedContent";
>+
>+function test()
>+{
>+  SpecialPowers.pushPrefEnv({'set': [["security.mixed_content.block_active_content",false],
>+                            ["security.mixed_content.block_display_content", false]]}, loadingMixedContentTest);
>+}
Change to loadingMixedContentTest1 (since it's the first test)

>+
>+function checkLoadingMixedContentMessages(aIsDoorHangerTest)
>+{
I think this function needs to be reworked a little, because the flow of this mochitest is confusing.  Perhaps have one function (runTests) that does runs all 3 mochitests consecutively.  Take out the logic for individual tests and put them into different functions.

Change aIsDoorHangerTest to aIsMixedContentOverrideTest3 (3 for the third test) (assuming it is still needed after the refactor).

>+  openConsole(null, function testLoadingMixedContent(hud) {
>+    waitForMessages({
>+      webconsole: hud,
>+      messages: [
>+        {
>+        name: "Logged blocking mixed active content",
>+        text: "Loading mixed (insecure) active content on a secure"+
>+              " page \"http://example.com/\"",
>+        category: CATEGORY_SECURITY,
>+        severity: SEVERITY_WARNING
>+      },
>+      {
>+        name: "Logged blocking mixed passive content - image",
>+        text: "Loading mixed (insecure) display content on a secure page"+
>+              " \"http://example.com/tests/image/test/mochitest/blue.png\"",
>+        category: CATEGORY_SECURITY,
>+        severity: SEVERITY_WARNING
>+      },
>+      ],
>+    }).then( () =>{
>+      testClickOpenNewTab(hud)
Can you move this to a different function?

>+      if(aIsDoorHangerTest) {
Change to if(MixedContentOverrideTest)

>+        // We are done testing -> finish the test.
Make a comment that we are done with all 3 tests.

>+        finishTest();
>+      } else {
>+        // Activate MCB and test that we log the blocked mixed content
>+        // messages.
>+        SpecialPowers.pushPrefEnv({
>+          'set': [["security.mixed_content.block_active_content",true],
>+                 ["security.mixed_content.block_display_content", true]]
>+        }, blockingMixedContentTest);
Change to blockingMixedContentTest2

>+      }
>+    });
>+  });
>+}
>+
>+function loadingMixedContentTest()
loadingMixedContentTest1

>+{
>+  addTab(TEST_URI);
>+  browser.addEventListener("load", function onLoad(aEvent) {
>+    browser.removeEventListener(aEvent.type, onLoad, true);
>+    openConsole(null, function testSecurityErrorLogged (hud) {
>+      content.location = TEST_URI;
Why do you need to set content.location here?

>+      checkLoadingMixedContentMessages(false);
>+    });
>+  }, true);
>+}
>+
>+function blockingMixedContentTest()
blockingMixedContentTest2

>+{
>+    addTab(TEST_URI);
>+    browser.addEventListener("load", function onLoad(aEvent) {
>+      browser.removeEventListener(aEvent.type, onLoad, true);
>+      openConsole(null, function testSecurityErrorLogged (hud) {
>+        content.location = TEST_URI;
Why do you need to set content.location here?

>+        waitForMessages({
>+          webconsole: hud,
>+          messages: [
>+            {
>+            name: "Logged blocking mixed active content",
>+            text: "Blocked loading mixed active content \"http://example.com/\"",
>+            category: CATEGORY_SECURITY,
>+            severity: SEVERITY_ERROR
>+          },
>+          {
>+            name: "Logged blocking mixed passive content - image",
>+            text: "Blocked loading mixed active content \"http://example.com/\"",
>+            category: CATEGORY_SECURITY,
>+            severity: SEVERITY_ERROR
>+          },
>+          ],
>+        }).then(()=>{
>+          testClickOpenNewTab(hud);
>+          testDoorHangerClick();
change to MixedContentOverrideTest3

>+        });
>+      });
>+    }, true);
>+}
>+
>+function testDoorHangerClick()
change to MixedContentOverrideTest3

>+{
>+  addTab(TEST_URI);
>+  browser.addEventListener("load", function onLoad(aEvent) {
>+    browser.contentWindow.location = TEST_URI;
Why do you need this?

>+    browser.removeEventListener(aEvent.type, onLoad, true);
>+
>+    // Clicking on doorhanger will cause us to reload the page.
>+    // I want to validate the messages after the reload, so I
>+    // need to register the test as a listener for "load" event.
>+    browser.addEventListener(aEvent.type, checkLoadingMixedContentMessages, true);
>+
>+    var notification = PopupNotifications.getNotification("mixed-content-blocked", browser);
>+    ok(notification, "Mixed Content Doorhanger didn't appear");
>+    notification.secondaryActions[0].callback();
>+  }, true);
>+}
This function needs to be reworked.  You already have an open tab with Mixed Content blocked, so you just need to use the last few lines of this function to "disable protection" and then check if the loading messages appear in the webconsole.

Comment 13

[Ivan Alagenchev :ialagenchev]

As per offline discussion, we decided to go back to having the blocking and the non-blocking tests split into separate files, since that appears to be the cleanest approach.

Comment 14

[Ivan Alagenchev :ialagenchev]

Attachment: 875456.patch

Please note that I haven't been able to test this patch, because of bug 901293. 
Test failures are possible because of failures related to bug 900077 and bug 900026

Comment 15

[Ivan Alagenchev :ialagenchev]

Attachment: 762593.patch

Comment 16

[Tanvi Vyas[:tanvi]]

Comment on attachment 785474 [details] [diff] [review]
875456.patch

>From: Ivan Alagenchev <[e-mail]>
>patch for 875456

>diff --git a/browser/devtools/webconsole/test/browser_webconsole_bug_875456_block_mixedcontent_securityerrors.js b/browser/devtools/webconsole/test/browser_webconsole_bug_875456_block_mixedcontent_securityerrors.js

>+function blockMixedContentTest1()
>+{
>+  addTab(TEST_URI);
>+  browser.addEventListener("load", function onLoad(aEvent) {
>+    browser.removeEventListener(aEvent.type, onLoad, true);
>+    openConsole(null, function testSecurityErrorLogged (hud) {
>+      waitForMessages({
>+        webconsole: hud,
>+        messages: [
>+          {
>+            name: "Logged blocking mixed active content",
>+            text: "Blocked loading mixed active content \"http://example.com/\"",
>+            category: CATEGORY_SECURITY,
>+            severity: SEVERITY_ERROR
>+          },
>+          {
>+            name: "Logged blocking mixed passive content - image",
>+            text: "Blocked loading mixed active content \"http://example.com/\"",
>+            category: CATEGORY_SECURITY,
>+            severity: SEVERITY_ERROR
>+          },
>+        ],
>+      }).then( () => {
>+        testClickOpenNewTab(hud);
>+        // Call the second (MCB override) test.
>+        mixedContentOverrideTest2(hud);
>+      });
>+    });
>+  }, true);
>+}

Will testClickOpenNewTab(hud) complete before mixedContentOverrideTest2() is called?  mixedContentOverrideTest2() will reload the page, and hence clear the webconsole.  The Mixed Content Loading message you were trying to click may be gone by the time you try and click it.

Comment 17

[Ivan Alagenchev :ialagenchev]

(In reply to Tanvi Vyas [:tanvi] from comment #16)
> Comment on attachment 785474 [details] [diff] [review]
> 875456.patch
> 
> Will testClickOpenNewTab(hud) complete before mixedContentOverrideTest2() is
> called?  mixedContentOverrideTest2() will reload the page, and hence clear
> the webconsole.  The Mixed Content Loading message you were trying to click
> may be gone by the time you try and click it.

I think so. The problem with special powers is that the callback is called async. The call to testClickOpenNewTab is sync, so we should be safe.

Comment 18

[Garrett Robinson [:grobinson]]

Comment on attachment 785474 [details] [diff] [review]
875456.patch

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

::: browser/devtools/webconsole/test/browser_webconsole_bug_875456_allow_mixedcontent_securityerrors.js
@@ +13,5 @@
> +
> +function test()
> +{
> +  SpecialPowers.pushPrefEnv({'set': [["security.mixed_content.block_active_content",false],
> +                            ["security.mixed_content.block_display_content", false]]}, loadingMixedContentTest);

Nit: I would reindent this as follows:

  SpecialPowers.pushPrefEnv({'set':
      [["security.mixed_content.block_active_content",false],
       ["security.mixed_content.block_display_content", false]
  ]}, loadingMixedContentTest);

@@ +28,5 @@
> +        messages: [
> +          {
> +          name: "Logged mixed active content",
> +          text: "Loading mixed (insecure) active content on a secure"+
> +            " page \"http://example.com/\"",

Don't split string literals across lines; it makes it hard to grep for them.

::: browser/devtools/webconsole/test/test-bug-875456-mixedcontent-securityerrors.html
@@ +1,2 @@
> +<!DOCTYPE HTML>
> +<html dir="ltr" xml:lang="en-US" lang="en-US">i

Nit: i?

@@ +6,5 @@
> +    <script src="testscript.js"></script>
> +    <!--
> +       - Any copyright is dedicated to the Public Domain.
> +       - http://creativecommons.org/publicdomain/zero/1.0/
> +       -->

Nit: indentation. Line this up with the start comment marker and get rid of the leading -'s on the actual comment text. The comment text should be indented one level in from the comment markers.

@@ +9,5 @@
> +       - http://creativecommons.org/publicdomain/zero/1.0/
> +       -->
> +  </head>
> +  <body>
> +    <iframe src = "http://example.com"></iframe>

Nit: spaces surrounding '=' are unnecessary. Remove them.

@@ +11,5 @@
> +  </head>
> +  <body>
> +    <iframe src = "http://example.com"></iframe>
> +    <img src =
> +        "http://example.com/tests/image/test/mochitest/blue.png"></img>

Nit: The 80-character line limit does not apply to HTML because it is often nested deeply. HTML formatting is very subjective, but in my opinion this should all be on one line.

::: browser/devtools/webconsole/webconsole.js
@@ +1401,2 @@
>      // We have a single category for now, but more are to be
>      // expected soon

Remove this comment; multiple categories have arrived!

@@ +1415,5 @@
>     * as a parameter to the function. When a user clicks on the appended
>     * warning node, the browser navigates to a page where the user can learn
> +   * more about security issues associated with the specific message being
> +   * logged. The page url corresponds to the url passed in as the second
> +   * parameter of the function.

This is a good comment, but rather than explaining the function parameters in prose, you should use the @-syntax you used for addMoreInfoLink.

::: content/base/src/nsMixedContentBlocker.cpp
@@ +155,3 @@
>  {
> +  nsAutoCString messageTag;
> +  uint32_t scriptErrorFlag;

messageSeverityFlag or similar is a better name for this, as there are two possible flags and one if this is also called "errorFlag".

::: dom/locales/en-US/chrome/security/security.properties
@@ +14,5 @@
>  InsecurePasswordsPresentOnPage=Password fields present on an insecure (http://) page. This is a security risk that allows user login credentials to be stolen.
>  InsecureFormActionPasswordsPresent=Password fields present in a form with an insecure (http://) form action. This is a security risk that allows user login credentials to be stolen.
>  InsecurePasswordsPresentOnIframe=Password fields present on an insecure (http://) iframe. This is a security risk that allows user login credentials to be stolen.
> +LoadingMixedActiveContent=Loading mixed (insecure) active content on a secure page "%1$S"
> +LoadingMixedDisplayContent=Loading mixed (insecure) display content on a secure page "%1$S"

Do we need to say (insecure) here?

Comment 19

[Ivan Alagenchev :ialagenchev]

> 
> Nit: The 80-character line limit does not apply to HTML because it is often
> nested deeply. HTML formatting is very subjective, but in my opinion this
> should all be on one line.
What do you mean by "nested deeply"?
> >  InsecurePasswordsPresentOnPage=Password fields present on an insecure (http://) page. This is a security risk that allows user login credentials to be stolen.
> >  InsecureFormActionPasswordsPresent=Password fields present in a form with an insecure (http://) form action. This is a security risk that allows user login credentials to be stolen.
> >  InsecurePasswordsPresentOnIframe=Password fields present on an insecure (http://) iframe. This is a security risk that allows user login credentials to be stolen.
> > +LoadingMixedActiveContent=Loading mixed (insecure) active content on a secure page "%1$S"
> > +LoadingMixedDisplayContent=Loading mixed (insecure) display content on a secure page "%1$S"
> 
> Do we need to say (insecure) here?

Adding back some information from irc discussion here ... We agreed that "insecure" is useful for developers who are not familiar with mixed content and don't realize that mixed content is a security risk.

Comment 20

[Ivan Alagenchev :ialagenchev]

I decided to change the name of the "messageTag" variable in nsMixedContentBlocker to messageLookupKey since I believe that describes its meaning better. Afterall that serves as the lookup key in the localization file. I am planning on changing that in nsISecurityConsoleMessage.idl as part of bug 89740. 
I've seen this referred to as "message tag" and "message name" throughout the code base and I've found the inconsistency confusing. "message name", while debate-ably accurate doesn't describe the purpose of this variable and "message tag" is vague at best. Therefore, I've decided to add yet a third naming to the same meaning and calling it "messageLookupKey", which I believe adds both clarity and meaning to the purpose of the variable.

Comment 21

[Ivan Alagenchev :ialagenchev]

The above comment was meant to refer to bug 897240

Comment 22

[Ivan Alagenchev :ialagenchev]

Attachment: 875456_DOM.patch

Comment 23

[Ivan Alagenchev :ialagenchev]

Attachment: 875456_TOOLKIT.patch

Just removes code for checking if url is secure that bug 737873 used.

Comment 24

[Ivan Alagenchev :ialagenchev]

Attachment: 875456_DEVTOOLS.patch

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787609 [details] [diff] [review]
875456_DOM.patch

>+LogMixedContentMessage(MixedContentTypes aClassification,
>                         nsIURI* aContentLocation,

Please fix the indent.

>+                        bool aIsBlockedMessage)

I'd weakly prefer an enum here, not a boolean.

Is there a reason this function is not static?

   } else if (sBlockMixedScript && classification == eMixedScript) {
...
     if (allowMixedContent) {
+      LogMixedContentMessage(classification, aContentLocation, rootDoc, false);

Please fix indent.

r=me

Comment 26

[Mihai Sucan [:msucan]]

Comment on attachment 787612 [details] [diff] [review]
875456_DEVTOOLS.patch

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

Thanks for the patch. This is looking good but I do have two concerns: it adds [learn more] to all script errors (this seems to be a bug), and it also changes all of the network logs into log type, no more network warnings for mixed content. Is this desirable? Do we want to show messages for every mixed content resource that is allowed/blocked?

More comments below.

::: browser/devtools/webconsole/test/Makefile.in
@@ +146,5 @@
>  	browser_console_navigation_marker.js \
>  	browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js \
>  	browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js \
> +	browser_webconsole_bug_875456_allow_mixedcontent_securityerrors.js \
> +	browser_webconsole_bug_875456_block_mixedcontent_securityerrors.js \

Please remove the bug number and mention it in the test description comment.

@@ +253,5 @@
>  	test-consoleiframes.html \
>  	test-iframe1.html \
>  	test-iframe2.html \
>  	test-iframe3.html \
> +	test-bug-875456-mixedcontent-securityerrors.html \

Please remove the bug number.

::: browser/devtools/webconsole/test/browser_webconsole_bug_875456_allow_mixedcontent_securityerrors.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/*
> + * The test loads a web page with mixed active and display content
> + * on it while the "block mixed content" settins are _off_.

typo: settings.

nit: for consistency with other tests, please use // instead of /*... (here and in other places)

@@ +13,5 @@
> +
> +function test()
> +{
> +  SpecialPowers.pushPrefEnv({'set':
> +      [["security.mixed_content.block_active_content",false],

nits: please use double quotes, and space before |false|.

@@ +40,5 @@
> +          category: CATEGORY_SECURITY,
> +          severity: SEVERITY_WARNING
> +        },
> +        ],
> +      }).then( () => testClickOpenNewTab(hud));

nit: remove space after |then(|.

@@ +47,5 @@
> +}
> +
> +function testClickOpenNewTab(hud) {
> +  let warningNode = hud.outputNode.querySelector(
> +    ".webconsole-msg-body .webconsole-learn-more-link");

nit: why not just query for .webconsole-learn-more-link? in a single line

::: browser/devtools/webconsole/test/browser_webconsole_bug_875456_block_mixedcontent_securityerrors.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/*
> + * The test loads a web page with mixed active and display content
> + * on it while the "block mixed content" settins are _on_.

typo: settings

... and the rest of comments from the other test.

::: browser/devtools/webconsole/webconsole.js
@@ +1392,5 @@
>     *        The script error object that we are reporting to the console
>     */
>    addMoreInfoLink: function WCF_addMoreInfoLink(aNode, aScriptError)
>    {
> +    var url;

s/var/let/

@@ +1397,5 @@
>      if (aScriptError.category == "Insecure Password Field") {
> +      url = INSECURE_PASSWORDS_LEARN_MORE;
> +    } else if (aScriptError.category == "Mixed Content Message" ||
> +               aScriptError.category == "Mixed Content Blocker") {
> +      url = MIXED_CONTENT_LEARN_MORE;

What happens if this method is invoked for an unknown category?

Comment 27

[Ivan Alagenchev :ialagenchev]

> 
> I'd weakly prefer an enum here, not a boolean.

I changed the code to use enums
 
> Is there a reason this function is not static?
No. I changed it to static.

Thank you for the review.

Comment 28

[Tanvi Vyas[:tanvi]]

(In reply to Mihai Sucan [:msucan] from comment #26)
> Comment on attachment 787612 [details] [diff] [review]
> 875456_DEVTOOLS.patch
> 
> Review of attachment 787612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch. This is looking good but I do have two concerns: it
> adds [learn more] to all script errors (this seems to be a bug), and it also
> changes all of the network logs into log type, no more network warnings for
> mixed content. Is this desirable? Do we want to show messages for every
> mixed content resource that is allowed/blocked?
> 

Hi Mihai,

I'm not sure about adding "learn more" to all script errors.  I will let Ivan speak to that.  But for the network log change, we decided that instead of showing the mixed content warning in two places (the net pane and the security pane), we should only show it in the security pane.  The actual request should still show up in the net pane (as it did before we even introduced a mixed content warning to the webconsole), but just without the red color and mixed content link.

For every mixed content load or block, we should show a message in the Security pane.

Thanks!

Comment 29

[Ivan Alagenchev :ialagenchev]

(In reply to Tanvi Vyas [:tanvi] from comment #28)

Tanvi, thank you for the info about the network messages.

> Hi Mihai,
> 
> I'm not sure about adding "learn more" to all script errors.  I will let
> Ivan speak to that. 


Mihai is right(thank you Mihai). I am changing the logic to address that and will reply to the rest of his concerns after I am done addressing them.

Comment 30

[Ivan Alagenchev :ialagenchev]

(In reply to Tanvi Vyas [:tanvi] from comment #28)
> (In reply to Mihai Sucan [:msucan] from comment #26)
> > Comment on attachment 787612 [details] [diff] [review]
> > 875456_DEVTOOLS.patch

I just had a discussion on IRC to clarify what Mihai meant by "and it also changes all of the network logs into log type" and he mentioned that mixed content network messages used to be SEVERITY_WARNING and now after backing out Tanvi's MCB related code in webconsole.js, the severity changed to SEVERITY_LOG. Is this what we want, or should we keep it at SEVERITY_WARNING?

Comment 31

[Ivan Alagenchev :ialagenchev]

(In reply to Mihai Sucan [:msucan] from comment #26)
> Comment on attachment 787612 [details] [diff] [review]
> 875456_DEVTOOLS.patch
> 
> @@ +146,5 @@
> >  	browser_console_navigation_marker.js \
> >  	browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js \
> >  	browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js \
> > +	browser_webconsole_bug_875456_allow_mixedcontent_securityerrors.js \
> > +	browser_webconsole_bug_875456_block_mixedcontent_securityerrors.js \
> 
> Please remove the bug number and mention it in the test description comment.

The bug numbers are very useful when wanting to list all files in the directory that are related to a particular bug. This is also the naming convention adopted by the other files in the test directory. 

> @@ +1397,5 @@
> >      if (aScriptError.category == "Insecure Password Field") {
> > +      url = INSECURE_PASSWORDS_LEARN_MORE;
> > +    } else if (aScriptError.category == "Mixed Content Message" ||
> > +               aScriptError.category == "Mixed Content Blocker") {
> > +      url = MIXED_CONTENT_LEARN_MORE;
> 
> What happens if this method is invoked for an unknown category?

Something that shouldn't happen :-). I am adding an
else {
   return;
}

to bail out without adding the link node.

Comment 32

[Tanvi Vyas[:tanvi]]

(In reply to Ivan Alagenchev :ialagenchev from comment #30)
> (In reply to Tanvi Vyas [:tanvi] from comment #28)
> > (In reply to Mihai Sucan [:msucan] from comment #26)
> > > Comment on attachment 787612 [details] [diff] [review]
> > > 875456_DEVTOOLS.patch
> 
> I just had a discussion on IRC to clarify what Mihai meant by "and it also
> changes all of the network logs into log type" and he mentioned that mixed
> content network messages used to be SEVERITY_WARNING and now after backing
> out Tanvi's MCB related code in webconsole.js, the severity changed to
> SEVERITY_LOG. Is this what we want, or should we keep it at SEVERITY_WARNING?

Yes, we would go back to using SEVERITY_LOG.  That is what all the network entries were before my patch and so that is what they should go back to now. (https://bugzilla.mozilla.org/attachment.cgi?id=639774&action=diff#a/browser/devtools/webconsole/HUDService.jsm_sec3)

Comment 33

[Mihai Sucan [:msucan]]

(In reply to Tanvi Vyas [:tanvi] from comment #28)
> Hi Mihai,
> 
> I'm not sure about adding "learn more" to all script errors.  I will let
> Ivan speak to that.  But for the network log change, we decided that instead
> of showing the mixed content warning in two places (the net pane and the
> security pane), we should only show it in the security pane.  The actual
> request should still show up in the net pane (as it did before we even
> introduced a mixed content warning to the webconsole), but just without the
> red color and mixed content link.
> 
> For every mixed content load or block, we should show a message in the
> Security pane.

This is fine with me then. However, do note that this means that if the user chooses to only see network requests she will not see any indication that something went wrong or that there might be something wrong. Shouldn't we have something for such cases?

On a related note, should we have warnings and error messages for mixed content network requests in the network monitor? As a web dev, I would expect to see such warnings when I click a network request in the netmonitor.

Comment 34

[Mihai Sucan [:msucan]]

(In reply to Ivan Alagenchev :ialagenchev from comment #31)
> (In reply to Mihai Sucan [:msucan] from comment #26)
> > Comment on attachment 787612 [details] [diff] [review]
> > 875456_DEVTOOLS.patch
> > 
> > @@ +146,5 @@
> > >  	browser_console_navigation_marker.js \
> > >  	browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js \
> > >  	browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js \
> > > +	browser_webconsole_bug_875456_allow_mixedcontent_securityerrors.js \
> > > +	browser_webconsole_bug_875456_block_mixedcontent_securityerrors.js \
> > 
> > Please remove the bug number and mention it in the test description comment.
> 
> The bug numbers are very useful when wanting to list all files in the
> directory that are related to a particular bug. This is also the naming
> convention adopted by the other files in the test directory. 

True, but we are trying to avoid adding more tests with bug numbers in their file names. Thanks!

Comment 35

[Ivan Alagenchev :ialagenchev]

(In reply to Mihai Sucan [:msucan] from comment #34)
> True, but we are trying to avoid adding more tests with bug numbers in their
> file names. Thanks!

Alright, they are going to get removed then.

Comment 36

[Tanvi Vyas[:tanvi]]

(In reply to Mihai Sucan [:msucan] from comment #33)
> This is fine with me then. However, do note that this means that if the user
> chooses to only see network requests she will not see any indication that
> something went wrong or that there might be something wrong. Shouldn't we
> have something for such cases?
> 
> On a related note, should we have warnings and error messages for mixed
> content network requests in the network monitor? As a web dev, I would
> expect to see such warnings when I click a network request in the netmonitor.

Mihai, Ivan and I discussed this on Wednesday.  And we haven't quite come to a conclusion, but wanted to investigate some things.

* If we loaded mixed content appeared in both the Net and Security consoles, in which cases would the two report inconsistent data?

* How difficult/easy/hacky would it be to properly alert the Net console about loaded mixed content (as opposed to the hack that is in place today).

Comment 37

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #36)
> * If we loaded mixed content appeared in both the Net and Security consoles,
> in which cases would the two report inconsistent data?
> 

Looked into this bit and found the following.  There may be other cases that I haven't found, but this should give us a good idea.

Cases the Net panel misses that the Security Panel catches:

-Favicons don't show up in the Net panel.  Mixed Content favicon loads will be reported in the Security panel but not the Net panel.

-If an http page has an https iframe that contains mixed content, the mixed content loaded will not be flagged by the Net panel but will be flagged by the security panel.  This is because top.location is http and that is what we check for in the Net panel hack. (Example: http://people.mozilla.com/~tvyas/mixediframe2.html)

-When mixed content occurs on a page that was created with document.open, the mixed content load would is not flagged in the Net panel but is flagged in the security panel.  Ex: https://people.mozilla.com/~tvyas/mixeddocument.html.  Similarly for a window.open (https://people.mozilla.com/~tvyas/windowopen3.html)

Cases the Security panel misses that the Net panel catches:

-MCB does not properly handle redirects (bug 878890).  Until that bug is fixed ->
If an https resource is redirected to an http resource, the Net panel will properly identify the resource as a mixed content load.  The security panel will not.

Comment 38

[Justin Dolske [:Dolske]]

Comment on attachment 787611 [details] [diff] [review]
875456_TOOLKIT.patch

No reason to keep this as a separate patch, just roll it into the same devtools patch Mihai is reviewing.

Comment 39

[Ivan Alagenchev :ialagenchev]

We are going to discuss how to move forward with the MCB messages in the Net panel during our security engineering meeting on Thursday. In the mean time, I've put the code for logging to the Net panel back in. There is no need to hold up the security pane logging until the Net pane logging is fully fixed. Additionally the correct fix for the Net panel might not be feasible until I land bug 897240, therefore we will proceed forward with the security pane messages and punt on the net pane for now. I'll open a separate bug for that after Thursday's meeting.

Comment 40

[Ivan Alagenchev :ialagenchev]

Attachment: 875456_DEVTOOLS.patch

new devtools patch with comments from reviews addressed and net pane messages back in.

Comment 41

[Ivan Alagenchev :ialagenchev]

Attachment: 875456_DOM.patch

Comment 42

[Mihai Sucan [:msucan]]

Comment on attachment 793640 [details] [diff] [review]
875456_DEVTOOLS.patch

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

Patch looks good. Thanks for the update! r+

::: browser/devtools/webconsole/test/browser_webconsole_allow_mixedcontent_securityerrors.js
@@ +9,5 @@
> +// Bug 875456 - Log mixed content messages from the Mixed Content
> +// Blocker to the Security Pane in the Web Console
> +
> +const TEST_URI = "https://example.com/browser/browser/devtools/webconsole/test/test-mixedcontent-securityerrors.html";
> +const LEARN_MORE_URI = "https://developer.mozilla.org/en/Security/MixedContent";

just a reminder that this URL needs to be adjusted to not include /en/. (you plan to do this in a separate bug?)

::: browser/devtools/webconsole/webconsole.js
@@ +1450,3 @@
>      if (aScriptError.category == "Insecure Password Field") {
> +      url = INSECURE_PASSWORDS_LEARN_MORE;
> +    } else if (aScriptError.category == "Mixed Content Message" ||

nit: }\nelse if {

Comment 43

[Ivan Alagenchev :ialagenchev]

(In reply to Mihai Sucan [:msucan] from comment #42)
> Comment on attachment 793640 [details] [diff] [review]
> 875456_DEVTOOLS.patch
> 
> Review of attachment 793640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. Thanks for the update! r+
> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_allow_mixedcontent_securityerrors.js
> @@ +9,5 @@
> > +// Bug 875456 - Log mixed content messages from the Mixed Content
> > +// Blocker to the Security Pane in the Web Console
> > +
> > +const TEST_URI = "https://example.com/browser/browser/devtools/webconsole/test/test-mixedcontent-securityerrors.html";
> > +const LEARN_MORE_URI = "https://developer.mozilla.org/en/Security/MixedContent";
> 
> just a reminder that this URL needs to be adjusted to not include /en/. (you
> plan to do this in a separate bug?)

Yes, I will update all of the LEARN_MORE urls that we use as part of bug 902691. I'll also change the if else block to a switch block.

Comment 44

[Ivan Alagenchev :ialagenchev]

Thanks for the r+

Comment 45

[Ivan Alagenchev :ialagenchev]

During Thursday's discussion, we decided to go with Mihai's suggestion to keep the Mixed Content messages in the Net Pane as well. I created bug 908703 for changing that code to use the same logic as MCB, or nsISecurityConsoleService notifications for determining if a resource is Mixed Content.

Comment 46

[Ivan Alagenchev :ialagenchev]

Attachment: 875456_DEVTOOLS.patch

Comment 47

[Ivan Alagenchev :ialagenchev]

Attachment: 875456_DOM.patch

Comment 48

[Ivan Alagenchev :ialagenchev]

Here is a tbpl run: https://tbpl.mozilla.org/?tree=Try&rev=8cf38b102fd8

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acd9c818e61c
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d4867eeb1f

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/acd9c818e61c
https://hg.mozilla.org/mozilla-central/rev/02d4867eeb1f