Closed Bug 1104022 Opened 10 years ago Closed 10 years ago

Create a page that loads insecure content while the page itself is secured

Categories

(Mozilla QA Graveyard :: Infrastructure, defect)

Product:
Mozilla QA Graveyard
Component:
Infrastructure
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

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

References

Details

(Whiteboard: [sprint])

Attachments

(1 file, 6 obsolete files)

patchv3.2.patch
5.98 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review 
We need to add a page to testcase-data, which will try to open unsecured content, to test that Firefox blocks the unsecured content.

A referenced page would be: https://people.mozilla.com/~bsterne/tests/62178/test.html
Attached patch 1104022.patch (obsolete) — — Splinter Review 
I took the code from the link above, I uncommented the other tests and tested them, I removed the delay, and served the pages via https so I could test all the steps from bug 1098351. 
Right now Firefox blocks the unsecured scripts, if the page is secured, loaded directly in page, in an iFrame or the style-sheet referenced from an unsecured address.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Attachment #8527695 - Flags: review?(andrei.eftimie)
Attachment #8527695 - Flags: review?(andreea.matei)
Comment on attachment 8527695 [details] [diff] [review]
1104022.patch

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

How will the test that references this page will assert if everything passed or not?
It would be great if it could have a a couple expect calls, so if something fails we know exactly which unsecured inclusion worked or not.

::: firefox/security/mixed_content_blocked/index.html
@@ +2,5 @@
> +<html>
> +  <head>
> +    <title>Test Mixed Content Blocking</title>
> +    <link rel="stylesheet" type="text/css" href="style/style.css" />
> +    <link rel="stylesheet" type="text/css" href="http://mozqa.com/data/firefox/security/mixed_content_blocked/style/insecure_style.css" />

You can remove the type attribute. The default is "text/css".

