Problem/Motivation

The UrlHelper::isExternal() always checking for destination query string (even if it missing) and causes following in watchdog

Deprecated function: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in Drupal\Component\Utility\UrlHelper::isExternal() (line 214 of /var/www/html/web/core/lib/Drupal/Component/Utility/UrlHelper.php)

#0 /var/www/html/web/core/includes/bootstrap.inc(346): _drupal_error_handler_real()
#1 [internal function]: _drupal_error_handler()
#2 /var/www/html/web/core/lib/Drupal/Component/Utility/UrlHelper.php(214): strpos()
#3 /var/www/html/web/core/lib/Drupal/Core/Routing/RedirectDestination.php(60): Drupal\Component\Utility\UrlHelper::isExternal()
#4 /var/www/html/web/core/lib/Drupal/Core/Routing/RedirectDestination.php(51): Drupal\Core\Routing\RedirectDestination->get()
#5 /var/www/html/web/core/modules/big_pipe/big_pipe.module(61): Drupal\Core\Routing\RedirectDestination->getAsArray()
#6 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(312): big_pipe_page_attachments()
#7 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(288): Drupal\Core\Render\MainContent\HtmlRenderer->invokePageAttachmentHooks()
#8 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(132): Drupal\Core\Render\MainContent\HtmlRenderer->prepare()
#9 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php(90): Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse()
#10 [internal function]: Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray()
#11 /var/www/html/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func()
#12 /var/www/html/web/vendor/symfony/http-kernel/HttpKernel.php(163): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()
#13 /var/www/html/web/vendor/symfony/http-kernel/HttpKernel.php(80): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#14 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle()
#15 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle()
#16 /var/www/html/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle()
#17 /var/www/html/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass()
#18 /var/www/html/web/vendor/asm89/stack-cors/src/Asm89/Stack/Cors.php(49): Drupal\page_cache\StackMiddleware\PageCache->handle()
#19 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Asm89\Stack\Cors->handle()
#20 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
#21 /var/www/html/web/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
#22 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(708): Stack\StackedHttpKernel->handle()
#23 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle()
#24 {main}

Proposed resolution

check if destination exist before passing to url-helper

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3238942-6.patch1.04 KBandypost
#6 interdiff.txt1.07 KBandypost
#2 3238942-2.patch702 bytesandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3238942-2.patch702 bytes

the fix

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.
No problems with the extra check for if the destination exists.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

This can be written better.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
diff --git a/core/lib/Drupal/Core/Routing/RedirectDestination.php b/core/lib/Drupal/Core/Routing/RedirectDestination.php
index 1945e333c14..008e5e01f9e 100644
--- a/core/lib/Drupal/Core/Routing/RedirectDestination.php
+++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php
@@ -57,11 +57,11 @@ public function getAsArray() {
   public function get() {
     if (!isset($this->destination)) {
       $query = $this->requestStack->getCurrentRequest()->query;
-      if (UrlHelper::isExternal($query->get('destination'))) {
-        $this->destination = '/';
-      }
-      elseif ($query->has('destination')) {
+      if ($query->has('destination')) {
         $this->destination = $query->get('destination');
+        if (UrlHelper::isExternal($this->destination)) {
+          $this->destination = '/';
+        }
       }
       else {
         $this->destination = $this->urlGenerator->generateFromRoute('<current>', [], ['query' => UrlHelper::filterQueryParameters($query->all())]);
diff --git a/core/lib/Drupal/Core/Routing/RouteProvider.php b/core/lib/Drupal/Core/Routing/RouteProvider.php

Is the patch for the meta issue. It means we don't call $query->has('destination') and $query->get('destination') (potentially) twice.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Related issues: +#2455083: Open redirect fixes from SA-CORE-2015-001 need to be ported to Drupal 8
StatusFileSize
new interdiff.txt1.07 KB
new 3238942-6.patch1.04 KB

Thank you, moreover I checked git history and found that this hunk added in #2455083: Open redirect fixes from SA-CORE-2015-001 need to be ported to Drupal 8 (security)

Added comment pointing to SA

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

+1 for comment linking to SA explaining why we can't use external destinations.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed c7774d2 and pushed to 9.3.x. Thanks!

  • alexpott committed c7774d2 on 9.3.x 
    Issue #3238942 by andypost, alexpott: \Drupal\Core\Routing\...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.