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

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

Tracking

unspecified

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed, firefox37 fixed)

Details

(Whiteboard: [sprint])

Attachments

(2 attachments, 6 obsolete attachments)

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.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Whiteboard: [sprint]
(Assignee)

Updated

4 years ago
Depends on: 1104022
(Assignee)

Comment 1

4 years ago
Created attachment 8531995 [details] [diff] [review]
patch v1.0

This patch is still blocked by bug 1104022, but I used the testing page from git-hub.
Attachment #8531995 - Flags: review?(mihaela.velimiroviciu)
Attachment #8531995 - Flags: review?(andreea.matei)
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
Attachment #8531995 - Flags: review?(mihaela.velimiroviciu)
Attachment #8531995 - Flags: review?(andreea.matei)
Attachment #8531995 - Flags: review-
(Assignee)

Comment 3

4 years ago
Created attachment 8532522 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8532522 - Flags: review?(mihaela.velimiroviciu)
Attachment #8532522 - Flags: review?(andreea.matei)
(Assignee)

Comment 4

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

># HG changeset patch
># User Cosmin Malutan <cosmin.malutan@softvision.ro>
># 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 + ".");
>+  });
>+}
Attachment #8531995 - Attachment is obsolete: true
Comment on attachment 8532522 [details] [diff] [review]
patch v2.0

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

To me it looks good.
Attachment #8532522 - Flags: review?(mihaela.velimiroviciu) → review+
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.
Attachment #8532522 - Flags: review?(andreea.matei)
Attachment #8532522 - Flags: review-
Attachment #8532522 - Flags: review+
(Assignee)

Comment 7

4 years ago
Created attachment 8539332 [details] [diff] [review]
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.
Attachment #8532522 - Attachment is obsolete: true
Attachment #8539332 - Flags: review?(mihaela.velimiroviciu)
Attachment #8539332 - Flags: review?(andreea.matei)
(Assignee)

Comment 8

4 years ago
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?
Attachment #8539332 - Flags: feedback?(hskupin)
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?
Attachment #8539332 - Flags: feedback?(hskupin) → feedback+
Oh, do we have to reset the setting for this page, so blocking would be enabled if it is used by another test?
(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!
> (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.
Created attachment 8543947 [details] [diff] [review]
patch v3.1

Thanks for review Henrik, handing over to Andreea!
Attachment #8539332 - Attachment is obsolete: true
Attachment #8539332 - Flags: review?(mihaela.velimiroviciu)
Attachment #8539332 - Flags: review?(andreea.matei)
Attachment #8543947 - Flags: review?(andreea.matei)
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
Attachment #8543947 - Flags: review?(andreea.matei) → review-
Created attachment 8544597 [details] [diff] [review]
patch v3.2

Fixed those nits, thanks.
Attachment #8543947 - Attachment is obsolete: true
Attachment #8544597 - Flags: review?(andreea.matei)
Created attachment 8544600 [details] [diff] [review]
patch v3.3

Ops, wrong param type.
Attachment #8544597 - Attachment is obsolete: true
Attachment #8544597 - Flags: review?(andreea.matei)
Attachment #8544600 - Flags: review?(andreea.matei)
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 :)
Attachment #8544600 - Flags: review?(andreea.matei) → review-
Created attachment 8545225 [details] [diff] [review]
patch v4.0

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

http://mozmill-crowd.blargon7.com/#/remote/report/04e54d608fc589a28217304fe67f736e
Attachment #8544600 - Attachment is obsolete: true
Attachment #8545225 - Flags: review?(andreea.matei)
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.
Attachment #8545225 - Flags: review?(andreea.matei) → review+
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → fixed
This ran nicely with the latest Nightly, let's back-port it to aurora and beta.
Flags: needinfo?(andreea.matei)
Created attachment 8546597 [details] [diff] [review]
patch v4.0 aurora

There is a conflict in manifest on aurora. 
http://mozmill-crowd.blargon7.com/#/remote/report/5bb4668cfa6af53a0ba505835d040c1b
Attachment #8546597 - Flags: review?(andreea.matei)
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.
Attachment #8546597 - Flags: review?(andreea.matei)
Andreea, the patch above applies on beta and runs well.
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)
Attachment #8546597 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox35: affected → fixed
status-firefox36: affected → fixed
Flags: needinfo?(andreea.matei)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.