1333483 - Populate test flash blocking lists with test page addresses

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robin Steuber (she/her) [:bytesized]]

To allow for quick manual testing of Safe Browsing, the test lists are populated with URLs that can be visited to easily check behavior (see [1]). The same functionality should be added for list based Flash blocking.

This requires populating the test lists with these URLs and adding HTML and Flash assets to https://github.com/mozilla/itisatrap

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/url-classifier/SafeBrowsing.jsm#330-395

Comment 1

[Robin Steuber (she/her) [:bytesized]]

PR submitted for the HTML and Flash assets: https://github.com/mozilla/itisatrap/pull/22

Comment 2

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

Review commit: https://reviewboard.mozilla.org/r/107650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107650/

Comment 3

[François Marier [:francois]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

https://reviewboard.mozilla.org/r/107650/#review108794

::: toolkit/components/url-classifier/SafeBrowsing.jsm:381
(Diff revision 1)
>  
> +    const flashDenyURL = "flashblock.itisatrap.org/";
> +    const flashDenyExceptURL = "except.flashblock.itistrap.org/";
> +    const flashAllowURL = "flashallow.itistrap.org/";
> +    const flashAllowExceptURL = "except.flashallow.itistrap.org/";
> +    const flashThirdPartyURL = "flashthirdparty.itistrap.org";

Missing trailing slash.

::: toolkit/components/url-classifier/SafeBrowsing.jsm:382
(Diff revision 1)
> +    const flashDenyURL = "flashblock.itisatrap.org/";
> +    const flashDenyExceptURL = "except.flashblock.itistrap.org/";
> +    const flashAllowURL = "flashallow.itistrap.org/";
> +    const flashAllowExceptURL = "except.flashallow.itistrap.org/";
> +    const flashThirdPartyURL = "flashthirdparty.itistrap.org";
> +    const flashThirdPartyExceptURL = "except.flashthirdparty.itistrap.org";

ditto

Comment 4

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

https://reviewboard.mozilla.org/r/107650/#review108794

> Missing trailing slash.

Good catch

Comment 5

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107650/diff/1-2/

Comment 6

[François Marier [:francois]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

https://reviewboard.mozilla.org/r/107650/#review108800

::: toolkit/components/url-classifier/SafeBrowsing.jsm:382
(Diff revision 2)
> +    const flashDenyURL = "flashblock.itisatrap.org/";
> +    const flashDenyExceptURL = "except.flashblock.itistrap.org/";
> +    const flashAllowURL = "flashallow.itistrap.org/";
> +    const flashAllowExceptURL = "except.flashallow.itistrap.org/";
> +    const flashThirdPartyURL = "flashthirdparty.itistrap.org/";
> +    const flashThirdPartyExceptURL = "except.flashthirdparty.itistrap.org/";

`itisatrap.org` (missing A everywhere except the first one)

Comment 7

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107650/diff/2-3/

Comment 8

[François Marier [:francois]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

https://reviewboard.mozilla.org/r/107650/#review109002

Comment 9

[Robin Steuber (she/her) [:bytesized]]

@francois I had two questions related to this that I think you might be able to answer.

First, I believe that there is no problem merging this before Bug 1307604 (which adds the tables referenced in this bug). Is that correct?

Second, after |addMozEntries()| is called, are the entries added tables added regardless of whether the tables were added in |readPrefs()| and |controlUpdateChecking()|?
Really what I want to know is this: If |SafeBrowsing.flashBlockEnabled = false| (so |listManager.enableUpdate| was not called and |listManager.disableUpdate| may have been called on the lists), will the test URLs added in |addMozEntries()| match when |Classify()| is called? What about if |SafeBrowsing.flashBlockEnabled| is then set to |true| and |listManager.enableUpdate| is called (but |addMozEntries()| is not called again). Will |Classifiy()| return those URLs then?

Comment 10

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107650/diff/3-4/

Comment 11

[François Marier [:francois]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

https://reviewboard.mozilla.org/r/107650/#review110484

::: toolkit/components/url-classifier/SafeBrowsing.jsm:381
(Diff revisions 3 - 4)
>  
>      const flashDenyURL = "flashblock.itisatrap.org/";
>      const flashDenyExceptURL = "except.flashblock.itisatrap.org/";
>      const flashAllowURL = "flashallow.itisatrap.org/";
>      const flashAllowExceptURL = "except.flashallow.itisatrap.org/";
> -    const flashThirdPartyURL = "flashthirdparty.itisatrap.org/";
> +    const flashSubDocURL = "flashsubdocument.itisatrap.org/";

You're using `subdoc` everywhere else. Maybe the test domains should use that too?

Comment 12

[François Marier [:francois]]

(In reply to Kirk Steuber [:bytesized] from comment #9)
> Second, after |addMozEntries()| is called, are the entries added tables
> added regardless of whether the tables were added in |readPrefs()| and
> |controlUpdateChecking()|?

As far as I know, the test entries will be added regardless of whether or not the feature is enabled. The list entries of course won't have any effect.

These test tables are not associated with an update URL so they won't be scheduled to be updated again.

Comment 13

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107650/diff/4-5/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3360defc879
Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module r=francois

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3360defc879

Comment 16

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Shield study will need to restart the browser in order to work correctly
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: bug 1307604
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds single pref to the list of prefs SafeBrowsing tracks.
[String changes made/needed]: none

Comment 17

[Robin Steuber (she/her) [:bytesized]]

Oops! Pasted in the wrong thing for one question:

[User impact if declined]: Shield study will lack easy manual testing that this patch provides.

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

For shield study purpose. Aurora53+.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9e47c4ce5e1a

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

add URLs for flash blocking manual testing, beta52+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4603dc332d7f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Would have been nice if the UI test permafail this patch caused was called out in the Beta uplift (bug 1333303) since we clearly knew about it by the time approvals were requested.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Looks like the fix from bug 1333303 needs more rebasing than I'm willing to attempt and anybody with the knowledge for doing so is already long-gone for the weekend. Backed out.

https://hg.mozilla.org/releases/mozilla-beta/rev/f29a51aaae32824ec1202953991cdb098f25b056

Comment 24

[Robin Steuber (she/her) [:bytesized]]

Once Bug 1333303 has been uplifted, this should be uplifted as well please.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/30d276f390d3

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/30d276f390d3