334575 - protection should have a config flag to enable/disable building

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

protection should be off by default while being developed but with a config flag to toggle it on

Reproducible: Always

Comment 1

[Tony Chang (Google)]

Attachment: --enable-protection configure flag

Comment 2

[Darin Fisher]

Comment on attachment 218926 [details] [diff] [review]
--enable-protection configure flag

>Index: configure.in

> dnl ========================================================
>+dnl = Enable protection (anti-phishing protection)
>+dnl ========================================================

"Do you use protection?" ;-)

The name of this option could maybe be more specific, but I'm not
sure what else to call it.  You might want to ask bsmedberg about
this (he's the owner of the build system).  I'll request additional
review from him.


>Index: toolkit/components/Makefile.in

>+ifdef MOZ_PROTECTION
>+DIRS += protection
>+endif # MOZ_PROTECTION
> endif # MOZ_XUL
> endif # MOZ_SUITE
> endif # MOZ_THUNDERBIRD

So, this will need to change once thunderbird starts making use of
this component.  OK.


r=darin

Comment 3

[Darin Fisher]

-> tony

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

What protection does this enable?  It seems like the directory and flag should both have a clearer name ("anti-phishing" would be my first suggestion, or "url-list-manager" if we really think it's important to name it maximally generically).

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(bz just suggested "url-classifier" in IRC, which I like.)

Comment 6

[Benjamin Smedberg]

Comment on attachment 218926 [details] [diff] [review]
--enable-protection configure flag

Please rename the configure flag to --enable-antiphishing-apis (you can keep using the MOZ_PROTECTION variable internally).

In toolkit/components/Makefile.in there's no need to put the DIRS += protection ifdef inside all the other ifdefs, move it out to a toplevel ifdef.

Comment 7

[Benjamin Smedberg]

Yeah, --enable-url-classifier sounds good

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Please also rename the directory to match whatever better naming is chosen here.

Comment 9

[Tony Chang (Google)]

Ok, I'll make a new patch for this after renaming the directory to url-classifier.

I was planning on the command line flag also enabling anti-phishing functionality in browser/components/[something] as well as the toolkit/components/url-classifier.  Also, in the long run, the anti-phishing framekwork can be extended to protect against other things like spyware (e.g., warning the user before following links that may try to install software).  This was one of the reasons that the original extension was called safe browsing rather than anti-phishing.  So I'm not sure what the best flag name is to enable/disable this functionality is.

Comment 10

[Darin Fisher]

It might make sense to have an --enable-safe-browsing configure flag that causes MOZ_URL_CLASSIFIER to be enabled as well.

Comment 11

[Tony Chang (Google)]

Attachment: safe-browsing and url-classifier flags

Creating two flags, one specifically for the url classifier and a safe-browsing flag that forces MOZ_URL_CLASSIFIER on.  The safe-browsing flag will be used to toggle off the browser/UI components.

Comment 12

[Benjamin Smedberg]

Comment on attachment 219823 [details] [diff] [review]
safe-browsing and url-classifier flags

the safe-browsing bits are being built as an extension, so they can be disabled by the user, correct?

Comment 13

[Tony Chang (Google)]

No, it will be built into the browser.  The user will be able to disable from the prefs pane.

Comment 14

[Benjamin Smedberg]

Hrm, that's not what Darin and I had originally discussed. I'm not sure it's a big deal, I just wonder what changed... was it originally a google-specific extension, and now it has generic hooks for customizable "protection providers"?

Comment 15

[Benjamin Smedberg]

Also, when you check this in please file a followup bug to remove this option once it is enabled by default and it no longer needs to be configurable.

Comment 16

[Benjamin Smedberg]

Comment on attachment 219823 [details] [diff] [review]
safe-browsing and url-classifier flags

There seems to be a planning/policy issue here: the url-classifier bits of this patch are fine and can land now. The safe-browsing bits need discussion, in light of bug 329292; I haven't seen an updated plan since then to move from an extension model to a builtin model.

Comment 17

[Darin Fisher]

bsmedberg: yeah, much has changed since this was originally committed to mozilla cvs.  i think there are some wiki pages somewhere that explain the latest design.  if not, tony can surely bring you up to speed on the latest plans that he, ben and others have developed.

Comment 18

[Tony Chang (Google)]

The original proposal was on dev.apps.firefox:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/b4c0497c67c44cf7/b2743e6d758bf05a

Comment 19

[Benjamin Smedberg]

Why do we need a config flag at all (for safe-browsing)? Is there uncertainty about whether we're going to turn it on? I don't mind config flags if they are useful (to separate development efforts), but this code has already basically been tested, no?

Comment 20

[Tony Chang (Google)]

(In reply to comment #19)
> Why do we need a config flag at all (for safe-browsing)? Is there uncertainty
> about whether we're going to turn it on? I don't mind config flags if they are
> useful (to separate development efforts), but this code has already basically
> been tested, no?

The extension code has been tested, but it has some performance problems that we're trying to address by using sqlite to store a the url lists.  There is some risk in this change so it seems beneficial to have a config flag for it.

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 219823 [details] [diff] [review]
safe-browsing and url-classifier flags

sr=shaver.  (Didn't hear anything about this switch at the Fx2 meetings, but the note-taker and I might have both missed the one in question. :) )

Comment 22

[Darin Fisher]

fixed-on-trunk, fixed1.8.1