Restrict safebrowsing tests from reaching support.mozilla.org

RESOLVED FIXED in Firefox -esr52

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: raajitr, Mentored)

Tracking

Version 3
mozilla55
Points:
---

Firefox Tracking Flags

(firefox-esr52 fixed, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [lang=py])

Attachments

(1 attachment, 2 obsolete attachments)

This is a follow-up from the latest discussion on bug 1353182, which is about to make the tests less fragile due to outages of remote services like support.mozilla.org. Here what needs to be done:

(In reply to François Marier [:francois] from bug 1353182 comment #11)
> (In reply to Henrik Skupin (:whimboo) from comment #10)
> > No, but I assume there is a pref with formatting options, which is retrieved
> > by Firefox and the target url in this case SUMO is build from it? We could
> > easily update this pref. May this is
> > `app.support.baseURL;https://support.mozilla.org/1/firefox/%VERSION%/%OS%/
> > %LOCALE%/`? If yes, we can get this switched to our local server on a new
> > bug.
> 
> Yes, it does use that pref.
> 
> The code that opens SUMO ("Why was this page blocked?" button) is here:
> https://searchfox.org/mozilla-central/rev/
> d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/browser.js#3195
> 
> and it uses this code:
> 
>         openHelpLink("phishing-malware", false, "current");
> 
> The openHelpLink() function is defined here:
> https://searchfox.org/mozilla-central/rev/
> d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/utilityOverlay.
> js#906
> 
> and uses getHelpLinkUrl():
> https://searchfox.org/mozilla-central/rev/
> d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/utilityOverlay.
> js#901
> 
> which does the use app.support.baseURL pref.
> 
> So if we can redirect all SUMO requests to a local server that always
> returns a 200, we should eliminate all of the false positives.

Tests we are referring to here are located in:

https://dxr.mozilla.org/mozilla-central/search?q=file%3Atest_safe_browsing+path%3Atesting%2Ffirefox-ui%2F&redirect=false

To test which of those tests actually need an update, run them via `mach firefox-ui-functional testing/firefox-ui/tests/functional/test_safe_browsing*` and check if support.mozilla.org is getting opened.

To setup the development environment please follow those steps:
https://wiki.mozilla.org/User:Mjzffr/New_Contributors
And to add... the preference `app.support.baseURL` can be changed to a locally served URL via wptserve. Therefore a new HTML page can be added by placing a file into `https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/resources`. With `self.marionette.absolute_url(path)` the location can be retrieved, and the preference updated in `setUp()`.

Something we should figure out is which parameters of the final URL are important to check. I think at least the type of page would be necessary. Francois, what else do you think would be good or a must have for you?
Flags: needinfo?(francois)
(Assignee)

Comment 2

2 years ago
I want to work on this bug. Can I get more information?
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Something we should figure out is which parameters of the final URL are
> important to check. I think at least the type of page would be necessary.
> Francois, what else do you think would be good or a must have for you?

I'm not sure I understand what you're asking for, but looking at the warning page test (https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py#96), it only seems to care that button press lead to a page loading correctly. So the dummy page could just be an empty page that returns an HTTP 200.
Flags: needinfo?(francois) → needinfo?(hskupin)
Flags: needinfo?(hskupin)
Ok, so I had a look at the URLs which are given in comment 0. And as `getHelpLinkUrl()` shows, we simply concatenate the help topic to the SUMO base URL:

https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/utilityOverlay.js#902

Which means for `phishing-malware` we end-up with:

> 'https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/' + 'phishing-malware'

Whereby the placeholders for version, os, and locale are getting converted to real values in `getHelpLinkURL()`. None of those three seem to be important for us, except the help topic.

So I would suggest the following:

1. The safebrowsing tests set the value for `app.support.baseURL` to `self.marionette.absolute_url("sumo.html?topic=") in setUp. Which means that when clicking the button we have a URL like "support.html?topic=phishing-malware". 

2. A new HTML file gets added to `testing/firefox-ui/resources/support.html`, which itself can retrieve the =`topic` parameter, and displays its value in a div with an id `topic`.

3. The test can retrieve the element via `find_element(By.ID, "topic")` and check that the correct value is set

Comment 5

2 years ago
Hi. I would like work on this bug.
Hi Blaji. You are welcome to work on this bug. Please see comment 0 in how to get started. If there are questions you can also reach me on IRC in the #automation channel. I will assign the bug to you when a first patch has been uploaded - mozreview is preferred for that. Thanks.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8865094 - Flags: review?(hskupin)
(Reporter)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8865094 [details]
Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org

https://reviewboard.mozilla.org/r/136764/#review140272

It's a great start Raajit! I like the patch and from what I can see it's exactly doing what it should do. But there are some issues left which you will have to fix before you get my r+. Thank you.

::: testing/firefox-ui/resources/support.html:12
(Diff revision 1)
> +</body>
> +<script>
> +  var url = window.location.search;
> +  topic = url.replace("?topic=", ''); // get the 'topic' param from URL.
> +
> +  document.getElementById('topic').innerHTML = topic;

Please follow the logic as used in this test file:

http://mozqa.com/data/firefox/tabbedbrowsing/openinnewtab_target.html?id=1

It's important to not inject insecure content to the HTML document.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:28
(Diff revision 1)
>              'https://www.itisatrap.org/firefox/its-an-attack.html'
>          ]
>  
>          self.marionette.set_pref('browser.safebrowsing.phishing.enabled', True)
>          self.marionette.set_pref('browser.safebrowsing.malware.enabled', True)
> +        self.marionette.set_pref('app.support.baseURL', self.marionette.absolute_url("support.html?topic="))

Please move this up, so we keep the alphabetical order.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:45
(Diff revision 1)
>          try:
>              self.puppeteer.utils.permissions.remove('https://www.itisatrap.org', 'safe-browsing')
>              self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs[0]])
>              self.marionette.clear_pref('browser.safebrowsing.malware.enabled')
>              self.marionette.clear_pref('browser.safebrowsing.phishing.enabled')
> +            self.marionette.clear_pref('app.support.baseURL')

Same here as above.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:90
(Diff revision 1)
>  
>          button = self.marionette.find_element(By.ID, "reportButton")
>          button.click()
>  
> +        topic = self.marionette.find_element(By.ID, "topic")
> +        self.assertEquals(topic.text, "phishing-malware")

You should not access page elements before the page load has been finished. See the Wait().until() call right after which ensures this. I would please this check at the end of the method.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:100
(Diff revision 1)
>              expected.element_stale(button))
>  
>          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
>          expected_url = self.browser.get_final_url(url)
> +

Please remove this addition of an empty line.
Attachment #8865094 - Flags: review?(hskupin) → review-
(Assignee)

Comment 9

2 years ago
Thanks for your review and sorry I should have read the guidelines more carefully. I have a question which is unrelated to this fix, you normally assign bugs after the first patch but why not here?
Anyways, I will update this with fixes ASAP.
I just missed to assign it to you. Sorry for that. It's done now.
Assignee: nobody → raajit.raj
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8865094 [details]
Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org

https://reviewboard.mozilla.org/r/136764/#review141004

We are close. Two more smaller things to fix before we can get it landed. Thanks.

::: testing/firefox-ui/resources/support.html:9
(Diff revision 2)
>    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
> -</head>
> -<body>
> -  <div id="topic"></div>
> -</body>
> -<script>
> +  <script type="text/javascript">
> +    function show() {
> +      var results = /\?topic=(.+)$/.exec(window.document.location);
> +      var topic = decodeURIComponent(results[1].replace(/\+/g, " "))
> +      var node = document.createElement("div");

Instead of creating it dynamically, lets hard-code the div with the id under the body. You can use getElementById to retrieve it, and update its textContent.

Reason is if something fails in the JS code we won't have the element available. With the above code, Marionette can retrieve the element, but the value will be not set to the correct value.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:26
(Diff revision 2)
>              'https://www.itisatrap.org/firefox/its-a-trap.html',
>              # Malware URL
>              'https://www.itisatrap.org/firefox/its-an-attack.html'
>          ]
>  
> +        self.marionette.set_pref('app.support.baseURL', self.marionette.absolute_url("support.html?topic="))

This line extends the 100 chars and would cause a linting failure. Please wrap the second argument to the next line.
Attachment #8865094 - Flags: review?(hskupin) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8865094 [details]
Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org

https://reviewboard.mozilla.org/r/136764/#review141004

> This line extends the 100 chars and would cause a linting failure. Please wrap the second argument to the next line.

Please test with a linter. The indentation here is wrong, which will cause a failure.
(Reporter)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8865094 [details]
Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org

https://reviewboard.mozilla.org/r/136764/#review141524

::: commit-message-37a5b:2
(Diff revision 3)
> +Bug 1357372 - Added hard coded div tag instead of dynamically creating one in support.html and wrapped the line to prevent lint failures in test file
> +

Please use a commit message which describes all the changes, and not what you pushed most recently. If possible even add an additional paragraph for details.
Attachment #8865094 - Flags: review?(hskupin) → review-
(Reporter)

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8865094 [details]
Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org

https://reviewboard.mozilla.org/r/136764/#review141004

> Please test with a linter. The indentation here is wrong, which will cause a failure.

Also see https://www.python.org/dev/peps/pep-0008/#indentation
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8865094 - Attachment is obsolete: true
Attachment #8865094 - Flags: review?(hskupin)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8866830 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8866887 - Flags: review?(hskupin)
(Assignee)

Comment 20

2 years ago
Sorry, I was facing some issues with hg so have to make a new review request without any previous diffs, hope thats not a problem.
For the linting problem I've fixed and verified using pylint.

And sorry about commit message, version control is still new to me.

Thanks
Please really stop pushing new mozreview patches and marking existing ones as obsolete. This completely kills the review history, and forces me to do everything again. If you have problems like this ask on IRC and we can get this fixed for upcoming patches. Thanks.
(Reporter)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8866887 [details]
Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org

https://reviewboard.mozilla.org/r/138496/#review142772

This looks fine. I triggered some test jobs again which all passed. I will try to land it right now. Thank you again for this patch. It will make this test way more stable.
Attachment #8866887 - Flags: review?(hskupin) → review+

Comment 23

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12e5e6e8cad4
Restrict safebrowsing tests from reaching support.mozilla.org r=whimboo

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12e5e6e8cad4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
It's a test-only change which replaces a remote URL with a locally served one for better stability. We should try to uplift it to beta and esr52.
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Whiteboard: [lang=py] → [lang=py][checkin-needed-beta][checkin-needed-esr52]

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1a5b19210b16
status-firefox54: affected → fixed
Whiteboard: [lang=py][checkin-needed-beta][checkin-needed-esr52] → [lang=py][checkin-needed-esr52]

Comment 27

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/71fc70a0f832
status-firefox-esr52: affected → fixed
Whiteboard: [lang=py][checkin-needed-esr52] → [lang=py]
You need to log in before you can comment on or make changes to this bug.