Populate test flash blocking lists with test page addresses

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bytesized, Assigned: bytesized)

Tracking

(Blocks 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(1 attachment)

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
Blocks: 1277346
Depends on: 1333487
PR submitted for the HTML and Flash assets: https://github.com/mozilla/itisatrap/pull/22
Attachment #8830993 - Flags: review?(francois)
Depends on: 1334350

Comment 3

2 years ago
mozreview-review
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
Attachment #8830993 - Flags: review?(francois) → review-
Assignee

Comment 4

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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)
Attachment #8830993 - Flags: review?(francois) → review-
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
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
Attachment #8830993 - Flags: review?(francois) → review+
@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?
Flags: needinfo?(francois)
Depends on: 1335475
Comment hidden (mozreview-request)
Attachment #8830993 - Flags: review+ → review?(francois)

Comment 11

2 years ago
mozreview-review
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?
Attachment #8830993 - Flags: review?(francois) → review+
(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.
Flags: needinfo?(francois)
Comment hidden (mozreview-request)

Comment 14

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c3360defc879
Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module r=francois
Keywords: checkin-needed
Blocks: 1336895
No longer blocks: 1336895
Depends on: 1336895

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3360defc879
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
No longer depends on: 1335475
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
Attachment #8830993 - Flags: approval-mozilla-beta?
Attachment #8830993 - Flags: approval-mozilla-aurora?
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 on attachment 8830993 [details]
Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module

For shield study purpose. Aurora53+.
Attachment #8830993 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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+
Attachment #8830993 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
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
Once Bug 1333303 has been uplifted, this should be uplifted as well please.
Flags: needinfo?(jcristau)
Flags: needinfo?(jcristau)
You need to log in before you can comment on or make changes to this bug.