Merge pull request #1560 from ArthurHoaro/fix/redirect-wrong-path

Fix invalid redirection using the path of an external domain

5 files changed, 54 insertions, 16 deletions

diff --git a/application/front/controller/visitor/ShaarliVisitorController.php b/application/front/controller/visitor/ShaarliVisitorController.php
index cd27455b..55c075a2 100644
--- a/application/front/controller/visitor/ShaarliVisitorController.php
+++ b/application/front/controller/visitor/ShaarliVisitorController.php
@@ -142,6 +142,13 @@ abstract class ShaarliVisitorController
 
         if (null !== $referer) {
             $currentUrl = parse_url($referer);
+            // If the referer is not related to Shaarli instance, redirect to default
+            if (isset($currentUrl['host'])
+                && strpos(index_url($this->container->environment), $currentUrl['host']) === false
+            ) {
+                return $response->withRedirect($defaultPath);
+            }
+
             parse_str($currentUrl['query'] ?? '', $params);
             $path = $currentUrl['path'] ?? $defaultPath;
         } else {
diff --git a/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php b/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php
index dabcd60d..58eaaa9b 100644
--- a/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php
+++ b/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php
@@ -43,7 +43,7 @@ class SaveBookmarkTest extends TestCase
             'lf_description' => 'Provided description.',
             'lf_tags' => 'abc def',
             'lf_private' => '1',
-            'returnurl' => 'http://shaarli.tld/subfolder/admin/add-shaare'
+            'returnurl' => 'http://shaarli/subfolder/admin/add-shaare'
         ];
 
         $request = $this->createMock(Request::class);
@@ -124,7 +124,7 @@ class SaveBookmarkTest extends TestCase
             'lf_description' => 'Provided description.',
             'lf_tags' => 'abc def',
             'lf_private' => '1',
-            'returnurl' => 'http://shaarli.tld/subfolder/?page=2'
+            'returnurl' => 'http://shaarli/subfolder/?page=2'
         ];
 
         $request = $this->createMock(Request::class);
