274928 - Firefox abuses vendor portions of user agent string

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Redistributors of Firefox should be able to add their vendor name and versions
of distributions to the user agent string, but Firefox currently steals vendor
and vendorSub.

I talked to dbaron and we came up with either adding a vendor2 and vendorSub2 or
creating app and appSub for Firefox to use.  I prefer the latter to keep
consistency across the apps.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

This is the patch I added to our firefox RPMs.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 168878 [details] [diff] [review]
Patch

>@@ -99,12 +99,22 @@ interface nsIHttpProtocolHandler : nsIPr

Rev UUID?

>@@ -520,12 +522,14 @@ nsHttpHandler::BuildUserAgent()
>                            mOscpu.Length() +
>                            mLanguage.Length() +
>                            mMisc.Length() +
>                            mProduct.Length() +
>                            mProductSub.Length() +
>                            mProductComment.Length() +
>+                           mApp.Length() +
>+                           mAppSub.Length() +
>                            mVendor.Length() +
>                            mVendorSub.Length() +
>                            mVendorComment.Length() +
>                            22);

24?



One other thing:  I didn't realize when I was talking to you that the Mozilla
and 5.0 were currently called mAppName and mAppVersion.  Adding something
called mApp and mAppSub seems a little confusing, but maybe it's OK.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #2)
> (From update of attachment 168878 [details] [diff] [review] [edit])
> >@@ -99,12 +99,22 @@ interface nsIHttpProtocolHandler : nsIPr
> 
> Rev UUID?

Yeah, should do that.  :-\


> 
> >@@ -520,12 +522,14 @@ nsHttpHandler::BuildUserAgent()
> >                            mOscpu.Length() +
> >                            mLanguage.Length() +
> >                            mMisc.Length() +
> >                            mProduct.Length() +
> >                            mProductSub.Length() +
> >                            mProductComment.Length() +
> >+                           mApp.Length() +
> >+                           mAppSub.Length() +
> >                            mVendor.Length() +
> >                            mVendorSub.Length() +
> >                            mVendorComment.Length() +
> >                            22);
> 
> 24?


Oops, right.  Good catch.


> One other thing:  I didn't realize when I was talking to you that the Mozilla
> and 5.0 were currently called mAppName and mAppVersion.  Adding something
> called mApp and mAppSub seems a little confusing, but maybe it's OK.

Couldn't think of anything else at the time.  product and productSub are already
taken by Gecko/BUILDID.  Maybe prog/progSub?  ::shrug::  Other suggestions welcome.

Comment 4

[Darin Fisher]

Comment on attachment 168878 [details] [diff] [review]
Patch

can these values not be gotten via prefs?  i'm not happy to see
nsIHttpProtocolHandler changing.  it is one of the only ways for an embedder or
extension author to find out the Gecko BuildID at runtime.  i know of at least
one embedder who is forced to use it.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So in a discussion with darin, he pointed out the idea of using enumeration of a
tree of preferences to solve this problem.  This allows enumeration of an
arbitrary number of comments.

I suspect the cleanest way to do this would be to get rid of the vendor and
vendorSub prefs and add a comment.* tree of prefs, such that Firefox would set
the pref:

pref("general.useragent.comment.firefox", "Firefox/@APP_VERSION@");

and Fedora could set the pref

pref("general.useragent.comment.fedora", "Fedora Core/3");

etc.


