diff --git a/core/lib/Drupal/Component/HttpFoundation/SafeRedirectResponse.php b/core/lib/Drupal/Component/HttpFoundation/SafeRedirectResponse.php new file mode 100644 index 0000000..e7d637e --- /dev/null +++ b/core/lib/Drupal/Component/HttpFoundation/SafeRedirectResponse.php @@ -0,0 +1,46 @@ +getTargetUrl(), $response->getStatusCode(), $response->headers->allPreserveCase()); + $safe_response->setProtocolVersion($response->getProtocolVersion()); + $safe_response->setCharset($response->getCharset()); + return $safe_response; + } + + /** + * {@inheritdoc} + */ + public function setTargetUrl($url) { + if (!$this->isSafe($url)) { + throw new \InvalidArgumentException(sprintf('It is not safe to redirect to %s', $url)); + } + return parent::setTargetUrl($url); + } + + /** + * Returns TRUE if the URL is safe to redirect to. + * + * @param string $url + * + * @return bool + */ + abstract protected function isSafe($url); + +} diff --git a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php index 0573eea..6392903 100644 --- a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php @@ -7,13 +7,15 @@ namespace Drupal\Core\EventSubscriber; +use Drupal\Component\HttpFoundation\SafeRedirectResponse; use Drupal\Component\Utility\UrlHelper; +use Drupal\Core\Routing\LocalRedirectResponse; use Drupal\Core\Routing\RequestContext; use Drupal\Core\Routing\UrlGeneratorInterface; +use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Event\FilterResponseEvent; -use Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\EventDispatcher\EventSubscriberInterface; @@ -51,46 +53,74 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext public function checkRedirectUrl(FilterResponseEvent $event) { $response = $event->getResponse(); if ($response instanceOf RedirectResponse) { - $options = array(); - $request = $event->getRequest(); + + // Let the 'destination' query parameter override the redirect target. + // If $response is already a SafeRedirectResponse, it might reject the + // new target as invalid, in which case proceed with the old target. $destination = $request->query->get('destination'); - // A destination from \Drupal::request()->query always overrides the - // current RedirectResponse. We do not allow absolute URLs to be passed - // via \Drupal::request()->query, as this can be an attack vector, with - // the following exception: - // - Absolute URLs that point to this site (i.e. same base URL and - // base path) are allowed. if ($destination) { - if (!UrlHelper::isExternal($destination)) { - // The destination query parameter can be a relative URL in the sense - // of not including the scheme and host, but its path is expected to - // be absolute (start with a '/'). For such a case, prepend the - // scheme and host, because the 'Location' header must be absolute. - if (strpos($destination, '/') === 0) { - $destination = $request->getSchemeAndHttpHost() . $destination; - } - else { - // Legacy destination query parameters can be relative paths that - // have not yet been converted to URLs (outbound path processors - // and other URL handling still needs to be performed). - // @todo As generateFromPath() is deprecated, remove this in - // https://www.drupal.org/node/2418219. - $destination = UrlHelper::parse($destination); - $path = $destination['path']; - $options['query'] = $destination['query']; - $options['fragment'] = $destination['fragment']; - // The 'Location' HTTP header must always be absolute. - $options['absolute'] = TRUE; - $destination = $this->urlGenerator->generateFromPath($path, $options); - } + // The 'Location' HTTP header must always be absolute. + $destination = $this->getDestinationAsAbsoluteUrl($destination, $request->getSchemeAndHttpHost()); + try { $response->setTargetUrl($destination); } - elseif (UrlHelper::externalIsLocal($destination, $this->requestContext->getCompleteBaseUrl())) { - $response->setTargetUrl($destination); + catch (\InvalidArgumentException $e) { + } + } + + // Regardless of whether the target is the original one or the overridden + // destination, ensure that all redirects are safe. + if (!($response instanceOf SafeRedirectResponse)) { + try { + // SafeRedirectResponse is an abstract class that requires a + // concrete implementation. Default to LocalRedirectResponse, which + // considers only redirects to within the same site as safe. + $safe_response = LocalRedirectResponse::createFromRedirectResponse($response); + } + 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. + $message = 'Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.'; + trigger_error($message, E_USER_ERROR); + $safe_response = new Response($message, 400); } + $event->setResponse($safe_response); + } + } + } + + /** + * @todo Document. + */ + protected function getDestinationAsAbsoluteUrl($destination, $scheme_and_host) { + if (!UrlHelper::isExternal($destination)) { + // The destination query parameter can be a relative URL in the sense + // of not including the scheme and host, but its path is expected to + // be absolute (start with a '/'). For such a case, prepend the + // scheme and host, because the 'Location' header must be absolute. + if (strpos($destination, '/') === 0) { + $destination = $scheme_and_host . $destination; + } + else { + // Legacy destination query parameters can be relative paths that + // have not yet been converted to URLs (outbound path processors + // and other URL handling still needs to be performed). + // @todo As generateFromPath() is deprecated, remove this in + // https://www.drupal.org/node/2418219. + $destination = UrlHelper::parse($destination); + $path = $destination['path']; + $options = [ + 'query' => $destination['query'], + 'fragment' => $destination['fragment'], + 'absolute' => TRUE, + ]; + $destination = $this->urlGenerator->generateFromPath($path, $options); } } + return $destination; } /** diff --git a/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php new file mode 100644 index 0000000..9b06e74 --- /dev/null +++ b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php @@ -0,0 +1,44 @@ +getRequestContext()->getCompleteBaseUrl()); + } + + /** + * Returns the request context. + * + * @return \Drupal\Core\Routing\RequestContext + */ + protected function getRequestContext() { + if (!isset($this->requestContext)) { + $this->requestContext = \Drupal::service('router.request_context'); + } + return $this->requestContext; + } + +} diff --git a/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php new file mode 100644 index 0000000..c316e1e --- /dev/null +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php @@ -0,0 +1,48 @@ +trustedUrls[$url] = TRUE; + parent::__construct($url, $status, $headers); + } + + /** + * Sets the target URL to a trusted URL. + * + * @param string $url + * A trusted URL. + * + * @return $this + */ + public function setTrustedTargetUrl($url) { + $this->trustedUrls[$url] = TRUE; + return $this->setTargetUrl($url); + } + + /** + * {@inheritdoc} + */ + protected function isSafe($url) { + return !empty($this->trustedUrls[$url]) || parent::isSafe($url); + } + +} diff --git a/core/modules/field_ui/src/Tests/ManageFieldsTest.php b/core/modules/field_ui/src/Tests/ManageFieldsTest.php index 3b90bff..fb4a5e6 100644 --- a/core/modules/field_ui/src/Tests/ManageFieldsTest.php +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php @@ -617,6 +617,31 @@ function testDuplicateFieldName() { } /** + * Tests that external URLs in the 'destinations' query parameter are blocked. + */ + public function testExternalDestinations() { + $options = [ + 'query' => ['destinations' => ['http://example.com']], + ]; + $this->drupalPostForm('admin/structure/types/manage/article/fields/node.article.body/storage', [], 'Save field settings', $options); + // The external redirect should not fire. + $this->assertUrl('admin/structure/types/manage/article/fields/node.article.body/storage', $options); + $this->assertResponse(400); + + foreach ($this->assertions as $id => $assertion) { + if ($assertion['status'] === 'exception') { + // Ensure that the right error is thrown. + $this->assertEqual('User error', $assertion['message_group']); + $this->assertEqual('Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\RedirectResponseAllowExternalUrl for it', $assertion['message']); + // Finally remove that exception entry in order to not fail the test + // itself. + unset($this->assertions[$id]); + $this->deleteAssert($assertion['message_id']); + } + } + } + + /** * Tests that deletion removes field storages and fields as expected for a term. */ function testDeleteTaxonomyField() { diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php index e100205..51568ca 100644 --- a/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php @@ -9,6 +9,7 @@ use Drupal\Core\EventSubscriber\RedirectResponseSubscriber; use Drupal\Core\Routing\RequestContext; +use Drupal\Core\Routing\TrustedRedirectResponse; use Drupal\Tests\UnitTestCase; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\RedirectResponse; @@ -87,12 +88,10 @@ public function testDestinationRedirect(Request $request, $expected) { public static function providerTestDestinationRedirect() { return array( array(new Request(), FALSE), - array(new Request(array('destination' => 'http://example.com')), FALSE), - array(new Request(array('destination' => 'http://example.com/foobar')), FALSE), array(new Request(array('destination' => 'http://example.ca/drupal')), FALSE), array(new Request(array('destination' => 'test')), 'http://example.com/drupal/test'), - array(new Request(array('destination' => '/test')), 'http://example.com/test'), - array(new Request(array('destination' => '/example.com')), 'http://example.com/example.com'), + array(new Request(array('destination' => '/drupal/test')), 'http://example.com/drupal/test'), + array(new Request(array('destination' => 'example.com')), 'http://example.com/drupal/example.com'), array(new Request(array('destination' => 'example:com')), 'http://example.com/drupal/example:com'), array(new Request(array('destination' => 'javascript:alert(0)')), 'http://example.com/drupal/javascript:alert(0)'), array(new Request(array('destination' => 'http://example.com/drupal/')), 'http://example.com/drupal/'), @@ -101,7 +100,94 @@ public static function providerTestDestinationRedirect() { } /** - * @expectedException \InvalidArgumentException + * @dataProvider providerTestDestinationRedirectToExternalUrl + * + * @expectedException \PHPUnit_Framework_Error + */ + public function testDestinationRedirectToExternalUrl($request, $expected) { + $dispatcher = new EventDispatcher(); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $response = new RedirectResponse('http://other-example.com'); + $url_generator = $this->getMockBuilder('Drupal\Core\Routing\UrlGenerator') + ->disableOriginalConstructor() + ->setMethods(array('generateFromPath')) + ->getMock(); + + if ($expected) { + $url_generator + ->expects($this->any()) + ->method('generateFromPath') + ->willReturnMap([ + ['test', ['query' => [], 'fragment' => '', 'absolute' => TRUE], FALSE, 'http://example.com/drupal/test'], + ['example.com', ['query' => [], 'fragment' => '', 'absolute' => TRUE], FALSE, 'http://example.com/drupal/example.com'], + ['example:com', ['query' => [], 'fragment' => '', 'absolute' => TRUE], FALSE, 'http://example.com/drupal/example:com'], + ['javascript:alert(0)', ['query' => [], 'fragment' => '', 'absolute' => TRUE], FALSE, 'http://example.com/drupal/javascript:alert(0)'], + ['/test', ['query' => [], 'fragment' => '', 'absolute' => TRUE], FALSE, 'http://example.com/test'], + ]); + } + + $request_context = $this->getMockBuilder('Drupal\Core\Routing\RequestContext') + ->disableOriginalConstructor() + ->getMock(); + $request_context->expects($this->any()) + ->method('getCompleteBaseUrl') + ->willReturn('http://example.com/drupal'); + $request->headers->set('HOST', 'example.com'); + + $listener = new RedirectResponseSubscriber($url_generator, $request_context); + $dispatcher->addListener(KernelEvents::RESPONSE, array($listener, 'checkRedirectUrl')); + $event = new FilterResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response); + $dispatcher->dispatch(KernelEvents::RESPONSE, $event); + + $this->assertEquals(400, $event->getResponse()->getStatusCode()); + } + + /** + * @covers ::checkRedirectUrl + */ + public function testRedirectWithOptInExternalUrl() { + $dispatcher = new EventDispatcher(); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $response = new TrustedRedirectResponse('http://external-url.com'); + $url_generator = $this->getMockBuilder('Drupal\Core\Routing\UrlGenerator') + ->disableOriginalConstructor() + ->setMethods(array('generateFromPath')) + ->getMock(); + + $request_context = $this->getMockBuilder('Drupal\Core\Routing\RequestContext') + ->disableOriginalConstructor() + ->getMock(); + $request_context->expects($this->any()) + ->method('getCompleteBaseUrl') + ->willReturn('http://example.com/drupal'); + + $request = Request::create(''); + $request->headers->set('HOST', 'example.com'); + + $listener = new RedirectResponseSubscriber($url_generator, $request_context); + $dispatcher->addListener(KernelEvents::RESPONSE, array($listener, 'checkRedirectUrl')); + $event = new FilterResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response); + $dispatcher->dispatch(KernelEvents::RESPONSE, $event); + + $target_url = $event->getResponse()->getTargetUrl(); + $this->assertEquals('http://external-url.com', $target_url); + } + + /** + * Data provider for testDestinationRedirectToExternalUrl(). + */ + public function providerTestDestinationRedirectToExternalUrl() { + return [ + 'absolute external url' => [new Request(['destination' => 'http://example.com']), 'http://example.com'], + 'absolute external url with folder' => [new Request(['destination' => 'http://example.com/foobar']), 'http://example.com/foobar'], + 'path without drupal basepath' => [new Request(['destination' => '/test']), 'http://example.com/test'], + 'path with URL' => [new Request(['destination' => '/example.com']), 'http://example.com/example.com'], + 'path with URL and two slashes' => [new Request(['destination' => '//example.com']), 'http://example.com//example.com'], + ]; + } + + /** + * @expectedException \PHPUnit_Framework_Error * * @dataProvider providerTestDestinationRedirectWithInvalidUrl */ @@ -119,6 +205,8 @@ public function testDestinationRedirectWithInvalidUrl(Request $request) { $dispatcher->addListener(KernelEvents::RESPONSE, array($listener, 'checkRedirectUrl')); $event = new FilterResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response); $dispatcher->dispatch(KernelEvents::RESPONSE, $event); + + $this->assertEquals(400, $event->getResponse()->getStatusCode()); } /** diff --git a/core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php b/core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php new file mode 100644 index 0000000..4ac1333 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php @@ -0,0 +1,57 @@ +setTargetUrl('/example2'); + + $this->assertEquals('/example2', $redirect_response->getTargetUrl()); + } + + /** + * @covers ::setTargetUrl + * @expectedException \InvalidArgumentException + */ + public function testSetTargetUrlWithUntrustedUrl() { + $request_context = new RequestContext(); + $request_context->setCompleteBaseUrl('https://www.drupal.org'); + $container = new ContainerBuilder(); + $container->set('router.request_context', $request_context); + \Drupal::setContainer($container); + + $redirect_response = new TrustedRedirectResponse('/example'); + + $redirect_response->setTargetUrl('http://evil-url.com/example'); + } + + /** + * @covers ::setTargetUrl + */ + public function testSetTargetUrlWithTrustedUrl() { + $redirect_response = new TrustedRedirectResponse('/example'); + + $redirect_response->setTrustedTargetUrl('http://good-external-url.com/example'); + $this->assertEquals('http://good-external-url.com/example', $redirect_response->getTargetUrl()); + } + +}