diff --git a/tests/front/controller/admin/SessionFilterControllerTest.php b/tests/front/controller/admin/SessionFilterControllerTest.php
index d306c6e9..c4253167 100644
--- a/tests/front/controller/admin/SessionFilterControllerTest.php
+++ b/tests/front/controller/admin/SessionFilterControllerTest.php
@@ -31,7 +31,7 @@ class SessionFilterControllerTest extends TestCase
     {
         $arg = ['visibility' => 'private'];
 
-        $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc'];
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller/?searchtag=abc';
 
         $this->container->loginManager->method('isLoggedIn')->willReturn(true);
         $this->container->sessionManager
@@ -57,7 +57,7 @@ class SessionFilterControllerTest extends TestCase
     {
         $arg = ['visibility' => 'private'];
 
-        $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc'];
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller/?searchtag=abc';
 
         $this->container->loginManager->method('isLoggedIn')->willReturn(true);
         $this->container->sessionManager
@@ -121,7 +121,7 @@ class SessionFilterControllerTest extends TestCase
     {
         $arg = ['visibility' => 'test'];
 
-        $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc'];
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller/?searchtag=abc';
 
         $this->container->loginManager->method('isLoggedIn')->willReturn(true);
         $this->container->sessionManager
@@ -151,7 +151,7 @@ class SessionFilterControllerTest extends TestCase
     {
         $arg = ['visibility' => 'test'];
 
-        $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc'];
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller/?searchtag=abc';
 
         $this->container->loginManager = $this->createMock(LoginManager::class);
         $this->container->loginManager->method('isLoggedIn')->willReturn(false);
diff --git a/tests/front/controller/visitor/PublicSessionFilterControllerTest.php b/tests/front/controller/visitor/PublicSessionFilterControllerTest.php
index 06352750..b45fbe53 100644
--- a/tests/front/controller/visitor/PublicSessionFilterControllerTest.php
+++ b/tests/front/controller/visitor/PublicSessionFilterControllerTest.php
@@ -28,7 +28,7 @@ class PublicSessionFilterControllerTest extends TestCase
      */
     public function testLinksPerPage(): void
     {
-        $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc'];
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller/?searchtag=abc';
 
         $request = $this->createMock(Request::class);
         $request->method('getParam')->with('nb')->willReturn('8');
@@ -74,7 +74,7 @@ class PublicSessionFilterControllerTest extends TestCase
      */
     public function testUntaggedOnly(): void
     {
-        $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc'];
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller/?searchtag=abc';
 
         $request = $this->createMock(Request::class);
         $response = new Response();
@@ -97,7 +97,7 @@ class PublicSessionFilterControllerTest extends TestCase
      */
     public function testUntaggedOnlyToggleOff(): void
     {
-        $this->container->environment = ['HTTP_REFERER' => 'http://shaarli/subfolder/controller/?searchtag=abc'];
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller/?searchtag=abc';
 
         $request = $this->createMock(Request::class);
         $response = new Response();
diff --git a/tests/front/controller/visitor/ShaarliVisitorControllerTest.php b/tests/front/controller/visitor/ShaarliVisitorControllerTest.php
index 316ce49c..00188c02 100644
--- a/tests/front/controller/visitor/ShaarliVisitorControllerTest.php
+++ b/tests/front/controller/visitor/ShaarliVisitorControllerTest.php
@@ -110,7 +110,7 @@ class ShaarliVisitorControllerTest extends TestCase
      */
     public function testRedirectFromRefererDefault(): void
     {
-        $this->container->environment['HTTP_REFERER'] = 'http://shaarli.tld/subfolder/controller?query=param&other=2';
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller?query=param&other=2';
 
         $response = new Response();
 
@@ -125,7 +125,7 @@ class ShaarliVisitorControllerTest extends TestCase
      */
     public function testRedirectFromRefererWithUnmatchedLoopTerm(): void
     {
-        $this->container->environment['HTTP_REFERER'] = 'http://shaarli.tld/subfolder/controller?query=param&other=2';
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller?query=param&other=2';
 
         $response = new Response();
 
@@ -140,7 +140,7 @@ class ShaarliVisitorControllerTest extends TestCase
      */
     public function testRedirectFromRefererWithMatchingLoopTermInPath(): void
     {
-        $this->container->environment['HTTP_REFERER'] = 'http://shaarli.tld/subfolder/controller?query=param&other=2';
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller?query=param&other=2';
 
         $response = new Response();
 
@@ -155,7 +155,7 @@ class ShaarliVisitorControllerTest extends TestCase
      */
     public function testRedirectFromRefererWithMatchingLoopTermInQueryParam(): void
     {
-        $this->container->environment['HTTP_REFERER'] = 'http://shaarli.tld/subfolder/controller?query=param&other=2';
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller?query=param&other=2';
 
         $response = new Response();
 
@@ -171,7 +171,7 @@ class ShaarliVisitorControllerTest extends TestCase
      */
     public function testRedirectFromRefererWithMatchingLoopTermInQueryValue(): void
     {
-        $this->container->environment['HTTP_REFERER'] = 'http://shaarli.tld/subfolder/controller?query=param&other=2';
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller?query=param&other=2';
 
         $response = new Response();
 
@@ -187,7 +187,7 @@ class ShaarliVisitorControllerTest extends TestCase
      */
     public function testRedirectFromRefererWithLoopTermInDomain(): void
     {
-        $this->container->environment['HTTP_REFERER'] = 'http://shaarli.tld/subfolder/controller?query=param&other=2';
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller?query=param&other=2';
 
         $response = new Response();
 
@@ -203,7 +203,7 @@ class ShaarliVisitorControllerTest extends TestCase
      */
     public function testRedirectFromRefererWithMatchingClearedParam(): void
     {
-        $this->container->environment['HTTP_REFERER'] = 'http://shaarli.tld/subfolder/controller?query=param&other=2';
+        $this->container->environment['HTTP_REFERER'] = 'http://shaarli/subfolder/controller?query=param&other=2';
 
         $response = new Response();
 
@@ -212,4 +212,35 @@ class ShaarliVisitorControllerTest extends TestCase
         static::assertSame(302, $result->getStatusCode());
         static::assertSame(['/subfolder/controller?other=2'], $result->getHeader('location'));
     }
+
+    /**
+     * Test redirectFromReferer() - From another domain -> we ignore the given referrer.
+     */
+    public function testRedirectExternalReferer(): void
+    {
+        $this->container->environment['HTTP_REFERER'] = 'http://other.domain.tld/controller?query=param&other=2';
+
+        $response = new Response();
+
+        $result = $this->controller->redirectFromReferer($this->request, $response, ['query'], ['query']);
+
+        static::assertSame(302, $result->getStatusCode());
+        static::assertSame(['/subfolder/'], $result->getHeader('location'));
+    }
+
+    /**
+     * Test redirectFromReferer() - From another domain -> we ignore the given referrer.
+     */
+    public function testRedirectExternalRefererExplicitDomainName(): void
+    {
+        $this->container->environment['SERVER_NAME'] = 'my.shaarli.tld';
+        $this->container->environment['HTTP_REFERER'] = 'http://your.shaarli.tld/controller?query=param&other=2';
+
+        $response = new Response();
+
+        $result = $this->controller->redirectFromReferer($this->request, $response, ['query'], ['query']);
+
+        static::assertSame(302, $result->getStatusCode());
+        static::assertSame(['/subfolder/'], $result->getHeader('location'));
+    }
 }