1316204 - Don't include non-ASCII characters (e.g., emoji) in User-Agent strings

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect, P1)

Description

[Farhan Patel [:farhan]]

The emoji I've added to the nightly build bundle name is breaking the testSyncUA test. What do we want to do with this test? 

I really like the emojis! So lets try to fix/disable this test for nightly only?
How important is this test? Cause of course emojis are more important 

Comment 1

[Farhan Patel [:farhan]]

edit: I was being sarcastic at the end there. I even added an emoji to signal that. But bugzilla removed it. :(

Comment 2

[Stephan Leroux [:sleroux]]

Darn. I like the emojis too. We might be able to make the regex less strict in its checking or find a way to match against a unicode character for the emoji. Looking at the regex, I'm sure weakening the the regex check devalues the test a bit but I'm not sure if it's that big of a deal.

A more forgiving regex could be: ^Firefox-iOS-Sync/[0-9\\.]+ .* 
This would match the prefix of the strings and version and just leave the app name part to be whatever.

Comment 3

[Richard Newman [:rnewman]]

The User-Agent header must only contain ASCII.

See e.g., Bug 1188550.

Don't weaken the test, fix the code.

Comment 4

[Richard Newman [:rnewman]]

A more thorough response:

RFC 2616 specifies that User-Agent consists of product and version strings, which are composed of tokens. Token is defined as


       token          = 1*<any CHAR except CTLs or separators>
       separators     = "(" | ")" | "<" | ">" | "@"
                      | "," | ";" | ":" | "\" | <">
                      | "/" | "[" | "]" | "?" | "="
                      | "{" | "}" | SP | HT

       CHAR           = <any US-ASCII character (octets 0 - 127)>

       CTL            = <any US-ASCII control character
                        (octets 0 - 31) and DEL (127)>


       product         = token ["/" product-version]
       product-version = token

       User-Agent     = "User-Agent" ":" 1*( product | comment )

   Examples:

       User-Agent: CERN-LineMode/2.15 libwww/2.17b3


This is why Android has MOZ_APP_ASCII_DISPLAYNAME, so that the User-Agent string it sends is plain ASCII.

If you don't do this correctly, you make Firefox diverge from the HTTP specs, and that's a bad thing.

Comment 5

[Farhan Patel [:farhan]]

Attachment: Pull Request

Lets remove the emoji as failing tests are not a good thing to have.

Comment 6

[Brian Nicholson (:bnicholson)]

Why not just fix the UA while keeping the emoji in the app name?

Comment 7

[Farhan Patel [:farhan]]

We could do that. If it was something important it might have been worth it to make changes to how we handle UA and display names. But for something small like this the path of least resistance is just to revert it back to normal. :(

Comment 8

[Brian Nicholson (:bnicholson)]

Heh, OK. I think the fix would be pretty straightforward if we wanted to keep them though -- we could just fall back to "Firefox" instead of the app name if the AppConstants.BuildChannel == Aurora/Beta/Nightly. Up to you! :winkemoji:

Comment 9

[Farhan Patel [:farhan]]

Ah I see. I'll look into this. But for now, I'll close this one. 

master https://github.com/mozilla-mobile/firefox-ios/commit/5d748436938c2088756ade1009cf5f4944486247

Comment 10

[Stephan Leroux [:sleroux]]

v6.x 3a0227a