@@ +36,5 @@
> +      }
> +
> +      window.addEventListener("message", function(event) {
> +        log("Received message from " + event.origin + ": " + event.data);
> +        if (event.origin != "http://cosmin-malutan.github.io")

This can't be hardcoded to your github.io domain.

@@ +49,5 @@
> +      }, false);
> +
> +      function runTests() {
> +        // Test 1:
> +        // load the first insecure script after 1 second so we can test what

There's a reference here for a 1 second delay, but I can't find it. The original page had these wrapped in setTimeout
Attachment #8527695 - Flags: review?(andrei.eftimie)
Attachment #8527695 - Flags: review?(andreea.matei)
Attachment #8527695 - Flags: review-
Attached patch patch v1.0 (obsolete) — — Splinter Review 
(In reply to Andrei Eftimie from comment #2)
> 
> How will the test that references this page will assert if everything passed
> or not?
> It would be great if it could have a a couple expect calls, so if something
> fails we know exactly which unsecured inclusion worked or not.
That's what I had in mind when I added all cases. 

> > +      function runTests() {
> > +        // Test 1:
> > +        // load the first insecure script after 1 second so we can test what
> 
> There's a reference here for a 1 second delay, but I can't find it. The
> original page had these wrapped in setTimeout
The timeout was only to ease the visualization of blocking scripts, it shouldn't delay our tests so I removed.
Attachment #8527695 - Attachment is obsolete: true
Attachment #8529063 - Flags: review?(andrei.eftimie)
(In reply to Cosmin Malutan from comment #3)
> Created attachment 8529063 [details] [diff] [review]
> patch v1.0
> 
> (In reply to Andrei Eftimie from comment #2)
> > 
> > How will the test that references this page will assert if everything passed
> > or not?
> > It would be great if it could have a a couple expect calls, so if something
> > fails we know exactly which unsecured inclusion worked or not.
> That's what I had in mind when I added all cases. 

Good, but how will we test this? Should we save an array or object of all results so a test can run this file, import the results JSON and populate a few expect calls and a final assert?
I would check all results <span> tags for fail classes, this is for knowing exactly which entry failed. 
On a more general level the test is more about checking the notification and the doorhanger icon.
Comment on attachment 8529063 [details] [diff] [review]
patch v1.0

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

(In reply to Cosmin Malutan from comment #5)
> I would check all results <span> tags for fail classes, this is for knowing
> exactly which entry failed. 
> On a more general level the test is more about checking the notification and
> the doorhanger icon.

r+ from me with the following:

Then please also put some data on the elements (I see they are <div>'s actually), maybe an id.
So then when you query for each element in the test you know which one failed.

With that r+ from me.
Attachment #8529063 - Flags: review?(andrei.eftimie) → review+
Attached patch patch v2.0 (obsolete) — — Splinter Review 
I added the IDs for the div tags, that changes the classes when the content loads.
Attachment #8529063 - Attachment is obsolete: true
Attachment #8531193 - Flags: review?(mihaela.velimiroviciu)
Attachment #8531193 - Flags: review?(andreea.matei)
Comment on attachment 8531193 [details] [diff] [review]
patch v2.0

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

Looks good to me. Henrik, if you want to have a look, this is for the test in bug 1098351.
Attachment #8531193 - Flags: review?(mihaela.velimiroviciu)
Attachment #8531193 - Flags: review?(hskupin)
Attachment #8531193 - Flags: review?(andreea.matei)
Attachment #8531193 - Flags: review+
Comment on attachment 8531193 [details] [diff] [review]
patch v2.0

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

Something we really have to do is to not hard-code the location to mozqa.com. I mentioned it partly but would apply to all the instances.

::: firefox/security/mixed_content_blocked/iframe.html
@@ +4,5 @@
> +    <link rel="stylesheet" href="style/style.css" />
> +  </head>
> +  <body>
> +    <div class="test">
> +      Insecure script 3: <span id="result1">did not load</span>

Please default to "NOT LOADED" in all cases, so that we have a consistent naming for both cases.

@@ +9,5 @@
> +      <script>
> +        // load an insecure script from a secure iframe
> +        parent.log("Attemping to load insecure script 3.");
> +        var s = document.createElement("script");
> +        s.src = "http://mozqa.com/data/firefox/security/mixed_content_blocked/scripts/script3.js";

You cannot hard-code the URL. Please take the origin and strip off the 's' from 'https'.

::: firefox/security/mixed_content_blocked/index.html
@@ +2,5 @@
> +<html>
> +  <head>
> +    <title>Test Mixed Content Blocking</title>
> +    <link rel="stylesheet" href="style/style.css" />
> +    <link rel="stylesheet" href="http://mozqa.com/data/firefox/security/mixed_content_blocked/style/insecure_style.css" />

We should not hard-code this URL.

@@ +33,5 @@
> +        var log = document.getElementById("log");
> +        log.textContent = log.textContent + s + "\n";
> +      }
> +
> +      window.addEventListener("message", function(event) {

You should also remove the listener. I assume it is only triggered once anyway.

@@ +35,5 @@
> +      }
> +
> +      window.addEventListener("message", function(event) {
> +        log("Received message from " + event.origin + ": " + event.data);
> +        if (event.origin != "http://mozqa.com")

We cannot hard-code mozqa.com.

@@ +49,5 @@
> +
> +      function runTests() {
> +
> +        // Test 1:
> +        // load the first insecure script

Wrong comment

::: firefox/security/mixed_content_blocked/style/style.css
@@ +5,5 @@
> +  border: 1px solid #080;
> +  border-radius: 5px;
> +  min-width: 250px;
> +}
> +.fail {

nit: empty lines between CSS selector definitions.
Attachment #8531193 - Flags: review?(hskupin) → review-
Whiteboard: [sprint]
Attached patch patch v3.0 (obsolete) — — Splinter Review 
I fixed the nits.
I added style in the secure script and insecure one, so no JS code is needed to change the test 4 values, but I do that from css.

(In reply to Henrik Skupin (:whimboo) from comment #10)
> 
> Something we really have to do is to not hard-code the location to
> mozqa.com. I mentioned it partly but would apply to all the instances.
> > +        // load an insecure script from a secure iframe
> > +        parent.log("Attemping to load insecure script 3.");
> > +        var s = document.createElement("script");
> > +        s.src = "http://mozqa.com/data/firefox/security/mixed_content_blocked/scripts/script3.js";
> 
> You cannot hard-code the URL. Please take the origin and strip off the 's'
> from 'https'.
I can't just replace the protocol of location.origin as I have to handle the first directory too, this "data" for mozqa, repository name on github("testcase-data"), so I take the current directory address and I replace the protocol.

This is the page for testing: 
https://cosmin-malutan.github.io/testcase-data/firefox/security/mixed_content_blocked/index.html
Attachment #8531193 - Attachment is obsolete: true
Attachment #8534326 - Flags: review?(hskupin)
Comment on attachment 8534326 [details] [diff] [review]
patch v3.0

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

This updated patch didn't went through Andreea's review first.
Attachment #8534326 - Flags: review?(hskupin) → review?(andreea.matei)
Comment on attachment 8534326 [details] [diff] [review]
patch v3.0

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

Looks good to me. Tested manually and with the test in the other bug.
Attachment #8534326 - Flags: review?(hskupin)
Attachment #8534326 - Flags: review?(andreea.matei)
Attachment #8534326 - Flags: review+
Comment on attachment 8534326 [details] [diff] [review]
patch v3.0

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

Looks way better. Simply fix the nits and we are good to go.

::: firefox/security/mixed_content_blocked/iframe.html
@@ +2,5 @@
> +<html>
> +  <head>
> +    <script>
> +      var baseAddress = window.location.href.replace("https", "http");
> +      var insecureAddress = baseAddress.slice(0, baseAddress.lastIndexOf("/"));

I think you mixed up the wording here. The first variable is the insecureURL, while the second is the baseURL.

::: firefox/security/mixed_content_blocked/index.html
@@ +23,5 @@
> +
> +    <pre id="log"></pre>
> +    <script>
> +      var baseAddress = window.location.href.replace("https", "http");
> +      var insecureAddress = baseAddress.slice(0, baseAddress.lastIndexOf("/"));

Same here regarding mixed names.
Attachment #8534326 - Flags: review?(hskupin) → review+
Attached patch patch v3.1 (obsolete) — — Splinter Review 
Thanks Henrik!
Attachment #8534326 - Attachment is obsolete: true
Attachment #8539266 - Flags: review?(andreea.matei)
Comment on attachment 8539266 [details] [diff] [review]
patch v3.1

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

What I understood in Henrik's review is not about the name itself, from baseAddress to baseURL, but that they should be switched.

>var baseAddress = window.location.href.replace("https", "http");
> +      var insecureAddress = baseAddress.slice(0, baseAddress.lastIndexOf("/"));
> I think you mixed up the wording here. The first variable is the insecureURL, while the second is the baseURL.

So the one you replace https with http is the insecureURL and the second creates the baseURL. Please switch them.
Attachment #8539266 - Flags: review?(andreea.matei) → review-
Attached patch patchv3.2.patch (obsolete) — — Splinter Review 
(In reply to Andreea Matei [:AndreeaMatei] from comment #16)
> So the one you replace https with http is the insecureURL and the second
> creates the baseURL. Please switch them.

Switched the variables names.
Attachment #8539266 - Attachment is obsolete: true
Attachment #8539281 - Flags: review?(andreea.matei)
Attached patch patchv3.2.patch — — Splinter Review 
(In reply to Teodor Druta from comment #17)
> Created attachment 8539281 [details] [diff] [review]
> patchv3.2.patch
> 
> (In reply to Andreea Matei [:AndreeaMatei] from comment #16)
> > So the one you replace https with http is the insecureURL and the second
> > creates the baseURL. Please switch them.
> 
> Switched the variables names.

Sorry about that, I forgot to switch them in index.html.
Attachment #8539281 - Attachment is obsolete: true
Attachment #8539281 - Flags: review?(andreea.matei)
Attachment #8539282 - Flags: review?(andreea.matei)
Comment on attachment 8539282 [details] [diff] [review]
patchv3.2.patch

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

http://hg.mozilla.org/qa/testcase-data/rev/1805f21bb580 (testcase-data)
Attachment #8539282 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: