add domains white list for target path url (hwi#1600)

* add domains white list for target path url * cr fixes * use expectException and expectExceptionMessage to avoid deperactedd warning for PHPUnit 9.
bruno-ds · Feb 12, 2020 · 5b2b3f0 · 5b2b3f0
1 parent 5f93f6e
commit 5b2b3f0
Show file tree

Hide file tree

Showing 9 changed files with 171 additions and 2 deletions.
diff --git a/Controller/RedirectToServiceController.php b/Controller/RedirectToServiceController.php
@@ -12,10 +12,12 @@
 namespace HWI\Bundle\OAuthBundle\Controller;
 
 use HWI\Bundle\OAuthBundle\Security\OAuthUtils;
+use HWI\Bundle\OAuthBundle\Util\DomainWhitelist;
 use RuntimeException;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 
 /**
  * @author Alexander <[email protected]>
@@ -27,6 +29,11 @@ final class RedirectToServiceController
      */
     private $oauthUtils;
 
+    /**
+     * @var DomainWhitelist
+     */
+    private $domainWhitelist;
+
     /**
      * @var array
      */
@@ -47,9 +54,10 @@ final class RedirectToServiceController
      */
     private $useReferer;
 
-    public function __construct(OAuthUtils $oauthUtils, array $firewallNames, ?string $targetPathParameter, bool $failedUseReferer, bool $useReferer)
+    public function __construct(OAuthUtils $oauthUtils, DomainWhitelist $domainWhitelist, array $firewallNames, ?string $targetPathParameter, bool $failedUseReferer, bool $useReferer)
     {
         $this->oauthUtils = $oauthUtils;
+        $this->domainWhitelist = $domainWhitelist;
         $this->firewallNames = $firewallNames;
         $this->targetPathParameter = $targetPathParameter;
         $this->failedUseReferer = $failedUseReferer;
@@ -92,6 +100,11 @@ private function storeReturnPath(Request $request, string $authorizationUrl): vo
             $sessionKeyFailure = '_security.'.$providerKey.'.failed_target_path';
 
             if (!empty($param) && $targetUrl = $request->get($param)) {
+
+                if (!$this->domainWhitelist->isValidTargetUrl($targetUrl)) {
+                    throw new AccessDeniedHttpException('Not allowed to redirect to '.$targetUrl);
+                }
+
                 $session->set($sessionKey, $targetUrl);
             }
 

diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php
@@ -164,6 +164,10 @@ public function getConfigTreeBuilder()
                     ->prototype('scalar')->end()
                 ->end()
                 ->scalarNode('target_path_parameter')->defaultNull()->end()
+                ->arrayNode('target_path_domains_whitelist')
+                    ->defaultValue([])
+                    ->prototype('scalar')->end()
+                ->end()
                 ->booleanNode('use_referer')->defaultFalse()->end()
                 ->booleanNode('failed_use_referer')->defaultFalse()->end()
                 ->scalarNode('failed_auth_path')->defaultValue('hwi_oauth_connect')->end()

diff --git a/DependencyInjection/HWIOAuthExtension.php b/DependencyInjection/HWIOAuthExtension.php
@@ -48,6 +48,7 @@ public function load(array $configs, ContainerBuilder $container)
         $loader->load('oauth.xml');
         $loader->load('templating.xml');
         $loader->load('twig.xml');
+        $loader->load('util.xml');
 
         $processor = new Processor();
         $config = $processor->processConfiguration(new Configuration(), $configs);
@@ -63,6 +64,9 @@ public function load(array $configs, ContainerBuilder $container)
         // set target path parameter
         $container->setParameter('hwi_oauth.target_path_parameter', $config['target_path_parameter']);
 
+        // set target path domains whitelist parameter
+        $container->setParameter('hwi_oauth.target_path_domains_whitelist', $config['target_path_domains_whitelist']);
+
         // set use referer parameter
         $container->setParameter('hwi_oauth.use_referer', $config['use_referer']);
 

diff --git a/Resources/config/controller.xml b/Resources/config/controller.xml
@@ -30,6 +30,7 @@
 
         <service id="HWI\Bundle\OAuthBundle\Controller\RedirectToServiceController" public="true">
             <argument type="service" id="hwi_oauth.security.oauth_utils" />
+            <argument type="service" id="hwi_oauth.util.domain_whitelist" />
             <argument>%hwi_oauth.firewall_names%</argument>
             <argument>%hwi_oauth.target_path_parameter%</argument>
             <argument>%hwi_oauth.failed_use_referer%</argument>

diff --git a/Resources/config/util.xml b/Resources/config/util.xml
@@ -0,0 +1,12 @@
+<?xml version="1.0" ?>
+
+<container xmlns="http://symfony.com/schema/dic/services"
+           xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+           xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+
+    <services>
+        <service id="hwi_oauth.util.domain_whitelist" class="HWI\Bundle\OAuthBundle\Util\DomainWhitelist">
+            <argument>%hwi_oauth.target_path_domains_whitelist%</argument>
+        </service>
+    </services>
+</container>
diff --git a/Resources/doc/internals/reference_configuration.md b/Resources/doc/internals/reference_configuration.md
@@ -67,6 +67,9 @@ hwi_oauth:
     # optional target_path_parameter to provide an explicit return URL
     #target_path_parameter: _destination
 
+    # optional target_path_domains_whitelist to provide a white list of domains that can be used for target_path_parameter
+    #target_path_domains_whitelist: [_destination.com]
+
     # use referer as fallback to determine default return URL
     #use_referer: true
 

diff --git a/Tests/Controller/RedirectToServiceControllerTest.php b/Tests/Controller/RedirectToServiceControllerTest.php
@@ -13,9 +13,11 @@
 
 use HWI\Bundle\OAuthBundle\Controller\RedirectToServiceController;
 use HWI\Bundle\OAuthBundle\Security\OAuthUtils;
+use HWI\Bundle\OAuthBundle\Util\DomainWhitelist;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Session\SessionInterface;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 
 class RedirectToServiceControllerTest extends TestCase
 {
@@ -24,6 +26,11 @@ class RedirectToServiceControllerTest extends TestCase
      */
     private $oAuthUtils;
 
+    /**
+     * @var \PHPUnit_Framework_MockObject_MockObject|DomainWhitelist
+     */
+    private $domainsWhiteList;
+
     /**
      * @var \PHPUnit_Framework_MockObject_MockObject|SessionInterface
      */
@@ -49,6 +56,8 @@ protected function setUp(): void
         parent::setUp();
 
         $this->oAuthUtils = $this->createMock(OAuthUtils::class);
+        $this->domainsWhiteList = $this->createMock(DomainWhitelist::class);
+
 
         $this->session = $this->createMock(SessionInterface::class);
         $this->request = Request::create('/');
@@ -80,6 +89,12 @@ public function testTargetPathParameter()
 
         $controller = $this->createController();
 
+        $this->domainsWhiteList->expects($this->once())
+            ->method('isValidTargetUrl')
+            ->with('/target/path')
+            ->willReturn(true)
+        ;
+
         $controller->redirectToServiceAction($this->request, 'facebook');
     }
 
@@ -129,8 +144,31 @@ public function testUnknownResourceOwner()
         $controller->redirectToServiceAction($this->request, 'unknown');
     }
 
+    public function testThrowAccessDeniedExceptionForNonWhitelistedTargetPath()
+    {
+        $this->request->attributes->set($this->targetPathParameter, '/malicious/target/path');
+
+        $this->session->expects($this->never())
+            ->method('set')
+            ->with('_security.default.target_path', '/malicious/target/path')
+        ;
+
+        $controller = $this->createController();
+
+        $this->domainsWhiteList->expects($this->once())
+            ->method('isValidTargetUrl')
+            ->with('/malicious/target/path')
+            ->willReturn(false)
+        ;
+
+        $this->expectException(AccessDeniedHttpException::class);
+        $this->expectExceptionMessage('Not allowed to redirect to /malicious/target/path');
+
+        $controller->redirectToServiceAction($this->request, 'facebook');
+    }
+
     private function createController(bool $failedUseReferer = false, bool $useReferer = false): RedirectToServiceController
     {
-        return new RedirectToServiceController($this->oAuthUtils, $this->firewallNames, $this->targetPathParameter, $failedUseReferer, $useReferer);
+        return new RedirectToServiceController($this->oAuthUtils, $this->domainsWhiteList, $this->firewallNames, $this->targetPathParameter, $failedUseReferer, $useReferer);
     }
 }
