Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#6 | 3238942-6.patch | 1.04 KB | andypost |
#6 | interdiff.txt | 1.07 KB | andypost |
#2 | 3238942-2.patch | 702 bytes | andypost |
Comments
Comment #2
andypostthe fix
Comment #3
daffie CreditAttribution: daffie commentedLooks good to me.
No problems with the extra check for if the destination exists.
Comment #4
alexpottThis can be written better.
Comment #5
alexpottIs the patch for the meta issue. It means we don't call $query->has('destination') and $query->get('destination') (potentially) twice.
Comment #6
andypostThank 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
Comment #7
longwave+1 for comment linking to SA explaining why we can't use external destinations.
Comment #8
alexpottCommitted c7774d2 and pushed to 9.3.x. Thanks!