Scheme-relative URL rejected by validation

Yes, please. A side effect of this is letting l() and url() use scheme-relative links too. This would need to be applied to Drupal 8 first, then backported.

This will need a security review.

nice work, can you add tests?

Sure. In doing so, I realized that support also needs to be added to valid_url(), so I made that change as well.

Here is a reroll and a backport.

This is an issue beyond menu items. I ran into it using a scheme-relative path for a CDN. It's an issue in D6 as well.

Patch for D6 (Since the botched D7 upgrade I can't add a patch?)

diff --git a/includes/common.inc b/includes/common.inc
index 7ef1e51..f4c2d03 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -1471,7 +1471,8 @@ function url($path = NULL, $options = array()) {
     // Only call the slow filter_xss_bad_protocol if $path contains a ':' before
     // any / ? or #.
     $colonpos = strpos($path, ':');
-    $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path));
+    $slashpos = (strpos($path, '//') === 0);
+    $options['external'] = (($colonpos !== FALSE  || $slashpos !== FALSE) && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path));
   }

   // May need language dependent rewriting if language.inc is present.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/UrlTest.php
@@ -290,4 +290,37 @@ function testExternalUrls() {
+  public function testSchemeRelativeUrls() {

Yes, this tests it beyond menu specific code.

(For uploading patches, edit the issue node)

Thanks Tim. I am running both the D6 patch and the D7 patch in production. It allows me to use relative schemas with a CDN.

It seems that #2195983: Support scheme-relative URLs is a more invasive change for D8 to do the same thing. If that indeed goes in, this can just be marked back to D7.

Either this or the issue linked in ##1 need a reroll for 8.x.

Re-roll for d7. Part of this patch went into core with the security release. Not sure if tests are needed anymore since some tests went in with that patch. I left them in just in case.

If it wasn't for the tests this would be a one line patch. So it's greatly simplified.

The comment doesn't make sense: "# Look for ftp, http, http, feed or scheme relative schemes"

Reroll for D8 attached.

Note that after #2455083: Open redirect fixes from SA-CORE-2015-001 need to be ported to Drupal 8 went in, UrlHelper::isExternal() now always treats urls starting with // as external, so this patch needs to take that into account.

I'm going to see if I can get this working.

This should fix the failing tests.

Patch no longer applies.

This will also help in other issues, such as: #2573635: Url::fromUri() should accept protocol-relative URLs

Rerolled and also added a test for a schema relative URL at UnroutedUrlTest.php

I think this is a better component, as it affects other modules, too.

Maybe we need a bigger rewrite for this validation: #2691099: Improve external URL validation in many ways

This will also help in other issues

Indeed. The CDN module for Drupal 8 will only do protocol-relative URLs, to avoid having to render everything twice: once for HTTP and once for HTTPS. And e.g. the image dialog in the text editor uses #type => managed_file, which uses #theme => file_link, which uses Url::fromUri()… and since that does not support protocol-relative URIs, it breaks.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -302,6 +302,10 @@ public static function fromUri($uri, $options = []) {
   +    if (empty($uri_parts['scheme']) &&  strpos($uri, '//') === 0) {
   

   Two spaces after &&.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -302,6 +302,10 @@ public static function fromUri($uri, $options = []) {
   +      $uri_parts['scheme'] = 'relative';
   

   Eh… that doesn't seem quite right.

3. +++ b/core/modules/system/src/Tests/Common/UrlTest.php
   @@ -321,4 +321,37 @@ function testExternalUrls() {
   +    // Verify scheme-relative URL can contain a fragment.
   ...
   +    $this->assertEqual($url, $result, 'Scheme-relative URL with fragment works without a fragment in $options.');
   

   s/scheme/protocol/

   … because that is the accepted term.

4. +++ b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
   @@ -87,6 +87,8 @@ public function providerFromUri() {
   +      // A protocol relative URL.
   

   Missing dash.

5. +++ b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
   @@ -119,7 +121,6 @@ public function providerFromInvalidUri() {
          // Schemeless paths.
   ...
          // Schemeless path with a query string.
   

   s/Schemeless path/Relative URL/

@Wim Leers, really cool to know we are also helping the CDN module and other contribs here

First bug report against the CDN module because of this core bug: #2711529: File upload widget broken when using CDN module, fixed in Drupal 8.1.4: require that version.

Composer patches just told me the patch does not longer apply on 8.1.2 so tagging and setting to needs work

rebased on 8.1.x

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -384,7 +384,7 @@ public static function isValid($url, $absolute = FALSE) {
   -        (?:ftp|https?|feed):\/\/                                # Look for ftp, http, https or feed schemes
   +        (?:ftp:|https?:|feed:)?\/\/                                # Look for ftp, http, https or feed schemes
   

   This is a problem that did not exist in #26.

2. +++ b/core/tests/Drupal/Tests/Core/UrlTest.php
   @@ -294,6 +294,38 @@ public function testUrlFromRequestInvalid() {
   +    $this->assertEqual($url, $result, 'Scheme-relative URL with query string works without a query string in $options.');
   +    ¶
   

   Nor this.

Plus the patch is smaller now, which probably means some hunks were lost, which probably explains the failing tests.

This looks like a bad reroll. I'll do it over.

Straight reroll of #26, with only one tiny trivial conflict.

Tests pass locally.

@Wim Leers I see you did not apply your proposed changes at #30 and I was going to apply them myself, but I realize that at UrlHelperTest in many places it calls methods and variables 'scheme'. Changing everything there would be a big change and no reason for such, then I did not replace all strings 'scheme' by 'protocol as pointed on item 3, only the ones that makes more sense.

1- fixed.
2- I think it is right, why not?
3- not going to fix all entries, as commented above.
4- fixed.
5- These lines are not being patched and these comments are regarding invalid URLs, which makes sense to keep the comment as is.

One thing I'm wondering, should absolute URLs generated in the HTML use a scheme relative URL by default?

+++ b/core/modules/menu_ui/src/Tests/MenuTest.php
index 062e3e4..1717114 100644
--- a/core/modules/system/src/Tests/Common/UrlTest.php

--- a/core/modules/system/src/Tests/Common/UrlTest.php
+++ b/core/modules/system/src/Tests/Common/UrlTest.php

+++ b/core/modules/system/src/Tests/Common/UrlTest.php
+++ b/core/modules/system/src/Tests/Common/UrlTest.php
@@ -317,4 +317,36 @@ function testExternalUrls() {

@@ -317,4 +317,36 @@ function testExternalUrls() {
     $this->assertEqual($url . '&' . http_build_query($query, '', '&'), $result);
   }
 
+  public function testSchemeRelativeUrls() {

Ideally we would adapt the unit test instead of this old web test

One thing I'm wondering, should absolute URLs generated in the HTML use a scheme relative URL by default?

That's a very good question. Technically (well, pedantically to be pedantically precise) they're not an absolute URL anymore, but in practice that's how they behave. So I think this makes sense.
(Note that this would only affect local URLs that we render in an absolute manner. This would not affect external URLs. If it would, that'd result in problems, because we cannot know whether external URLs in fact support HTTP or HTTPS.)

Needs work due to the test in #1783278-41: Scheme-relative URL rejected by validation

Needs a reroll.

Reroll that applies to commit 2db400d0bc4fefb.

Below are the conflicts addressed:

drupal\core\lib\Drupal\Core\Url.php
<<<<<<< HEAD
// We support protocol-relative URLs.
if (strpos($uri, '//') === 0) {
$uri_parts['scheme'] = '';
}
elseif (empty($uri_parts['scheme'])) {
=======
// Support for scheme-relative URLs.
if (empty($uri_parts['scheme']) && strpos($uri, '//') === 0) {
$uri_parts['scheme'] = 'relative';
}
if (empty($uri_parts['scheme'])) {
>>>>>>> Applying patch from issue 1448e883d1a3 comment 400d0bc4fefb

drupal\core\tests\Drupal\Tests\Component\Utility\UrlHelperTest.php
<<<<<<< HEAD
$url_schemes = ['http', 'https', 'ftp'];
$data = [];
=======
$url_schemes = array('http://', 'https://', 'ftp://', '//');
$data = array();
>>>>>>> Applying patch from issue 1448e883d1a3 comment 400d0bc4fefb

drupal\core\tests\Drupal\Tests\Core\UnroutedUrlTest.php
<<<<<<< HEAD
$this->expectException(\InvalidArgumentException::class);
$url = Url::fromUri($uri);
=======
Url::fromUri($uri);
>>>>>>> Applying patch from issue 1448e883d1a3 comment 400d0bc4fefb

Reroll is done w.r.t 8.9.x

This is a duplicate of #2195983: Support scheme-relative URLs. In a comment-14 over there from Feb 2014 sun stated that that issue was the preferred solution "because it also removes that apparently hard-coded list of schemes in the preg_match() of isValid(), which has absolutely no business in there."

I'll work on combining the patches and post it over in the other issue and move credit etc.

Closing this as a duplicate of #219598: Query trying to use invalid field in node_access table despite the fact that this is the older issue. In that being the older issue in #14 sun states that the other solution is the preferred. "because it also removes that apparently hard-coded list of schemes in the preg_match() of isValid(), which has absolutely no business in there. :)"

The work here has been added to that patch over there and credit moved.