1098351 - Add new automated test for "Override mixed script content blocking"

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

https://moztrap.mozilla.org/manage/case/8881/

1 Pre-requisites: have security.mixedcontent.blockactive_content set to true
 > Launch Firefox.
2 Load a page with mixed script content (e.g. https://people.mozilla.com/~bsterne/tests/62178/test.html)
 > The lock icon should be displayed in the location bar. The script shouldn't load.
3 Click the shield icon.
4 Click the arrow next to Keep blocking
5 Select Disable protection on this page
 > The yellow triangle icon should be displayed in the location bar.The Insecure script should be loaded.
6 Reload the page.
 > The shield and lock icons shouldn't be displayed again and the script should be loaded."

This needs to access remote resources so it's a mozmill test candidate.

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

This patch is still blocked by bug 1104022, but I used the testing page from git-hub.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8531995 [details] [diff] [review]
patch v1.0

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

Misses a commit message too.

::: firefox/tests/remote/testSecurity/testMixedScriptContentBlocking.js
@@ +9,5 @@
> +
> +var browser = require("../../../lib/ui/browser");
> +
> +const TEST_DATA = {
> +  url: "https://cosmin-malutan.github.io/testcase-data/firefox/security/mixed_content_blocked/index.html",

Maybe you can update it so it's in final state to land. Apply the patch in the other bug first and use here accordingly.

@@ +35,5 @@
> +  aModule.locationBar = aModule.browserWindow.navBar.locationBar;
> +
> +  aModule.targetPanel = null;
> +
> +  aModule.browserWindow.tabs.closeAllTabs();

The prerequisites mention to make sure we have security.mixed_content.block_active_content pref set to true. I see it is by default, but maybe we want to add a check too, in case that changes.

@@ +79,5 @@
> +  }, {type: "notification", open: false});
> +
> +  checkProtectionEnabled(false);
> +
> +  // After a reload, the scripts should still not be loaded

Which is the right behavior here? Last step says " > The shield and lock icons shouldn't be displayed again and the script should be loaded."
I manually checked and I see on reload the shield icon but it's crossed with a red line and the triangle (so no lock icon ideed). The script on Insecure says LOADED, but the page is in red, not green.
The comment needs to be updated.

