1237766 - Write a Marionette test that downloads malware and verifies that it's blocked.

Status: NEW

(Toolkit :: Safe Browsing, enhancement, P3)

Description

[François Marier [:francois]]

We should download the three files under "Download Warnings":

  https://testsafebrowsing.appspot.com/

and check that Firefox blocks them.

Comment 1

[François Marier [:francois]]

We probably want to fix bug 1237396 first since download protection will fail if the Safe Browsing lists are not available.

Comment 2

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

How reliable is this external website? For remote content we try to limit our access to internal servers. Means we have http://www.mozqa.com for this purpose. This is just to reduce test failures due to network latency.

Comment 3

[François Marier [:francois]]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> How reliable is this external website? For remote content we try to limit
> our access to internal servers. Means we have http://www.mozqa.com for this
> purpose. This is just to reduce test failures due to network latency.

It's run by the Google Safe Browsing team and I haven't seen it down yet. So probably reliable enough for us to use in our tests.

If it doesn't work, then we'll have to host our own fake "malware" file and then ask Google if they could flag it as bad on their end.

Comment 4

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

(In reply to François Marier [:francois] from comment #0)
> We should download the three files under "Download Warnings":
> 
>   https://testsafebrowsing.appspot.com/
> 
> and check that Firefox blocks them.

So even with the new test to check the initial safe browsing files have been downloaded, I wonder how we should organize the tests. I don't think it makes sense to have different test files, which would all operate on a new profile and have to download the files again. What I would suggest is the following:

* One single test file for safebrowsing
* A setup class method which is run once and actually checks for the files being present
* A setup test method to ensure we prepare the tests
* A new test for this particular issue (we can add more tests later)
* A teardown test method to clean up test artifacts
* A teardown class method for general clean-up and to restart with a fresh profile

Francois, does it sound fine with you? Matt, are you still interested to create this test?

Comment 5

[Matt Wobensmith [:mwobensmith][:matt:]]

Henrik, should the test from bug 1237396 be included in this somewhere? I think that any test of safe browsing would begin after verifying that the tables have been downloaded first.

I am committed to writing Marionette test(s) for both safe browsing (this) and tracking protection. Since they both rely on downloading these tables, they are related. We may or may not want to put them together.

Comment 6

[François Marier [:francois]]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> * One single test file for safebrowsing
> * A setup class method which is run once and actually checks for the files
> being present
> * A setup test method to ensure we prepare the tests
> * A new test for this particular issue (we can add more tests later)
> * A teardown test method to clean up test artifacts
> * A teardown class method for general clean-up and to restart with a fresh
> profile
> 
> Francois, does it sound fine with you? Matt, are you still interested to
> create this test?

That sounds good to me.

Comment 7

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

(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #5)
> Henrik, should the test from bug 1237396 be included in this somewhere? I
> think that any test of safe browsing would begin after verifying that the
> tables have been downloaded first.

As mentioned in my comment 4 and what Francois agreed on now is that we should get rid of the specific test from bug 1237396 and put the code in the setupClass method. It's the primary requirement for all safebrowsing tests and by doing that we can ensure that we have to wait only once. In regards of that I checked a couple of instances of that test being run on Taskcluster. The timing varies a bit but in general it looks like that the test is taking ~20s to run. So I really would like to see that we have to wait only once for any other safebrowsing test which depends on those downloaded files. 

> I am committed to writing Marionette test(s) for both safe browsing (this)
> and tracking protection. Since they both rely on downloading these tables,
> they are related. We may or may not want to put them together.

That's great! Let me know when you have questions and other blocking items. I'm happy to assist you through the creation of those tests.

Comment 8

[François Marier [:francois]]

I assume that Matt doesn't mind if someone else takes care of this.

Comment 9

[François Marier [:francois]]

We can use the official test file (https://testsafebrowsing.appspot.com/s/content.exe) as long as it's not hosted on a mozilla.org or mozilla.net domain.

Comment 10

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

Dimi, is this still something which would be needed?

Comment 11

[Dimi Lee [:dimi]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #10)

Dimi, is this still something which would be needed?

Yes, this is definitely something good to have.

Comment 12

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

Ok, so we should leave it open. Note that with bug 1573410 I'm going to remove the firefox-ui harness and tests, and existing safe browsing tests will end-up under /toolkit/components/url-classifier/tests/marionette. So it might be wise to wait with the implementation until that has been done.

Also note that someone from your team will have to work on that.

Comment 13

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

As first step we removed any tests from Firefox UI jobs that rely on locally hosted test files only. It means that firefox-ui test jobs only contain tests that require remote access to some external server. As such we cannot easily integrate them into Marionette because of restrictions to Tier2 on Treeherder. But that means whenever we want to get away with Firefox UI tests all of them will be run as part of another Mn job in CI.

Dimi, that means if someone from you team wants to work on such a test it could be done at any time. I'm happy to mentor if needed. For now the test would still have to be added under: https://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/functional/safebrowsing.

Comment 14

[Dimi Lee [:dimi]]

Hi Henrik,
Thank you for the information! We don't have a plan to add the new test really soon.
But I would like to know whether we plan to port all the reamining firefox-ui tests to Marionette tests, if yes, is there a deadline for the work?

Comment 15

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

There is no deadline and porting all the tests over is fully transparent to you. Note that right now these are 100% Marionette tests. The only thing that we would have to do is to actually move the tests into toolkit/components/url-classifier/tests/marionette and keep them running in a separate CI job (tier 2).