Opened 4 years ago

Closed 4 years ago

#13047 closed defect (fixed)

Updater should not send Kernel and GTK version

Reported by: gk Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-firefox-patch, TorBrowserTeam201409, MikePerry201409R
Cc: brade, boklm, mikeperry, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Not sure how exactly the script works that detects the .mar file the user needs to download but I guess the user should not be in a need to send the Kernel and GTK version to get the correct one. Testing a bit my update URL looks something like

https://www.torproject.org/dist/torbrowser/update/alpha/Linux_x86-gcc3/Linux%203.14-2-686-pae%20(GTK%202.24.24)/4.0-alpha-2/en-US?force=1

I somehow doubt we have an own directory for every Kernel+GTK combination. Thus, what the updater should send in my opinion is something like

https://www.torproject.org/dist/torbrowser/update/alpha/Linux_x86-gcc3/4.0-alpha-2/en-US?force=1

You have then the update channel, the platform + architecture, the current version and the language. That should be enough for a successful update.

Child Tickets

Attachments (1)

0001-Fix-up-for-bug-13047-Remove-the-OS-Version-in-URL.patch (875 bytes) - added by mcs 4 years ago.
update_responses fix for no-update.xml case

Download all attachments as: .zip

Change History (19)

comment:1 Changed 4 years ago by mcs

You are right -- we do not need the info that Mozilla calls OS_VERSION in order to return the correct update manifest. But we included it in the update URL because if, for example, we drop support for an older OS such as Mac OS 10.6, having this info will allow us to return an update manifest that informs the user that updates are no longer available for their OS version.

But maybe it is too intrusive from a privacy / anonymity point of view (I also did not realize that GTK version was included on Linux). To remove it, we just need to remove %OS_VERSION% from the app.update.url preference inside browser/app/profile/firefox.js.

Another possibility would be to reduce the amount of detail that is returned within the OS_VERSION component, e.g., Linux-3.14

comment:2 in reply to:  1 ; Changed 4 years ago by gk

Replying to mcs:

You are right -- we do not need the info that Mozilla calls OS_VERSION in order to return the correct update manifest. But we included it in the update URL because if, for example, we drop support for an older OS such as Mac OS 10.6, having this info will allow us to return an update manifest that informs the user that updates are no longer available for their OS version.

Good thinking.

But maybe it is too intrusive from a privacy / anonymity point of view (I also did not realize that GTK version was included on Linux). To remove it, we just need to remove %OS_VERSION% from the app.update.url preference inside browser/app/profile/firefox.js.

I bet there are a couple of users that are not happy to send this data to us. So, I would like to see it removed as it is stricly speaking not necessary. BUT: on the other side being able to show the user that support of an OS (version) got dropped seems important to me too. I think the best solution would be to not transmit %OS_VERSION% over the wire at all but rather to compute it locally and compare that locally with a notice the update.xml contains. Looking at it we could certainly encode something in the buildID we don't need or we could even make up a new element, say <obsolete/>, specifying the respective OS and version + architecture. But probably there are even better ideas on how to do this which I am currently missing.

comment:3 in reply to:  2 ; Changed 4 years ago by mcs

Keywords: tbb-firefox-patch TorBrowserTeam201409 added

Replying to gk:

I think the best solution would be to not transmit %OS_VERSION% over the wire at all but rather to compute it locally and compare that locally with a notice the update.xml contains. Looking at it we could certainly encode something in the buildID we don't need or we could even make up a new element, say <obsolete/>, specifying the respective OS and version + architecture. But probably there are even better ideas on how to do this which I am currently missing.

I like your idea. Since we will already be returning different XML update manifests for different OS platforms, we can probably just add something like this:

minSupportedOSVersion="10.7"

(e.g., for Mac OS when we drop support for 10.6), Kathy and I will try to make the necessary changes next week.

comment:4 in reply to:  3 ; Changed 4 years ago by gk

Replying to mcs:

Replying to gk:

I think the best solution would be to not transmit %OS_VERSION% over the wire at all but rather to compute it locally and compare that locally with a notice the update.xml contains. Looking at it we could certainly encode something in the buildID we don't need or we could even make up a new element, say <obsolete/>, specifying the respective OS and version + architecture. But probably there are even better ideas on how to do this which I am currently missing.

I like your idea. Since we will already be returning different XML update manifests for different OS platforms, we can probably just add something like this:

minSupportedOSVersion="10.7"

(e.g., for Mac OS when we drop support for 10.6), Kathy and I will try to make the necessary changes next week.

