[TASK] Migrate Redirect Url Validator into own class 48/59648/3
authorBenni Mack <benni@typo3.org>
Tue, 5 Feb 2019 18:50:42 +0000 (19:50 +0100)
committerAnja Leichsenring <aleichsenring@ab-softlab.de>
Wed, 6 Feb 2019 07:48:39 +0000 (08:48 +0100)
The logic within EXT:felogin for validating proper redirect/referer
URLs is extracted into a new PHP class, which can now be called
separately.

This is useful for further refactorings within the Controller,
and it lays the foundation to use proper validation in various
places other than Extbase.

Resolves: #87660
Releases: master
Change-Id: I79aab5908bde869b7ee3cf730fbf61d658f2ee88
Reviewed-on: https://review.typo3.org/59648
Tested-by: TYPO3com <noreply@typo3.com>
Reviewed-by: Georg Ringer <georg.ringer@gmail.com>
Tested-by: Georg Ringer <georg.ringer@gmail.com>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
typo3/sysext/felogin/Classes/Controller/FrontendLoginController.php
typo3/sysext/felogin/Classes/Validation/RedirectUrlValidator.php [new file with mode: 0644]
typo3/sysext/felogin/Tests/Unit/Controller/FrontendLoginControllerTest.php
typo3/sysext/felogin/Tests/Unit/Validation/RedirectUrlValidatorTest.php [new file with mode: 0644]

index f957d1e..b4163bb 100644 (file)
@@ -14,8 +14,6 @@ namespace TYPO3\CMS\Felogin\Controller;
  * The TYPO3 project - inspiring people to share!
  */
 
-use Psr\Log\LoggerAwareInterface;
-use Psr\Log\LoggerAwareTrait;
 use TYPO3\CMS\Core\Authentication\LoginType;
 use TYPO3\CMS\Core\Context\Context;
 use TYPO3\CMS\Core\Crypto\PasswordHashing\PasswordHashFactory;
@@ -23,10 +21,10 @@ use TYPO3\CMS\Core\Crypto\Random;
 use TYPO3\CMS\Core\Database\Connection;
 use TYPO3\CMS\Core\Database\ConnectionPool;
 use TYPO3\CMS\Core\Database\Query\Restriction\FrontendRestrictionContainer;
-use TYPO3\CMS\Core\Exception\SiteNotFoundException;
 use TYPO3\CMS\Core\Site\SiteFinder;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Core\Utility\HttpUtility;
+use TYPO3\CMS\Felogin\Validation\RedirectUrlValidator;
 use TYPO3\CMS\Frontend\Plugin\AbstractPlugin;
 
 /**
@@ -34,10 +32,8 @@ use TYPO3\CMS\Frontend\Plugin\AbstractPlugin;
  *
  * @internal this is a concrete TYPO3 implementation and solely used for EXT:felogin and not part of TYPO3's Core API.
  */
-class FrontendLoginController extends AbstractPlugin implements LoggerAwareInterface
+class FrontendLoginController extends AbstractPlugin
 {
-    use LoggerAwareTrait;
-
     /**
      * Same as class name
      *
@@ -97,9 +93,6 @@ class FrontendLoginController extends AbstractPlugin implements LoggerAwareInter
      */
     protected $logintype;
 
-    /** @var SiteFinder */
-    protected $siteFinder;
-
     /**
      * A list of page UIDs, either an integer or a comma-separated list of integers
      *
@@ -115,6 +108,11 @@ class FrontendLoginController extends AbstractPlugin implements LoggerAwareInter
     public $referer;
 
     /**
+     * @var RedirectUrlValidator
+     */
+    protected $urlValidator;
+
+    /**
      * The main method of the plugin
      *
      * @param string $content The PlugIn content
@@ -124,7 +122,11 @@ class FrontendLoginController extends AbstractPlugin implements LoggerAwareInter
      */
     public function main($content, $conf)
     {
-        $this->siteFinder = GeneralUtility::makeInstance(SiteFinder::class);
+        $this->urlValidator = GeneralUtility::makeInstance(
+            RedirectUrlValidator::class,
+            GeneralUtility::makeInstance(SiteFinder::class),
+            (int)$this->frontendController->id
+        );
 
         // Loading TypoScript array into object variable:
         $this->conf = $conf;
@@ -147,16 +149,16 @@ class FrontendLoginController extends AbstractPlugin implements LoggerAwareInter
         }
         // GPvars:
         $this->logintype = GeneralUtility::_GP('logintype');
-        $this->referer = $this->validateRedirectUrl(GeneralUtility::_GP('referer'));
-        $this->noRedirect = $this->piVars['noredirect'] || $this->conf['redirectDisable'];
-        // If config.typolinkLinkAccessRestrictedPages is set, the var is return_url
-        $returnUrl = GeneralUtility::_GP('return_url');
-        if ($returnUrl) {
-            $this->redirectUrl = $returnUrl;
+
+        if ($this->urlValidator->isValid(GeneralUtility::_GP('referer'))) {
+            $this->referer = GeneralUtility::_GP('referer');
         } else {
-            $this->redirectUrl = GeneralUtility::_GP('redirect_url');
+            $this->referer = '';
         }
-        $this->redirectUrl = $this->validateRedirectUrl($this->redirectUrl);
+        $this->noRedirect = $this->piVars['noredirect'] || $this->conf['redirectDisable'];
+        // If config.typolinkLinkAccessRestrictedPages is set, the var is return_url
+        $this->redirectUrl = GeneralUtility::_GP('return_url') ?: GeneralUtility::_GP('redirect_url');
+        $this->redirectUrl = $this->urlValidator->isValid($this->redirectUrl) ? $this->redirectUrl : '';
         // Get Template
         $templateFile = $this->conf['templateFile'] ?: 'EXT:felogin/Resources/Private/Templates/FrontendLogin.html';
         $template = GeneralUtility::getFileAbsFileName($templateFile);
@@ -1007,87 +1009,4 @@ class FrontendLoginController extends AbstractPlugin implements LoggerAwareInter
         }
         return $marker;
     }
-
-    /**
-     * Returns a valid and XSS cleaned url for redirect, checked against configuration "allowedRedirectHosts"
-     *
-     * @param string $url
-     * @return string cleaned referer or empty string if not valid
-     */
-    protected function validateRedirectUrl($url)
-    {
-        $url = strval($url);
-        if ($url === '') {
-            return '';
-        }
-        // Validate the URL:
-        if ($this->isRelativeUrl($url) || $this->isInCurrentDomain($url) || $this->isInLocalDomain($url)) {
-            return $url;
-        }
-        // URL is not allowed
-        $this->logger->warning('Url "' . $url . '" for redirect was not accepted!');
-        return '';
-    }
-
-    /**
-     * Determines whether the URL is on the current host and belongs to the
-     * current TYPO3 installation. The scheme part is ignored in the comparison.
-     *
-     * @param string $url URL to be checked
-     * @return bool Whether the URL belongs to the current TYPO3 installation
-     */
-    protected function isInCurrentDomain($url)
-    {
-        $urlWithoutSchema = preg_replace('#^https?://#', '', $url);
-        $siteUrlWithoutSchema = preg_replace('#^https?://#', '', GeneralUtility::getIndpEnv('TYPO3_SITE_URL'));
-        return strpos($urlWithoutSchema . '/', GeneralUtility::getIndpEnv('HTTP_HOST') . '/') === 0
-            && strpos($urlWithoutSchema, $siteUrlWithoutSchema) === 0;
-    }
-
-    /**
-     * Determines whether the URL matches a domain
-     * in the sys_domain database table.
-     *
-     * @param string $url Absolute URL which needs to be checked
-     * @return bool Whether the URL is considered to be local
-     */
-    protected function isInLocalDomain($url)
-    {
-        $result = false;
-        if (GeneralUtility::isValidUrl($url)) {
-            $parsedUrl = parse_url($url);
-            if ($parsedUrl['scheme'] === 'http' || $parsedUrl['scheme'] === 'https') {
-                $host = $parsedUrl['host'];
-
-                try {
-                    $site = $this->siteFinder->getSiteByPageId((int)$this->frontendController->id);
-                    return $site->getBase()->getHost() === $host;
-                } catch (SiteNotFoundException $e) {
-                    // nothing found
-                }
-            }
-        }
-
-        return $result;
-    }
-
-    /**
-     * Determines whether the URL is relative to the
-     * current TYPO3 installation.
-     *
-     * @param string $url URL which needs to be checked
-     * @return bool Whether the URL is considered to be relative
-     */
-    protected function isRelativeUrl($url)
-    {
-        $url = GeneralUtility::sanitizeLocalUrl($url);
-        if (!empty($url)) {
-            $parsedUrl = @parse_url($url);
-            if ($parsedUrl !== false && !isset($parsedUrl['scheme']) && !isset($parsedUrl['host'])) {
-                // If the relative URL starts with a slash, we need to check if it's within the current site path
-                return $parsedUrl['path'][0] !== '/' || GeneralUtility::isFirstPartOfStr($parsedUrl['path'], GeneralUtility::getIndpEnv('TYPO3_SITE_PATH'));
-            }
-        }
-        return false;
-    }
 }
diff --git a/typo3/sysext/felogin/Classes/Validation/RedirectUrlValidator.php b/typo3/sysext/felogin/Classes/Validation/RedirectUrlValidator.php
new file mode 100644 (file)
index 0000000..62e3168
--- /dev/null
@@ -0,0 +1,131 @@
+<?php
+declare(strict_types = 1);
+
+namespace TYPO3\CMS\Felogin\Validation;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use Psr\Log\LoggerAwareInterface;
+use Psr\Log\LoggerAwareTrait;
+use TYPO3\CMS\Core\Exception\SiteNotFoundException;
+use TYPO3\CMS\Core\Site\SiteFinder;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+
+/**
+ * Used to check if a referrer or a redirect URL is valid to be used as within Frontend Logins
+ * for redirects.
+ * @internal for now as it might get adopted for further streamlining against other validation paradigms
+ */
+class RedirectUrlValidator implements LoggerAwareInterface
+{
+    use LoggerAwareTrait;
+
+    /**
+     * @var SiteFinder
+     */
+    protected $siteFinder;
+
+    /**
+     * @var int
+     */
+    protected $pageId;
+
+    /**
+     * @param SiteFinder|null $siteFinder
+     * @param int $currentPageId
+     */
+    public function __construct(?SiteFinder $siteFinder, int $currentPageId)
+    {
+        $this->siteFinder = $siteFinder ?? GeneralUtility::makeInstance(SiteFinder::class);
+        $this->pageId = $currentPageId;
+    }
+
+    /**
+     * Checks if a given URL is valid / properly sanitized and/or the domain is known to TYPO3.
+     *
+     * @param string $value
+     * @return bool
+     */
+    public function isValid(string $value): bool
+    {
+        if ($value === '') {
+            return false;
+        }
+        // Validate the URL
+        if ($this->isRelativeUrl($value) || $this->isInCurrentDomain($value) || $this->isInLocalDomain($value)) {
+            return true;
+        }
+        // URL is not allowed
+        $this->logger->warning('Url "' . $value . '" was not accepted.');
+        return false;
+    }
+
+    /**
+     * Determines whether the URL is on the current host and belongs to the
+     * current TYPO3 installation. The scheme part is ignored in the comparison.
+     *
+     * @param string $url URL to be checked
+     * @return bool Whether the URL belongs to the current TYPO3 installation
+     */
+    protected function isInCurrentDomain(string $url): bool
+    {
+        $urlWithoutSchema = preg_replace('#^https?://#', '', $url);
+        $siteUrlWithoutSchema = preg_replace('#^https?://#', '', GeneralUtility::getIndpEnv('TYPO3_SITE_URL'));
+        return strpos($urlWithoutSchema . '/', GeneralUtility::getIndpEnv('HTTP_HOST') . '/') === 0
+            && strpos($urlWithoutSchema, $siteUrlWithoutSchema) === 0;
+    }
+
+    /**
+     * Determines whether the URL matches a domain known to TYPO3.
+     *
+     * @param string $url Absolute URL which needs to be checked
+     * @return bool Whether the URL is considered to be local
+     */
+    protected function isInLocalDomain(string $url): bool
+    {
+        if (!GeneralUtility::isValidUrl($url)) {
+            return false;
+        }
+        $parsedUrl = parse_url($url);
+        if ($parsedUrl['scheme'] === 'http' || $parsedUrl['scheme'] === 'https') {
+            $host = $parsedUrl['host'];
+            try {
+                $site = $this->siteFinder->getSiteByPageId($this->pageId);
+                return $site->getBase()->getHost() === $host;
+            } catch (SiteNotFoundException $e) {
+                // nothing found
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Determines whether the URL is relative to the current TYPO3 installation.
+     *
+     * @param string $url URL which needs to be checked
+     * @return bool Whether the URL is considered to be relative
+     */
+    protected function isRelativeUrl($url): bool
+    {
+        $url = GeneralUtility::sanitizeLocalUrl($url);
+        if (!empty($url)) {
+            $parsedUrl = @parse_url($url);
+            if ($parsedUrl !== false && !isset($parsedUrl['scheme']) && !isset($parsedUrl['host'])) {
+                // If the relative URL starts with a slash, we need to check if it's within the current site path
+                return $parsedUrl['path'][0] !== '/' || GeneralUtility::isFirstPartOfStr($parsedUrl['path'], GeneralUtility::getIndpEnv('TYPO3_SITE_PATH'));
+            }
+        }
+        return false;
+    }
+}
index 8284a2a..a7e7a26 100644 (file)
@@ -15,11 +15,8 @@ namespace TYPO3\CMS\Felogin\Tests\Unit\Controller;
  * The TYPO3 project - inspiring people to share!
  */
 
-use Psr\Log\NullLogger;
 use TYPO3\CMS\Core\Authentication\LoginType;
-use TYPO3\CMS\Core\Core\Environment;
-use TYPO3\CMS\Core\Site\Entity\Site;
-use TYPO3\CMS\Core\Site\SiteFinder;
+use TYPO3\CMS\Felogin\Controller\FrontendLoginController;
 use TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer;
 use TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController;
 use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
@@ -29,218 +26,16 @@ use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
  */
 class FrontendLoginControllerTest extends UnitTestCase
 {
-    /**
-     * @var bool Restore Environment after tests
-     */
-    protected $backupEnvironment = true;
-
-    /**
-     * @var \TYPO3\CMS\Felogin\Controller\FrontendLoginController|\TYPO3\TestingFramework\Core\AccessibleObjectInterface
-     */
-    protected $accessibleFixture;
-
-    /**
-     * @var string
-     */
-    protected $testHostName;
-
-    /**
-     * @var string
-     */
-    protected $testSitePath;
-
     protected $resetSingletonInstances = true;
 
-    /**
-     * Set up
-     */
-    protected function setUp()
+    public function setUp()
     {
-        parent::setUp();
         $GLOBALS['TSFE'] = new \stdClass();
-        $this->testHostName = 'hostname.tld';
-        $this->testSitePath = '/';
-        $this->accessibleFixture = $this->getAccessibleMock(\TYPO3\CMS\Felogin\Controller\FrontendLoginController::class, ['dummy']);
-        $this->accessibleFixture->cObj = $this->createMock(\TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer::class);
-        $this->accessibleFixture->_set('frontendController', $this->createMock(\TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController::class));
-        $this->accessibleFixture->setLogger(new NullLogger());
-
-        $site = new Site('dummy', 1, ['base' => 'http://sub.domainhostname.tld/path/']);
-        $mockedSiteFinder = $this->getAccessibleMock(SiteFinder::class, ['getSiteByPageId'], [], '', false, false);
-        $mockedSiteFinder->method('getSiteByPageId')->willReturn($site);
-        $this->accessibleFixture->_set('siteFinder', $mockedSiteFinder);
-
-        $this->setUpFakeSitePathAndHost();
-    }
-
-    /**
-     * Set up a fake site path and host
-     */
-    protected function setUpFakeSitePathAndHost()
-    {
-        $_SERVER['ORIG_PATH_INFO'] = $_SERVER['PATH_INFO'] = $_SERVER['ORIG_SCRIPT_NAME'] = $_SERVER['SCRIPT_NAME'] = $this->testSitePath . TYPO3_mainDir;
-        $_SERVER['HTTP_HOST'] = $this->testHostName;
-    }
-
-    /**
-     * Data provider for validateRedirectUrlClearsUrl
-     *
-     * @return array
-     */
-    public function validateRedirectUrlClearsUrlDataProvider()
-    {
-        return [
-            'absolute URL, hostname not in site, trailing slash' => ['http://badhost.tld/'],
-            'absolute URL, hostname not in site, no trailing slash' => ['http://badhost.tld'],
-            'absolute URL, subdomain in site, but main domain not, trailing slash' => ['http://domainhostname.tld.badhost.tld/'],
-            'absolute URL, subdomain in site, but main domain not, no trailing slash' => ['http://domainhostname.tld.badhost.tld'],
-            'non http absolute URL 1' => ['its://domainhostname.tld/itunes/'],
-            'non http absolute URL 2' => ['ftp://domainhostname.tld/download/'],
-            'XSS attempt 1' => ['javascript:alert(123)'],
-            'XSS attempt 2' => ['" onmouseover="alert(123)"'],
-            'invalid URL, HTML break out attempt' => ['" >blabuubb'],
-            'invalid URL, UNC path' => ['\\\\foo\\bar\\'],
-            'invalid URL, backslashes in path' => ['http://domainhostname.tld\\bla\\blupp'],
-            'invalid URL, linefeed in path' => ['http://domainhostname.tld/bla/blupp' . LF],
-            'invalid URL, only one slash after scheme' => ['http:/domainhostname.tld/bla/blupp'],
-            'invalid URL, illegal chars' => ['http://(<>domainhostname).tld/bla/blupp'],
-        ];
-    }
-
-    /**
-     * @test
-     * @dataProvider validateRedirectUrlClearsUrlDataProvider
-     * @param string $url Invalid Url
-     */
-    public function validateRedirectUrlClearsUrl($url)
-    {
-        Environment::initialize(
-            Environment::getContext(),
-            true,
-            false,
-            Environment::getProjectPath(),
-            Environment::getPublicPath(),
-            Environment::getVarPath(),
-            Environment::getConfigPath(),
-            Environment::getBackendPath() . '/index.php',
-            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
-        );
-        $this->assertEquals('', $this->accessibleFixture->_call('validateRedirectUrl', $url));
-    }
-
-    /**
-     * Data provider for validateRedirectUrlKeepsCleanUrl
-     *
-     * @return array
-     */
-    public function validateRedirectUrlKeepsCleanUrlDataProvider()
-    {
-        return [
-            'sane absolute URL' => ['http://sub.domainhostname.tld/path/'],
-            'sane absolute URL with script' => ['http://sub.domainhostname.tld/path/index.php?id=1'],
-            'sane absolute URL with realurl' => ['http://sub.domainhostname.tld/path/foo/bar/foo.html'],
-            'sane absolute URL with homedir' => ['http://sub.domainhostname.tld/path/~user/'],
-            'sane absolute URL with some strange chars encoded' => ['http://sub.domainhostname.tld/path/~user/a%cc%88o%cc%88%c3%9fa%cc%82/foo.html'],
-            'relative URL, no leading slash 1' => ['index.php?id=1'],
-            'relative URL, no leading slash 2' => ['foo/bar/index.php?id=2'],
-            'relative URL, leading slash, no realurl' => ['/index.php?id=1'],
-            'relative URL, leading slash, realurl' => ['/de/service/imprint.html'],
-        ];
-    }
-
-    /**
-     * @test
-     * @dataProvider validateRedirectUrlKeepsCleanUrlDataProvider
-     * @param string $url Clean URL to test
-     */
-    public function validateRedirectUrlKeepsCleanUrl($url)
-    {
-        Environment::initialize(
-            Environment::getContext(),
-            true,
-            false,
-            Environment::getProjectPath(),
-            Environment::getPublicPath(),
-            Environment::getVarPath(),
-            Environment::getConfigPath(),
-            Environment::getBackendPath() . '/index.php',
-            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
-        );
-        $this->assertEquals($url, $this->accessibleFixture->_call('validateRedirectUrl', $url));
-    }
-
-    /**
-     * Data provider for validateRedirectUrlClearsInvalidUrlInSubdirectory
-     *
-     * @return array
-     */
-    public function validateRedirectUrlClearsInvalidUrlInSubdirectoryDataProvider()
-    {
-        return [
-            'absolute URL, missing subdirectory' => ['http://hostname.tld/'],
-            'absolute URL, wrong subdirectory' => ['http://hostname.tld/hacker/index.php'],
-            'absolute URL, correct subdirectory, no trailing slash' => ['http://hostname.tld/subdir'],
-            'relative URL, leading slash, no path' => ['/index.php?id=1'],
-            'relative URL, leading slash, wrong path' => ['/de/sub/site.html'],
-            'relative URL, leading slash, slash only' => ['/'],
-        ];
-    }
-
-    /**
-     * @test
-     * @dataProvider validateRedirectUrlClearsInvalidUrlInSubdirectoryDataProvider
-     * @param string $url Invalid Url
-     */
-    public function validateRedirectUrlClearsInvalidUrlInSubdirectory($url)
-    {
-        $this->testSitePath = '/subdir/';
-        $this->setUpFakeSitePathAndHost();
-        $this->assertEquals('', $this->accessibleFixture->_call('validateRedirectUrl', $url));
-    }
-
-    /**
-     * Data provider for validateRedirectUrlKeepsCleanUrlInSubdirectory
-     *
-     * @return array
-     */
-    public function validateRedirectUrlKeepsCleanUrlInSubdirectoryDataProvider()
-    {
-        return [
-            'absolute URL, correct subdirectory' => ['http://hostname.tld/subdir/'],
-            'absolute URL, correct subdirectory, realurl' => ['http://hostname.tld/subdir/de/imprint.html'],
-            'absolute URL, correct subdirectory, no realurl' => ['http://hostname.tld/subdir/index.php?id=10'],
-            'absolute URL, correct subdirectory of site base' => ['http://sub.domainhostname.tld/path/'],
-            'relative URL, no leading slash, realurl' => ['de/service/imprint.html'],
-            'relative URL, no leading slash, no realurl' => ['index.php?id=1'],
-            'relative nested URL, no leading slash, no realurl' => ['foo/bar/index.php?id=2']
-        ];
-    }
-
-    /**
-     * @test
-     * @dataProvider validateRedirectUrlKeepsCleanUrlInSubdirectoryDataProvider
-     * @param string $url Invalid Url
-     */
-    public function validateRedirectUrlKeepsCleanUrlInSubdirectory($url)
-    {
-        Environment::initialize(
-            Environment::getContext(),
-            true,
-            false,
-            Environment::getProjectPath(),
-            Environment::getPublicPath(),
-            Environment::getVarPath(),
-            Environment::getConfigPath(),
-            Environment::getBackendPath() . '/index.php',
-            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
-        );
-        $this->testSitePath = '/subdir/';
-        $this->setUpFakeSitePathAndHost();
-        $this->assertEquals($url, $this->accessibleFixture->_call('validateRedirectUrl', $url));
+        parent::setUp();
     }
 
     /*************************
-     * Test concerning getPreverveGetVars
+     * Test concerning getPreserveGetVars
      *************************/
 
     /**
@@ -378,116 +173,10 @@ class FrontendLoginControllerTest extends UnitTestCase
     public function getPreserveGetVarsReturnsCorrectResult(array $getArray, $preserveVars, $expected)
     {
         $_GET = $getArray;
-        $this->accessibleFixture->conf['preserveGETvars'] = $preserveVars;
-        $this->assertSame($expected, $this->accessibleFixture->_call('getPreserveGetVars'));
-    }
-
-    /**************************************************
-     * Tests concerning isInLocalDomain
-     **************************************************/
-
-    /**
-     * Dataprovider for isInCurrentDomainIgnoresScheme
-     *
-     * @return array
-     */
-    public function isInCurrentDomainIgnoresSchemeDataProvider()
-    {
-        return [
-            'url https, current host http' => [
-                'example.com', // HTTP_HOST
-                '0', // HTTPS
-                'https://example.com/foo.html' // URL
-            ],
-            'url http, current host https' => [
-                'example.com',
-                '1',
-                'http://example.com/foo.html'
-            ],
-            'url https, current host https' => [
-                'example.com',
-                '1',
-                'https://example.com/foo.html'
-            ],
-            'url http, current host http' => [
-                'example.com',
-                '0',
-                'http://example.com/foo.html'
-            ]
-        ];
-    }
-
-    /**
-     * @test
-     * @dataProvider isInCurrentDomainIgnoresSchemeDataProvider
-     * @param string $host $_SERVER['HTTP_HOST']
-     * @param string $https $_SERVER['HTTPS']
-     * @param string $url The url to test
-     */
-    public function isInCurrentDomainIgnoresScheme($host, $https, $url)
-    {
-        Environment::initialize(
-            Environment::getContext(),
-            true,
-            false,
-            Environment::getProjectPath(),
-            Environment::getPublicPath(),
-            Environment::getVarPath(),
-            Environment::getConfigPath(),
-            Environment::getBackendPath() . '/index.php',
-            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
-        );
-        $_SERVER['HTTP_HOST'] = $host;
-        $_SERVER['HTTPS'] = $https;
-        $this->assertTrue($this->accessibleFixture->_call('isInCurrentDomain', $url));
-    }
-
-    /**
-     * @return array
-     */
-    public function isInCurrentDomainReturnsFalseIfDomainsAreDifferentDataProvider()
-    {
-        return [
-            'simple difference' => [
-                'example.com', // HTTP_HOST
-                'http://typo3.org/foo.html' // URL
-            ],
-            'subdomain different' => [
-                'example.com',
-                'http://foo.example.com/bar.html'
-            ]
-        ];
-    }
-
-    /**
-     * @test
-     * @dataProvider isInCurrentDomainReturnsFalseIfDomainsAreDifferentDataProvider
-     * @param string $host $_SERVER['HTTP_HOST']
-     * @param string $url The url to test
-     */
-    public function isInCurrentDomainReturnsFalseIfDomainsAreDifferent($host, $url)
-    {
-        $_SERVER['HTTP_HOST'] = $host;
-        $this->assertFalse($this->accessibleFixture->_call('isInCurrentDomain', $url));
-    }
-
-    /**
-     * @test
-     */
-    public function processRedirectReferrerDomainsMatchesDomains()
-    {
-        $conf = [
-            'redirectMode' => 'refererDomains',
-            'domains' => 'example.com'
-        ];
-
-        $this->accessibleFixture->_set('conf', $conf);
-        $this->accessibleFixture->_set('logintype', LoginType::LOGIN);
-        $this->accessibleFixture->_set('referer', 'http://www.example.com/snafu');
-        /** @var TypoScriptFrontendController $tsfe */
-        $tsfe = $this->accessibleFixture->_get('frontendController');
-        $this->accessibleFixture->_set('userIsLoggedIn', true);
-        $this->assertSame(['http://www.example.com/snafu'], $this->accessibleFixture->_call('processRedirect'));
+        $subject = $this->getAccessibleMock(FrontendLoginController::class, ['dummy'], ['_',  $this->createMock(TypoScriptFrontendController::class)]);
+        $subject->cObj = $this->createMock(ContentObjectRenderer::class);
+        $subject->conf['preserveGETvars'] = $preserveVars;
+        $this->assertSame($expected, $subject->_call('getPreserveGetVars'));
     }
 
     /**
@@ -572,10 +261,27 @@ class FrontendLoginControllerTest extends UnitTestCase
         $tsfe->fe_user = new \stdClass();
         $tsfe->fe_user->user = $userRecord;
         $conf = ['userfields.' => $fieldConf];
-        $this->accessibleFixture->_set('cObj', new ContentObjectRenderer());
-        $this->accessibleFixture->_set('frontendController', $tsfe);
-        $this->accessibleFixture->_set('conf', $conf);
-        $actualResult = $this->accessibleFixture->_call('getUserFieldMarkers');
-        $this->assertEquals($expectedMarkers, $actualResult);
+        $subject = $this->getAccessibleMock(FrontendLoginController::class, ['dummy']);
+        $subject->cObj = new ContentObjectRenderer();
+        $subject->_set('frontendController', $tsfe);
+        $subject->_set('conf', $conf);
+        $this->assertEquals($expectedMarkers, $subject->_call('getUserFieldMarkers'));
+    }
+
+    /**
+     * @test
+     */
+    public function processRedirectReferrerDomainsMatchesDomains()
+    {
+        $conf = [
+            'redirectMode' => 'refererDomains',
+            'domains' => 'example.com'
+        ];
+        $subject = $this->getAccessibleMock(FrontendLoginController::class, ['dummy']);
+        $subject->_set('conf', $conf);
+        $subject->_set('logintype', LoginType::LOGIN);
+        $subject->_set('referer', 'http://www.example.com/snafu');
+        $subject->_set('userIsLoggedIn', true);
+        $this->assertSame(['http://www.example.com/snafu'], $subject->_call('processRedirect'));
     }
 }
diff --git a/typo3/sysext/felogin/Tests/Unit/Validation/RedirectUrlValidatorTest.php b/typo3/sysext/felogin/Tests/Unit/Validation/RedirectUrlValidatorTest.php
new file mode 100644 (file)
index 0000000..e73e6e2
--- /dev/null
@@ -0,0 +1,325 @@
+<?php
+declare(strict_types = 1);
+
+namespace TYPO3\CMS\Felogin\Tests\Unit\Validation;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use Psr\Log\NullLogger;
+use TYPO3\CMS\Core\Core\Environment;
+use TYPO3\CMS\Core\Site\Entity\Site;
+use TYPO3\CMS\Core\Site\SiteFinder;
+use TYPO3\CMS\Felogin\Validation\RedirectUrlValidator;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
+
+/**
+ * Test case
+ */
+class RedirectUrlValidatorTest extends UnitTestCase
+{
+    /**
+     * @var bool Restore Environment after tests
+     */
+    protected $backupEnvironment = true;
+
+    /**
+     * @var RedirectUrlValidator|\TYPO3\TestingFramework\Core\AccessibleObjectInterface
+     */
+    protected $accessibleFixture;
+
+    /**
+     * @var string
+     */
+    protected $testHostName;
+
+    /**
+     * @var string
+     */
+    protected $testSitePath;
+
+    protected $resetSingletonInstances = true;
+
+    /**
+     * Set up
+     */
+    protected function setUp()
+    {
+        parent::setUp();
+
+        $site = new Site('dummy', 1, ['base' => 'http://sub.domainhostname.tld/path/']);
+        $mockedSiteFinder = $this->getAccessibleMock(SiteFinder::class, ['getSiteByPageId'], [], '', false, false);
+        $mockedSiteFinder->method('getSiteByPageId')->willReturn($site);
+
+        $this->testHostName = 'hostname.tld';
+        $this->testSitePath = '/';
+        $this->accessibleFixture = $this->getAccessibleMock(RedirectUrlValidator::class, ['dummy'], [$mockedSiteFinder, 1]);
+        $this->accessibleFixture->setLogger(new NullLogger());
+        $this->setUpFakeSitePathAndHost();
+    }
+
+    /**
+     * Set up a fake site path and host
+     */
+    protected function setUpFakeSitePathAndHost()
+    {
+        $_SERVER['ORIG_PATH_INFO'] = $_SERVER['PATH_INFO'] = $_SERVER['ORIG_SCRIPT_NAME'] = $_SERVER['SCRIPT_NAME'] = $this->testSitePath . TYPO3_mainDir;
+        $_SERVER['HTTP_HOST'] = $this->testHostName;
+    }
+
+    /**
+     * Data provider for validateRedirectUrlClearsUrl
+     *
+     * @return array
+     */
+    public function validateRedirectUrlClearsUrlDataProvider()
+    {
+        return [
+            'absolute URL, hostname not in site, trailing slash' => ['http://badhost.tld/'],
+            'absolute URL, hostname not in site, no trailing slash' => ['http://badhost.tld'],
+            'absolute URL, subdomain in site, but main domain not, trailing slash' => ['http://domainhostname.tld.badhost.tld/'],
+            'absolute URL, subdomain in site, but main domain not, no trailing slash' => ['http://domainhostname.tld.badhost.tld'],
+            'non http absolute URL 1' => ['its://domainhostname.tld/itunes/'],
+            'non http absolute URL 2' => ['ftp://domainhostname.tld/download/'],
+            'XSS attempt 1' => ['javascript:alert(123)'],
+            'XSS attempt 2' => ['" onmouseover="alert(123)"'],
+            'invalid URL, HTML break out attempt' => ['" >blabuubb'],
+            'invalid URL, UNC path' => ['\\\\foo\\bar\\'],
+            'invalid URL, backslashes in path' => ['http://domainhostname.tld\\bla\\blupp'],
+            'invalid URL, linefeed in path' => ['http://domainhostname.tld/bla/blupp' . LF],
+            'invalid URL, only one slash after scheme' => ['http:/domainhostname.tld/bla/blupp'],
+            'invalid URL, illegal chars' => ['http://(<>domainhostname).tld/bla/blupp'],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider validateRedirectUrlClearsUrlDataProvider
+     * @param string $url Invalid Url
+     */
+    public function validateRedirectUrlClearsUrl($url)
+    {
+        Environment::initialize(
+            Environment::getContext(),
+            true,
+            false,
+            Environment::getProjectPath(),
+            Environment::getPublicPath(),
+            Environment::getVarPath(),
+            Environment::getConfigPath(),
+            Environment::getBackendPath() . '/index.php',
+            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
+        );
+        $this->assertFalse($this->accessibleFixture->isValid($url));
+    }
+
+    /**
+     * Data provider for validateRedirectUrlKeepsCleanUrl
+     *
+     * @return array
+     */
+    public function validateRedirectUrlKeepsCleanUrlDataProvider()
+    {
+        return [
+            'sane absolute URL' => ['http://sub.domainhostname.tld/path/'],
+            'sane absolute URL with script' => ['http://sub.domainhostname.tld/path/index.php?id=1'],
+            'sane absolute URL with realurl' => ['http://sub.domainhostname.tld/path/foo/bar/foo.html'],
+            'sane absolute URL with homedir' => ['http://sub.domainhostname.tld/path/~user/'],
+            'sane absolute URL with some strange chars encoded' => ['http://sub.domainhostname.tld/path/~user/a%cc%88o%cc%88%c3%9fa%cc%82/foo.html'],
+            'relative URL, no leading slash 1' => ['index.php?id=1'],
+            'relative URL, no leading slash 2' => ['foo/bar/index.php?id=2'],
+            'relative URL, leading slash, no realurl' => ['/index.php?id=1'],
+            'relative URL, leading slash, realurl' => ['/de/service/imprint.html'],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider validateRedirectUrlKeepsCleanUrlDataProvider
+     * @param string $url Clean URL to test
+     */
+    public function validateRedirectUrlKeepsCleanUrl($url)
+    {
+        Environment::initialize(
+            Environment::getContext(),
+            true,
+            false,
+            Environment::getProjectPath(),
+            Environment::getPublicPath(),
+            Environment::getVarPath(),
+            Environment::getConfigPath(),
+            Environment::getBackendPath() . '/index.php',
+            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
+        );
+        $this->assertTrue($this->accessibleFixture->isValid($url));
+    }
+
+    /**
+     * Data provider for validateRedirectUrlClearsInvalidUrlInSubdirectory
+     *
+     * @return array
+     */
+    public function validateRedirectUrlClearsInvalidUrlInSubdirectoryDataProvider()
+    {
+        return [
+            'absolute URL, missing subdirectory' => ['http://hostname.tld/'],
+            'absolute URL, wrong subdirectory' => ['http://hostname.tld/hacker/index.php'],
+            'absolute URL, correct subdirectory, no trailing slash' => ['http://hostname.tld/subdir'],
+            'relative URL, leading slash, no path' => ['/index.php?id=1'],
+            'relative URL, leading slash, wrong path' => ['/de/sub/site.html'],
+            'relative URL, leading slash, slash only' => ['/'],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider validateRedirectUrlClearsInvalidUrlInSubdirectoryDataProvider
+     * @param string $url Invalid Url
+     */
+    public function validateRedirectUrlClearsInvalidUrlInSubdirectory($url)
+    {
+        $this->testSitePath = '/subdir/';
+        $this->setUpFakeSitePathAndHost();
+        $this->assertFalse($this->accessibleFixture->isValid($url));
+    }
+
+    /**
+     * Data provider for validateRedirectUrlKeepsCleanUrlInSubdirectory
+     *
+     * @return array
+     */
+    public function validateRedirectUrlKeepsCleanUrlInSubdirectoryDataProvider()
+    {
+        return [
+            'absolute URL, correct subdirectory' => ['http://hostname.tld/subdir/'],
+            'absolute URL, correct subdirectory, realurl' => ['http://hostname.tld/subdir/de/imprint.html'],
+            'absolute URL, correct subdirectory, no realurl' => ['http://hostname.tld/subdir/index.php?id=10'],
+            'absolute URL, correct subdirectory of site base' => ['http://sub.domainhostname.tld/path/'],
+            'relative URL, no leading slash, realurl' => ['de/service/imprint.html'],
+            'relative URL, no leading slash, no realurl' => ['index.php?id=1'],
+            'relative nested URL, no leading slash, no realurl' => ['foo/bar/index.php?id=2']
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider validateRedirectUrlKeepsCleanUrlInSubdirectoryDataProvider
+     * @param string $url Invalid Url
+     */
+    public function validateRedirectUrlKeepsCleanUrlInSubdirectory($url)
+    {
+        Environment::initialize(
+            Environment::getContext(),
+            true,
+            false,
+            Environment::getProjectPath(),
+            Environment::getPublicPath(),
+            Environment::getVarPath(),
+            Environment::getConfigPath(),
+            Environment::getBackendPath() . '/index.php',
+            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
+        );
+        $this->testSitePath = '/subdir/';
+        $this->setUpFakeSitePathAndHost();
+        $this->assertTrue($this->accessibleFixture->isValid($url));
+    }
+
+    /**************************************************
+     * Tests concerning isInLocalDomain
+     **************************************************/
+
+    /**
+     * Dataprovider for isInCurrentDomainIgnoresScheme
+     *
+     * @return array
+     */
+    public function isInCurrentDomainIgnoresSchemeDataProvider()
+    {
+        return [
+            'url https, current host http' => [
+                'example.com', // HTTP_HOST
+                '0', // HTTPS
+                'https://example.com/foo.html' // URL
+            ],
+            'url http, current host https' => [
+                'example.com',
+                '1',
+                'http://example.com/foo.html'
+            ],
+            'url https, current host https' => [
+                'example.com',
+                '1',
+                'https://example.com/foo.html'
+            ],
+            'url http, current host http' => [
+                'example.com',
+                '0',
+                'http://example.com/foo.html'
+            ]
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider isInCurrentDomainIgnoresSchemeDataProvider
+     * @param string $host $_SERVER['HTTP_HOST']
+     * @param string $https $_SERVER['HTTPS']
+     * @param string $url The url to test
+     */
+    public function isInCurrentDomainIgnoresScheme($host, $https, $url)
+    {
+        Environment::initialize(
+            Environment::getContext(),
+            true,
+            false,
+            Environment::getProjectPath(),
+            Environment::getPublicPath(),
+            Environment::getVarPath(),
+            Environment::getConfigPath(),
+            Environment::getBackendPath() . '/index.php',
+            Environment::isWindows() ? 'WINDOWS' : 'UNIX'
+        );
+        $_SERVER['HTTP_HOST'] = $host;
+        $_SERVER['HTTPS'] = $https;
+        $this->assertTrue($this->accessibleFixture->_call('isInCurrentDomain', $url));
+    }
+
+    /**
+     * @return array
+     */
+    public function isInCurrentDomainReturnsFalseIfDomainsAreDifferentDataProvider()
+    {
+        return [
+            'simple difference' => [
+                'example.com', // HTTP_HOST
+                'http://typo3.org/foo.html' // URL
+            ],
+            'subdomain different' => [
+                'example.com',
+                'http://foo.example.com/bar.html'
+            ]
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider isInCurrentDomainReturnsFalseIfDomainsAreDifferentDataProvider
+     * @param string $host $_SERVER['HTTP_HOST']
+     * @param string $url The url to test
+     */
+    public function isInCurrentDomainReturnsFalseIfDomainsAreDifferent($host, $url)
+    {
+        $_SERVER['HTTP_HOST'] = $host;
+        $this->assertFalse($this->accessibleFixture->_call('isInCurrentDomain', $url));
+    }
+}