@@ +86,5 @@
> +
> +  checkProtectionEnabled(false);
> +}
> +
> +function checkProtectionEnabled(aState) {

JS doc please.

@@ +105,5 @@
> +  assert.waitFor(() => {
> +    var faviconImage = utils.getElementStyle(favicon, "list-style-image");
> +
> +    return faviconImage.indexOf(iconParts) !== -1;
> +  }, "There correct identity icon is displayed.");

nit: The

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

(In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> @@ +35,5 @@
> > +  aModule.locationBar = aModule.browserWindow.navBar.locationBar;
> > +
> > +  aModule.targetPanel = null;
> > +
> > +  aModule.browserWindow.tabs.closeAllTabs();
> 
> The prerequisites mention to make sure we have
> security.mixed_content.block_active_content pref set to true. I see it is by
> default, but maybe we want to add a check too, in case that changes.
The preference was for enabling the feature while it was in development state. I see no reason for checking if the pref is set. If it will change later on then the test will fail anyway. If we check if the pref is set, and in case is not we set it inside of test then we might miss the disabling of feature.

I made all other changes.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8531995 [details] [diff] [review]
patch v1.0

># HG changeset patch
># User Cosmin Malutan <[e-mail]>
># Date 1417690342 -7200
>#      Thu Dec 04 12:52:22 2014 +0200
># Node ID 4d1632bab15f5e546665f54636867c77ae71e865
># Parent  655bcf44d8a7ae2569a7984260cecad9c5a59fe3
>[mq]: 1098351
>
>diff --git a/firefox/tests/remote/testSecurity/manifest.ini b/firefox/tests/remote/testSecurity/manifest.ini
>--- a/firefox/tests/remote/testSecurity/manifest.ini
>+++ b/firefox/tests/remote/testSecurity/manifest.ini
>@@ -1,14 +1,15 @@
> [parent:../manifest.ini]
> 
> [testDVCertificate.js]
> [testGreenLarry.js]
> [testMD5HashSignature.js]
> [testMixedContentPage.js]
>+[testMixedScriptContentBlocking.js]
> [testSSLDisabledErrorPage.js]
> [testSafeBrowsingNotificationBar.js]
> disabled = Bug 1062330 - waitForPageLoad failures
> [testSafeBrowsingWarningPages.js]
> [testSecurityInfoViaMoreInformation.js]
> [testSecurityNotification.js]
> [testSSLStatusAfterRestart.js]
> disabled = Bug 1088551 - Test failure 'Panel is in the correct state - 'closed' should equal 'open''
>diff --git a/firefox/tests/remote/testSecurity/testMixedScriptContentBlocking.js b/firefox/tests/remote/testSecurity/testMixedScriptContentBlocking.js
>new file mode 100644
>--- /dev/null
>+++ b/firefox/tests/remote/testSecurity/testMixedScriptContentBlocking.js
>@@ -0,0 +1,117 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+"use strict";
>+
>+// Include necessary modules
>+var utils = require("../../../../lib/utils");
>+
>+var browser = require("../../../lib/ui/browser");
>+
>+const TEST_DATA = {
>+  url: "https://cosmin-malutan.github.io/testcase-data/firefox/security/mixed_content_blocked/index.html",
>+  tests: [{
>+    id: "test1",
>+    description: "Insecure script from iFrame"
>+  }, {
>+    id: "test2",
>+    description: "Insecure script one"
>+  }, {
>+    id: "test3",
>+    description: "Insecure script two"
>+  }, {
>+    id: "test4",
>+    description: "Insecure plugin"
>+  }, {
>+    id: "test5",
>+    description: "Insecure stylesheet"
>+  }]
>+};
>+
>+function setupModule(aModule) {
>+  aModule.browserWindow = new browser.BrowserWindow();
>+  aModule.controller = aModule.browserWindow.controller;
>+  aModule.locationBar = aModule.browserWindow.navBar.locationBar;
>+
>+  aModule.targetPanel = null;
>+
>+  aModule.browserWindow.tabs.closeAllTabs();
>+}
>+
>+function teardownModule(aModule) {
>+  if (aModule.targetPanel) {
>+    aModule.targetPanel.getNode().hidePopup();
>+  }
>+}
>+
>+/**
>+ * Test warning about viewing a mixed content page
>+ */
>+function testMixedContentPage() {
>+  controller.open(TEST_DATA.url);
>+  controller.waitForPageLoad();
>+
>+  checkProtectionEnabled(true);
>+
>+  // Open 'Bad Content Notification'
>+  locationBar.waitForNotificationPanel(aPanel => {
>+    targetPanel = aPanel;
>+    var popupBox = locationBar.getElement({type: "notification_element",
>+                                           subtype: "notification-popup-box"});
>+    popupBox.click();
>+  }, {type: "notification"});
>+
>+  // Disable the protection
>+  locationBar.waitForNotificationPanel(() => {
>+    var buttonLabel = browserWindow.getEntity("mixedContentBlocked2.options");
>+    var optionsButton = locationBar.getNotificationElement(
>+      "bad-content-notification",  {type: "label", value: buttonLabel}
>+    );
>+    optionsButton.click();
>+
>+    var itemLabel = browserWindow.getEntity("mixedContentBlocked2.unblock.label");
>+    var menuItem = locationBar.getNotificationElement(
>+      "bad-content-notification",
>+      {type: "label", value: itemLabel}
>+    );
>+    menuItem.click();
>+  }, {type: "notification", open: false});
>+
>+  checkProtectionEnabled(false);
>+
>+  // After a reload, the scripts should still not be loaded
>+  locationBar.reload();
>+  controller.waitForPageLoad();
>+
>+  checkProtectionEnabled(false);
>+}
>+
>+function checkProtectionEnabled(aState) {
>+  var className =  (aState) ? "test" : "test fail";
>+  var iconParts = (aState) ? "identity-icons-https.png"
>+                           : "identity-icons-https-mixed-active.png";
>+  var state = (aState) ? "blocked" : "unblocked";
>+
>+  // Check for the shield icon
>+  var badContentIcon = locationBar.getElement({type: "notificationIcon",
>+                                               subtype: "bad-content-" + state});
>+  assert.waitFor(() => badContentIcon.exists() &&
>+                       utils.isDisplayed(controller, badContentIcon),
>+                 "The shield notification icon has been displayed.");
>+
>+  // Check the identity icon
>+  var favicon = locationBar.getElement({type: "favicon"});
>+  assert.waitFor(() => {
>+    var faviconImage = utils.getElementStyle(favicon, "list-style-image");
>+
>+    return faviconImage.indexOf(iconParts) !== -1;
>+  }, "There correct identity icon is displayed.");
>+
>+  // Check elements form page for the correct class name
>+  TEST_DATA.tests.forEach((test) => {
>+    var element = findElement.ID(controller.tabs.activeTab, test.id);
>+    assert.waitFor(() => (element.getNode().getAttribute("class") === className),
>+                   test.description + " has been " + state + ".");
>+  });
>+}

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8532522 [details] [diff] [review]
patch v2.0

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

To me it looks good.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8532522 [details] [diff] [review]
patch v2.0

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

This needs an update with the changes in the mozqa page - depending bug 1104022.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

I updated the patch so it's applies, in the dependent bug we changed the id's so it needed an update and also for elements I check for color so I can check for the insecure style too.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8539332 [details] [diff] [review]
patch v3.0

Henrik could you give a feedback for this new test before Andreea provides a review?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8539332 [details] [diff] [review]
patch v3.0

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

f+ given that it looks generally fine.

::: firefox/tests/remote/testSecurity/testMixedScriptContentBlocking.js
@@ +78,5 @@
> +  checkProtectionEnabled(false);
> +
> +  // After a reload, the scripts should still load
> +  locationBar.reload();
> +  controller.waitForPageLoad();

In comment 0 we have "The shield and lock icons shouldn't be displayed again and the script should be loaded.". But I still see that as also mentioned by Andreea but not answered yet. Is that a problem with the moztrap test? If yes, it would need an update.

@@ +98,5 @@
> +
> +  // Check for the shield icon
> +  var badContentIcon = locationBar.getElement({type: "notificationIcon",
> +                                               subtype: "bad-content-" + state});
> +  assert.waitFor(() => badContentIcon.exists() &&

nit: no need for this check anymore since you updated isDisplayed with it.

@@ +114,5 @@
> +  // Check elements form page for the correct class name
> +  TEST_DATA.tests.forEach((test) => {
> +    var element = findElement.ID(controller.tabs.activeTab, test.id);
> +    assert.waitFor(() => utils.getElementStyle(element, "color") === color,
> +                   test.description + " has been " + state + ".");

You do not check for the class name here but for the color. Can you please clarify what should be really done here?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, do we have to reset the setting for this page, so blocking would be enabled if it is used by another test?

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> > +  // After a reload, the scripts should still load
> > +  locationBar.reload();
> > +  controller.waitForPageLoad();
> 
> In comment 0 we have "The shield and lock icons shouldn't be displayed again
> and the script should be loaded.". But I still see that as also mentioned by
> Andreea but not answered yet. Is that a problem with the moztrap test? If
> yes, it would need an update.
The moztrap is correct, it's just a little bit confusing, when the protection is enabled the script does not load, when the protection is disabled the scripts does load. Also when we reload the page after we disabled the protection the scrip should still load. 
 
> @@ +114,5 @@
> > +  // Check elements form page for the correct class name
> > +  TEST_DATA.tests.forEach((test) => {
> > +    var element = findElement.ID(controller.tabs.activeTab, test.id);
> > +    assert.waitFor(() => utils.getElementStyle(element, "color") === color,
> > +                   test.description + " has been " + state + ".");
> 
> You do not check for the class name here but for the color. Can you please
> clarify what should be really done here?
Here I just have to update the comment in test, I already explained the change in comment 7, but as I look back it might be hard to understand, so:
When we load an insecure style-sheet link we can't change the classes or ids but plainly the style of the element, so what happened before was that we had a javascript code that checked for the stile and the then we set the class. That was bad for many reasons, one we had to add an asyncrinous js call to check for the style in page and set the class, but we had no event or notification that the style-sheet page has been loaded and applied; furthermore we had to add an wait for check in test itself to check for the class. So I decided to check for the style directly in test, and change the class(it changes the style) for the nodes that get's updated by the insecure scripts.     

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Oh, do we have to reset the setting for this page, so blocking would be
> enabled if it is used by another test?
If we refresh the page the protection state is kept, whatsoever, if we close the page and navigate again to this page the protection is reset, so no additional steps for cleanup are needed.  

If all is good I'll update the nit and the comment! Thanks!

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

> (In reply to Henrik Skupin (:whimboo) from comment #10)
> If we refresh the page the protection state is kept, whatsoever, if we close
> the page and navigate again to this page the protection is reset, so no
> additional steps for cleanup are needed.

Sounds good then! Also for the other answered questions.

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

Thanks for review Henrik, handing over to Andreea!

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8543947 [details] [diff] [review]
patch v3.1

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

::: firefox/tests/remote/testSecurity/testMixedScriptContentBlocking.js
@@ +84,5 @@
> +  checkProtectionEnabled(false);
> +}
> +
> +/**
> + * Check whatever the mixed content script protection is enabled or not

..whether the mixed..

@@ +86,5 @@
> +
> +/**
> + * Check whatever the mixed content script protection is enabled or not
> + *
> + * @param aState

type of aState?

@@ +109,5 @@
> +
> +    return faviconImage.indexOf(iconParts) !== -1;
> +  }, "The correct identity icon is displayed.");
> +
> +  // Check the color of elements from page, the color depends on whatever the 

"Check the color of the page elements", "whether" and a trailing whitespace

@@ +111,5 @@
> +  }, "The correct identity icon is displayed.");
> +
> +  // Check the color of elements from page, the color depends on whatever the 
> +  // scripts are blocked or not
> +  TEST_DATA.tests.forEach((test) => {

aTest

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.2

Fixed those nits, thanks.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.3

Ops, wrong param type.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8544600 [details] [diff] [review]
patch v3.3

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

::: firefox/tests/remote/testSecurity/testMixedScriptContentBlocking.js
@@ +114,5 @@
> +  // scripts are blocked or not
> +  TEST_DATA.tests.forEach((aTest) => {
> +    var element = findElement.ID(controller.tabs.activeTab, aTest.id);
> +    assert.waitFor(() => utils.getElementStyle(element, "color") === color,
> +                   test.description + " has been " + state + ".");

aTest.description

You should run a test after updating patches, this is failing :)

Comment 18

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

Ops, sorry for that, here it's the update.

http://mozmill-crowd.blargon7.com/#/remote/report/04e54d608fc589a28217304fe67f736e

Comment 19

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8545225 [details] [diff] [review]
patch v4.0

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

http://hg.mozilla.org/qa/mozmill-tests/rev/6bcbd51de6f2 (default)
We want this down to beta for the qa testing team.

Comment 20

[Cosmin Malutan, [:cosmin-malutan]]

This ran nicely with the latest Nightly, let's back-port it to aurora and beta.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0 aurora

There is a conflict in manifest on aurora. 
http://mozmill-crowd.blargon7.com/#/remote/report/5bb4668cfa6af53a0ba505835d040c1b

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8546597 [details] [diff] [review]
patch v4.0 aurora

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

I'll have this done with the merge today. Lets check beta next.

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

Andreea, the patch above applies on beta and runs well.

Comment 24

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8546597 [details] [diff] [review]
patch v4.0 aurora

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

http://hg.mozilla.org/qa/mozmill-tests/rev/9c19d8e422fd (beta)