Closed Bug 1366384 Opened 6 years ago Closed 6 years ago

Add Google attribution on Safe Browsing warning pages

Categories

(Toolkit :: Safe Browsing, enhancement, P1)

Product:
Toolkit
Component:
Safe Browsing
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: francois, Assigned: tnguyen)

References

Details

Attachments

(2 files)

Bug 1366384 - Add Google attribution on Safe Browsing warning pages
59 bytes, text/x-review-board-request
maliu
: review+
johannh
: review+
francois
: review+
Details
Bug 1366384 - Add Advisory test on Safe Browsing warning page
59 bytes, text/x-review-board-request
francois
: review+
Details 
As per Google's guidelines(https://developers.google.com/safe-browsing/v4/usage-limits#UserWarnings), we should attribute the warning to Google when a Safe Browsing warning page shows up due to a Google list:

  "When you show warnings for pages identified as risky by the Safe 
  Browsing Service, you must give attribution to Google by including the 
  line "Advisory provided by Google," with a link to the Safe Browsing 
  Advisory. If your product also shows warnings based on other sources, 
  you may not include the Google attribution in warnings derived from non-
  Google data."

I believe we already keep track of the provider when we display these interstitial pages, so it should be pretty easy to add another paragraph to the existing warning pages that insert the provider-specific attribution link for `google` and `google4`.
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review152284

I may need an frontend expert take a look at this first. Hi Johann, could you take a look at this?
Basically, we only show the advisory text if the provider of warning is Google. (Provider is added to about:blocked?e=error_code&xxxxx&advisory=google&xxxxx from docshell, but I have to enable the chrome privilege of blockedSite.xhtml)
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review152294

Please don't give in-content pages chrome privileges. You could instead listen for the AboutBlockedLoaded event in content.js and set the service provider from the bundle there.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:17
(Diff revision 1)
>    safeb.palm.notdeceptive.label and reportDeceptiveSiteMenu.title from
>    report-phishing.dtd are never shown at the same time, the same accesskey can
>    be used for them. -->
>  <!ENTITY safeb.palm.notdeceptive.accesskey "d">
>  <!ENTITY safeb.palm.reportPage.label "Why was this page blocked?">
> +<!ENTITY safeb.palm.advisory.desc "Advisory provided by ">

Please use a <span id="..." /> like in safeb.blocked.malwarePage.shortDesc and others, for localization.
Attachment #8876638 - Flags: review?(jhofmann) → review-
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review152294

Thanks for your review.
Then we can get provider directly in content.js though docshell.
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review154506

This is generally the right approach, just need to fix a couple of issues. We probably want to get a mobile person to look at the Android parts. I haven't found one yet, but whenever we find someone you should flag them for additional review.

