SafeBrowsing.jsm should use nsUrlFormatter

RESOLVED FIXED in Firefox 17

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

unspecified
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
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+
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/83e0b185e888
(Assignee)

Comment 3

5 years ago
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.
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f2e6fa487c

Comment 5

5 years ago
+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.
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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
Attachment #647041 - Attachment is obsolete: true
Attachment #647813 - Flags: review?(gpascutto)
Attachment #647813 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 10

5 years ago
Created attachment 647845 [details] [diff] [review]
Patch v.3

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+
(Assignee)

Comment 12

5 years ago
Added comment to Makefile.

http://hg.mozilla.org/integration/mozilla-inbound/rev/a799b5bff84c
https://hg.mozilla.org/mozilla-central/rev/a799b5bff84c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.