Urlencode file name before passing it to cURL

[Ansud]

Large file helper use cURL to determine file sizes. Thus filenames must be
urlencoded in case special symbols like '#' can cause BadRequest errors.

Signed-off-by: Tony Zelenoff antonz@parallels.com

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[karlitschek]

looks good. @LukasReschke What do you think?

[Ansud]

BTW, the issue #11253
And for @owncloud-bot
My contribution is MIT licensed.

[LukasReschke]

Confirmed to work by @brumsoel and approved by @bantu - I agree with @bantu, very nice catch! 👍

Thanks a lot for contributing back to ownCloud!

\cc @PVince81

[LukasReschke]

@owncloud-bot This is okay to test

[LukasReschke]

@owncloud-bot Retest this please

[LukasReschke]

@karlitschek I believe we should backport this - this shouldn't hurt.

[PVince81]

Nice catch and great avatar 👍

[karlitschek]

Yes. Please backport

[bantu]

Don't like the variable name too much, but 👍 anyway.

[ghost]

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/191/

Build result: FAILURE

[...truncated 13 lines...] > git rev-parse origin/pr/11501/merge^{commit} # timeout=10Checking out Revision 44288da81785403dcd8cf59584b2ff052dbe68b7 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 44288da81785403dcd8cf59584b2ff052dbe68b7 > git rev-list f003d57d4c0de95c7a5490a7b7b26ec6baa535b5 # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng » sqlite,vm-slave-02Configuration pull-request-analyser-ng » sqlite,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02Triggering pull-request-analyser-ng » pgsql,vm-slave-02Triggering pull-request-analyser-ng » mysql,vm-slave-02Triggering pull-request-analyser-ng » oci,vm-slave-02Configuration pull-request-analyser-ng » pgsql,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02pull-request-analyser-ng » pgsql,vm-slave-02 completed with result SUCCESSConfiguration pull-request-analyser-ng » mysql,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02pull-request-analyser-ng » mysql,vm-slave-02 completed with result SUCCESSpull-request-analyser-ng » oci,vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 1 second:bomb: Test FAILed. :bomb:

[PVince81]

@owncloud-bot retest this please

[scrutinizer-notifier]

The inspection completed: No new issues

[ghost]

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/213/

Build result: FAILURE

[...truncated 9 lines...]Checking out Revision cddf37ba88ced5f6683c9d47665fa346de0b3e80 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f cddf37ba88ced5f6683c9d47665fa346de0b3e80 > git rev-list 237cb7a1895d5cdbae6ebca4d8c21b1f313e1101 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng » sqlite,vm-slave-02Configuration pull-request-analyser-ng » sqlite,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02Triggering pull-request-analyser-ng » pgsql,vm-slave-02Triggering pull-request-analyser-ng » mysql,vm-slave-02Triggering pull-request-analyser-ng » oci,vm-slave-02Configuration pull-request-analyser-ng » pgsql,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02pull-request-analyser-ng » pgsql,vm-slave-02 completed with result FAILUREpull-request-analyser-ng » mysql,vm-slave-02 completed with result FAILUREConfiguration pull-request-analyser-ng » oci,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02pull-request-analyser-ng » oci,vm-slave-02 completed with result SUCCESSStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 4 second:bomb: Test FAILed. :bomb:

[PVince81]

@owncloud-bot retest this please

[ghost]

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/250/:rocket: Test PASSed. 🚀

[PVince81]

Backported to stable7 as 34c1760
(this will be released as 7.0.3)

[dajusc]