diff --git a/Tests/Util/DomainWhitelistTest.php b/Tests/Util/DomainWhitelistTest.php
@@ -0,0 +1,41 @@
+<?php
+
+/*
+ * This file is part of the HWIOAuthBundle package.
+ *
+ * (c) Hardware Info <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace HWI\Bundle\OAuthBundle\Tests\Util;
+
+use PHPUnit\Framework\TestCase;
+use HWI\Bundle\OAuthBundle\Util\DomainWhitelist;
+
+class DomainWhitelistTest extends TestCase
+{
+    /**
+     * @dataProvider targetUrlProvider
+     *
+     * @param string $targetUrl
+     * @param array $domainsWhitelistParameter
+     * @param bool $isValidTargetUrl
+     */
+    public function testValidateTargetUrl($targetUrl, $domainsWhitelistParameter, $isValidTargetUrl)
+    {
+        $domainsWhitelist = new DomainWhitelist($domainsWhitelistParameter);
+        $this->assertSame($isValidTargetUrl, $domainsWhitelist->isValidTargetUrl($targetUrl));
+    }
+
+    public function targetUrlProvider()
+    {
+        return [
+            ['https://example.com/redirect', ['example.com'], true],
+            ['https://example.com/redirect', ['foobar.com'], false],
+            ['blabla', ['foobar.com'], false],
+            ['https://example.com/redirect', ['foobar.com', 'example.com'], true],
+        ];
+    }
+}
diff --git a/Util/DomainWhitelist.php b/Util/DomainWhitelist.php
@@ -0,0 +1,53 @@
+<?php
+
+/*
+ * This file is part of the HWIOAuthBundle package.
+ *
+ * (c) Hardware Info <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace HWI\Bundle\OAuthBundle\Util;
+
+/**
+ * @final
+ */
+class DomainWhitelist
+{
+    /**
+     * @var array
+     */
+    private $targetPathDomainsWhiteList;
+
+    /**
+     * @param array $targetPathDomainsWhiteList
+     */
+    public function __construct(array $targetPathDomainsWhiteList)
+    {
+        $this->targetPathDomainsWhiteList = $targetPathDomainsWhiteList;
+    }
+
+    /**
+     * @param string $targetUrl
+     * @return bool
+     */
+    public function isValidTargetUrl(string $targetUrl)
+    {
+        if (0 === count($this->targetPathDomainsWhiteList)) {
+            return true;
+        }
+
+        $urlParts = parse_url($targetUrl);
+        if (!isset($urlParts['host'])) {
+            return false;
+        }
+
+        if (!in_array($urlParts['host'], $this->targetPathDomainsWhiteList, true)) {
+            return false;
+        }
+
+        return true;
+    }
+}