Last Comment Bug 778606 - SafeBrowsing.jsm should use nsUrlFormatter
: SafeBrowsing.jsm should use nsUrlFormatter
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on: 769960
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-29 19:07 PDT by Justin Dolske [:Dolske]
Modified: 2014-05-27 12:25 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (12.11 KB, patch)
2012-07-29 19:07 PDT, Justin Dolske [:Dolske]
gpascutto: review+
Details | Diff | Review
Patch v.2 (18.76 KB, patch)
2012-07-31 19:03 PDT, Justin Dolske [:Dolske]
gavin.sharp: feedback-
Details | Diff | Review
Patch v.3 (15.97 KB, patch)
2012-07-31 22:34 PDT, Justin Dolske [:Dolske]
gpascutto: review+
Details | Diff | Review

Description Justin Dolske [:Dolske] 2012-07-29 19:07:02 PDT
Created attachment 647041 [details] [diff] [review]
Patch v.1

A little more cleanup after bug 769960.

The safebrowsing code was using its own regexs to tweak URLs, I've change this to use the url formatting service. (And, since on of the tokens was offical-build dependent, I moved the #ifdef over there.) I verified that the final before-and-after URLs were identical in my build.

Also moved the default prefs into the JSM so that apps don't have to provide all these separately, along with with a hook to suggest how we can test with different URLs.

Nuked browser.safebrowsing.provider.0.name entirely, since it appears to be unused.
Comment 1 Gian-Carlo Pascutto [:gcp] 2012-07-30 03:06:04 PDT
Comment on attachment 647041 [details] [diff] [review]
Patch v.1

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

::: browser/app/profile/firefox.js
@@ +719,1 @@
>  pref("browser.safebrowsing.malware.enabled", true);

There was no reason to remove the comments here, though the settings are probably self-explanatory enough.
Comment 3 Justin Dolske [:Dolske] 2012-07-30 17:09:19 PDT
Derp. Thought I had run this through try...

Looks to be busted because automation.py.in is setting prefs:

420 user_pref("browser.safebrowsing.provider.0.gethashURL", "http://%(server)s/safebrowsing-dummy/gethash");
421 user_pref("browser.safebrowsing.provider.0.keyURL", "http://%(server)s/safebrowsing-dummy/newkey");
422 user_pref("browser.safebrowsing.provider.0.updateURL", "http://%(server)s/safebrowsing-dummy/update");

Which of course the code isn't looking at any more.

Will modify patch to either return to looking at prefs (but using defaults if not set), or fix the tests to do the new init() thing.
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-30 17:16:27 PDT
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f2e6fa487c
Comment 5 Philip Chee 2012-07-30 20:29:26 PDT
+var urlList = {
+  updateURL:             "http://safebrowsing.clients.google.com/safebrowsing/downloads?client=%SAFEBROWSING_ID%&appver=%VERSION%&pver=2.2",
+  keyURL:                "https://sb-ssl.google.com/safebrowsing/newkey?client=%SAFEBROWSING_ID%&appver=%VERSION%&pver=2.2",
+  reportURL:             "http://safebrowsing.clients.google.com/safebrowsing/report?",
+  gethashURL:            "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=%SAFEBROWSING_ID%&appver=%VERSION%&pver=2.2",
+  reportGenericURL:      "http://%LOCALE%.phish-generic.mozilla.com/?hl=%LOCALE%",
+  reportErrorURL:        "http://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%",
+  reportPhishURL:        "http://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%",
+  reportMalwareURL:      "http://%LOCALE%.malware-report.mozilla.com/?hl=%LOCALE%",
+  reportMalwareErrorURL: "http://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%",
+};
....
+    SAFEBROWSING_ID:  function() MOZ_OFFICIAL_BUILD ? "navclient-auto-ffox" : this.appInfo.name,

SeaMonkey being a purely community driven project may not be allowed to use "navclient-auto-ffox"

+  reportGenericURL:      "http://%LOCALE%.phish-generic.mozilla.com/?hl=%LOCALE%",
+  reportErrorURL:        "http://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%",
+  reportPhishURL:        "http://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%",
+  reportMalwareURL:      "http://%LOCALE%.malware-report.mozilla.com/?hl=%LOCALE%",
+  reportMalwareErrorURL: "http://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%",

