Codecov Report

Merging #11163 into master will decrease coverage by 1.7%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #11163      +/-   ##
============================================
- Coverage     94.86%   93.15%   -1.71%     
- Complexity    12859    12980     +121     
============================================
  Files           437      437              
  Lines         32774    33621     +847     
============================================
+ Hits          31090    31319     +229     
- Misses         1684     2302     +618

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d25953...fef1ebf. Read the comment docs.

Some servers set //example.com as a Location header.

Can you explain the actual use case this is for please.

Do you mean the reason for this change is to follow redirects like so:

$ curl -I http://example.com/not/here.html
HTTP/1.1 301 Moved Permanently
Location: //example.com/but/here.html

?

Feels like there is significant scope for "upgrading broke my app" with this as buildUrl('//some/path') will change behavior.

Do you mean the reason for this change is to follow redirects like so:

Yes.

To me it looks like a bug that those URL weren't supported.
Locations like //example.com were redirected to http://example.com/example.com.

In the scope of a location header the change makes sense, but it applies much broader than that

I think this is a problem:

-> git diff
diff --git tests/TestCase/Http/ClientTest.php tests/TestCase/Http/ClientTest.php
index d8c379e..b9b4d52 100644
--- tests/TestCase/Http/ClientTest.php
+++ tests/TestCase/Http/ClientTest.php
@@ -146,6 +146,15 @@ class ClientTest extends TestCase
                 ],
                 'url without a scheme',
             ],
+            [
+                'http://example.com//some/file',
+                '//some/file',
+                [],
+                [
*                    'host' => 'example.com',
+                    'scheme' => 'http'
+                ],
+                'Borked but valid path should not be understood as a protocol-relative url',
+            ],
         ];
     }
 
-> phpunit tests/TestCase/Http/ClientTest.php
PHPUnit 6.3.0 by Sebastian Bergmann and contributors.

............F..........................                           39 / 39 (100%)

Time: 354 ms, Memory: 12.00MB

There was 1 failure:

1) Cake\Test\TestCase\Http\ClientTest::testBuildUrl with data set #11 ('http://example.com//some/file', '//some/file', array(), array('http'), 'Borked but valid path')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://example.com//some/file'
+'http://some/file'

/tmp/cakephp/tests/TestCase/Http/ClientTest.php:169

I could be wrong, but I feel the right place for fixing that problem is https://github.com/cakephp/cakephp/blob/master/src/Http/Client.php#L401-L402

For comparison, curl doesn't understand protocol-relative urls:

$ curl -IL //example.com
curl: (3) <url> malformed

unless they are in a location header:

$ curl -IL http://cake.dev/foo
HTTP/1.1 302 Moved Temporarily
Location: //example.com

HTTP/1.1 200 OK
...

Note also that there is no need to use preg_match to test if a string matches one permutation. (substr($url, 0, 2) === '//') would suffice.

//example.com syntax is used in a scope where a protocol is known, this is a "Protocol relative URL".

Maybe the buildUrl method should append the scheme only if a protocol has been explicitly passed as an option?

if (isset($options['scheme'] && (substr($url, 0, 2) === '//')) {
    $url = $options['scheme'] . ':' . $url;
}
...
$options += $defaults;

@AD7six //some/file doesn't seem like valid path to me. It is ambiguous as //host/path style URLs are used quite often to load CSS/JS assets, and // should likely be interpreted the same in our Client as it would in a browser.

rfc 7231 states that Location should be a Relative Reference which indicates that the first segment after // is the authority or domain.

@markstory my concern/point is that a request like this https://cakephp.org//css/cake.css is a valid response on most webservers, and is affected by the modified code. It's easy to write code which would make a request like that:

$someVariable = '/css/cake.css';
...
$path = '/';
...
$path .= $someVariable;
$result = (new Client())->get($path, [], ['host' => 'cakephp.org', 'scheme' => 'https']);

The behavior of this request is going to significantly change when this PR is merged. Hoping that's understood (if it already is - well 🤷‍♂️ :)) - I have no problem with the intended scope of the PR (handling protocol relative redirects) as I previously mentioned.

That's a good point @AD7six I hadn't considered that. @robertpustulka Is there a way your change could be scoped closer to where Location headers are followed? Perhaps the scheme should only be applied if the scheme option was set as you mentioned earlier.

Protocol relative fix is now opt-in. I added a protocolRelative option which is set to true in redirects.

Nice work!