Closed Bug 1336915 Opened 7 years ago Closed 7 years ago

Disable Safe Browsing V4 updates and fullhash requests when the Google API key is missing

Categories

(Toolkit :: Safe Browsing, defect, P2)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: francois, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m7)

Attachments

(1 file, 1 obsolete file)

Bug 1336915 - Disable updates and fullhash requests when the Google API key is missing
59 bytes, text/x-review-board-request
francois
: review+
Details 
For Safe Browsing V4 providers, we should look at the updateurl. If it includes "=no-google-api-key" then we block the network request and return an error, which will trigger the backoff code.

This should suppress most spurious traffic to Google from clients without a valid API key.

We should also have a telemetry probe that keeps track of whether or not the required API key was available everytime we do an update.
As part of this patch, we should re-enable testing of V4 lists in test_safe_browsing_initial_download.py by backing out the patch from bug 1336922.
Depends on: 1336922
Whiteboard: #sbv4-m7
(In reply to François Marier [:francois] from comment #2)
> As part of this patch, we should re-enable testing of V4 lists in
> test_safe_browsing_initial_download.py by backing out the patch from bug
> 1336922.

This part has already landed (see bug 1330253).
Google is seeing thousands of update requests without an API key from Nightly every day.

API key errors during fullhash requests are way less frequent, but do occur so we should add a check before doing a completion too.
Summary: Disable Safe Browsing V4 updates to Google when the API key is missing → Disable Safe Browsing V4 updates and fullhash requests when the Google API key is missing
Assignee: nobody → tnguyen
We only support update/gethash request for
- http/https scheme
- Valid url
- Valid google api (for V4)

MozReview-Commit-ID: CJmJErxSOqa
Comment on attachment 8861306 [details] [diff] [review]
Disable Safe Browsing updates and fullhash requests with invalid url request

I am sorry that it seems we could not push to mozreview at the moment, could you look at the raw patch?
Attachment #8861306 - Flags: review?(francois)
Comment on attachment 8861306 [details] [diff] [review]
Disable Safe Browsing updates and fullhash requests with invalid url request

Review of attachment 8861306 [details] [diff] [review]:
-----------------------------------------------------------------

I would use a simpler approach here: only looking at the API key, not the URL.

Basically, before we add a table for the "google" or "google4" providers, we should check that the API key is "valid" like we did for about:support in bug 1336920:

https://hg.mozilla.org/mozilla-central/rev/6a1d6726edd0#l7.12
Attachment #8861306 - Flags: review?(francois) → review-
Attachment #8861820 - Flags: review?(francois)
Attachment #8861306 - Attachment is obsolete: true
Thanks Francois,
I changed as you suggested. Do we need care about odd users who manually change the URL from about:config to weird things (or not http/https)?
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #9)
> I changed as you suggested. Do we need care about odd users who manually
> change the URL from about:config to weird things (or not http/https)?

I can't think of a reason why we should care, so I wouldn't worry about it now.
Comment on attachment 8861820 [details]
Bug 1336915 - Disable updates and fullhash requests when the Google API key is missing

https://reviewboard.mozilla.org/r/133816/#review137020

Nice and simple.

A few nits before you land, otherwise r+.

::: toolkit/components/url-classifier/SafeBrowsing.jsm:278
(Diff revision 1)
>        let gethashURL = Services.urlFormatter.formatURLPref(
>          "browser.safebrowsing.provider." + provider + ".gethashURL");
>        updateURL = updateURL.replace("SAFEBROWSING_ID", clientID);
>        gethashURL = gethashURL.replace("SAFEBROWSING_ID", clientID);
>  
> +      // We will not do update/gethash google table if the table has no google api

nit: I'd suggest:

    // Disable updates and gethash if the Google API key is missing.

::: toolkit/components/url-classifier/content/listmanager.js:104
(Diff revision 1)
>   */
>  PROT_ListManager.prototype.registerTable = function(tableName,
>                                                      providerName,
>                                                      updateUrl,
>                                                      gethashUrl) {
>    log("registering " + tableName + " with " + updateUrl);

We could move this line to just after the if statement so that we either:

- print "Can't register table goog-phish-shavar" or
- print "registering goog-phish-shavar"

but not both.

::: toolkit/components/url-classifier/content/listmanager.js:105
(Diff revision 1)
>  PROT_ListManager.prototype.registerTable = function(tableName,
>                                                      providerName,
>                                                      updateUrl,
>                                                      gethashUrl) {
>    log("registering " + tableName + " with " + updateUrl);
> +  // Clear the tablesData and only add a table if that table have valid update

nit: this comment is probably not necessary
Attachment #8861820 - Flags: review?(francois) → review+
Status: NEW → ASSIGNED
A small update adding condition check to fix test failures in testing machine that does not have google api key
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c97d7533b82
Disable updates and fullhash requests when the Google API key is missing r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c97d7533b82
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: