Issue #2076325 by neclimdul, Mile23, alexpott: Fixed Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.

core/lib/Drupal/Component/Utility/UrlHelper.php

@@ -224,21 +224,29 @@ class UrlHelper {
    * @param string $base_url
    *   The base URL string to check against, such as "http://example.com/"
    *
-   * @return
+   * @return bool
    *   TRUE if the URL has the same domain and base path.
+   *
+   * @throws \InvalidArgumentException
+   *   Exception thrown when a either $url or $bath_url are not fully qualified.
    */
   public static function externalIsLocal($url, $base_url) {
     $url_parts = parse_url($url);
-    $base_host = parse_url($base_url, PHP_URL_HOST);
+    $base_parts = parse_url($base_url);
 
-    if (!isset($url_parts['path'])) {
-      return ($url_parts['host'] == $base_host);
+    if (empty($base_parts['host']) || empty($url_parts['host'])) {
+      throw new \InvalidArgumentException(String::format('A path was passed when a fully qualified domain was expected.'));
+    }
+
+    if (!isset($url_parts['path']) || !isset($base_parts['path'])) {
+      return (!isset($base_parts['path']) || $base_parts['path'] == '/')
+        && ($url_parts['host'] == $base_parts['host']);
     }
     else {
       // When comparing base paths, we need a trailing slash to make sure a
       // partial URL match isn't occurring. Since base_path() always returns
       // with a trailing slash, we don't need to add the trailing slash here.
-      return ($url_parts['host'] == $base_host && stripos($url_parts['path'], $base_url) === 0);
+      return ($url_parts['host'] == $base_parts['host'] && stripos($url_parts['path'], $base_parts['path']) === 0);
     }
   }
 

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

@@ -40,7 +40,7 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
   /**
    * Allows manipulation of the response object when performing a redirect.
    *
-   * @param Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
+   * @param \Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
    *   The Event to process.
    */
   public function checkRedirectUrl(FilterResponseEvent $event) {
@@ -55,7 +55,7 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
       // the following exception:
       // - Absolute URLs that point to this site (i.e. same base URL and
       //   base path) are allowed.
-      if ($destination && (!UrlHelper::isExternal($destination) || UrlHelper::externalIsLocal($destination, base_path()))) {
+      if ($destination && (!UrlHelper::isExternal($destination) || UrlHelper::externalIsLocal($destination, $GLOBALS['base_url']))) {
         $destination = UrlHelper::parse($destination);
 
         $path = $destination['path'];

core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php

@@ -465,4 +465,89 @@ class UrlHelperTest extends UnitTestCase {
     return $data;
   }
 
+  /**
+   * Test detecting external urls that point to local resources.
+   *
+   * @param string $url
+   *   The external url to test.
+   * @param string $base_url
+   *   The base url.
+   * @param bool $expected
+   *   TRUE if an external URL points to this installation as determined by the
+   *   base url.
+   *
+   * @covers ::externalIsLocal
+   * @dataProvider providerTestExternalIsLocal
+   */
+  public function testExternalIsLocal($url, $base_url, $expected) {
+    $this->assertSame($expected, UrlHelper::externalIsLocal($url, $base_url));
+  }
+
+  /**
+   * Provider for local external url detection.
+   *
+   * @see \Drupal\Tests\Component\Utility\UrlHelperTest::testExternalIsLocal()
+   */
+  public function providerTestExternalIsLocal() {
+    return array(
+      // Different mixes of trailing slash.
+      array('http://example.com', 'http://example.com', TRUE),
+      array('http://example.com/', 'http://example.com', TRUE),
+      array('http://example.com', 'http://example.com/', TRUE),
+      array('http://example.com/', 'http://example.com/', TRUE),
+      // Sub directory of site.
+      array('http://example.com/foo', 'http://example.com/', TRUE),
+      array('http://example.com/foo/bar', 'http://example.com/foo', TRUE),
+      array('http://example.com/foo/bar', 'http://example.com/foo/', TRUE),
+      // Different sub-domain.
+      array('http://example.com', 'http://www.example.com/', FALSE),
+      array('http://example.com/', 'http://www.example.com/', FALSE),
+      array('http://example.com/foo', 'http://www.example.com/', FALSE),
+      // Different TLD.
+      array('http://example.com', 'http://example.ca', FALSE),
+      array('http://example.com', 'http://example.ca/', FALSE),
+      array('http://example.com/', 'http://example.ca/', FALSE),
+      array('http://example.com/foo', 'http://example.ca', FALSE),
+      array('http://example.com/foo', 'http://example.ca/', FALSE),
+      // Different site path.
+      array('http://example.com/foo', 'http://example.com/bar', FALSE),
+      array('http://example.com', 'http://example.com/bar', FALSE),
+      array('http://example.com/bar', 'http://example.com/bar/', FALSE),
+    );
+  }
+
+  /**
+   * Test invalid url arguments.
+   *
+   * @param string $url
+   *   The url to test.
+   * @param string $base_url
+   *   The base url.
+   *
+   * @covers ::externalIsLocal
+   * @dataProvider providerTestExternalIsLocalInvalid
+   * @expectedException \InvalidArgumentException
+   */
+  public function testExternalIsLocalInvalid($url, $base_url) {
+    UrlHelper::externalIsLocal($url, $base_url);
+  }
+
+  /**
+   * Provides invalid argument data for local external url detection.
+   *
+   * @see \Drupal\Tests\Component\Utility\UrlHelperTest::testExternalIsLocalInvalid()
+   */
+  public function providerTestExternalIsLocalInvalid() {
+    return array(
+      array('http://example.com/foo', ''),
+      array('http://example.com/foo', 'bar'),
+      array('http://example.com/foo', 'http://'),
+      // Invalid destination urls.
+      array('', 'http://example.com/foo'),
+      array('bar', 'http://example.com/foo'),
+      array('/bar', 'http://example.com/foo'),
+      array('bar/', 'http://example.com/foo'),
+      array('http://', 'http://example.com/foo'),
+    );
+  }
 }

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

@@ -0,0 +1,113 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\Core\EventSubscriber\RedirectResponseSubscriberTest.
+ */
+
+namespace Drupal\Tests\Core\EventSubscriber;
+
+use Drupal\Core\EventSubscriber\RedirectResponseSubscriber;
+use Drupal\Tests\UnitTestCase;
+use Symfony\Component\EventDispatcher\EventDispatcher;
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+/**
+ * Tests response redirection using destination get argument.
+ *
+ * @group Drupal
+ * @group Routing
+ *
+ * @coversDefaultClass \Drupal\Core\EventSubscriber\RedirectResponseSubscriber
+ */
+class RedirectResponseSubscriberTest extends UnitTestCase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'Tests response redirection using destination get argument.',
+      'description' => '',
+      'group' => 'Routing'
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp() {
+    parent::setUp();
+    $GLOBALS['base_url'] = 'http://example.com/drupal';
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function tearDown() {
+    unset($GLOBALS['base_url']);
+    parent::tearDown();
+  }
+
+  /**
+   * Test destination detection and redirection.
+   *
+   * @param Request $request
+   *   The request object with destination query set.
+   * @param bool $expected
+   *   Whether or not a redirect will occur.
+   *
+   * @covers ::checkRedirectUrl
+   * @dataProvider providerTestDestinationRedirect
+   */
+  public function testDestinationRedirect(Request $request, $expected) {
+    $dispatcher = new EventDispatcher();
+    $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
+    $response = new RedirectResponse('http://example.com/drupal');
+    $url_generator = $this->getMockBuilder('Drupal\Core\Routing\UrlGenerator')
+      ->disableOriginalConstructor()
+      ->setMethods(array('generateFromPath'))
+      ->getMock();
+
+    if ($expected) {
+      $url_generator
+        ->expects($this->once())
+        ->method('generateFromPath')
+          ->will($this->returnValue('success'));
+    }
+
+    $listener = new RedirectResponseSubscriber($url_generator);
+    $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();
+    if ($expected) {
+      $this->assertEquals('success', $target_url);
+    }
+    else {
+      $this->assertEquals('http://example.com/drupal', $target_url);
+    }
+  }
+
+  /**
+   * Data provider for testDestinationRedirect().
+   *
+   * @see \Drupal\Tests\Core\EventSubscriber\RedirectResponseSubscriberTest::testDestinationRedirect()
+   */
+  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')), TRUE),
+      array(new Request(array('destination' => 'http://example.com/drupal/')), TRUE),
+      array(new Request(array('destination' => 'http://example.com/drupal/test')), TRUE),
+    );
+  }
+}