Updater follows Location headers for valid archive filename #12817

milux · 2016-11-07T15:34:51Z

Pull Request for Issue #12816

Summary of Changes

Changed method "download()" of class "JoomlaupdateModelDefault", found in administrator/components/com_joomlaupdate/models/default.php.
The method does now fetch the HTTP headers of the update download location $updateInfo['object']->downloadurl->_data until the actual file (on AWS) is reached.
It further removes the path and the query string from the URL, resulting in the actual filename of the update package, which should work fine for all platforms.

Testing Instructions

Some people should check whether I messed up something for some specific platform setup.

Documentation Changes Required

none

… of the update archive

wojsmol · 2016-11-07T15:59:36Z

@milux In a moment I'll be testing this PR.
cc @trzepiz @ot2sen @noras666 @brianteeman Please test this.

wojsmol · 2016-11-07T16:19:36Z

I have tested this item ✅ successfully on 613a6c1

OS Windows 8.1

#Fields: datetime   priority clientip   category    message
2016-11-07T16:11:17+00:00   INFO ::1    update  Update started by user Super User (410). Old version is 3.6.2.
2016-11-07T16:11:20+00:00   INFO ::1    update  Downloading update file from http://joomla-official-downloads.s3.amazonaws.com/joomladownloads/joomla3/Joomla_3.6.4-Stable-Update_Package.zip?[AWS patams].
2016-11-07T16:11:25+00:00   INFO ::1    update  File Joomla_3.6.4-Stable-Update_Package.zip successfully downloaded.
2016-11-07T16:11:27+00:00   INFO ::1    update  Starting installation of new version.
2016-11-07T16:11:49+00:00   INFO ::1    update  Finalising installation.
2016-11-07T16:11:49+00:00   INFO ::1    update  Ran query from file 3.6.3-2016-08-15. Query text: ALTER TABLE `#__newsfeeds` MODIFY `link` VARCHAR(2048) NOT NULL;.
2016-11-07T16:11:49+00:00   INFO ::1    update  Ran query from file 3.6.3-2016-08-16. Query text: INSERT INTO `#__postinstall_messages` (`extension_id`, `title_key`, `description.
2016-11-07T16:11:50+00:00   INFO ::1    update  Deleting removed files and folders.
2016-11-07T16:11:56+00:00   INFO ::1    update  Cleaning up after installation.
2016-11-07T16:11:56+00:00   INFO ::1    update  Update to version 3.6.4 is complete.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12817.}

ot2sen · 2016-11-07T16:23:52Z

I have tested this item ✅ successfully on 613a6c1

Applied the patch using patchtester to the 3.6.2 that couldn´t be updated yesterday.
After applying patch and running the update it completes and is now at 3.6.4

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12817.}

jeckodevelopment · 2016-11-07T16:37:14Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12817.}

zero-24 · 2016-11-07T16:43:39Z

Hmm i'm not sure here. @mbabker can you take a look here?

jeckodevelopment · 2016-11-07T16:45:28Z

Waiting for review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12817.}

milux · 2016-11-07T16:49:53Z

@zero-24 The change could be problematic when the last observed location does not represent a file ending on ".zip". Is that possible? In that case I might check for proper file extension and append it manually if necessary...

mbabker · 2016-11-07T17:10:28Z

Well there are two options:

Just remove the query string segment and leave it as the URL fragment without a file extension (probably bad news)
Follow the headers to get the correct file name with file extension.

I don't think there's an issue here honestly, unless someone's got a scenario I'm not aware of to look at.

zero-24 · 2016-11-07T17:12:58Z

Thanks @mbabker 👍

milux · 2016-11-07T17:15:15Z

@mbabker

Already tried that one... doesn't work, Joomla doesn't extract the file without proper file extension.
Exactly what I did, however if the file from AWS doesn't end on ".zip", the process will fail

There are actually two more options:
3) Just remove the query string, check if the basename ends on ".zip", append if necessary
4) Resolve until destination and do the same thing as in 3)

So, what should it be? ;)

mbabker · 2016-11-07T17:18:15Z

Leave it as is. I wouldn't hardcode a .zip suffix check in the off chance the file extension ever changes (like if we tried serving the smaller .tar.* files instead of the ZIP package).

ggppdk · 2016-11-07T17:36:46Z

Leave it as is. I wouldn't hardcode a .zip suffix check in the off chance the file extension ever changes (like if we tried serving the smaller .tar.* files instead of the ZIP package).

administrator/components/com_joomlaupdate/restore.php
method: getUnarchiver()

it only supports: JPA, JPS, ZIP

milux · 2016-11-07T17:42:31Z

... with JPA and JPS being Akeeba-specific formats afaik, but how about a 5th solution:
5) We take the file extension from the original URL and append it to the basepath if it is not there yet. 😜

mbabker · 2016-11-07T17:46:36Z

This is fine as is. Really we'd only have further problems if someone uploaded packages to AWS in the future without a file extension, in which case it'd probably be a broken file anyway. Plus our main interface to AWS is through the downloads site and is a Joomla extension, so the packages are going through validation on upload there. Let's roll with it.

@joomla/cms-maintainers When this is merged, I'd suggest tagging a standalone update for the update component since it looks like anyone hosting a production site on a Windows platform will have issues upgrading with packages pulled from AWS thanks to the URL query strings in both the XML update file and the resulting URL after following all redirects.

ggppdk · 2016-11-07T18:17:02Z

There is a rare problem (maybe not possible problem) that might occur with this PR

if the redirect/final URL does not have a query string (no character ?) then:

// Remove URL and query string
$urlBasename = basename($packageURL);
$basename    = substr($urlBasename, 0, strpos($urlBasename, '?'));

$basename will be empty, because substr will return an empty string, because strpos will give FALSE as length parameter

This should be safer :

// Remove URL and query string
$basename = basename($packageURL);
if (strpos($basename, '?'))
{
    $basename = substr($basename , 0, strpos($basename, '?'));
}

ggppdk · 2016-11-07T18:40:56Z

Please see my above comment

about adding zip as extension

only in the case that extension is not detected in the redirected URL, is a good sanity check,

Also we can try to grab the extension for the original update URL too,
Besides that is the purpose of &jcompat=my.zip , right ?

Is it preferable if no extension is detected that upgrade will fail ?
Plus currently as mentioned above, the getUnarchiver() only supports ZIP and joomla will not distrubute packages with JPA or JPS format

milux · 2016-11-07T21:14:51Z

@ggppdk Thanks for your suggestion, integrated that.
Because of what @mbabker suggested, I don't think that we need to hassle around with the extension any more.
Is this ready for RTC then?

jeckodevelopment · 2016-11-07T21:23:12Z

Needs tests again and it seems that Travis is not happy

milux · 2016-11-07T21:26:27Z

Exactly... I just wonder how I made it unhappy? 🤔

mbabker · 2016-11-07T21:27:02Z

administrator/components/com_joomlaupdate/models/default.php

+		$basename = basename($packageURL);
+		if (strpos($basename, '?') !== false)
+		{
+			$basename = substr($basename , 0, strpos($basename, '?'));


This line, remove the space between $basename and the comma.

Thanks, I didn't recognize this when copy-pasting the suggestion of @ggppdk

milux · 2016-11-07T21:42:30Z

@jeckodevelopment Maybe now? 😉

jeckodevelopment · 2016-11-07T21:46:47Z

test test test 😄
thanks!

wojsmol · 2016-11-07T22:04:29Z

I have tested this item ✅ successfully on 4c66273

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12817.}

milux · 2016-11-07T22:42:56Z

I have tested this item ✅ successfully on 4c66273

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12817.}

milux · 2016-11-07T22:49:29Z

@jeckodevelopment Done, for the sake of it. (There wasn't really a change, and yes, I actually tested it even though and didn't just press the submit test button. 😉 )

jeckodevelopment · 2016-11-08T00:24:03Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12817.}

…2817) * made download() follow Location headers to resolve the valid filename of the update archive * integrated suggestion of ggppdk, valid behavior if no query string * strict type checking * make Travis happy again... code style error

wojsmol · 2016-11-09T15:58:59Z

Is it planned a new version of the com_joomlaupdate component with this fix before the release of Joomla 3.7.0?

zero-24 · 2016-11-09T16:07:28Z

@wilsonge @rdeutz any plans on this here?

wilsonge · 2016-11-09T16:42:49Z

No. We only distribute com_joomlaupdate in exceptional circumstances - basically when we make break the updater in a release. All other modifications ship as per the usual release cycle of the CMS

wojsmol · 2016-11-09T16:53:36Z

@wilsonge Without this fix com_joomlaupdate does not work under Windows. See #12817 (comment) and joomla/joomla-websites#742

mbabker · 2016-11-09T16:54:10Z

I would suggest someone publish a page with guidance for users on a Windows platform as they cannot direct download and upgrade if the decision is to not release an interim update for the component.

trzepiz · 2016-11-09T17:08:42Z

@mbabker This is a big global problem for all Windows users and Windows servers. Should I tell everyone in Poland that "One click update" does not work on Windows?.

mbabker · 2016-11-09T17:17:12Z

It's fixed in the code for 3.7. I can't do anything about any other decisions.

brianteeman · 2016-11-09T17:35:50Z

How many people run Joomla on a windows server?

mbabker · 2016-11-09T17:41:11Z

13% of sites sending stats in the last 60 days are Windows. How many of those are local dev versus production, we don't track, but that's definitely a higher use rate than other configurations we support.

* made download() follow Location headers to resolve the valid filename of the update archive * integrated suggestion of ggppdk, valid behavior if no query string * strict type checking * make Travis happy again... code style error

made download() follow Location headers to resolve the valid filename…

613a6c1

… of the update archive

joomla-cms-bot added the PR-staging label Nov 7, 2016

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 7, 2016

milux added this to the Joomla 3.7.0 milestone Nov 7, 2016

joomla-cms-bot removed this from the Joomla 3.7.0 milestone Nov 7, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 7, 2016

milux added this to the Joomla 3.7.0 milestone Nov 7, 2016

milux removed this from the Joomla 3.7.0 milestone Nov 7, 2016

integrated suggestion of ggppdk, valid behavior if no query string

a5a8613

jeckodevelopment and others added 2 commits November 7, 2016 22:23

Merge branch 'staging' into coreupdate-filename-patch

330ead0

strict type checking

7395c87

mbabker reviewed Nov 7, 2016

View reviewed changes

make Travis happy again... code style error

4c66273

jeckodevelopment mentioned this pull request Nov 8, 2016

Update 3.6.2 to 3.6.4 seems broken in Windows environment #12816

Closed

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 8, 2016

jeckodevelopment added this to the Joomla 3.7.0 milestone Nov 8, 2016

rdeutz merged commit 7aa5e65 into joomla:staging Nov 8, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 8, 2016

zero-24 mentioned this pull request Nov 21, 2016

Joomla update/reinstall fails on windows #12955

Closed

This comment was marked as abuse.

Sign in to view

milux mentioned this pull request Apr 28, 2017

updates broken on servers with allow_url_fopen disabled #15599

Closed

joomla deleted a comment from alo24h Jan 19, 2018

zero-24 mentioned this pull request Jan 27, 2022

Automatic update with PROXY #36869

Closed

Uh oh!

Updater follows Location headers for valid archive filename #12817

Updater follows Location headers for valid archive filename #12817

Uh oh!

Conversation

milux commented Nov 7, 2016 • edited by jeckodevelopment Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Documentation Changes Required

Uh oh!

wojsmol commented Nov 7, 2016

Uh oh!

wojsmol commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ot2sen commented Nov 7, 2016

Uh oh!

jeckodevelopment commented Nov 7, 2016

Uh oh!

zero-24 commented Nov 7, 2016

Uh oh!

jeckodevelopment commented Nov 7, 2016

Uh oh!

milux commented Nov 7, 2016

Uh oh!

mbabker commented Nov 7, 2016

Uh oh!

zero-24 commented Nov 7, 2016

Uh oh!

milux commented Nov 7, 2016

Uh oh!

mbabker commented Nov 7, 2016

Uh oh!

ggppdk commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milux commented Nov 7, 2016

Uh oh!

mbabker commented Nov 7, 2016

Uh oh!

ggppdk commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggppdk commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milux commented Nov 7, 2016

Uh oh!

jeckodevelopment commented Nov 7, 2016

Uh oh!

milux commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbabker Nov 7, 2016

Choose a reason for hiding this comment

Uh oh!

milux Nov 7, 2016

Choose a reason for hiding this comment

Uh oh!

milux commented Nov 7, 2016

Uh oh!

jeckodevelopment commented Nov 7, 2016

Uh oh!

wojsmol commented Nov 7, 2016

Uh oh!

milux commented Nov 7, 2016

Uh oh!

milux commented Nov 7, 2016

Uh oh!

jeckodevelopment commented Nov 8, 2016

Uh oh!

wojsmol commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zero-24 commented Nov 9, 2016

Uh oh!

wilsonge commented Nov 9, 2016

Uh oh!

wojsmol commented Nov 9, 2016

milux commented Nov 7, 2016 •

edited by jeckodevelopment

Loading

wojsmol commented Nov 7, 2016 •

edited

Loading

ggppdk commented Nov 7, 2016 •

edited

Loading

ggppdk commented Nov 7, 2016 •

edited

Loading

ggppdk commented Nov 7, 2016 •

edited

Loading

milux commented Nov 7, 2016 •

edited

Loading

wojsmol commented Nov 9, 2016 •

edited

Loading

trzepiz commented Nov 9, 2016 •

edited

Loading