Bad news: I manually applied the patch and now filenames containing "(" and ")" lead to the "Bad Request" problem. After reverting to the unpatched version of largefilehelper.php those files sync normal again (but of course files with # in their names do not anymore).

@Ansud @yannick12 Can anyone confirm this?

[LukasReschke]

Works locally for me on OS X and PHP 5.5.17

<?php

     function getFileSizeViaCurl($filename) {
        if (function_exists('curl_init')) {
            $fencoded = urlencode($filename);
            $ch = curl_init("file://$fencoded");
            curl_setopt($ch, CURLOPT_NOBODY, true);
            curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
            curl_setopt($ch, CURLOPT_HEADER, true);
            $data = curl_exec($ch);
            curl_close($ch);
            if ($data !== false) {
                $matches = array();
                preg_match('/Content-Length: (\d+)/', $data, $matches);
                if (isset($matches[1])) {
                    return 0 + $matches[1];
                }
            }
        }
        return null;
    }
    var_dump(getFileSizeViaCurl('./test(1).php'));

Can you please retest with the above snippet?

[dajusc]

Oh dear, the problem seems to be more complicated. Your snipped works as intended, giving back the correct size. But for another file named 'test (1).php' (additional space before the bracket) the corresponding var_dump gives back 'NULL'.

var_dump(getFileSizeViaCurl('./test(1).php'));   // => int(770)
var_dump(getFileSizeViaCurl('./test (1).php'));   // => NULL

[LukasReschke]

@dajusc Please test #11528 with all possible combinations. Thanks a lot!

[dajusc]

Problem solved! Good work @LukasReschke ! :)

PS.: Tested with 'test(1).php', 'test (1).php' and '#test#.php'.

[LukasReschke]

@dajusc Can I ask you to also write this down under #11528? Thanks a lot!

[dajusc]

As a GitHub-noob I'm not totally shure what you expect me to write there, but ok. ^^

[yannick12]

I am slightly losing track of which patches we are talking about, but let me at least give you a concrete data point.

Running an unpatched OC 7.0.2 on a 32 bit server I had problems synching files whose names began with a # symbol. I applied (only) the following patch:

# diff largefilehelper.php.old largefilehelper.php.new
105c105,106
<                       $ch = curl_init("file://$filename");
---
>                       $fencoded = urlencode($filename);
>                       $ch = curl_init("file://$fencoded");

Files called fishy(1).txt, testing (1).txt and #test.txt all sync fine for me.

[dajusc]

@yannick12 can you check the return values of var_dump(getFileSizeViaCurl()) for those files?

PS.: I tested PHP 5.5.16 and PHP 5.3.29 (@yannick12 you use PHP 5.3.10 right?). For both PHP versions var_dump(getFileSizeViaCurl('./test (1).php')); returns NULL on my server.

[yannick12]

Oh -- sorry -- I stopped following this thread once my issues were fixed!

Unfortunately I don't know any php at all. Well, I know enough to type php -v and report back that I am using PHP 5.5.17-2+deb.sury.org~precise+1 (cli) (built: Sep 25 2014 09:08:12) . I was originally using 5.3 but I upgraded in desperation at some point in an attempt to fix my synching problems. If you explain how to check the return values that you want me to check then I can do it -- e.g. I can certainly make trivial changes to core php files and then do some synching for you. But you'll have to tell me what to do.

[dajusc]

@yannick12 no problem! :)

Simply save the snipet provided above by @LukasReschke into a text/php-file named "test (1).php", whereby you replace the last line with echo var_dump(getFileSizeViaCurl('./test (1).php'));. Then put the file into the document folder of your webserver, access it via browser and check the output. It should demonstrate the regression (new bug) of the #11528 commit and display "NULL".

[yannick12]

I'm sorry -- I'm really busy at home and at work at the minute. Yes it says "null".

[yannick12]

Am I right in thinking that I should change my owncloud installation in the following way: in lib/private/largefilehelper.php I should edit the function getFileSizeViaCurl as in LukasResche's snippet above, except that instead of the third line down:

$fencoded = urlencode($filename);

I should instead write

$fencoded = rawurlencode($filename);

? This might be all moot anyway as it seems that this issue will be fixed in 7.0.3 .

[PS I'm sorry, I'm busy at work right now; I can only find the time to look at this thread at weekends]

[dajusc]

I think you got it right. :) If you want to be 100% shure, you could also take a look at LukasReschkes commit (automaticly linked 4 comments above) and compare it to your manually applied fix.