When calendar.useragent.extra is an empty string, Lightning still adds a space to the user-agent string

RESOLVED FIXED in 5.2

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Tracking

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Lightning adds an extra token "Lightning/4.7.x" at the end to the UA string, which comes from the calendar.useragent.extra preference. Some users don't want this addition, and it serves no function if no remote calendar is used. Clearing that pref to an empty string removes that token.

This only affects HTTP traffic, it's not included in the UA header of mail/news messages.

(Quoting rsx11m from bug 1275679 comment #2)
> https://hg.mozilla.org/comm-central/annotate/4083a38d6dc8/calendar/base/src/ calCalendarManager.js#l150
> the token is simply concatenated if the pref is set by 
> 
> >  httpChannel.setRequestHeader("User-Agent", ua + " " + calUAString, false);
(Assignee)

Comment 1

2 years ago
Created attachment 8756899 [details] [diff] [review]
Proposed fix

Adds calUAString != "" requirement for performing httpChannel.setRequestHeader().
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #8756899 - Flags: review?(philipp)
(Assignee)

Comment 2

2 years ago
Created attachment 8756907 [details] [diff] [review]
More general patch

On a second thought, let's verify that the string to be added actually contains any non-whitespace character (which resolves false for an empty string), thus also ignoring a " " userpref.
Attachment #8756899 - Attachment is obsolete: true
Attachment #8756899 - Flags: review?(philipp)
Attachment #8756907 - Flags: review?(philipp)
Comment on attachment 8756907 [details] [diff] [review]
More general patch

Review of attachment 8756907 [details] [diff] [review]:
-----------------------------------------------------------------

Give the below I'm not sure we should even push this patch. An empty string should not trigger a space to be added, and if the user has a whitespace in the preference, then that is user error (or intention!?). I wouldn't see a need to push a patch with just the trim() call.

Can you clarify?

::: calendar/base/src/calCalendarManager.js
@@ +151,5 @@
>                      // NOTE: For some reason, this observer call doesn't have
>                      // the "cal" namespace defined
>                      let ua = httpChannel.getRequestHeader("User-Agent");
>                      let calUAString = Preferences.get("calendar.useragent.extra");
> +                    if (calUAString && !/^\s*$/.test(calUAString) && !ua.includes(calUAString)) {

I'd suggest just using this instead of a regex:
let calUAString = Preferences.get("calendar.useragent.extra").trim();

Also, I did a quick check to confirm on scratchpad:

var x = "";
if (x) { alert('hi'); }

The alert never shows. That would mean with the trim,

if (calUAString && !ua.includes(calUAString))

should actually be sufficient to ignore empty strings and whitespace strings.
Attachment #8756907 - Flags: review?(philipp) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8757290 [details] [diff] [review]
Proposed patch (v3)

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> Also, I did a quick check to confirm on scratchpad:
> 
> var x = "";
> if (x) { alert('hi'); }
> 
> The alert never shows.

Sorry, clearly insufficient testing on my end... :-\

You are right, both if(null) and if("") yield false in JavaScript whereas if(" ") yields true. I've got distracted by the behavior in C where if(NULL) is false but if("") yields true. In addition to that, ua.includes(calUAString) yields true if the test is an empty string (which is always contained in any string, though I'm not sure if that's standard behavior), thus we should be double-safe here.

> I'd suggest just using this instead of a regex:
> let calUAString = Preferences.get("calendar.useragent.extra").trim();

This would have the advantage that extra spaces around a valid UA addition would be removed.

> Give the below I'm not sure we should even push this patch. An empty string
> should not trigger a space to be added, and if the user has a whitespace in
> the preference, then that is user error (or intention!?). I wouldn't see a
> need to push a patch with just the trim() call.

I see your point. Per https://tools.ietf.org/html/rfc7231#section-5.5.3 the string is defined as

   User-Agent      = product *( RWS ( product / comment ) )
   product         = token ["/" product-version]
   product-version = token

thus the product as such doesn't contain any spaces. On the other hand, RWS can be 1+ white spaces. While a parser should therefore be able to handle redundant white spaces (we know how good websites are with reading UA strings, cough), there is also the instruction to limit complexity of the UA string for reasons of latency and fingerprinting (where a custom token obviously is an excellent fingerprinting target). Given that, I'd tend to trim left/right spaces anyway to keep equal distances between tokens. I wouldn't remove whitespaces within calendar.useragent.extra in case someone wants to just add another token, e.g., "Lightning/4.7.1 MyStuff/0.1" where token separation matters. A comment would also have delimiting "(...)" parentheses, where we wouldn't strip any space within that comment.

Anyway, so here is the patch for the trimming of the UA string before adding it, your call. I've added a comment what the if() statement actually tests before anybody else falls into the same trap.
Attachment #8756907 - Attachment is obsolete: true
Attachment #8757290 - Flags: review?(philipp)
Comment on attachment 8757290 [details] [diff] [review]
Proposed patch (v3)

Review of attachment 8757290 [details] [diff] [review]:
-----------------------------------------------------------------

Please use "" as the second argument for Preferences.get() so that it can't ever return undefined, otherwise the trim() may fail. I'm ok with this so r+, but given the calendar.useragent.extra is a hidden pref I'm really inclined to WONTFIX this because if the (power-)user makes a mistake and turns this into a single space, then this is the user's issue. We don't validate all the other hidden prefs either.
Attachment #8757290 - Flags: review?(philipp) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8761858 [details] [diff] [review]
Patch for checkin (v4)

Thanks for the review. I still somewhat disagree with your line of argumentation, though. True, it's a hidden pref (which SeaMonkey would partially expose by bug 1275679, however in a very controlled way), but the content of that pref is in turn used to form /part/ of the UA string (in contrast to general.useragent.override which is used to rewrite the UA string entirely and thus should be taken literally). So, it's our choice if we insert it in a way that redundant leading or trailing spaces are ignored/removed. That's not validating the pref per se.

I'm a bit puzzled by an r+ with a notion to WONTFIX, but I take it that I can push this to trunk unless there are any other concerns.
Attachment #8757290 - Attachment is obsolete: true
Attachment #8761858 - Flags: review+
(In reply to rsx11m from comment #6)
> I'm a bit puzzled by an r+ with a notion to WONTFIX, but I take it that I
> can push this to trunk unless there are any other concerns.
My opinion on this topic is not strong enough that I would insist on the WONTFIX, therefore I gave r+ since I figured you would want to take this patch if you don't agree to my argumentation. Go ahead and push.
(Assignee)

Comment 8

2 years ago
Ok, pushed as http://hg.mozilla.org/comm-central/rev/bdf682a3f887 then.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.2
You need to log in before you can comment on or make changes to this bug.