Last Comment Bug 274928 - Firefox abuses vendor portions of user agent string
: Firefox abuses vendor portions of user agent string
Status: RESOLVED FIXED
[patch]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.8beta2
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
:
Mentors:
: 217407 (view as bug list)
Depends on:
Blocks: 289786
  Show dependency treegraph
 
Reported: 2004-12-16 08:18 PST by Christopher Aillon (sabbatical, not receiving bugmail)
Modified: 2007-10-16 17:08 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.32 KB, patch)
2004-12-16 08:21 PST, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review
patch (7.45 KB, patch)
2005-04-06 15:52 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch (21.55 KB, patch)
2005-04-06 15:57 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
darin.moz: review+
Details | Diff | Splinter Review
patch (22.85 KB, patch)
2005-04-06 21:34 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch (22.87 KB, patch)
2005-04-06 21:42 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
darin.moz: review+
darin.moz: superreview+
mozilla: approval1.8b2+
Details | Diff | Splinter Review

Description Christopher Aillon (sabbatical, not receiving bugmail) 2004-12-16 08:18:56 PST
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) 2004-12-16 08:21:13 PST
Created attachment 168878 [details] [diff] [review]
Patch

This is the patch I added to our firefox RPMs.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-12-16 10:41:46 PST
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) 2004-12-16 14:05:08 PST
(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 2004-12-16 14:21:56 PST
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-7 (busy September 14-25) 2005-02-12 09:42:48 PST
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) 2005-02-13 09:08:04 PST
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-7 (busy September 14-25) 2005-02-13 11:01:18 PST
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-7 (busy September 14-25) 2005-04-06 15:52:20 PDT
Created attachment 179889 [details] [diff] [review]
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-7 (busy September 14-25) 2005-04-06 15:57:21 PDT
Created attachment 179890 [details] [diff] [review]
patch

oops, wrong patch
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-04-06 16:16:56 PDT
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 2005-04-06 18:52:42 PDT
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 2005-04-06 18:54:39 PDT
Did some version of Fedora Core really ship with those changes to
nsIHttpProtocolHandler?  /me hopes not...
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-04-06 19:10:51 PDT
(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 2005-04-06 19:28:46 PDT
> 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-7 (busy September 14-25) 2005-04-06 21:34:20 PDT
Created attachment 179917 [details] [diff] [review]
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-7 (busy September 14-25) 2005-04-06 21:42:56 PDT
Created attachment 179919 [details] [diff] [review]
patch

I didn't have as many browser windows open as I thought, so tested now.
Comment 17 Darin Fisher 2005-04-06 22:52:35 PDT
Comment on attachment 179919 [details] [diff] [review]
patch

r+sr=darin
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-04-06 23:44:04 PDT
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] 2005-04-07 06:48:03 PDT
Comment on attachment 179919 [details] [diff] [review]
patch

a=mkaply
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-04-07 11:17:38 PDT
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-7 (busy September 14-25) 2005-04-07 11:25:35 PDT
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] 2005-04-10 04:17:51 PDT
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 2005-04-12 15:35:58 PDT
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-7 (busy September 14-25) 2005-04-12 15:51:39 PDT
Firefox is not and never was a vendor.
Comment 25 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-04-12 15:52:11 PDT
(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) 2005-04-12 16:01:07 PDT
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) 2005-05-17 15:56:05 PDT
(nevermind my last comment. instead, see comment 24)
Comment 28 Bamm Gabriana 2005-11-01 00:12:38 PST
(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
Comment 29 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-10-16 17:08:07 PDT
*** Bug 217407 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.