From abe033be855f76fde9e8576ce36460fbb23b1e57 Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Tue, 22 Sep 2020 15:17:13 +0200 Subject: Fix invalid redirection using the path of an external domain Fixes #1554 --- .../visitor/ShaarliVisitorController.php | 7 ++++ .../SaveBookmarkTest.php | 4 +- .../admin/SessionFilterControllerTest.php | 8 ++-- .../visitor/PublicSessionFilterControllerTest.php | 6 +-- .../visitor/ShaarliVisitorControllerTest.php | 45 ++++++++++++++++++---- 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')); + } } -- cgit v1.2.3