default arg_separator.output to & not &

[butonic]

ownCloud currently sets arg_separator.output to & to force the separation of query parameters to work correctly when a url is rendered in an HTML page, see http://www.w3.org/QA/2005/04/php-session
found in https://github.com/owncloud/core/blob/master/lib/base.php#L495:

        ini_set('arg_separator.output', '&');

This setting influences how http_build_query behaves and might cause problems for libraries we use when they expect the default query parameter to be & because they want to execute a curl request.

acking my core repo gives various results:

❯ ack http_build_query
3rdparty/mrclay/minify/min/lib/Minify/JS/ClosureCompiler.php
215:        return http_build_query(

3rdparty/symfony/routing/Symfony/Component/Routing/Generator/UrlGenerator.php
281:        if ($extra && $query = http_build_query($extra, '', '&')) {

3rdparty/kriswallsmith/assetic/src/Assetic/Filter/GoogleClosure/CompilerApiFilter.php
81:                'content' => http_build_query($query),

3rdparty/guzzle/parser/Guzzle/Parser/UriTemplate/UriTemplate.php
171:                                $var = strtr(http_build_query(array($key => $var)), $rfc1738to3986);

3rdparty/guzzle/http/Guzzle/Http/EntityBody.php
47:                return self::fromString(http_build_query($resource));

tests/lib/appframework/http/RequestTest.php
143:        $data = http_build_query(array('name' => 'John Q. Public', 'nickname' => 'Joey'), '', '&');

apps-repos/files_texteditor/js/vendor/ace/src-noconflict/mode-php.js
351:        'html_entity_decode|htmlentities|htmlspecialchars|htmlspecialchars_decode|http_build_cookie|http_build_query|http_build_str|http_build_url|' +

apps-repos/search_lucene/3rdparty/zendframework/zend-stdlib/Zend/Stdlib/Parameters.php
73:        return http_build_query($this);

apps-repos/templateeditor/3rdparty/twig/twig/doc/filters/url_encode.rst
24:    ``true`` as the first parameter) or the `http_build_query`_ function.
28:.. _`http_build_query`: http://php.net/http_build_query

apps-repos/templateeditor/3rdparty/twig/twig/lib/Twig/Extension/Core.php
600:        return http_build_query($url, '', '&');

lib/private/urlgenerator.php
101:        if ($args && $query = http_build_query($args, '', '&')) {

apps/files_sharing/lib/external/storage.php
224:            http_build_query(array('password' => $password)));

apps/files_external/3rdparty/google-api-php-client/src/Google/Auth/OAuth2.php
172:    return self::OAUTH2_AUTH_URL . "?" . http_build_query($params, '', '&');

apps/files_external/3rdparty/google-api-php-client/src/Google/Http/Request.php
391:        http_build_query($this->queryParams);

apps/files_external/3rdparty/google-api-php-client/src/Google/IO/Abstract.php
180:      $postBody = http_build_query($postBody, '', '&');

apps/files_external/3rdparty/Dropbox/OAuth/Curl.php
76://               $arguments=http_build_query($arguments);
80:         curl_setopt($ch, CURLOPT_URL, $uri.'?'.http_build_query($arguments));

apps/files_external/3rdparty/aws-sdk-php/Guzzle/Parser/UriTemplate/UriTemplate.php
171:                                $var = strtr(http_build_query(array($key => $var)), $rfc1738to3986);

apps/files_external/3rdparty/aws-sdk-php/Guzzle/Http/EntityBody.php
47:                return self::fromString(http_build_query($resource));

apps-repo/user_oauth/3rdparty/fkooman/php-oauth-lib-rs/src/fkooman/oauth/rs/RemoteResourceServer.php
142:            $introspectionEndpoint .= $separator . http_build_query($get, null, "&");

apps-repo/news/vendor/pear/net_url2/Net/URL2.php
1117:     * A simple version of http_build_query in userland. The encoded string is

of particular interest is https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/external/storage.php#L224

        curl_setopt($ch, CURLOPT_URL, $url);
        curl_setopt($ch, CURLOPT_POST, 1);
        curl_setopt($ch, CURLOPT_POSTFIELDS,
            http_build_query(array('password' => $password)));
        curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);

It will do a curl request with the query, which will fail should we add another parameter.

We could either use our url generator here, or switch back to the default & and make sure our urls are encoded correctly when using PHPSESSID as a query param instead of a cookie.

@LukasReschke any security related concerns? @karlitschek It seems this has been in the first commit, care to shed some light if this is still good practice today?
cc @dagan

[DeepDiver1975]

@karlitschek can you answer @butonic's question? THX

[karlitschek]

@butonic honestly I can´t recall the reason for this at the moment.

[DeepDiver1975]

close or fix?

[DeepDiver1975]

@LukasReschke opinion?

[LukasReschke]

Did some digging. From a security perspective changing this seems not to be a risk, however it might actually break some other stuff that relied on the non-default behaviour we added in here. But that seems unlikely to me.

That said it is not really a good practice to have such non-default behaviour and I created a PR to kill it for master. But we certainly shouldn't backport it.

PR is at #15232