I wonder how we should handle things if we want to drop support just for 32bit systems. (See: https://lists.torproject.org/pipermail/tor-talk/2014-September/034728.html for my idea) That plan has IMO a bunch of advantages and not much disadvantages compared to the "Let's move to 10.7 as minimum proposal"...

comment:5 in reply to:  4 Changed 4 years ago by mcs

Replying to gk:

I wonder how we should handle things if we want to drop support just for 32bit systems. (See: https://lists.torproject.org/pipermail/tor-talk/2014-September/034728.html for my idea) That plan has IMO a bunch of advantages and not much disadvantages compared to the "Let's move to 10.7 as minimum proposal"...

I think we will be able to detect the difference because of the presence of the %BUILD_TARGET% component within our update URL (we do not plan to remove that component). In TB 4.0a2 that component turns into this:

Darwin_x86-gcc3

I think it will turn into the following in a 64-bit build (we will need to test to be 100% sure):

Darwin_x86_64-gcc3

Then we just need to modify boklm's tb-update-response script to return a "end of life" XML response if _64 is missing. In the simplest case, that can look like:

<?xml version="1.0"?>
<updates>
    <update unsupported="true" detailsURL="..." />
</updates>

The detailsURL should point to a helpful page that tells people they need to stop using Tor Browser on their outdated systems.

Also worth noting: Google is apparently dropping their 32-bit Mac builds:
http://blog.chromium.org/2014/08/mac-chrome-when-im-sixty-four-bits.html
(or rather, they are switching from 32 to 64 like we may do).

comment:6 Changed 4 years ago by mcs

Cc: brade boklm mikeperry gk added
Owner: changed from tbb-team to mcs
Status: newassigned

comment:7 Changed 4 years ago by mcs

Keywords: MikePerry201409R added
Status: assignedneeds_review

The browser changes are here (bug13047 branch in brade remote):

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/1aa441fe6408ea1b0e94431f284f0d583a877a2e

Note that if/when we use minSupportedVersion in the XML update manifest, we will need to make sure we use the same OS version that Mozilla uses for %OS_VERSION%. On Mac OS and Linux, it is the kernel version. Here is a table for Mac OS that might be helpful when the time comes:

http://en.wikipedia.org/wiki/Darwin_%28operating_system%29#Release_history

On Windows, the version is compared against dwMajorVersion.dwMinorVersion from the OSVERSIONINFO struct. See the Remarks section on the following page for details:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms724834%28v=vs.85%29.aspx

Still needed: some modifications to boklm's tb-update-response to handle the new update URL that omits the %OS_VERSION% component.

comment:8 in reply to:  7 ; Changed 4 years ago by gk

Replying to mcs:

The browser changes are here (bug13047 branch in brade remote):

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/1aa441fe6408ea1b0e94431f284f0d583a877a2e

What about writing

      let sysInfo = Cc["@mozilla.org/system-info;1"]
                      .getService(Ci.nsIPropertyBag2);
      let osVersion = sysInfo.getProperty("version");

just as

      let osVersion = Services.sysinfo.getProperty("version");

? If that is not an option then using nsIPropertyBag should be enough as we only need getProperty().

And, yes, detecting 64/32 bit via BUILD_TARGET should do the trick, nice catch.

comment:9 in reply to:  8 Changed 4 years ago by mcs

Replying to gk:

What about writing ... as

      let osVersion = Services.sysinfo.getProperty("version");

?

Good idea. The revised fix is on the bug13047-02 branch in the brade remote, here:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/69d061cbca8e08b6e16b76613886b2affd02ebf6

comment:10 in reply to:  7 ; Changed 4 years ago by mikeperry

Replying to mcs:

Still needed: some modifications to boklm's tb-update-response to handle the new update URL that omits the %OS_VERSION% component.

How careful do we need to be about synchronizing these changes with the update response script? It seems like we'll want boklm's scripts to handle both forms of these urls for some time while 4.0a2 users upgrade, right? And will the new version property in the update manifest cause problems for 4.0a2?

comment:11 Changed 4 years ago by boklm

In order to make this kind of change easier, maybe we should include a format version in the URL.

For instance we can replace the "update" in the URL with "update_%formatversion".

So instead of having URLs starting with :
https://www.torproject.org/dist/torbrowser/update/
we could use URLs starting with:
https://www.torproject.org/dist/torbrowser/update_2/

If we need to change the URL format again the future, we can then change it to use "update_3", and keep the old directories on the server until everybody upgraded.

comment:12 in reply to:  10 Changed 4 years ago by mikeperry

boklm - I like that idea. It would also allow 4.0a2 users to continue to update to 4.0a3 (which we would leave as the only update at https://www.torproject.org/dist/torbrowser/update/, since they cannot update to anything past 4.0a3 due to #13049). We are then free to update 4.0a3 users separately to a ff31-esr based TBB via https://www.torproject.org/dist/torbrowser/update_2/.

comment:13 Changed 4 years ago by mcs

I also like boklm's idea; it is similar to what Mozilla has done for "versioning" of the update URLs over time. Hopefully, we do not have to keep tweaking the URL.

Mike -- It looks like the Mozilla code ignores attributes that it doesn't know about so we should be OK.

comment:14 Changed 4 years ago by mcs

A revised fix that uses .../update_2/ instead of /.../update/ is on the bug13047-02 branch in the brade remote, here:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/e4eca4759ead07562e4f6526b1ce79fe056fd03a

bolkm -- do you have time to modify your update_responses script so it will generate the correct files for the new .../update_2/ URL format? (I think you just need to modify the RewriteRule expressions within the .htaccess file.

It should work OK to use the current/older version of the update_responses script to generate the files needed for TB 4.0a2 (.../update/).

comment:15 in reply to:  14 Changed 4 years ago by boklm

Replying to mcs:

bolkm -- do you have time to modify your update_responses script so it will generate the correct files for the new .../update_2/ URL format? (I think you just need to modify the RewriteRule expressions within the .htaccess file.

Yes, I updated the script so it does not include anymore the OS version in the URL (I reverted the commit which added it).

So it should be ready to use when we have the .mar files of a new build available.

I also added support for the minSupportedOSVersion and unsupported attributes. When we need to set them on a specific platform, we can uncomment the minSupportedOSVersion or unsupported line in config.yml:
https://github.com/boklm/tb-update-response/blob/master/config.yml

It should work OK to use the current/older version of the update_responses script to generate the files needed for TB 4.0a2 (.../update/).

Yes.

Changed 4 years ago by mcs

update_responses fix for no-update.xml case

comment:16 Changed 4 years ago by mcs

boklm – please apply the patch I just attached (it fixes the "no updates" case).

comment:17 in reply to:  16 Changed 4 years ago by boklm

Replying to mcs:

boklm – please apply the patch I just attached (it fixes the "no updates" case).

Patch applied, thanks.

comment:18 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, this looked good on the browser side. I merged this into the 4.0 branch for 4.0a3.

Note: See TracTickets for help on using tickets.