344897 - anti-phishing needs different update urls for official builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Tony Chang (Google)]

Some of the url in the black lists that Google uses for anti-phishing come from third parties.  There is a request that these urls only get used in official builds of Firefox.  The server that sends the url lists to Firefox use a parameter in the update url to determine whether or not to send these third party lists.

So for official builds, we would like to have the update URL include an extra parameter to identify it as an official build.  This is similar to what happens with the search box url (MOZ_OFFICIAL and MOZ_DISTRIBUTION_ID).

Comment 1

[Mike Connor [:mconnor]]

Tony, do we need to localize this pref?  There's two ways, either #ifdef the firefox.js file, or package an extra pref file for official build overrides.

Comment 2

[Tony Chang (Google)]

(In reply to comment #1)
> Tony, do we need to localize this pref?

Localization is on bug 343766.  It's been baking on trunk for a while, I'll renominate for ff2.

> There's two ways, either #ifdef the
> firefox.js file, or package an extra pref file for official build overrides.

I think an extra pref file is preferred.  Third parties have requested that the official URLs not be in CVS.

Comment 3

[Benjamin Smedberg]

I absolutely think that we need these URLs in CVS; we need reproducability from source code and we don't want to have "separate" source code repositories. We can certainly make them dependent on official branding, and put the URLs in other-licenses/branding/firefox

Comment 4

[Sherman Dickman]

Do we have a standard for how we handle other URLs in the product that point to third parties?  Search?

Comment 5

[Benjamin Smedberg]

Not really... we have URLs in preferences, in localizable files, and in branding directories rather haphazardly dependending on whether those URLs need to change in official-branding builds, per-locale, etc

Comment 6

[Brian Rakowski]

From the perspective of minimizing Google's legal risk, it is very much preferable for the complete URL to NOT appear anywhere in CVS.  To be more precise, it's okay for a working URL, to which Google servers will actually respond, to be in CVS but we must at least reserve a parameter outside of the open source code, that Google can use to distinguish official from unofficial builds. Iff we see that param/clientID, we will know that we can send the licensed data to Firefox, otherwise, we'll just send our unlicensed data.

Here's why: Google is licensing URLs from third parties to supply this service to Mozilla. As part of the license agreement, Google has agreed to take reasonable steps to protect the data from use by applications other than official builds of Firefox. While we are doing other things to protect the data, we'd feel more comfortable taking this additional step.

Comment 7

[Benjamin Smedberg]

What is an "official" build? For instance, when the Solaris team prepares builds of Firefox (which are not prepared by the Mozilla build/release team), they typically use official branding and should be using the "official" URLs, no?

We obviously also need to ship various candidate builds with the "official" URLs for testing purposes which we know are not going to be the final releases (beta2, for example).

This is a matter of reproducability: adding another CVS repository for special files that are part of the build is quite painful and I'd like to avoid it at almost any cost.

Comment 8

[Brian Rakowski]

We are clear to use the licensed data in any official build so the Solaris build would qualify.

Comment 9

[J. Paul Reed [:preed]]

I agree with bsmedberg that for reasons of build reproducibility, these URLs should be contained within CVS. Speaking from experience, I can also say that dealing with separate repositories to house parts of a coherent build process is error prone and difficult to maintain. 

Would adding the build ID as a param to these URLs meet the requirement?

Then, the servers can provide the licensed data to whatever they want to define as "official" builds (identifying them by ID)?

Comment 10

[Mike Connor [:mconnor]]

I'm not sure that jumping through hoops here is really going to achieve much if we're just adding a param, that's easily sniffed even with LiveHTTPHeaders.  I understand the legal aspect is serious, but it sounds like its barely raising the bar, at the expense of a lot of painful and risky build changes.

I think tying this to branding at build time (where the extra param is pulled from the properties file) should be sufficient, given that all of the branded bits have licensing restrictions and legal implications.  There's no reasonable way to restrict this on the technical level, so I think legal restrictions are our next recourse.

Comment 11

[Brian Rakowski]

I do understand that this seems impractical but we need to be careful navigating these legal issues (sorry).

My understanding is that there is some closed-source software in official builds of Firefox. Can we leverage the approach used there? 

I'm not clear on what the legal solution you've alluded to is. Can you elaborate?

Comment 12

[Mike Schroepfer]

-> Preed to implement as discussed today

Comment 13

[J. Paul Reed [:preed]]

Attachment: Turn official URLs off for non-official builds, take 1

Comment 14

[Robert Strong (they/them - no direct email)]

reed, I think this would be better if it set the values via code rather than through pre-processing... I'm going to leave it to bsmedberg to make the call on that.

Comment 15

[Benjamin Smedberg]

Comment on attachment 232210 [details] [diff] [review]
Turn official URLs off for non-official builds, take 1

We should be using nsIXULAppInfo to get the version information, and avoid hardcoding the buildid in more files. Do you want me to write a patch to do that?

Comment 16

[J. Paul Reed [:preed]]

Attachment: Turn official URLs off for non-official builds, take 2

Now, with less preprocessing!! :-)

Comment 17

[J. Paul Reed [:preed]]

This will go in as soon as Tony says it's Ok; they need to make a change server side for this to work right.

Comment 18

[Tony Chang (Google)]

(In reply to comment #17)
> This will go in as soon as Tony says it's Ok; they need to make a change server
> side for this to work right.

The server changes have been made.  This is safe to commit.  Thanks!

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232623 [details] [diff] [review]
Turn official URLs off for non-official builds, take 2

a=drivers for MOZILLA_1_8_BRANCH

Comment 20

[J. Paul Reed [:preed]]

Checked in.

Comment 21

[Tony Chang (Google)]

I'm not seeing this on trunk.  We want it there too, right?

Comment 22

[J. Paul Reed [:preed]]

(In reply to comment #21)
> I'm not seeing this on trunk.  We want it there too, right?

We do... I'll get it in there.

Comment 23

[J. Paul Reed [:preed]]

Checked into trunk.

Comment 24

[Jeff Walden [:Waldo]]

For the record, this checkin backed out part of the previous checkin to browser/app/profile/firefox.js (I've backed out that portion of the change).

Comment 25

[J. Paul Reed [:preed]]

(In reply to comment #24)
> For the record, this checkin backed out part of the previous checkin to
> browser/app/profile/firefox.js (I've backed out that portion of the change).

D'oh!

Sorry about that...

Comment 26

[J. Paul Reed [:preed]]

Attachment: Fix trunk merge clobberage to firefox.js

This patch addresses the issues found in comment 24; it reverts the busted merging I did, so the the current rev of firefox.js will be rev 1.142 + my patch from the branch + flamingice's patch (rev 1.145).

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 234465 [details] [diff] [review]
Fix trunk merge clobberage to firefox.js

>Index: firefox.js

>-// 0 = blank, 1 = home (browser.startup.homepage), 2 = last visited page, 3 = resume previous browser session
>-// The behavior of option 3 is detailed at: http://wiki.mozilla.org/Session_Restore
>+// 0 = blank, 1 = home (browser.startup.homepage), 2 = last
>+// XXXBlake Remove this stupid pref

You should probably just leave this as-is (i.e. not check this hunk in on the trunk), this part was meant to be checked in on the trunk but forgotten (see bug 340898 comment 10). Or you could leave it to be corrected separately, if you want to keep this a straightforward reversal of your original clobbering.

r=me based on http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=firefox.js&branch=&root=/cvsroot&subdir=mozilla/browser/app/profile&command=DIFF_FRAMESET&rev1=1.142&rev2=1.143