From 14466fe0c250af8d335e2d4d9ff85401e311c601 Mon Sep 17 00:00:00 2001 From: UndefinedOffset Date: Thu, 2 Jul 2020 10:07:56 -0300 Subject: [PATCH 1/4] FEATURE: Added new middleware to rewrite the hash links and disabled SSViewer's rewriting by default fixes #8986 --- _config/requestprocessors.yml | 1 + .../Middleware/RewriteHashLinksMiddleware.php | 51 ++++++ src/View/SSViewer.php | 2 +- .../RewriteHashLinksMiddlewareTest.php | 163 ++++++++++++++++++ 4 files changed, 216 insertions(+), 1 deletion(-) create mode 100644 src/Control/Middleware/RewriteHashLinksMiddleware.php create mode 100644 tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php diff --git a/_config/requestprocessors.yml b/_config/requestprocessors.yml index 777fba21e..fcef93cb4 100644 --- a/_config/requestprocessors.yml +++ b/_config/requestprocessors.yml @@ -14,6 +14,7 @@ SilverStripe\Core\Injector\Injector: ChangeDetectionMiddleware: '%$SilverStripe\Control\Middleware\ChangeDetectionMiddleware' HTTPCacheControleMiddleware: '%$SilverStripe\Control\Middleware\HTTPCacheControlMiddleware' CanonicalURLMiddleware: '%$SilverStripe\Control\Middleware\CanonicalURLMiddleware' + RewriteHashLinksMiddleware: '%$SilverStripe\Control\Middleware\RewriteHashLinksMiddleware' SilverStripe\Control\Middleware\AllowedHostsMiddleware: properties: AllowedHosts: '`SS_ALLOWED_HOSTS`' diff --git a/src/Control/Middleware/RewriteHashLinksMiddleware.php b/src/Control/Middleware/RewriteHashLinksMiddleware.php new file mode 100644 index 000000000..78f7641cd --- /dev/null +++ b/src/Control/Middleware/RewriteHashLinksMiddleware.php @@ -0,0 +1,51 @@ +getHeader('content-type')); + $contentType = strtolower(trim(array_shift($contentType))); + if (in_array($contentType, $this->config()->handled_mime_types)) { + $body = $response->getBody(); + + if (strpos($body, ']+href *= *)"#/i', '\\1"' . Convert::raw2att(preg_replace("/^(\\/)+/", "/", $_SERVER['REQUEST_URI'])) . '#', $body); + $response->setBody($body); + } + } + + + return $response; + } +} diff --git a/src/View/SSViewer.php b/src/View/SSViewer.php index ca2388a39..688c0ad45 100644 --- a/src/View/SSViewer.php +++ b/src/View/SSViewer.php @@ -112,7 +112,7 @@ class SSViewer implements Flushable * @config * @var bool */ - private static $rewrite_hash_links = true; + private static $rewrite_hash_links = false; /** * Overridden value of rewrite_hash_links config diff --git a/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php new file mode 100644 index 000000000..952c65bf4 --- /dev/null +++ b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php @@ -0,0 +1,163 @@ + + + + + ExternalInlineLink + InlineLink + + + '; + + + //Mock a request + $request = new HTTPRequest('GET', $_SERVER['REQUEST_URI']); + + + //Hand through the Middleware to be "processed" + $middleware = new RewriteHashLinksMiddleware(); + $result = $middleware->process($request, function (HTTPRequest $request) use ($body) { + return HTTPResponse::create($body); + })->getBody(); + + + $this->assertContains( + 'InlineLink', + $result + ); + + $this->assertContains( + 'ExternalInlineLink', + $result + ); + + $this->assertContains( + '', + $result, + 'RewriteHashLinksMiddleware should only rewrite anchor hrefs' + ); + } + + /** + * Tests rewriting of HTML content types to ensure that it's not rewriting anchors when the base tag is not present + */ + public function testRewriteHTMLWithoutBase() + { + $_SERVER['HTTP_HOST'] = 'www.mysite.com'; + $_SERVER['REQUEST_URI'] = '//file.com?foo"onclick="alert(\'xss\')""'; + + $base = Convert::raw2att('/file.com?foo"onclick="alert(\'xss\')""'); + + + $body = ' + + + + ExternalInlineLink + InlineLink + + + '; + + + //Mock a request + $request = new HTTPRequest('GET', $_SERVER['REQUEST_URI']); + + + //Hand through the Middleware to be "processed" + $middleware = new RewriteHashLinksMiddleware(); + $result = $middleware->process($request, function (HTTPRequest $request) use ($body) { + return HTTPResponse::create($body); + })->getBody(); + + + $this->assertNotContains( + 'InlineLink', + $result + ); + + $this->assertContains( + 'ExternalInlineLink', + $result + ); + + $this->assertContains( + '', + $result, + 'RewriteHashLinksMiddleware should only rewrite anchor hrefs' + ); + } + + /** + * Tests rewriting of JSON content type to ensure that it's not rewriting anchors + */ + public function testRewriteJSONWithBase() + { + $_SERVER['HTTP_HOST'] = 'www.mysite.com'; + $_SERVER['REQUEST_URI'] = '//file.com?foo"onclick="alert(\'xss\')""'; + + $base = Convert::raw2att('/file.com?foo"onclick="alert(\'xss\')""'); + + + $body = json_encode(['test' => ' + + + + ExternalInlineLink + InlineLink + + + ']); + + + //Mock a request + $request = new HTTPRequest('GET', $_SERVER['REQUEST_URI']); + + + //Hand through the Middleware to be "processed" + $middleware = new RewriteHashLinksMiddleware(); + $result = $middleware->process($request, function (HTTPRequest $request) use ($body) { + return HTTPResponse::create($body)->addHeader('content-type', 'application/json; charset=utf-8'); + })->getBody(); + + + $this->assertNotContains( + 'InlineLink<\\/a>', + $result + ); + + $this->assertContains( + 'ExternalInlineLink<\\/a>', + $result + ); + + $this->assertContains( + '<\\/use><\\/svg>', + $result, + 'RewriteHashLinksMiddleware should only rewrite anchor hrefs' + ); + } +} From 5fffeca9de90817d4378d416abaa6b423453aec8 Mon Sep 17 00:00:00 2001 From: UndefinedOffset Date: Mon, 5 Oct 2020 10:45:43 -0300 Subject: [PATCH 2/4] Switched to case insensitive checking for the base tag Added config option to disable the rewriting Added support for single or double quotes in link checking Style adjustments --- .../Middleware/RewriteHashLinksMiddleware.php | 39 +++++++++++++------ .../RewriteHashLinksMiddlewareTest.php | 11 ------ 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/Control/Middleware/RewriteHashLinksMiddleware.php b/src/Control/Middleware/RewriteHashLinksMiddleware.php index 78f7641cd..60cd41e81 100644 --- a/src/Control/Middleware/RewriteHashLinksMiddleware.php +++ b/src/Control/Middleware/RewriteHashLinksMiddleware.php @@ -2,6 +2,8 @@ namespace SilverStripe\Control\Middleware; +use SilverStripe\Control\Director; +use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\Middleware\HTTPMiddleware; use SilverStripe\Core\Convert; @@ -13,7 +15,7 @@ class RewriteHashLinksMiddleware implements HTTPMiddleware /** * Mime types to be handled by this middleware - * @var string + * @var array * @config SilverStripe\Control\Middleware\RewriteHashLinksMiddleware.handled_mime_types */ private static $handled_mime_types = [ @@ -21,6 +23,12 @@ class RewriteHashLinksMiddleware implements HTTPMiddleware 'application/xhtml+xml', ]; + /** + * Set if hash links should be rewritten + * @var bool + * @config SilverStripe\Control\Middleware\RewriteHashLinksMiddleware.rewrite_hash_links + */ + private static $rewrite_hash_links = true; /** * Rewrites hash links in html responses @@ -32,19 +40,26 @@ class RewriteHashLinksMiddleware implements HTTPMiddleware { /** @var \SilverStripe\Control\HTTPResponse $response **/ $response = $delegate($request); - - - $contentType = explode(';', $response->getHeader('content-type')); - $contentType = strtolower(trim(array_shift($contentType))); - if (in_array($contentType, $this->config()->handled_mime_types)) { - $body = $response->getBody(); - - if (strpos($body, ']+href *= *)"#/i', '\\1"' . Convert::raw2att(preg_replace("/^(\\/)+/", "/", $_SERVER['REQUEST_URI'])) . '#', $body); - $response->setBody($body); - } + + if (!$this->config()->rewrite_hash_links) { + return $response; } + $contentType = explode(';', $response->getHeader('content-type')); + $mimeType = strtolower(trim(array_shift($contentType))); + if (!in_array($mimeType, $this->config()->handled_mime_types)) { + return $response; + } + + $body = $response->getBody(); + + if (stripos($body, ']+href *= *)("|\')#/i', '\\1\\2' . $link . '#', $body); + $response->setBody($body); return $response; } diff --git a/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php index 952c65bf4..4b119e89a 100644 --- a/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php +++ b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php @@ -21,7 +21,6 @@ class RewriteHashLinksMiddlewareTest extends SapphireTest $base = Convert::raw2att('/file.com?foo"onclick="alert(\'xss\')""'); - $body = ' @@ -32,18 +31,15 @@ class RewriteHashLinksMiddlewareTest extends SapphireTest '; - //Mock a request $request = new HTTPRequest('GET', $_SERVER['REQUEST_URI']); - //Hand through the Middleware to be "processed" $middleware = new RewriteHashLinksMiddleware(); $result = $middleware->process($request, function (HTTPRequest $request) use ($body) { return HTTPResponse::create($body); })->getBody(); - $this->assertContains( 'InlineLink', $result @@ -71,7 +67,6 @@ class RewriteHashLinksMiddlewareTest extends SapphireTest $base = Convert::raw2att('/file.com?foo"onclick="alert(\'xss\')""'); - $body = ' @@ -82,18 +77,15 @@ class RewriteHashLinksMiddlewareTest extends SapphireTest '; - //Mock a request $request = new HTTPRequest('GET', $_SERVER['REQUEST_URI']); - //Hand through the Middleware to be "processed" $middleware = new RewriteHashLinksMiddleware(); $result = $middleware->process($request, function (HTTPRequest $request) use ($body) { return HTTPResponse::create($body); })->getBody(); - $this->assertNotContains( 'InlineLink', $result @@ -121,7 +113,6 @@ class RewriteHashLinksMiddlewareTest extends SapphireTest $base = Convert::raw2att('/file.com?foo"onclick="alert(\'xss\')""'); - $body = json_encode(['test' => ' @@ -136,14 +127,12 @@ class RewriteHashLinksMiddlewareTest extends SapphireTest //Mock a request $request = new HTTPRequest('GET', $_SERVER['REQUEST_URI']); - //Hand through the Middleware to be "processed" $middleware = new RewriteHashLinksMiddleware(); $result = $middleware->process($request, function (HTTPRequest $request) use ($body) { return HTTPResponse::create($body)->addHeader('content-type', 'application/json; charset=utf-8'); })->getBody(); - $this->assertNotContains( 'InlineLink<\\/a>', $result From 446469614d799013ed41fd96917080eae85c4f27 Mon Sep 17 00:00:00 2001 From: UndefinedOffset Date: Mon, 5 Oct 2020 13:39:46 -0300 Subject: [PATCH 3/4] Added backup and restore of the HTTP_HOST and REQUEST_URI $_SERVER values when testing --- .../Middleware/RewriteHashLinksMiddleware.php | 2 -- .../RewriteHashLinksMiddlewareTest.php | 34 ++++++++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/Control/Middleware/RewriteHashLinksMiddleware.php b/src/Control/Middleware/RewriteHashLinksMiddleware.php index 60cd41e81..9b802ff85 100644 --- a/src/Control/Middleware/RewriteHashLinksMiddleware.php +++ b/src/Control/Middleware/RewriteHashLinksMiddleware.php @@ -2,8 +2,6 @@ namespace SilverStripe\Control\Middleware; -use SilverStripe\Control\Director; -use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\Middleware\HTTPMiddleware; use SilverStripe\Core\Convert; diff --git a/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php index 4b119e89a..bb24cdb80 100644 --- a/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php +++ b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php @@ -2,15 +2,47 @@ namespace SilverStripe\Control\Tests\Middleware; -use SilverStripe\Control\Middleware\RewriteHashLinksMiddleware; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; +use SilverStripe\Control\Middleware\RewriteHashLinksMiddleware; use SilverStripe\Core\Convert; use SilverStripe\Dev\SapphireTest; class RewriteHashLinksMiddlewareTest extends SapphireTest { + protected $currentHost; + protected $currentURI; + + /** + * Setup the test + */ + protected function setUp() + { + parent::setUp(); + + $this->currentHost = (isset($_SERVER['HTTP_HOST']) ? $_SERVER['HTTP_HOST'] : false); + $this->currentURI = (isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : false); + } + + /** + * Clean up the test + */ + protected function tearDown() + { + if ($this->currentHost === false) { + unset($_SERVER['HTTP_HOST']); + } else { + $_SERVER['HTTP_HOST'] = $this->currentHost; + } + + if ($this->currentURI === false) { + unset($_SERVER['REQUEST_URI']); + } else { + $_SERVER['REQUEST_URI'] = $this->currentURI; + } + } + /** * Tests rewriting of HTML content types to ensure that it's properly rewriting anchors when the base tag is present */ From 43a6aa52795fb215737ac53d03c91672301adbf8 Mon Sep 17 00:00:00 2001 From: UndefinedOffset Date: Tue, 6 Oct 2020 09:32:09 -0300 Subject: [PATCH 4/4] Fixed RewriteHashLinksMiddlewareTest::tearDown() not calling parent::tearDown() --- tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php index bb24cdb80..3be6e0a58 100644 --- a/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php +++ b/tests/php/Control/Middleware/RewriteHashLinksMiddlewareTest.php @@ -30,6 +30,8 @@ class RewriteHashLinksMiddlewareTest extends SapphireTest */ protected function tearDown() { + parent::tearDown(); + if ($this->currentHost === false) { unset($_SERVER['HTTP_HOST']); } else {