Issue #3419693 by mondrake, alexpott, smustgrave, catch: Log error instead of... (5959b69d) · Commits · project / drupal

core/core.services.yml

+1 −1

Original line number	Diff line number	Diff line
		@@ -1384,7 +1384,7 @@ services:
		class: Drupal\Core\EventSubscriber\ResponseGeneratorSubscriber
		redirect_response_subscriber:
		class: Drupal\Core\EventSubscriber\RedirectResponseSubscriber
		arguments: ['@unrouted_url_assembler', '@router.request_context']
		autowire: true
		redirect_leading_slashes_subscriber:
		class: Drupal\Core\EventSubscriber\RedirectLeadingSlashesSubscriber
		request_close_subscriber:

core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php

+21 −26

Original line number	Diff line number	Diff line
		@@ -7,24 +7,18 @@
		use Drupal\Core\Routing\LocalRedirectResponse;
		use Drupal\Core\Routing\RequestContext;
		use Drupal\Core\Utility\UnroutedUrlAssemblerInterface;
		use Symfony\Component\DependencyInjection\Attribute\AutowireServiceClosure;
		use Symfony\Component\EventDispatcher\EventSubscriberInterface;
		use Symfony\Component\HttpFoundation\RedirectResponse;
		use Symfony\Component\HttpFoundation\Response;
		use Symfony\Component\HttpKernel\KernelEvents;
		use Symfony\Component\HttpKernel\Event\ResponseEvent;
		use Symfony\Component\HttpFoundation\RedirectResponse;
		use Symfony\Component\EventDispatcher\EventSubscriberInterface;
		use Symfony\Component\HttpKernel\KernelEvents;

		/**
		* Allows manipulation of the response object when performing a redirect.
		*/
		class RedirectResponseSubscriber implements EventSubscriberInterface {

		/**
		* The unrouted URL assembler service.
		*
		* @var \Drupal\Core\Utility\UnroutedUrlAssemblerInterface
		*/
		protected $unroutedUrlAssembler;

		/**
		* Whether to ignore the destination query parameter when redirecting.
		*
		@@ -32,22 +26,22 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
		*/
		protected bool $ignoreDestination = FALSE;

		/**
		* The request context.
		*/
		protected RequestContext $requestContext;

		/**
		* Constructs a RedirectResponseSubscriber object.
		*
		* @param \Drupal\Core\Utility\UnroutedUrlAssemblerInterface $url_assembler
		* @param \Drupal\Core\Utility\UnroutedUrlAssemblerInterface $unroutedUrlAssembler
		* The unrouted URL assembler service.
		* @param \Drupal\Core\Routing\RequestContext $request_context
		* @param \Drupal\Core\Routing\RequestContext $requestContext
		* The request context.
		* @param \Closure $loggerClosure
		* A closure that wraps the 'logger.channel.php' service.
		*/
		public function __construct(UnroutedUrlAssemblerInterface $url_assembler, RequestContext $request_context) {
		$this->unroutedUrlAssembler = $url_assembler;
		$this->requestContext = $request_context;
		public function __construct(
		protected UnroutedUrlAssemblerInterface $unroutedUrlAssembler,
		protected RequestContext $requestContext,
		#[AutowireServiceClosure('logger.channel.php')]
		protected \Closure $loggerClosure,
		) {
		}

		/**
		@@ -87,13 +81,14 @@ public function checkRedirectUrl(ResponseEvent $event) {
		}
		catch (\InvalidArgumentException $e) {
		// If the above failed, it's because the redirect target wasn't
		// local. Do not follow that redirect. Display an error message
		// instead. We're already catching one exception, so trigger_error()
		// rather than throw another one.
		// We don't throw an exception, because this is a client error rather than a
		// server error.
		// local. Do not follow that redirect. Log an error message instead,
		// then return a 400 response to the client with the error message.
		// We don't throw an exception, because this is a client error rather
		// than a server error.
		$message = 'Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.';
		trigger_error($message, E_USER_ERROR);
		/** @var \Psr\Log\LoggerInterface $logger */
		$logger = ($this->loggerClosure)();
		$logger->error($message);
		$safe_response = new Response($message, 400);
		}
		$event->setResponse($safe_response);

core/phpstan-baseline.neon

+0 −8

Original line number	Diff line number	Diff line
		@@ -2724,14 +2724,6 @@ parameters:
		count: 1
		path: tests/Drupal/Tests/Core/Entity/EntityUrlTest.php

		-
		message: """
		#^Call to deprecated method expectError\$\$ of class PHPUnit\\\\Framework\\\\TestCase\\:
		https\\://github\\.com/sebastianbergmann/phpunit/issues/5062$#
		"""
		count: 2
		path: tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php

		-
		message: """
		#^Call to deprecated method expectWarning\$\$ of class PHPUnit\\\\Framework\\\\TestCase\\:

core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php

+17 −7

Original line number	Diff line number	Diff line
		@@ -4,12 +4,13 @@

		namespace Drupal\Tests\Core\EventSubscriber;

		use Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher as EventDispatcher;
		use Drupal\Core\EventSubscriber\RedirectResponseSubscriber;
		use Drupal\Core\Routing\TrustedRedirectResponse;
		use Drupal\Core\Utility\UnroutedUrlAssemblerInterface;
		use Drupal\Tests\UnitTestCase;
		use Psr\Log\LoggerInterface;
		use Symfony\Component\DependencyInjection\Container;
		use Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher as EventDispatcher;
		use Symfony\Component\HttpFoundation\RedirectResponse;
		use Symfony\Component\HttpFoundation\Request;
		use Symfony\Component\HttpKernel\Event\ResponseEvent;
		@@ -36,12 +37,21 @@ class RedirectResponseSubscriberTest extends UnitTestCase {
		*/
		protected $urlAssembler;

		/**
		* The mocked logger closure.
		*/
		protected \Closure $loggerClosure;

		/**
		* {@inheritdoc}
		*/
		protected function setUp(): void {
		parent::setUp();

		$this->loggerClosure = function (): LoggerInterface {
		return $this->prophesize(LoggerInterface::class)->reveal();
		};

		$this->requestContext = $this->getMockBuilder('Drupal\Core\Routing\RequestContext')
		->disableOriginalConstructor()
		->getMock();
		@@ -82,7 +92,7 @@ public function testDestinationRedirect(Request $request, $expected) {
		$response = new RedirectResponse('http://example.com/drupal');
		$request->headers->set('HOST', 'example.com');

		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
		$dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
		$event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
		$dispatcher->dispatch($event, KernelEvents::RESPONSE);
		@@ -122,11 +132,11 @@ public function testDestinationRedirectToExternalUrl($request, $expected) {
		$kernel = $this->createMock('Symfony\Component\HttpKernel\HttpKernelInterface');
		$response = new RedirectResponse('http://other-example.com');

		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
		$dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
		$event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
		$this->expectError();
		$dispatcher->dispatch($event, KernelEvents::RESPONSE);
		$this->assertSame(400, $event->getResponse()->getStatusCode());
		}

		/**
		@@ -139,7 +149,7 @@ public function testRedirectWithOptInExternalUrl() {
		$request = Request::create('');
		$request->headers->set('HOST', 'example.com');

		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
		$dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
		$event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
		$dispatcher->dispatch($event, KernelEvents::RESPONSE);
		@@ -170,11 +180,11 @@ public function testDestinationRedirectWithInvalidUrl(Request $request) {
		$kernel = $this->createMock('Symfony\Component\HttpKernel\HttpKernelInterface');
		$response = new RedirectResponse('http://example.com/drupal');

		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
		$listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
		$dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
		$event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
		$this->expectError();
		$dispatcher->dispatch($event, KernelEvents::RESPONSE);
		$this->assertSame(400, $event->getResponse()->getStatusCode());
		}

		/**