Closed Bug 778606 Opened 8 years ago Closed 8 years ago

SafeBrowsing.jsm should use nsUrlFormatter

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Attachment #647041 - Flags: review?(gpascutto)
Attachment #647041 - Attachment is patch: true
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.
Attachment #647041 - Flags: review?(gpascutto) → review+
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.
+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.
(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.
Attached patch Patch v.2 (obsolete) — Splinter Review
* 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
Attachment #647041 - Attachment is obsolete: true
Attachment #647813 - Flags: review?(gpascutto)
Attachment #647813 - Flags: feedback?(gavin.sharp)
(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.
Depends on: 769960
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.
Attachment #647813 - Flags: feedback?(gavin.sharp) → feedback-
Attached patch Patch v.3Splinter Review
Fiiiiine. ;)
Attachment #647813 - Attachment is obsolete: true
Attachment #647813 - Flags: review?(gpascutto)
Attachment #647845 - Flags: review?(gpascutto)
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?
Attachment #647845 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/a799b5bff84c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.