1275679 - Add option to the HTTP Networking preferences to disable Lightning user-agent addition

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[rsx11m]

Lightning is now shipped and enabled by default. It adds an extra token "Lightning/4.7.x" at the end to the UA string, which comes from the calendar.useragent.extra preference. Clearing that pref removes that token.

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

In the HTTP Networking prefpane, there is already an "Advertise Firefox compatibility" checkbox. Thus, a similar "Advertise Lightning installation" should be added, but hidden when Lightning is not installed or disabled (i.e., calendar.useragent.extra doesn't exist).

Comment 1

[rsx11m]

Test URL echoing back the UA string: http://www.delorie.com:81/some/url.txt

Comment 2

[rsx11m]

A question for the Calendar team: Looking at 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);

So, is it known if setRequestHeader() strips trailing spaces? If not, calendar.useragent.extra = "" would produce a trailing space in the HTTP User-Agent header, thus still "leaking" some information that Lightning is installed. The URL in comment #1 doesn't show any such trailing space, but it may have been stripped at the server side, thus it's ambiguous what is actually sent out.

Assuming that you don't want another pref to explicitly enable/disable the UA addition, the if() statement a few lines up should probably be extended with a calendar.useragent.extra != "" test.

I can file this as a Calendar bug and attach a patch, if you want.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

