591125 - Sanitize the app name for the UA string

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Daniel Veditz [:dveditz]]

Bug 581008 removes a minor sanitization step for the application Displayname when used in the user-agent string

-APP_UA_NAME = $(shell echo $(MOZ_APP_DISPLAYNAME) | sed -e's/[^A-Za-z]//g')

nsHTTPChannel::Init() needs to add back some basic sanitization. In the new code the UA name can come from either MOZ_APP_UA_NAME or from application.ini, either of which might be reused elsewhere and not follow the restrictions on acceptable characters in a UA string.

At a minimum we need to strip out spaces (or perhaps replace with an underscore) because Gecko-based products might well have a space in the name. Ctrl chars and "separators" are also illegal according to rfc 2616. So far I guess we've gotten by with the above sed regexp, but I think 0-9 plus hyphen and underscore should be added.

non-ascii characters are a problem, I can easily imagine a Gecko-based product with a completely non-Latin name for some local market. If we strip non-ascii characters to comply with the spec we could end up with an empty product name :-(

1) we could just strip it to nothing, and that app would have to create a separate ascii name to put in MOZ_APP_UA_NAME. Is that feasible for a XULRunner app which only has the application.ini mechanism? Does the name in application.ini show up in the UI anywhere?

2) we could ignore characters with the high-bit set. The API is ACString so either localization isn't supported anyway, or it's expected to be UTF-8 which will work in practice even if it technically violates the spec.

Comment 1

[Dão Gottwald [:dao]]

Apparently the name must be ASCII according to https://developer.mozilla.org/en/nsIXULAppInfo

Comment 2

[Dão Gottwald [:dao]]

The spec permits US-ASCII 33-126 except ()<>@,;:\"/[]?={}. We probably don't need to care about non-printable characters here...

Comment 3

[Dão Gottwald [:dao]]

Attachment: proposed patch

MOZ_APP_UA_NAME explicitly exists for the UA string, so we probably don't need to sanitize this? We didn't do it for the various UA prefs either.

Comment 4

[Ben Bucksch (:BenB)]

Dao,
please use a whitelist, not a blacklist, see bug 581008 comment 112.

> MOZ_APP_UA_NAME explicitly exists for the UA string, so we probably
> don't need to sanitize this?
> We didn't do it for the various UA prefs either.

... and many developers got it wrong, creating invalid UA strings, so if it's not hard, please validate MOZ_APP_UA_NAME as well.

Comment 5

[Ben Bucksch (:BenB)]

dveditz, nice write-up above. exactly right.
FWIW, the fallback, if the stripping leaves an empty string, could just be to add no product token.

Comment 6

[Dão Gottwald [:dao]]

We don't need a fallback since nsIXULAppInfo.name is ASCII. Protecting against a name that consists only of special characters seems pointless. Same goes for whitelist vs. blacklist: I was asking for a way to whitelist characters when I didn't know about nsIXULAppInfo's restrictions.

Comment 7

[[e-mail]]

Comment on attachment 469815 [details] [diff] [review]
proposed patch

r=dwitte

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/2514b1548e0c

Comment 9

[Daniel Veditz [:dveditz]]

Attachment: Sanitize MOZ_APP_UA_NAME too

If the name comes from MOZ_APP_UA_NAME then it is not sanitized. The original makefile-based sanitization tried to deal with this case specifically so here's an alternate patch.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> Created attachment 471558 [details] [diff] [review]
> Sanitize MOZ_APP_UA_NAME too
> 
> If the name comes from MOZ_APP_UA_NAME then it is not sanitized. The original
> makefile-based sanitization tried to deal with this case specifically so here's
> an alternate patch.

browser/app/Makefile.in rightfully did it it because it reused MOZ_APP_DISPLAYNAME --- that's not happening here.

Comment 11

[Ben Bucksch (:BenB)]

Comment on attachment 471558 [details] [diff] [review]
Sanitize MOZ_APP_UA_NAME too

Dao, please see comment 4, second part.

Comment 12

[Dão Gottwald [:dao]]

I don't see why we should arbitrarily sanitize this when we don't do it for other build options or pref values. Is "many developers got it wrong" based on real data? Is it somehow specific to MOZ_APP_UA_NAME?

MOZ_APP_UA_NAME doesn't have the same restrictions as nsIXULAppInfo, so if you wanted to be correct, this would be more involved than the proposed patch, I think.

Comment 13

[Ben Bucksch (:BenB)]

> Is "many developers got it wrong" based on real data? 

Yes, I have seem many, many UA strings like "... Firefox/3.0 Fred's Cookie Toolbar/1.0"

Comment 14

[Dão Gottwald [:dao]]

That's an add-on carelessly adding its cruft with an .extra.* pref, which won't happen anymore. Have you seen apps get their own token wrong, i.e. the Firefox/3.0 part?

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 471558 [details] [diff] [review]
Sanitize MOZ_APP_UA_NAME too

First of all, I don't think there's anything wrong with letting the app be responsible for the validity of MOZ_APP_UA_NAME if it defines it.

Secondly, rather than trying to sanitize MOZ_APP_UA_NAME, letting the build fail when it contains invalid parts would be more appropriate. Otherwise we'd be building a UA string that the vendor didn't expect.

Last but not least, there will be very limited use for MOZ_APP_UA_NAME anyway, because appinfo.name will do the right thing most of the time. The only user right now is Fennec, because it wants to be Firefox in appinfo.name (for whatever reason) but Fennec in the UA string (for whatever reason).