These pages are firefox branded. SeaMonkey would definitely want to point these to seamonkey branded pages on seamonkey-projects.org. Thunderbird likely would want to use TB branded pages.

> Will modify patch to either return to looking at prefs (but using defaults if not set), or fix
> the tests to do the new init() thing.

Wasn't it stated earlier that tests might want to point these to different urls e.g. an internal server.
Comment 6 Justin Dolske [:Dolske] 2012-07-31 18:55:29 PDT
(In reply to Philip Chee from comment #5)

> +    SAFEBROWSING_ID:  function() MOZ_OFFICIAL_BUILD ? "navclient-auto-ffox"
> : this.appInfo.name,
> 
> SeaMonkey being a purely community driven project may not be allowed to use
> "navclient-auto-ffox"

Not really relevant until after bug 778608, but I've tweaked it anyway.

> These pages are firefox branded. SeaMonkey would definitely want to point
> these to seamonkey branded pages on seamonkey-projects.org. Thunderbird
> likely would want to use TB branded pages.

Except neither SB nor TB currently does so. Patches welcome whenever that changes.

> Wasn't it stated earlier that tests might want to point these to different
> urls e.g. an internal server.

Yes, and the patch supports that. In any case, I've changed my mind about having these not be prefs. There's no user benefit, but it does end up making tests more of a PITA than they should be. And, well, storing these as prefs seems more consistent with how things are done elsewhere.
Comment 7 Justin Dolske [:Dolske] 2012-07-31 19:03:41 PDT
Created attachment 647813 [details] [diff] [review]
Patch v.2

* Restored prefs. I worried a bit about what would happen if a user currently had a modified value (that now likely wouldn't format correctly). Realized I could avoid that problem and ditch the legacy "provider.0" stuff in one fell swoop. So the prefs are renamed.

* En passant move of browser.geolocation.warning.infoURL (which was inside a #ifdef block) and browser.safebrowsing.malware.reportURL (which was outside the #ifdef block)

* Shuffled a bit of init() code so that things should work if safe browsing is initially disabled and then reenabled (or any of the prefs change). Filed bug 779317 to help with this oddness. (Verified through code inspection that all the setFooUrls calls are basically NOPs when value isn't actually changing.

* Tweaked url formatter some more with MOZ_PHOENIX, to further offend gavin. ;)


Currently working its way through Try, since running browser-chrome tests locally makes me cry.

https://tbpl.mozilla.org/?tree=Try&rev=8292f4da0737
Comment 8 Justin Dolske [:Dolske] 2012-07-31 19:10:57 PDT
(In reply to Justin Dolske [:Dolske] from comment #7)
> browser.safebrowsing.malware.reportURL (which was outside
> the #ifdef block)

By which I mean it was inside the #ifdef block, just separate from the other similar prefs.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-31 19:16:39 PDT
Comment on attachment 647813 [details] [diff] [review]
Patch v.2

I really think you should put the gross Firefox-specific safebrowsing hack in safebrowsing code, rather than unrelated urlformatter code. It's really not any harder - just make s/%SAFEBROWSING_ID%/_SAFEBROWSING_ID_/ or somesuch and do the replacing in the safe browsing code.
Comment 10 Justin Dolske [:Dolske] 2012-07-31 22:34:22 PDT
Created attachment 647845 [details] [diff] [review]
Patch v.3

Fiiiiine. ;)
Comment 11 Gian-Carlo Pascutto [:gcp] 2012-08-01 00:08:14 PDT
Comment on attachment 647845 [details] [diff] [review]
Patch v.3

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

::: browser/components/safebrowsing/Makefile.in
@@ +20,1 @@
>  endif

This at least need some comments. MOZ_PHOENIX? HISTORIC_ID?
Comment 12 Justin Dolske [:Dolske] 2012-08-01 15:53:20 PDT
Added comment to Makefile.

http://hg.mozilla.org/integration/mozilla-inbound/rev/a799b5bff84c
Comment 13 Ed Morley [:emorley] 2012-08-02 06:23:05 PDT
https://hg.mozilla.org/mozilla-central/rev/a799b5bff84c

Note You need to log in before you can comment on or make changes to this bug.