::: browser/base/content/content.js:355
(Diff revision 2)
> +          Services.prefs.getCharPref(
> +            "browser.safebrowsing.provider." + provider + ".advisoryURL", "");
> +        if (!advisoryUrl) {
> +          let el = content.document.getElementById("advisoryDesc");
> +          el.remove();
> +          return ;

Nit: There's a space after return

::: browser/base/content/content.js:364
(Diff revision 2)
> +          Services.strings.createBundle(
> +            "chrome://browser/locale/safebrowsing/safebrowsing.properties");
> +        let advisoryLinkText =
> +          bundle.GetStringFromName("advisoryProvider." + provider + ".link");
> +        // We may not want to set innerHtml/outerHtml which may have security
> +        // consideration So, create and add an anchor element to p tag

I think it should be clear that we don't want to use innerHTML, so I could live without this comment. :)

::: browser/base/content/content.js:366
(Diff revision 2)
> +        let advisoryLinkText =
> +          bundle.GetStringFromName("advisoryProvider." + provider + ".link");
> +        // We may not want to set innerHtml/outerHtml which may have security
> +        // consideration So, create and add an anchor element to p tag
> +        var anchorEl = content.document.createElement("a");
> +        anchorEl.setAttribute("id", "advisory_link");

Why does this need an id?

::: browser/base/content/content.js:368
(Diff revision 2)
> +        // We may not want to set innerHtml/outerHtml which may have security
> +        // consideration So, create and add an anchor element to p tag
> +        var anchorEl = content.document.createElement("a");
> +        anchorEl.setAttribute("id", "advisory_link");
> +        anchorEl.setAttribute("href", advisoryUrl);
> +        anchorEl.appendChild(content.document.createTextNode(advisoryLinkText));

Can probably be shortened to

anchorEl.textContent = advisoryLinkText

::: browser/base/content/content.js:371
(Diff revision 2)
> +        anchorEl.setAttribute("id", "advisory_link");
> +        anchorEl.setAttribute("href", advisoryUrl);
> +        anchorEl.appendChild(content.document.createTextNode(advisoryLinkText));
> +        content.document.getElementById("advisory_provider").appendChild(anchorEl);
> +      } catch (e) {
> +        // GetStringFromName will throw if the string is missing

Please only wrap GetStringFromName in the try catch then.

::: browser/locales/en-US/chrome/browser/safebrowsing/safebrowsing.properties:18
(Diff revision 2)
> +
> +# LOCALIZATION NOTE(advisoryProvider.google.link): The message displayed in advisory
> +# description. This will be linked to
> +# http://code.google.com/apis/safebrowsing/safebrowsing_faq.html#whyAdvisory
> +# "Google Safe Browsing" should not be translated.
> +advisoryProvider.google.link=Google Safe Browsing

Be aware that if Google ever decides to change the name to "Google Secure Surfing" or so, you'll have to change the key to advisoryProvider.google.link2 which messes with your logic above.

I wouldn't change it right now, just wanted to mention it. :)

::: mobile/android/chrome/content/content.js:19
(Diff revision 2)
>  
>  var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "Content");
>  
>  var global = this;
>  
> +function getSiteBlockedErrorDetails(docShell) {

Since you don't use the full error details getting the URI and returning the list is unnecessary. I think I'd make a function that only gets the provider, but I'll leave the details to your Android reviewer.

::: mobile/android/chrome/content/content.js:51
(Diff revision 2)
>  
>    _isLeavingReaderableReaderMode: false,
>  
>    init: function() {
>      addEventListener("AboutReaderContentLoaded", this, false, true);
> +    addEventListener("AboutBlockedLoaded", this, false, true);

You shouldn't use AboutReaderListener to listen to this event, you can just make a new listener object instead.

All my comments in b/b/c/content.js also apply to this file :)
Attachment #8876638 - Flags: review?(jhofmann) → review-
(In reply to Johann Hofmann [:johannh] from comment #6)
> Comment on attachment 8876638 [details]
> Bug 1366384 - Add Google attribution on Safe Browsing warning pages
> 
> https://reviewboard.mozilla.org/r/147952/#review154506
> 
> This is generally the right approach, just need to fix a couple of issues.
> We probably want to get a mobile person to look at the Android parts. I
> haven't found one yet, but whenever we find someone you should flag them for
> additional review.

Thanks for your review, Johann.
Hi Tim, sorry for tagging you here, do you know anyone who would take a look at this bug?
Flags: needinfo?(timdream)
Flags: needinfo?(timdream)
Attachment #8876638 - Flags: review?(max)
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review156226

I couldn't verify this patch work as expected. Would you hepl to provide STR please? Thank you.
Attachment #8876638 - Flags: review?(jhofmann)
(In reply to Max Liu [:maliu] from comment #9)
> Comment on attachment 8876638 [details]
> Bug 1366384 - Add Google attribution on Safe Browsing warning pages
> 
> https://reviewboard.mozilla.org/r/147952/#review156226
> 
> I couldn't verify this patch work as expected. Would you hepl to provide STR
> please? Thank you.

Thanks for looking at this :).
1. You may have to build fennec with an invalid google API (I am using official google api for nightly build) then make sure that Google Safe Browsing list has been updated. (Probably you can access to about:url-classifier to update google manually)

2. Access unsafe pages which are listed in
http://testsafebrowsing.appspot.com/
Expect: The pages should be blocked and there's an advisory line displayed:  "Advisory provided by Google Safe Browsing"

4. Access unsafe page which is provided by Mozilla https://itisatrap.org/firefox/its-a-trap.html
Expect: the page should be blocked without advisory text
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review159048

Works for me, thanks. This feels like something that should have a test, though I don't know if it's hard to test things that depend on a provider to show. I'd leave that up to you and François.

::: browser/base/content/content.js:380
(Diff revision 4)
> +      let el = content.document.getElementById("advisoryDesc");
> +      el.remove();
> +      return;
> +    }
> +
> +    var anchorEl = content.document.createElement("a");

Nit: let

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:18
(Diff revision 4)
>    report-phishing.dtd are never shown at the same time, the same accesskey can
>    be used for them. -->
>  <!ENTITY safeb.palm.notdeceptive.accesskey "d">
>  <!ENTITY safeb.palm.reportPage.label "Why was this page blocked?">
> +<!-- Localization note (safeb.palm.advisory.desc) - Please don't translate <span id="advisory_provider"/> tag.  It will be replaced at runtime with advisory link-->
> +<!ENTITY safeb.palm.advisory.desc "Advisory provided by <span id='advisory_provider'/>.">

Any particular reason why you're not making this an <a> element directly instead of injecting one into a span?
Attachment #8876638 - Flags: review?(jhofmann) → review+
See Also: → 1329860
If the brand name "Google Safe Browsing" should not be translated, would you consider putting it in a preference instead of a translation file?

We'd like this feature to be usable by Mozilla China as well to indicate that their safe browsing is provided by Baidu.

Since all of these features are added via preferences, it seems like the the name of the provider should be in preferences as well.
(In reply to Mike Kaply [:mkaply] from comment #13)
> If the brand name "Google Safe Browsing" should not be translated, would you
> consider putting it in a preference instead of a translation file?

That's a good idea.

We could add:

browser.safebrowsing.provider.google.name = "Google Safe Browsing"
browser.safebrowsing.provider.google4.name = "Google Safe Browsing"

and then in this patch, we could show the "Advisory provided by CompanyName" only if the provider.XXXX.name pref exists (and is non-empty).
rs=me on that idea, though feel free to request another review if you're not sure about something.
Thanks all for the review. I updated the patch as your suggestion.

I have no idea how to test (In reply to Johann Hofmann [:johannh] from comment #12)
> Comment on attachment 8876638 [details]
> Bug 1366384 - Add Google attribution on Safe Browsing warning pages
> 
> https://reviewboard.mozilla.org/r/147952/#review159048
> 
> Works for me, thanks. This feels like something that should have a test,
> though I don't know if it's hard to test things that depend on a provider to
> show. I'd leave that up to you and François.
> 
I have no idea how to test the provider and google SB list in functional ui test, but that's quite possible to test in mochitest.
Francois, could you please take the look?
Attachment #8876638 - Flags: review?(francois)
Comment on attachment 8885635 [details]
Bug 1366384 - Add Advisory test on Safe Browsing warning page

https://reviewboard.mozilla.org/r/156480/#review162354

::: toolkit/components/url-classifier/tests/mochitest/test_advisory_link.html:98
(Diff revision 1)
> +      await BrowserTestUtils.waitForContentEvent(browser, "DOMContentLoaded")
> +
> +      let doc = win.gBrowser.contentDocument;
> +      let advisoryEl = doc.getElementById("advisory_provider");
> +      if (aTestData.provider != "google" && aTestData.provider != "google4") {
> +        ok(!advisoryEl, "Advisory should not be showed");

"shown"
Attachment #8885635 - Flags: review?(francois) → review+
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review163978
Attachment #8876638 - Flags: review?(max) → review+
Comment on attachment 8876638 [details]
Bug 1366384 - Add Google attribution on Safe Browsing warning pages

https://reviewboard.mozilla.org/r/147952/#review166538

Looks good, but please update the URL as described below before landing.

::: modules/libpref/init/all.js:5289
(Diff revision 5)
>  pref("browser.safebrowsing.provider.google.updateURL", "https://safebrowsing.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%MAJOR_VERSION%&pver=2.2&key=%GOOGLE_API_KEY%");
>  pref("browser.safebrowsing.provider.google.gethashURL", "https://safebrowsing.google.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%MAJOR_VERSION%&pver=2.2");
>  pref("browser.safebrowsing.provider.google.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
>  pref("browser.safebrowsing.provider.google.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
>  pref("browser.safebrowsing.provider.google.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url=");
> +pref("browser.safebrowsing.provider.google.advisoryURL", "http://code.google.com/apis/safebrowsing/safebrowsing_faq.html#whyAdvisory");

This is pointing at the V3 docs. That's not what we implement and it's likely to go away at some point.

Let's just use the same URL as V4 instead.
Attachment #8876638 - Flags: review?(francois) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88f729384a45
Add Google attribution on Safe Browsing warning pages r=francois,johannh,maliu
https://hg.mozilla.org/integration/autoland/rev/eeeeccf397ef
Add Advisory test on Safe Browsing warning page r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88f729384a45
https://hg.mozilla.org/mozilla-central/rev/eeeeccf397ef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1388570
Depends on: 1388912
Depends on: 1388494
You need to log in before you can comment on or make changes to this bug.