scheme-less favicon and logo URL are not accepted

Hi Dupalers,

Please let me know if this could be a valid issue, if yes I have my code ready and will post a patch ASAP.

Thanks in advance!!

The bug report is valid.

I would agree that it would be good to support schema-less urls.

I think we should check that if the url starts with '//' that adding 'http:' makes it valid, e.g. is_file() does return TRUE then.

So the patch needs some work, but can be committed fairly quick then I think.

Oh and please check on http://simplytest.me if this is an issue for Drupal 8 as well.

Unfortunately per our backport policy it would need to be fixed there first - if that was the case.

And thank you for your continued contributions!

Patch for D8.

RTBC for D8

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -464,11 +465,20 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    // Scheme-independant URL (dual HTTP/HTTPS) are valid.
+    if (strncmp($path, "//", 2) == 0) {
+      $http_path = 'http:' . $path;
+      $file_headers = @get_headers($http_path);
+      if (UrlHelper::isValid($http_path) && strstr($file_headers[0], '404') === FALSE && $file_headers != FALSE) {
+        return $path;
+      }
+    }

Imho this should come after the absolute local path check. Also it should just use is_file() like other checks in the function instead of get_headers()

Updates as per #9.

I think it is RTBC - except for two nits.

1. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -473,6 +475,15 @@ protected function validatePath($path) {
   +      if (UrlHelper::isValid($http_path) && is_file(substr($file_path, 1, strlen($file_path)))) {
   

   nit - IIRC, substr() can just take 2 arguments as well.

   e.g.

   is_file(substr($path, 1));

   should be equivalent.

2. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -82,6 +82,11 @@ function testThemeSettings() {
   +      substr($base_url, strpos($base_url, '//')) . '/' . $default_theme_path . '/logo.svg' => [
   +        'form' => substr($base_url, strpos($base_url, '//')) . '/' . $default_theme_path . '/logo.svg',
   +        'src' => substr($base_url, strpos($base_url, '//')) . '/' . $default_theme_path . '/logo.svg',
   +      ],
   

   IIRC correctly base_url is deprecated, but unsure right now.

Updates as per #11.

Back to RTBC, thanks!

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -473,6 +475,15 @@ protected function validatePath($path) {
+    // Scheme-independant URL (dual HTTP/HTTPS) are valid.
+    if (strncmp($path, "//", 2) == 0) {
+      $http_path = 'http:' . $path;
+      $parsed_url = parse_url($http_path);
+      $file_path = $parsed_url['path'];
+      if (UrlHelper::isValid($http_path) && is_file(substr($file_path, 1))) {
+        return $path;
+      }

I cannot really express that, but I think we could use Url::fromUri to validate a good bunch of the stuff on the method.

#15 seems like this needs work

#15: Can you please provide a little more guidance for the contributors on this issue on what you would like to see? Thank you!

Rerolled the patch as per comments in #15.

Updated patch against 8.4.x branch.

Any Update on this issue ?

reroll patch from #24 to 8.7.x

Back to rtbc.

1. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -491,6 +492,17 @@ protected function validatePath($path) {
   +    if (strncmp($path, "//", 2) == 0) {
   +      $http_path = Url::fromUri('http:' . $path)->toString();
   +      $parsed_url = parse_url($http_path);
   +      $file_path = $parsed_url['path'];
   +      if (is_file(substr($file_path, 1))) {
   +        return $path;
   +      }
   +    }
   

   This is capable of throwing \InvalidArgumentException - these exceptions shouldn't make it through to the user and so need to be caught. That said reading the fromUri code I'm not that sure what we are getting here. I know @dawehner asked for something but there no additional validation and we're having to call parse_url twice. For me this is simpler as:

       // Scheme-independent URL (dual HTTP/HTTPS) are valid.
       if (strncmp($path, "//", 2) == 0) {
         $parsed_url = parse_url('http:' . $path);
         if (isset($parsed_url['path']) && is_file(substr($parsed_url['path'], 1))) {
           return $path;
         }
         return FALSE;
       }
   

   Also independent is spelt wrong.

2. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
   @@ -156,6 +163,7 @@ public function testThemeSettings() {
   +      '//core/misc/whatever.png',
   

   Needs a descriptive comment above it like the rest and perhaps we could add some more invalid scheme-independent URLs for testing.

3. Another thing that we might need is some sort of UI text change to say this is supported. What makes this difficult is that the existing favicon and logo files don't have great descriptions that are easy to change.

This came up as a daily BSI target.

Seems this got left on #34 feedback which appears valid.

IS should also follow issue template

Thanks.