(Does nsIPrefBranch and the default branch stuff mean this also provides a way
to add the string programmatically by adding to the default branch, which would
allow an extension to add only when it's being used?)

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Seems like a good idea.  Does order matter?  Firefox potentially can be in the
middle of several items.

> (Does nsIPrefBranch and the default branch stuff mean this also provides a way
> to add the string programmatically by adding to the default branch, which would
> allow an extension to add only when it's being used?)

Yep, that can be done.  I'll work on getting something hacked up.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should probably be consistent about the order (i.e., given a set of prefs we
should always produce the same output), but I don't think it should matter.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

NS_STRINGIFY didn't work and probably wasn't tested.

I'm not sure whether I want to use .extra. or .vendor., but .extra. seems more
generic and also doesn't have any potential issues with having both .vendor and
.vendor.* .

I probably need to test the app changes here a bit more (like making sure they
all compile).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

oops, wrong patch

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 179890 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/src/nsHttpHandler.h
>Index: calendar/sunbird/app/Makefile.in
> BUILD_ID = $(shell cat $(DEPTH)/config/build_number)
> DEFINES += -DBUILD_ID=\"$(BUILD_ID)\"

I removed these two lines as well, since I'm using NS_STRINGIFY now, and
BUILD_ID is global.

Comment 11

[Darin Fisher]

Comment on attachment 179890 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/src/nsHttpHandler.cpp

>+    if (!mExtraUA.IsEmpty()) {
>+        mUserAgent += mExtraUA;
>+    }

nit pick: this code avoids brackets when possible


>+    if (MULTI_PREF_CHANGED(UA_PREF("extra."))) {
>+        mExtraUA.Truncate();
>+
>+        // Unfortunately, we can't do this using the pref branch.
>+        nsCOMPtr<nsIPrefService> service =
>+            do_GetService(NS_PREFSERVICE_CONTRACTID);

So, you should be able to get back to the nsIPrefService using
QueryInterface from the nsIPrefBranch passed to this function.
That might be slightly better.


>+            PRUint32 extraCount;
>+            char **extraItems;
>+            branch->GetChildList("", &extraCount, &extraItems);
>+            for (char **item = extraItems, **item_end = extraItems + extraCount;
>+                 item < item_end; ++item) {
>+                nsXPIDLCString valStr;
>+                branch->GetCharPref(*item, getter_Copies(valStr));
>+                if (!valStr.IsEmpty()) {
>+                    mExtraUA += NS_LITERAL_CSTRING(" ") + valStr;
>+                }

nit: remove extra brackets.

It looks like extraItems is leaked.

also, it's interesting that the order in which these extra values
are appended to the UA string is sort of arbitrary, or are we 
relying on some sorting done by the preferences code?  i think it
might be nice to ensure that these are always listed in a consistent
order even though people shouldn't really care.


>Index: xpcom/base/nscore.h

>+#define NS_STRINGIFY_HELPER(x_) #x_
>+#define NS_STRINGIFY(x_) NS_STRINGIFY_HELPER(x_)

So, I'm curious... why is this needed?


>Index: browser/app/profile/firefox.js

>+pref("general.useragent.extra.firefox", "Firefox/@APP_VERSION@");

We should make the same change to
mozilla/xulrunner/examples/simple/simple-prefs.js
before too many people start copying the old firefox way.


>Index: mail/app/Makefile.in

>+ifdef MOZ_JPROF
>+LIBS += -ljprof
>+endif

this change is unrelated right?


r=darin with these issues addressed

Comment 12

[Darin Fisher]

Did some version of Fedora Core really ship with those changes to
nsIHttpProtocolHandler?  /me hopes not...

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #11)
> So, you should be able to get back to the nsIPrefService using
> QueryInterface from the nsIPrefBranch passed to this function.
> That might be slightly better.

I tried that first and got a null pointer.

> >Index: xpcom/base/nscore.h
> 
> >+#define NS_STRINGIFY_HELPER(x_) #x_
> >+#define NS_STRINGIFY(x_) NS_STRINGIFY_HELPER(x_)
> 
> So, I'm curious... why is this needed?

Currently NS_STRINGIFY(BUILD_ID) produces "BUILD_ID".

Comment 14

[Darin Fisher]

> I tried that first and got a null pointer.

Wow.  That's interesting.  It probably has to do with when we get nsIPrefBranch
via the Observe callback.


> > So, I'm curious... why is this needed?
> 
> Currently NS_STRINGIFY(BUILD_ID) produces "BUILD_ID".

Yikes.  That has surely caused interesting problems for Firefox nightlies :-(

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This should address darin's comments.  It does a sort by pref name since the
prefs code looks like it just uses hashtable enumeration order.

I haven't actually tested this and may not be able to for a few hours since
XRemote doesn't differentiate between profiles anymore, but it does compile.

Do you mind doing r+sr as well?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I didn't have as many browser windows open as I thought, so tested now.

Comment 17

[Darin Fisher]

Comment on attachment 179919 [details] [diff] [review]
patch

r+sr=darin

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 179919 [details] [diff] [review]
patch

This makes user-agent addition much easier for distributors and reduces the
chance that they'll make binary-incomptabile changes to the HTTP code.

Comment 19

[Mike Kaply [:mkaply]]

Comment on attachment 179919 [details] [diff] [review]
patch

a=mkaply

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, actually, my calendar checkout was way out of date, since I wasn't pulling
it by default, and once I fixed that the calendar changes looked just like the
other app changes...

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2005-04-07 11:11 -0700.

I'll try to document this on
http://www.mozilla.org/build/revised-user-agent-strings.html shortly.

Comment 22

[Simon Paquet [:sipaq]]

Apparently, this change broke Sunbird (at least on Windows XP). I filed bug
289786. See bug 289786 comment 0 and bug 289786 comment 1 for more information.

Should this bug be reopened or do we want to fix this in bug 289786?

Comment 23

[Doug Wright]

This has broken at least one well-known detection script
(http://www.webreference.com/tools/browser/javascript.html) - and will probably
break many more that make the same assumptions (looking at navigator.vendor for
the string "Firefox").

Would it be possible to still make navigator.vendor return "Firefox"?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Firefox is not and never was a vendor.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And any sniffing script that distinguishes Firefox from other Geckos is
seriously broken anyway.)

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

not to mention that this checkin should not have any visible effects to web
developers...

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

(nevermind my last comment. instead, see comment 24)

Comment 28

[Bamm Gabriana]

(In reply to comment #25)
> (And any sniffing script that distinguishes Firefox from other Geckos is
> seriously broken anyway.)

Not exactly. There are some cases where it does not mean bad code. For instance, I have a version of IE Skin for Mozilla, Firefox and Netscape. All three are available on my web site. But if you click on, say, a link for the theme for Netscape using Firefox, you get an alert saying "This theme requires Netscape".   This prevents the user from getting a failed-to-install error. Besides, this does not stop the user from downloading them anyway.

bamm.gabriana.com/ieskin.html