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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

Status: Active » Needs review
FileSize
702 bytes

the fix

daffie’s picture

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

Status: Reviewed & tested by the community » Needs work

This can be written better.

alexpott’s picture

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

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

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

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.