Closed Bug 591125 Opened 11 years ago Closed 11 years ago

Sanitize the app name for the UA string

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: dveditz, Assigned: dao)

References

Details

Attachments

(2 files)

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.
Blocks: 581008
Apparently the name must be ASCII according to https://developer.mozilla.org/en/nsIXULAppInfo
The spec permits US-ASCII 33-126 except ()<>@,;:\"/[]?={}. We probably don't need to care about non-printable characters here...
Attached patch proposed patchSplinter Review
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.
Attachment #469815 - Flags: review?(dwitte)
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.
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.
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 on attachment 469815 [details] [diff] [review]
proposed patch

r=dwitte
Attachment #469815 - Flags: review?(dwitte) → review+
Attachment #469815 - Flags: approval2.0?
Attachment #469815 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/2514b1548e0c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
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.
Attachment #471558 - Flags: review?(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 on attachment 471558 [details] [diff] [review]
Sanitize MOZ_APP_UA_NAME too

Dao, please see comment 4, second part.
Attachment #471558 - Flags: review+
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.
> 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"
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 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).
Attachment #471558 - Flags: review?(dao) → review-
You need to log in before you can comment on or make changes to this bug.