The whole reason this was added is to let caldav server vendors know which Lightning version is being used, so that they can block specific versions causing problems (we've had an endless request loop in one version), or to deploy workarounds for client behavior. If you disable this by default, then server vendors can do nothing other than block a whole Seamonkey version. I'd advise against being able to disable this via the UI, but if you really want this to happen I would be ok with a patch to Lightning that does not add the space if the UA string is empty.

Comment 4

[rsx11m]

Thanks for the quick reply. For the same reason the "Firefox/x.y" token was introduced, given that quite a few websites choke on SeaMonkey's UA string and produced wrong code. Nevertheless, some people want to be able to switch those off, hence the pref and the respective UI was introduced. For orthogonality, I think we should have the same for the Lightning UA addition.

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> The whole reason this was added is to let caldav server vendors know which
> Lightning version is being used

Ok, so if someone isn't using a remote calendar, disabling the UA addition shouldn't have any impact.

> If you disable this by default, then server vendors can do nothing

We don't want to change the default, it will stay the same - just provide an option in the Advanced preferences section to be able to switch both Firefox and Lightning compatibility additions off.

Comment 5

[rsx11m]

Attachment: Proposed patch

This adds a checkbox "Advertise Lightning installation" if it has been determined that Lightning is installed (based on calendar.useragent.extra having a default value; i.e., it is not sufficient if it only has a user-set value from a previously enabled Lightning installation).

The checkbox is cleared if the pref contains an empty string, checked otherwise. If unchecked, the pref is set to an empty string; if checked, the pref is reset.

Based on Fallen's concerns above, I have added a warning underneath that groupbox about not functioning properly if unchecked, and expanded the Help content respectively.

Comment 6

[rsx11m]

... and what's the reason for minusing the ui-r?

Comment 7

[Philip Chee]

Comment on attachment 8756999 [details] [diff] [review]
Proposed patch

(In reply to rsx11m from comment #6)
> ... and what's the reason for minusing the ui-r?
Strange. I had a ui-r all typed out and ready to go. Fortunately I saved a copy....


OK, personally I think this is a bad idea even if we are shipping Lightning by deafault. However the following is a UI review:

> "Advertise Lightning installation"
I think you should flip the pref and say something like:
Don't show Lightning information in the User Agent.
(this is just an example. If you can think of something better, please do).


Drive-by code comments:

> -  var prefHTTPVersion = document.getElementById("network.http.version");
> +  let prefHTTPVersion = document.getElementById("network.http.version");
Please keep to var all over the place.

> +      <checkbox id="uaLightningShow"
> +                hidden="true"
> +                label="&prefLightningShow.label;"
> +                accesskey="&prefLightningShow.accesskey;"

> +                oncommand="OnLightningChanged(this.checked);"/>
I think you can use onsynctopreference / onsyncfrompreference here:

onsynctopreference="return this.checked? prefLightningUA.defaultValue : '';"/>
Or the other way round if you've flipped the checkbox.

https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Preferences_system/New_attributes#onsyncfrompreference.2Fonsynctopreference

Comment 8

[Ian Neal]

Does the calendar.useragent.extra get added to all headers or just requests from lightning?

Comment 9

[rsx11m]

(In reply to Philip Chee from comment #7)
> > "Advertise Lightning installation"
> I think you should flip the pref and say something like:
> Don't show Lightning information in the User Agent.

This would be inconsistent with the "Advertise Firefox compatibility" checkbox just above it.
By default, both boxes are checked = both "Firefox/x.y" and "Lightning/a.b" are included.
If you uncheck one of the boxes, the respective string isn't sent.

Thus, I'd prefer to keep the positive logic as proposed, with both boxes checked by default.
Alternatively, the Firefox checkbox could be inverted as well to have both on negative logic.

> > -  var prefHTTPVersion = document.getElementById("network.http.version");
> > +  let prefHTTPVersion = document.getElementById("network.http.version");
> Please keep to var all over the place.

Ok, I just thought that "let" is now preferred and made those a drive-by fix.

> > +                oncommand="OnLightningChanged(this.checked);"/>
> I think you can use onsynctopreference / onsyncfrompreference here:

I don't want to touch the pref if someone has a custom string defined, thus avoiding to override it by accident. Aren't onsynctopreference / onsyncfrompreference executed every time the "Ok" button is hit? Or do I read "onsynctopreference is called when preferences are being written" wrong on that web page?

The oncommand event should only fire if someone explicitly changes the checkbox rather than routinely, thus leaving custom values entered by about:config intact.

(In reply to Ian Neal from comment #8)
> Does the calendar.useragent.extra get added to all headers or just requests
> from lightning?

Unfortunately it's injected into /every/ HTTP request, thus also those just coming from the browser.
You can check using the URL to the test site provided in this bug report.

Ideally, Lightning would only send it when actually submitting an HTTP request from its own code.

Comment 10

[rsx11m]

(In reply to Philip Chee from comment #7)
> OK, personally I think this is a bad idea even if we are shipping Lightning by default.

You don't say why, but following Fallen's concerns, there sure are plenty of ways to shoot your own foot. Websites can malfunction if you disable the Firefox compatibility token, yet there is a checkbox for it. Lightning is on the same level if you access calendar provider, but seems inconsequential as long as you don't have any remote calendar configured. I don't see why it shouldn't be possible for such users to disable that token if they desire so. It's better than having people google around for hacks what to do (and we can have the Warning and Help info to explain why/when you'd want to keep it).

Talking of breaking websites, you can do that already by simply enabling tracking protection. Quite a few websites I ran into seem to have hard dependencies to tracking sites which are blocked, thus those websites produce pages which are reduced in functionality or just broken entirely.

(In reply to rsx11m from comment #9)
> Alternatively, the Firefox checkbox could be inverted as well to have both on negative logic.

So, something like:

+--User Agent String---
| [ ] Don't advertise Firefox compatibility to websites
| [ ] Don't advertise Lightning installation to websites
+----------------------
WARNING: ...

Keep in mind though that we have all positive logic with the "Enable..." radios and checkboxes in the Connection Options as well. If we go with the positive logic, I sure can add "to websites" to the current labels to make it clearer whom something is advertised to.

Comment 11

[rsx11m]

(In reply to Philip Chee from comment #7)
> > +                oncommand="OnLightningChanged(this.checked);"/>
> I think you can use onsynctopreference / onsyncfrompreference here:
> onsynctopreference="return this.checked? prefLightningUA.defaultValue : '';"/>

The other problem I see here is the ambiguous behavior for onsynctopreference / onsyncfrompreference when there is no default value, as is in particular the case when Lightning is *not* enabled and in which case the pref should neither be read nor modified (or worse, created). Sure, I can catch that case for onsyncfrompreference the same way I'm checking for the default value in the current explicit JavaScript code, but it's still a question what onsynctopreference will do.

By any means, my proposed code may be longer and potentially less efficient, but it is certainly more easily read and understood than the onsynctopreference / onsyncfrompreference magic. ;-)

Comment 12

[Philip Chee]

Comment on attachment 8756999 [details] [diff] [review]
Proposed patch

> Thus, I'd prefer to keep the positive logic as proposed, with both boxes checked by default.
I see. I guess that makes sense. OK.

> I don't want to touch the pref if someone has a custom string defined, thus 
> avoiding to override it by accident. Aren't onsynctopreference / 
> onsyncfrompreference executed every time the "Ok" button is hit? Or do I 
> read "onsynctopreference is called when preferences are being written" wrong 
> on that web page?

> The oncommand event should only fire if someone explicitly changes the 
> checkbox rather than routinely, thus leaving custom values entered by 
> about:config intact.

I see. However from you code I inferred that you didn't care about destroying any custom values.

+function OnLightningChanged(value)
+{
+  let prefLightningUA = document.getElementById("calendar.useragent.extra");
+
+  if (value)
+    prefLightningUA.reset();
+  else
+    prefLightningUA.value = "";
+}

As far as I can tell (with a bit of testing) toggling the check box off then back on destroys any custom setting.
Perhaps you should save any custom setting somewhere during the onLoad() ?

My ideal situation: Lightning supplies an overlay that adds the preference and the checkbox and the logic to {pref-http.xul|js}
But maybe Fallen disagrees.

OK ui-r+ but please fix your code not to zap custom strings.

Comment 13

[rsx11m]

(In reply to Philip Chee from comment #12)
> I see. However from you code I inferred that you didn't care about
> destroying any custom values.

I don't want to alter any custom values unless the checkbox is toggled. I see you point though that someone toggles the box and then - oops - reverts it shouldn't lose that custom setting.

> Perhaps you should save any custom setting somewhere during the onLoad() ?

If we only want to cover the case where someone toggles and then reverts the checkbox, that should be straight-forward with some global variable. However, if you want to cover the case that someone unchecks the box, closes the pref window, then returns later to the prefpane to re-enable the UA addition and expects the same custom value to re-appear too, I'd have to save it into some parallel pref I'd guess, thus increasing the necessary logic quite a bit.

Comment 14

[rsx11m]

Attachment: Keep user-defined value (v2)

This retains any user-defined pref value other than an empty string as long as the prefpane is open while the checkbox is toggled and then reversed.

I've also undone the change of "var" to "let" in the other functions.

Comment 15

[Ian Neal]

Comment on attachment 8758381 [details] [diff] [review]
Keep user-defined value (v2)

>+++ b/suite/common/pref/pref-http.js
>+function OnLightningChanged(value)
>+{
>+  let prefLightningUA = document.getElementById("calendar.useragent.extra");
>+
>+  if (value)
>+    if (gLightningUAold)
>+      prefLightningUA.value = gLightningUAold;
>+    else
>+      prefLightningUA.reset();
>+  else
>+    prefLightningUA.value = "";
>+}
Would it be any better writing:

if (value && !gLightningUAold)
  prefLightningUA.reset();
else
  prefLightningUA.value = gLightningUAold || "";

Is there a need for a comment?

r/a=me with that addressed/tested

Comment 16

[rsx11m]

(In reply to Ian Neal from comment #15)
>   prefLightningUA.value = gLightningUAold || "";

This doesn't work, with value(checked)="false" it should always be prefLightningUA.value=""
Thus, the following should do, adding yet another geeky logic-on-string operation:

>   prefLightningUA.value = (value && gLightningUAold) || "";

Also, the "value" parameter should probably be "aChecked" rather, following common conventions.

> Is there a need for a comment?

Given the obscureness of that code mangling two conditions into one, I'd say yes. ;-)

Comment 17

[rsx11m]

Attachment: Remaining issues addressed (v3)

Changes in pref-http.js only, all other files remain the same:

 - Unbitrotted for comm-central changeset f050dc3ed12c (bug 1280817);
 - added a ".trim()" to treat whitespace-only strings equivalent to empty (bug 1275941);
 - changed OnLightningChanged() parameter from "value" to "aChecked" for better conformance;
 - changed if()/else statements per comment #15 as corrected with comment #16;
 - added comments for both CheckLightningUA() and OnLightningChanged().

Since IanN is currently rather busy, I'm asking Ratty for re-review of these follow-up changes.

Comment 18

[Philip Chee]

Comment on attachment 8764422 [details] [diff] [review]
Remaining issues addressed (v3)

r=me. With some language nits:

> +// Lightning adds a UA token if calendar.useragent.extra is not empty
Complete sentences should end with a period.

> +// Checkbox visible if Lightning is installed and enabled
Checkbox _is_ visible if Lightning is installed and enabled

> +// checked if the pref has any non-whitespace character
Checkbox is checked if the pref _contains_ any non-whitespace character

> I've also undone the change of "var" to "let" in the other functions.
OK I totally missed this. Since the existing content of this file uses "var" consistently, any new code should also use "var" instead of "let". Sorry about that.

> +  let prefLightningUA = document.getElementById("calendar.useragent.extra");
> +  let boxLightningUA  = document.getElementById("uaLightningShow");

Comment 19

[rsx11m]

Attachment: Patch as checked in (v4)

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/09e58bc00a89 with changes made as requested.

(In reply to Philip Chee from comment #18)
> OK I totally missed this. Since the existing content of this file uses "var"
> consistently, any new code should also use "var" instead of "let".

I thought that the policy was to use "let" for any new code but leave "var" in old code untouched. In this case it shouldn't make an difference, but a bit fuzzy anyway.