Closed Bug 1273395 Opened 4 years ago Closed 4 years ago

Add preference for Safe Browsing v2/v4 switch

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(1 file, 2 obsolete files)

The pref is likely to be browser.safebrowsing.google.version

It's "2" (using string type for flexibility) by default and "4" to switch to v4.

This preference should be used as less as possible.
Assignee: nobody → hchang
Blocks: 1264885
Attached patch Patch v1 (obsolete) — Splinter Review
Have a entirely different second though from the initial description:

1) Pref name: browser.safebrowsing.version.google
2) Value type: integer

Also, we provide nsIUrlClassifierUtils.googleApiVersion and it should 
be used to obtain the version.
Comment on attachment 8753252 [details] [diff] [review]
Patch v1

Hi Francois,

Could you help review the patch which adds a preference for v2/v4 swith? I eventually decide to use "browser.safebrowsing.version.google" as the pref name.

Thanks :)
Attachment #8753252 - Flags: review?(francois)
I would suggest browser.safebrowsing.provider.google.pver and browser.safebrowsing.provider.mozilla.pver instead for two reasons:

- It's not Google-specific since we might upgrade our own server (used for tracking protection) too at some point.

- "pver" stands for "protocol version" and that's what Google uses to describe V4. It's also the name of the parameter in browser.safebrowsing.provider.google.updateURL.

I agree that we should use a string for this. In fact, maybe we should use "2.2" for the current implementation to match the exact protocol we're using.

Once we have that, I suppose we could also remove the hard-coded protocol number from browser.safebrowsing.provider.google.updateURL and browser.safebrowsing.provider.google.gethashURL and instead have a string replacement done on it? Something like s/SAFEBROWSING_PVER/<value of browser.safebrowsing.provider.google.pver>/
Comment on attachment 8753252 [details] [diff] [review]
Patch v1

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

::: toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
@@ +24,5 @@
> +
> +  /**
> +   * The API version we use to communicate with google server.
> +   */
> +  readonly attribute unsigned short googleApiVersion;

Maybe safebrowsingProtocolVersion?

::: toolkit/components/url-classifier/tests/unit/test_pref.js
@@ +3,5 @@
> +                   .getService(Ci.nsIUrlClassifierUtils);
> +
> +  // The google API version should be 2 until we enable SB v4
> +  // by default.
> +  equal(urlUtils.googleApiVersion, 2);

I'd use a string here because since we're technically using 2.2 and they could come up with 4.1 in the future too.
Attachment #8753252 - Flags: review?(francois)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8753252 - Attachment is obsolete: true
Comment on attachment 8753651 [details] [diff] [review]
Patch v2

1. Changed the pref name to "browser.safebrowsing.provider.google/mozilla.pver"
2. Renamed the nsURIClassifier.googleApiVersion to getProtocolVersion.
Attachment #8753651 - Flags: review?(francois)
Comment on attachment 8753651 [details] [diff] [review]
Patch v2

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

::: modules/libpref/init/all.js
@@ +4988,5 @@
>  pref("urlclassifier.alternate_error_page", "blocked");
>  
>  // Enable phishing protection
>  pref("browser.safebrowsing.enabled", true);
> +pref("browser.safebrowsing.provider.google.pver", "2.2");

It might be better to move these prefs to be next to the updateurl and gethashurl for each provider. In the current position, it seems like these new prefs are only relevant to the phishing protection.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ +11,5 @@
>  #include "plbase64.h"
>  #include "prprf.h"
> +#include "nsPrintfCString.h"
> +
> +#define DEFAULT_GOOGLE_PROTOCOL_VERSION "2.2"

I think we can have a single default value ("2.2") for both. When we switch to V4, we'll simply set it so we won't have to rely on the default.

@@ +147,5 @@
> +    aVersion = DEFAULT_GOOGLE_PROTOCOL_VERSION;
> +  } else if (aProvider.EqualsLiteral("mozilla")) {
> +    aVersion = DEFAULT_MOZILLA_PROTOCOL_VERSION;
> +  } else {
> +    NS_WARNING(nsPrintfCString("Unsupported provider: %s", aProvider.get()));

If you have a single default then this becomes much easier and the function never returns an error:

if (SUCCEEDED) {
  aVersion = version
} else {
  aVersion = DEFAULT
}
return NS_OK
Attachment #8753651 - Flags: review?(francois)
Attached patch Patch v3Splinter Review
Attachment #8753651 - Attachment is obsolete: true
Comment on attachment 8754239 [details] [diff] [review]
Patch v3

Hi Francois,

I've addressed all the comments from your last review. Could you have a review again? By the way, just for your information, the patch on Bug 1264885 demonstrates how the preference would be used.

Thanks :)
Attachment #8754239 - Flags: review?(francois)
Comment on attachment 8754239 [details] [diff] [review]
Patch v3

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

Looks great Henry, thanks!
Attachment #8754239 - Flags: review?(francois) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2a51753a7784
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.