From fbcf7dc3e7484f2d55dcb323ed6e2ba9b7dd5de9 Mon Sep 17 00:00:00 2001 From: Florian Thoma Date: Tue, 11 Oct 2022 08:19:31 +1100 Subject: [PATCH] API Normalise trailing slashes for all paths NOTE: There will be additional related PRs required for at least silverstripe/cms and silverstripe/admin. Co-authored-by: Guy Sartorelli --- src/Control/Controller.php | 54 +++++ src/Control/Director.php | 9 +- src/Control/HTTP.php | 4 +- .../Middleware/CanonicalURLMiddleware.php | 136 ++++++++++- src/Control/RequestHandler.php | 2 +- src/Dev/DebugView.php | 5 +- src/Dev/TaskRunner.php | 2 +- src/Forms/HTMLEditor/TinyMCEConfig.php | 2 +- src/View/SSViewer.php | 3 +- tests/php/Control/ControllerTest.php | 164 +++++++++++++- tests/php/Control/DirectorTest.php | 213 +++++++++--------- tests/php/Control/HTTPTest.php | 174 +++++++------- .../Middleware/CanonicalURLMiddlewareTest.php | 168 +++++++++++--- tests/php/Control/RSS/RSSFeedTest.php | 6 +- tests/php/Forms/FormFactoryTest.php | 2 +- .../TinyMCECombinedGeneratorTest.php | 2 +- tests/php/ORM/PaginatedListTest.php | 3 +- tests/php/Security/SecurityTest.php | 6 +- tests/php/Security/SecurityTokenTest.php | 4 +- 19 files changed, 703 insertions(+), 256 deletions(-) diff --git a/src/Control/Controller.php b/src/Control/Controller.php index fa3ae3f36..da5a67266 100644 --- a/src/Control/Controller.php +++ b/src/Control/Controller.php @@ -66,6 +66,11 @@ class Controller extends RequestHandler implements TemplateGlobalProvider */ protected HTTPResponse $response; + /** + * If true, a trailing slash is added to the end of URLs, e.g. from {@link Controller::join_links()} + */ + private static bool $add_trailing_slash = false; + /** * Default URL handlers. * @@ -648,6 +653,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider parse_str($suffix ?? '', $localargs); $queryargs = array_merge($queryargs, $localargs); } + // Join paths together if ((is_string($arg) && $arg) || is_numeric($arg)) { $arg = (string) $arg; if ($result && substr($result ?? '', -1) != '/' && $arg[0] != '/') { @@ -658,6 +664,8 @@ class Controller extends RequestHandler implements TemplateGlobalProvider } } + $result = static::normaliseTrailingSlash($result); + if ($queryargs) { $result .= '?' . http_build_query($queryargs ?? []); } @@ -669,6 +677,52 @@ class Controller extends RequestHandler implements TemplateGlobalProvider return $result; } + /** + * Normalises a URL according to the configuration for add_trailing_slash + */ + public static function normaliseTrailingSlash(string $url): string + { + $querystring = null; + $fragmentIdentifier = null; + + // Find fragment identifier + if (strpos($url, '#') !== false) { + list($url, $fragmentIdentifier) = explode('#', $url, 2); + } + // Find querystrings + if (strpos($url, '?') !== false) { + list($url, $querystring) = explode('?', $url, 2); + } + + // Normlise trailing slash + $shouldHaveTrailingSlash = self::config()->uninherited('add_trailing_slash'); + if ($shouldHaveTrailingSlash + && !str_ends_with($url, '/') + && !preg_match('/^(.*)\.([^\/]*)$/', Director::makeRelative($url)) + ) { + // Add trailing slash if enabled and url does not end with a file extension + $url .= '/'; + } elseif (!$shouldHaveTrailingSlash) { + // Remove trailing slash if it shouldn't be there + $url = rtrim($url, '/'); + } + + // Ensure relative root URLs are represented with a slash + if ($url === '') { + $url = '/'; + } + + // Add back fragment identifier and querystrings + if ($querystring) { + $url .= '?' . $querystring; + } + if ($fragmentIdentifier) { + $url .= "#$fragmentIdentifier"; + } + + return $url; + } + /** * @return array */ diff --git a/src/Control/Director.php b/src/Control/Director.php index cd63dc5e3..972a60f3f 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -400,13 +400,13 @@ class Director implements TemplateGlobalProvider { // Check if there is already a protocol given if (preg_match('/^http(s?):\/\//', $url ?? '')) { - return $url; + return Controller::normaliseTrailingSlash($url); } // Absolute urls without protocol are added // E.g. //google.com -> http://google.com if (strpos($url ?? '', '//') === 0) { - return self::protocol() . substr($url ?? '', 2); + return Controller::normaliseTrailingSlash(self::protocol() . substr($url ?? '', 2)); } // Determine method for mapping the parent to this relative url @@ -686,7 +686,7 @@ class Director implements TemplateGlobalProvider $url = preg_replace('#([^:])//#', '\\1/', trim($url ?? '')); // If using a real url, remove protocol / hostname / auth / port - if (preg_match('#^(?https?:)?//(?[^/]*)(?(/.*)?)$#i', $url ?? '', $matches)) { + if (preg_match('@^(?https?:)?//(?[^/?#]*)(?([/?#].*)?)$@i', $url ?? '', $matches)) { $url = $matches['url']; } @@ -878,10 +878,11 @@ class Director implements TemplateGlobalProvider */ public static function absoluteBaseURL() { - return self::absoluteURL( + $baseURL = self::absoluteURL( self::baseURL(), self::ROOT ); + return Controller::normaliseTrailingSlash($baseURL); } /** diff --git a/src/Control/HTTP.php b/src/Control/HTTP.php index 4ccf5d7b7..0fa24e060 100644 --- a/src/Control/HTTP.php +++ b/src/Control/HTTP.php @@ -172,7 +172,7 @@ class HTTP $isRelative = false; // We need absolute URLs for parse_url() if (Director::is_relative_url($uri)) { - $uri = Director::absoluteBaseURL() . $uri; + $uri = Controller::join_links(Director::absoluteBaseURL(), $uri); $isRelative = true; } @@ -212,7 +212,7 @@ class HTTP $newUri = $scheme . '://' . $user . $host . $port . $path . $params . $fragment; if ($isRelative) { - return Director::baseURL() . Director::makeRelative($newUri); + return Controller::join_links(Director::baseURL(), Director::makeRelative($newUri)); } return $newUri; diff --git a/src/Control/Middleware/CanonicalURLMiddleware.php b/src/Control/Middleware/CanonicalURLMiddleware.php index 976f33ca6..feec6e109 100644 --- a/src/Control/Middleware/CanonicalURLMiddleware.php +++ b/src/Control/Middleware/CanonicalURLMiddleware.php @@ -4,11 +4,12 @@ namespace SilverStripe\Control\Middleware; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; -use SilverStripe\Control\HTTP; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse_Exception; +use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\CoreKernel; +use SilverStripe\Core\Environment; use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; @@ -17,10 +18,31 @@ use SilverStripe\Core\Injector\Injector; * - redirect basic auth requests to HTTPS * - force WWW, redirect to the subdomain "www." * - force SSL, redirect to https + * - force the correct path (with vs without trailing slash) */ class CanonicalURLMiddleware implements HTTPMiddleware { use Injectable; + use Configurable; + + /** + * If set, the trailing slash configuration set in {@link Controller::add_trailing_slash} is enforced + * with a redirect. + */ + protected bool $enforceTrailingSlashConfig = true; + + /** + * If enforceTrailingSlashConfig is enabled, this is the list of paths that are ignored + */ + protected array $enforceTrailingSlashConfigIgnorePaths = [ + 'admin/', + 'dev/', + ]; + + /** + * If enforceTrailingSlashConfig is enabled, this is the list of user agents that are ignored + */ + protected array $enforceTrailingSlashConfigIgnoreUserAgents = []; /** * Set if we should redirect to WWW @@ -76,6 +98,39 @@ class CanonicalURLMiddleware implements HTTPMiddleware */ protected $forceSSLDomain = null; + public function setEnforceTrailingSlashConfig(bool $value): static + { + $this->enforceTrailingSlashConfig = $value; + return $this; + } + + public function getEnforceTrailingSlashConfig(): bool + { + return $this->enforceTrailingSlashConfig; + } + + public function setEnforceTrailingSlashConfigIgnorePaths(array $value): static + { + $this->enforceTrailingSlashConfigIgnorePaths = $value; + return $this; + } + + public function getEnforceTrailingSlashConfigIgnorePaths(): array + { + return $this->enforceTrailingSlashConfigIgnorePaths; + } + + public function setEnforceTrailingSlashConfigIgnoreUserAgents(array $value): static + { + $this->enforceTrailingSlashConfigIgnoreUserAgents = $value; + return $this; + } + + public function getEnforceTrailingSlashConfigIgnoreUserAgents(): array + { + return $this->enforceTrailingSlashConfigIgnoreUserAgents; + } + /** * @return array */ @@ -214,6 +269,7 @@ class CanonicalURLMiddleware implements HTTPMiddleware // Get properties of current request $host = $request->getHost(); $scheme = $request->getScheme(); + $url = strtok(Environment::getEnv('REQUEST_URI'), '?'); // Check https if ($this->requiresSSL($request)) { @@ -228,8 +284,16 @@ class CanonicalURLMiddleware implements HTTPMiddleware $host = "www.{$host}"; } + // Check trailing Slash + if ($this->requiresTrailingSlashRedirect($request, $url)) { + $url = Controller::normaliseTrailingSlash($url); + } + // No-op if no changes - if ($request->getScheme() === $scheme && $request->getHost() === $host) { + if ($request->getScheme() === $scheme + && $request->getHost() === $host + && strtok(Environment::getEnv('REQUEST_URI'), '?') === $url + ) { return null; } @@ -309,6 +373,72 @@ class CanonicalURLMiddleware implements HTTPMiddleware return false; } + /** + * Check if a redirect for trailing slash is necessary + */ + protected function requiresTrailingSlashRedirect(HTTPRequest $request, string $url) + { + // Get the URL without querystrings or fragment identifiers + if (strpos($url, '#') !== false) { + $url = explode('#', $url, 2)[0]; + } + if (strpos($url, '?') !== false) { + $url = explode('?', $url, 2)[0]; + } + + // Check if force Trailing Slash is enabled + if ($this->getEnforceTrailingSlashConfig() !== true) { + return false; + } + + $requestPath = $request->getURL(); + + // skip if requesting root + if ($requestPath === '/' || $requestPath === '') { + return false; + } + + // Skip if is AJAX request + if (Director::is_ajax()) { + return false; + } + + // Skip if request has a file extension + if (!empty($request->getExtension())) { + return false; + } + + // Skip if any configured ignore paths match + $paths = (array) $this->getEnforceTrailingSlashConfigIgnorePaths(); + if (!empty($paths)) { + foreach ($paths as $path) { + if (str_starts_with(trim($path, '/'), trim($requestPath, '/'))) { + return false; + } + } + } + + // Skip if any configured ignore user agents match + $agents = (array) $this->getEnforceTrailingSlashConfigIgnoreUserAgents(); + if (!empty($agents)) { + if (in_array( + $request->getHeader('User-Agent'), + $agents + )) { + return false; + } + } + + // Already using trailing slash correctly + $addTrailingSlash = Controller::config()->uninherited('add_trailing_slash'); + $hasTrailingSlash = str_ends_with($url, '/'); + if (($addTrailingSlash && $hasTrailingSlash) || (!$addTrailingSlash && !$hasTrailingSlash)) { + return false; + } + + return true; + } + /** * @return int */ @@ -357,7 +487,7 @@ class CanonicalURLMiddleware implements HTTPMiddleware protected function isEnabled() { // At least one redirect must be enabled - if (!$this->getForceWWW() && !$this->getForceSSL()) { + if (!$this->getForceWWW() && !$this->getForceSSL() && $this->getEnforceTrailingSlashConfig() !== true) { return false; } diff --git a/src/Control/RequestHandler.php b/src/Control/RequestHandler.php index 2052b01ba..2c47efa6d 100644 --- a/src/Control/RequestHandler.php +++ b/src/Control/RequestHandler.php @@ -561,7 +561,7 @@ class RequestHandler extends ViewableData // Check configured url_segment $url = $this->config()->get('url_segment'); if ($url) { - $link = Controller::join_links($url, $action, '/'); + $link = Controller::join_links($url, $action); // Give extensions the chance to modify by reference $this->extend('updateLink', $link, $action); diff --git a/src/Dev/DebugView.php b/src/Dev/DebugView.php index b6bd389d1..5c380a7bf 100644 --- a/src/Dev/DebugView.php +++ b/src/Dev/DebugView.php @@ -135,8 +135,9 @@ class DebugView $pathLinks = []; foreach ($parts as $part) { if ($part != '') { - $pathPart .= "$part/"; - $pathLinks[] = "$part"; + $pathPart = Controller::join_links($pathPart, $part); + $href = Controller::join_links($base, $pathPart); + $pathLinks[] = "$part"; } } return implode(' → ', $pathLinks); diff --git a/src/Dev/TaskRunner.php b/src/Dev/TaskRunner.php index c8ae71a68..203030fc4 100644 --- a/src/Dev/TaskRunner.php +++ b/src/Dev/TaskRunner.php @@ -76,7 +76,7 @@ class TaskRunner extends Controller foreach ($tasks as $task) { $list->push(ArrayData::create([ - 'TaskLink' => $baseUrl . 'dev/tasks/' . $task['segment'], + 'TaskLink' => Controller::join_links($baseUrl, 'dev/tasks/', $task['segment']), 'Title' => $task['title'], 'Description' => $task['description'], ])); diff --git a/src/Forms/HTMLEditor/TinyMCEConfig.php b/src/Forms/HTMLEditor/TinyMCEConfig.php index 80379fc50..8e85d8e74 100644 --- a/src/Forms/HTMLEditor/TinyMCEConfig.php +++ b/src/Forms/HTMLEditor/TinyMCEConfig.php @@ -651,7 +651,7 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider $settings = $this->getSettings(); // https://www.tiny.cloud/docs/tinymce/6/url-handling/#document_base_url - $settings['document_base_url'] = Director::absoluteBaseURL(); + $settings['document_base_url'] = rtrim(Director::absoluteBaseURL(), '/') . '/'; // https://www.tiny.cloud/docs/tinymce/6/apis/tinymce.root/#properties $baseResource = $this->getTinyMCEResource(); diff --git a/src/View/SSViewer.php b/src/View/SSViewer.php index 61e74affe..c74766515 100644 --- a/src/View/SSViewer.php +++ b/src/View/SSViewer.php @@ -825,7 +825,8 @@ PHP; */ public static function get_base_tag($contentGeneratedSoFar) { - $base = Director::absoluteBaseURL(); + // Base href should always have a trailing slash + $base = rtrim(Director::absoluteBaseURL(), '/') . '/'; // Is the document XHTML? if (preg_match('/]+xhtml/i', $contentGeneratedSoFar ?? '')) { diff --git a/tests/php/Control/ControllerTest.php b/tests/php/Control/ControllerTest.php index 34559386e..ee1222dda 100644 --- a/tests/php/Control/ControllerTest.php +++ b/tests/php/Control/ControllerTest.php @@ -59,7 +59,7 @@ class ControllerTest extends FunctionalTest public function testDefaultAction() { /* For a controller with a template, the default action will simple run that template. */ - $response = $this->get("TestController/"); + $response = $this->get("TestController"); $this->assertStringContainsString("This is the main template. Content is 'default content'", $response->getBody()); } @@ -94,7 +94,7 @@ class ControllerTest extends FunctionalTest public function testAllowedActions() { - $response = $this->get("UnsecuredController/"); + $response = $this->get("UnsecuredController"); $this->assertEquals( 200, $response->getStatusCode(), @@ -115,7 +115,7 @@ class ControllerTest extends FunctionalTest 'Access denied on action without $allowed_actions on defining controller, ' . 'when called without an action in the URL' ); - $response = $this->get("AccessBaseController/"); + $response = $this->get("AccessBaseController"); $this->assertEquals( 200, $response->getStatusCode(), @@ -143,7 +143,7 @@ class ControllerTest extends FunctionalTest 'Access denied on action with empty $allowed_actions on defining controller, ' . 'even when action is allowed in subclasses (allowed_actions don\'t inherit)' ); - $response = $this->get("AccessSecuredController/"); + $response = $this->get("AccessSecuredController"); $this->assertEquals( 200, $response->getStatusCode(), @@ -238,7 +238,7 @@ class ControllerTest extends FunctionalTest "Access denied to method not defined in allowed_actions on extension, " . "where method is also defined on extension" ); - $response = $this->get('IndexSecuredController/'); + $response = $this->get('IndexSecuredController'); $this->assertEquals( 403, $response->getStatusCode(), @@ -253,7 +253,7 @@ class ControllerTest extends FunctionalTest ); $this->logInAs('admin'); - $response = $this->get('IndexSecuredController/'); + $response = $this->get('IndexSecuredController'); $this->assertEquals( 200, $response->getStatusCode(), @@ -276,18 +276,41 @@ class ControllerTest extends FunctionalTest { /* Controller::join_links() will reliably join two URL-segments together so that they will be * appropriately parsed by the URL parser */ + Controller::config()->set('add_trailing_slash', false); $this->assertEquals("admin/crm/MyForm", Controller::join_links("admin/crm", "MyForm")); $this->assertEquals("admin/crm/MyForm", Controller::join_links("admin/crm/", "MyForm")); + $this->assertEquals("https://www.test.com/admin/crm/MyForm", Controller::join_links("https://www.test.com", "admin/crm/", "MyForm")); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals("admin/crm/MyForm/", Controller::join_links("admin/crm", "MyForm")); + $this->assertEquals("admin/crm/MyForm/", Controller::join_links("admin/crm/", "MyForm")); + $this->assertEquals("https://www.test.com/admin/crm/MyForm/", Controller::join_links("https://www.test.com", "admin/crm/", "MyForm")); /* It will also handle appropriate combination of querystring variables */ + Controller::config()->set('add_trailing_slash', false); $this->assertEquals("admin/crm/MyForm?flush=1", Controller::join_links("admin/crm/?flush=1", "MyForm")); $this->assertEquals("admin/crm/MyForm?flush=1", Controller::join_links("admin/crm/", "MyForm?flush=1")); $this->assertEquals( "admin/crm/MyForm?field=1&other=1", Controller::join_links("admin/crm/?field=1", "MyForm?other=1") ); + $this->assertEquals("https://www.test.com/admin/crm/MyForm?flush=1", Controller::join_links("https://www.test.com", "admin/crm/", "MyForm?flush=1")); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals("admin/crm/MyForm/?flush=1", Controller::join_links("admin/crm/?flush=1", "MyForm")); + $this->assertEquals("admin/crm/MyForm/?flush=1", Controller::join_links("admin/crm/", "MyForm?flush=1")); + $this->assertEquals( + "admin/crm/MyForm/?field=1&other=1", + Controller::join_links("admin/crm/?field=1", "MyForm?other=1") + ); + $this->assertEquals("https://www.test.com/admin/crm/MyForm/?flush=1", Controller::join_links("https://www.test.com", "admin/crm/", "MyForm?flush=1")); /* It can handle arbitrary numbers of components, and will ignore empty ones */ + Controller::config()->set('add_trailing_slash', false); + $this->assertEquals("admin/crm/MyForm", Controller::join_links("admin/", "crm", "", "MyForm/")); + $this->assertEquals( + "admin/crm/MyForm?a=1&b=2", + Controller::join_links("admin/?a=1", "crm", "", "MyForm/?b=2") + ); + Controller::config()->set('add_trailing_slash', true); $this->assertEquals("admin/crm/MyForm/", Controller::join_links("admin/", "crm", "", "MyForm/")); $this->assertEquals( "admin/crm/MyForm/?a=1&b=2", @@ -295,49 +318,170 @@ class ControllerTest extends FunctionalTest ); /* It can also be used to attach additional get variables to a link */ + Controller::config()->set('add_trailing_slash', false); $this->assertEquals("admin/crm?flush=1", Controller::join_links("admin/crm", "?flush=1")); $this->assertEquals("admin/crm?existing=1&flush=1", Controller::join_links("admin/crm?existing=1", "?flush=1")); $this->assertEquals( "admin/crm/MyForm?a=1&b=2&c=3", Controller::join_links("?a=1", "admin/crm", "?b=2", "MyForm?c=3") ); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals("admin/crm/?flush=1", Controller::join_links("admin/crm", "?flush=1")); + $this->assertEquals("admin/crm/?existing=1&flush=1", Controller::join_links("admin/crm?existing=1", "?flush=1")); + $this->assertEquals( + "admin/crm/MyForm/?a=1&b=2&c=3", + Controller::join_links("?a=1", "admin/crm", "?b=2", "MyForm?c=3") + ); // And duplicates are handled nicely + Controller::config()->set('add_trailing_slash', false); $this->assertEquals( "admin/crm?foo=2&bar=3&baz=1", Controller::join_links("admin/crm?foo=1&bar=1&baz=1", "?foo=2&bar=3") ); + $this->assertEquals( + "https://www.test.com/admin/crm?foo=2&bar=3&baz=1", + Controller::join_links("https://www.test.com", "admin/crm?foo=1&bar=1&baz=1", "?foo=2&bar=3") + ); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals( + "admin/crm/?foo=2&bar=3&baz=1", + Controller::join_links("admin/crm?foo=1&bar=1&baz=1", "?foo=2&bar=3") + ); + $this->assertEquals( + "https://www.test.com/admin/crm/?foo=2&bar=3&baz=1", + Controller::join_links("https://www.test.com", "admin/crm?foo=1&bar=1&baz=1", "?foo=2&bar=3") + ); + Controller::config()->set('add_trailing_slash', false); $this->assertEquals( 'admin/action', Controller::join_links('admin/', '/', '/action'), 'Test that multiple slashes are trimmed.' ); - $this->assertEquals('/admin/action', Controller::join_links('/admin', 'action')); + $this->assertEquals( + 'https://www.test.com/admin/action', + Controller::join_links('https://www.test.com', '/', '/admin/', '/', '/action'), + 'Test that multiple slashes are trimmed.' + ); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals( + 'admin/action/', + Controller::join_links('admin/', '/', '/action'), + 'Test that multiple slashes are trimmed.' + ); + $this->assertEquals('/admin/action/', Controller::join_links('/admin', 'action')); + $this->assertEquals( + 'https://www.test.com/admin/action/', + Controller::join_links('https://www.test.com', '/', '/admin/', '/', '/action'), + 'Test that multiple slashes are trimmed.' + ); /* One fragment identifier is handled as you would expect */ + Controller::config()->set('add_trailing_slash', false); $this->assertEquals("my-page?arg=var#subsection", Controller::join_links("my-page#subsection", "?arg=var")); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals("my-page/?arg=var#subsection", Controller::join_links("my-page#subsection", "?arg=var")); /* If there are multiple, it takes the last one */ + Controller::config()->set('add_trailing_slash', false); $this->assertEquals( "my-page?arg=var#second-section", Controller::join_links("my-page#subsection", "?arg=var", "#second-section") ); + $this->assertEquals( + "https://www.test.com/my-page?arg=var#second-section", + Controller::join_links("https://www.test.com", "my-page#subsection", "?arg=var", "#second-section") + ); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals( + "my-page/?arg=var#second-section", + Controller::join_links("my-page#subsection", "?arg=var", "#second-section") + ); + $this->assertEquals( + "https://www.test.com/my-page/?arg=var#second-section", + Controller::join_links("https://www.test.com", "my-page#subsection", "?arg=var", "#second-section") + ); /* Does type-safe checks for zero value */ + Controller::config()->set('add_trailing_slash', false); $this->assertEquals("my-page/0", Controller::join_links("my-page", 0)); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals("my-page/0/", Controller::join_links("my-page", 0)); // Test array args + Controller::config()->set('add_trailing_slash', false); $this->assertEquals( - "admin/crm/MyForm?a=1&b=2&c=3", - Controller::join_links(["?a=1", "admin/crm", "?b=2", "MyForm?c=3"]) + "https://www.test.com/admin/crm/MyForm?a=1&b=2&c=3", + Controller::join_links(["https://www.test.com", "?a=1", "admin/crm", "?b=2", "MyForm?c=3"]) ); + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals( + "https://www.test.com/admin/crm/MyForm/?a=1&b=2&c=3", + Controller::join_links(["https://www.test.com", "?a=1", "admin/crm", "?b=2", "MyForm?c=3"]) + ); + } + + public function testNormaliseTrailingSlash() + { + foreach ([true, false] as $withTrailingSlash) { + Controller::config()->set('add_trailing_slash', $withTrailingSlash); + $slash = $withTrailingSlash ? '/' : ''; + + // Correctly gives slash to a relative root path + $this->assertEquals('/', Controller::normaliseTrailingSlash('')); + $this->assertEquals('/', Controller::normaliseTrailingSlash('/')); + + // Correctly adds or removes trailing slash + $this->assertEquals("some/path{$slash}", Controller::normaliseTrailingSlash('some/path/')); + $this->assertEquals("some/path{$slash}", Controller::normaliseTrailingSlash('some/path')); + + // Retains leading slash, if there is one + $this->assertEquals("/some/path{$slash}", Controller::normaliseTrailingSlash('/some/path/')); + $this->assertEquals("/some/path{$slash}", Controller::normaliseTrailingSlash('/some/path')); + + // Effectively treats absolute URL as relative + $this->assertEquals("https://www.google.com/some/path{$slash}", Controller::normaliseTrailingSlash('https://www.google.com/some/path/')); + $this->assertEquals("//www.google.com/some/path{$slash}", Controller::normaliseTrailingSlash('//www.google.com/some/path')); + $this->assertEquals("www.google.com/some/path{$slash}", Controller::normaliseTrailingSlash('www.google.com/some/path')); + $this->assertEquals("https://www.google.com{$slash}", Controller::normaliseTrailingSlash('https://www.google.com')); + $this->assertEquals("//www.google.com{$slash}", Controller::normaliseTrailingSlash('//www.google.com/')); + + // Retains query string and anchor if present + $this->assertEquals("some/path{$slash}?key=value&key2=value2", Controller::normaliseTrailingSlash('some/path/?key=value&key2=value2')); + $this->assertEquals("some/path{$slash}#some-id", Controller::normaliseTrailingSlash('some/path/#some-id')); + $this->assertEquals("some/path{$slash}?key=value&key2=value2#some-id", Controller::normaliseTrailingSlash('some/path/?key=value&key2=value2#some-id')); + $this->assertEquals("some/path{$slash}?key=value&key2=value2", Controller::normaliseTrailingSlash('some/path?key=value&key2=value2')); + $this->assertEquals("some/path{$slash}#some-id", Controller::normaliseTrailingSlash('some/path#some-id')); + $this->assertEquals("some/path{$slash}?key=value&key2=value2#some-id", Controller::normaliseTrailingSlash('some/path?key=value&key2=value2#some-id')); + + // Don't ever add a trailing slash to the end of a URL that looks like a file + $this->assertEquals("https://www.google.com/some/file.txt", Controller::normaliseTrailingSlash('https://www.google.com/some/file.txt')); + $this->assertEquals("//www.google.com/some/file.txt", Controller::normaliseTrailingSlash('//www.google.com/some/file.txt')); + $this->assertEquals("www.google.com/some/file.txt", Controller::normaliseTrailingSlash('www.google.com/some/file.txt')); + $this->assertEquals("/some/file.txt", Controller::normaliseTrailingSlash('/some/file.txt')); + $this->assertEquals("some/file.txt", Controller::normaliseTrailingSlash('some/file.txt')); + $this->assertEquals("file.txt", Controller::normaliseTrailingSlash('file.txt')); + $this->assertEquals("some/file.txt?key=value&key2=value2#some-id", Controller::normaliseTrailingSlash('some/file.txt?key=value&key2=value2#some-id')); + // NOTE: `www.google.com` is already treated as "relative" by Director::makeRelative(), which means we can't tell that it's a host (and not a file). + $this->assertEquals("www.google.com", Controller::normaliseTrailingSlash('www.google.com')); + } } public function testLink() { $controller = new HasAction(); + + Controller::config()->set('add_trailing_slash', false); + + $this->assertEquals('HasAction', $controller->Link()); + $this->assertEquals('HasAction', $controller->Link(null)); + $this->assertEquals('HasAction', $controller->Link(false)); + $this->assertEquals('HasAction/allowed-action', $controller->Link('allowed-action')); + + Controller::config()->set('add_trailing_slash', true); + $this->assertEquals('HasAction/', $controller->Link()); $this->assertEquals('HasAction/', $controller->Link(null)); $this->assertEquals('HasAction/', $controller->Link(false)); @@ -461,7 +605,7 @@ class ControllerTest extends FunctionalTest ); // BackURL is internal link - $internalAbsoluteUrl = Director::absoluteBaseURL() . '/some-url'; + $internalAbsoluteUrl = Controller::join_links(Director::absoluteBaseURL(), '/some-url'); $link = 'TestController/redirectbacktest?BackURL=' . urlencode($internalAbsoluteUrl ?? ''); $response = $this->get($link); $this->assertEquals($internalAbsoluteUrl, $response->getHeader('Location')); diff --git a/tests/php/Control/DirectorTest.php b/tests/php/Control/DirectorTest.php index 85a0979b1..bd97877e7 100644 --- a/tests/php/Control/DirectorTest.php +++ b/tests/php/Control/DirectorTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\Control\Tests; +use SilverStripe\Control\Controller; use SilverStripe\Control\Cookie_Backend; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPRequest; @@ -92,116 +93,126 @@ class DirectorTest extends SapphireTest public function testAbsoluteURL() { - Director::config()->set('alternate_base_url', 'http://www.mysite.com:9090/mysite/'); - $_SERVER['REQUEST_URI'] = "http://www.mysite.com:9090/mysite/sub-page/"; + foreach ([true, false] as $withTrailingSlash) { + Controller::config()->set('add_trailing_slash', $withTrailingSlash); + $slash = $withTrailingSlash ? '/' : ''; - //test empty / local urls - foreach (['', './', '.'] as $url) { - $this->assertEquals("http://www.mysite.com:9090/mysite/", Director::absoluteURL($url, Director::BASE)); - $this->assertEquals("http://www.mysite.com:9090/", Director::absoluteURL($url, Director::ROOT)); - $this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/", Director::absoluteURL($url, Director::REQUEST)); + Director::config()->set('alternate_base_url', 'http://www.mysite.com:9090/mysite/'); + $_SERVER['REQUEST_URI'] = "http://www.mysite.com:9090/mysite/sub-page/"; + + //test empty / local urls + foreach (['', './', '.'] as $url) { + $this->assertEquals("http://www.mysite.com:9090/mysite{$slash}", Director::absoluteURL($url, Director::BASE)); + $this->assertEquals("http://www.mysite.com:9090{$slash}", Director::absoluteURL($url, Director::ROOT)); + $this->assertEquals("http://www.mysite.com:9090/mysite/sub-page{$slash}", Director::absoluteURL($url, Director::REQUEST)); + } + + // Test site root url + $this->assertEquals("http://www.mysite.com:9090{$slash}", Director::absoluteURL('/')); + + // Test Director::BASE + $this->assertEquals("http://www.mysite.com:9090{$slash}", Director::absoluteURL('http://www.mysite.com:9090/', Director::BASE)); + $this->assertEquals("http://www.mytest.com{$slash}", Director::absoluteURL('http://www.mytest.com', Director::BASE)); + $this->assertEquals("http://www.mysite.com:9090/test{$slash}", Director::absoluteURL("http://www.mysite.com:9090/test", Director::BASE)); + $this->assertEquals("http://www.mysite.com:9090/root{$slash}", Director::absoluteURL("/root", Director::BASE)); + $this->assertEquals("http://www.mysite.com:9090/root/url{$slash}", Director::absoluteURL("/root/url", Director::BASE)); + + // Test Director::ROOT + $this->assertEquals("http://www.mysite.com:9090{$slash}", Director::absoluteURL('http://www.mysite.com:9090/', Director::ROOT)); + $this->assertEquals("http://www.mytest.com{$slash}", Director::absoluteURL('http://www.mytest.com', Director::ROOT)); + $this->assertEquals("http://www.mysite.com:9090/test{$slash}", Director::absoluteURL("http://www.mysite.com:9090/test", Director::ROOT)); + $this->assertEquals("http://www.mysite.com:9090/root{$slash}", Director::absoluteURL("/root", Director::ROOT)); + $this->assertEquals("http://www.mysite.com:9090/root/url{$slash}", Director::absoluteURL("/root/url", Director::ROOT)); + + // Test Director::REQUEST + $this->assertEquals("http://www.mysite.com:9090{$slash}", Director::absoluteURL('http://www.mysite.com:9090/', Director::REQUEST)); + $this->assertEquals("http://www.mytest.com{$slash}", Director::absoluteURL('http://www.mytest.com', Director::REQUEST)); + $this->assertEquals("http://www.mysite.com:9090/test{$slash}", Director::absoluteURL("http://www.mysite.com:9090/test", Director::REQUEST)); + $this->assertEquals("http://www.mysite.com:9090/root{$slash}", Director::absoluteURL("/root", Director::REQUEST)); + $this->assertEquals("http://www.mysite.com:9090/root/url{$slash}", Director::absoluteURL("/root/url", Director::REQUEST)); + + // Test evaluating relative urls relative to base (default) + $this->assertEquals("http://www.mysite.com:9090/mysite/test{$slash}", Director::absoluteURL("test")); + $this->assertEquals("http://www.mysite.com:9090/mysite/test/url{$slash}", Director::absoluteURL("test/url")); + $this->assertEquals("http://www.mysite.com:9090/mysite/test{$slash}", Director::absoluteURL("test", Director::BASE)); + $this->assertEquals("http://www.mysite.com:9090/mysite/test/url{$slash}", Director::absoluteURL("test/url", Director::BASE)); + + // Test evaluating relative urls relative to root + $this->assertEquals("http://www.mysite.com:9090/test{$slash}", Director::absoluteURL("test", Director::ROOT)); + $this->assertEquals("http://www.mysite.com:9090/test/url{$slash}", Director::absoluteURL("test/url", Director::ROOT)); + + // Test relative to requested page + $this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/test{$slash}", Director::absoluteURL("test", Director::REQUEST)); + $this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/test/url{$slash}", Director::absoluteURL("test/url", Director::REQUEST)); + + // Test that javascript links are not left intact + $this->assertStringStartsNotWith('javascript', Director::absoluteURL('javascript:alert("attack")')); + $this->assertStringStartsNotWith('alert', Director::absoluteURL('javascript:alert("attack")')); + $this->assertStringStartsNotWith('javascript', Director::absoluteURL('alert("attack")')); + $this->assertStringStartsNotWith('alert', Director::absoluteURL('alert("attack")')); } - - // Test site root url - $this->assertEquals("http://www.mysite.com:9090/", Director::absoluteURL('/')); - - // Test Director::BASE - $this->assertEquals('http://www.mysite.com:9090/', Director::absoluteURL('http://www.mysite.com:9090/', Director::BASE)); - $this->assertEquals('http://www.mytest.com', Director::absoluteURL('http://www.mytest.com', Director::BASE)); - $this->assertEquals("http://www.mysite.com:9090/test", Director::absoluteURL("http://www.mysite.com:9090/test", Director::BASE)); - $this->assertEquals("http://www.mysite.com:9090/root", Director::absoluteURL("/root", Director::BASE)); - $this->assertEquals("http://www.mysite.com:9090/root/url", Director::absoluteURL("/root/url", Director::BASE)); - - // Test Director::ROOT - $this->assertEquals('http://www.mysite.com:9090/', Director::absoluteURL('http://www.mysite.com:9090/', Director::ROOT)); - $this->assertEquals('http://www.mytest.com', Director::absoluteURL('http://www.mytest.com', Director::ROOT)); - $this->assertEquals("http://www.mysite.com:9090/test", Director::absoluteURL("http://www.mysite.com:9090/test", Director::ROOT)); - $this->assertEquals("http://www.mysite.com:9090/root", Director::absoluteURL("/root", Director::ROOT)); - $this->assertEquals("http://www.mysite.com:9090/root/url", Director::absoluteURL("/root/url", Director::ROOT)); - - // Test Director::REQUEST - $this->assertEquals('http://www.mysite.com:9090/', Director::absoluteURL('http://www.mysite.com:9090/', Director::REQUEST)); - $this->assertEquals('http://www.mytest.com', Director::absoluteURL('http://www.mytest.com', Director::REQUEST)); - $this->assertEquals("http://www.mysite.com:9090/test", Director::absoluteURL("http://www.mysite.com:9090/test", Director::REQUEST)); - $this->assertEquals("http://www.mysite.com:9090/root", Director::absoluteURL("/root", Director::REQUEST)); - $this->assertEquals("http://www.mysite.com:9090/root/url", Director::absoluteURL("/root/url", Director::REQUEST)); - - // Test evaluating relative urls relative to base (default) - $this->assertEquals("http://www.mysite.com:9090/mysite/test", Director::absoluteURL("test")); - $this->assertEquals("http://www.mysite.com:9090/mysite/test/url", Director::absoluteURL("test/url")); - $this->assertEquals("http://www.mysite.com:9090/mysite/test", Director::absoluteURL("test", Director::BASE)); - $this->assertEquals("http://www.mysite.com:9090/mysite/test/url", Director::absoluteURL("test/url", Director::BASE)); - - // Test evaluating relative urls relative to root - $this->assertEquals("http://www.mysite.com:9090/test", Director::absoluteURL("test", Director::ROOT)); - $this->assertEquals("http://www.mysite.com:9090/test/url", Director::absoluteURL("test/url", Director::ROOT)); - - // Test relative to requested page - $this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/test", Director::absoluteURL("test", Director::REQUEST)); - $this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/test/url", Director::absoluteURL("test/url", Director::REQUEST)); - - // Test that javascript links are not left intact - $this->assertStringStartsNotWith('javascript', Director::absoluteURL('javascript:alert("attack")')); - $this->assertStringStartsNotWith('alert', Director::absoluteURL('javascript:alert("attack")')); - $this->assertStringStartsNotWith('javascript', Director::absoluteURL('alert("attack")')); - $this->assertStringStartsNotWith('alert', Director::absoluteURL('alert("attack")')); } public function testAlternativeBaseURL() { - // relative base URLs - you should end them in a / - Director::config()->set('alternate_base_url', '/relativebase/'); - $_SERVER['HTTP_HOST'] = 'www.somesite.com'; - $_SERVER['REQUEST_URI'] = "/relativebase/sub-page/"; + foreach ([true, false] as $withTrailingSlash) { + Controller::config()->set('add_trailing_slash', $withTrailingSlash); + $slash = $withTrailingSlash ? '/' : ''; - $this->assertEquals('/relativebase/', Director::baseURL()); - $this->assertEquals('http://www.somesite.com/relativebase/', Director::absoluteBaseURL()); - $this->assertEquals( - 'http://www.somesite.com/relativebase/subfolder/test', - Director::absoluteURL('subfolder/test') - ); + // relative base URLs - you should end them in a / + Director::config()->set('alternate_base_url', '/relativebase/'); + $_SERVER['HTTP_HOST'] = 'www.somesite.com'; + $_SERVER['REQUEST_URI'] = "/relativebase/sub-page/"; - // absolute base URLS with subdirectory - You should end them in a / - Director::config()->set('alternate_base_url', 'http://www.example.org/relativebase/'); - $_SERVER['REQUEST_URI'] = "http://www.example.org/relativebase/sub-page/"; - $this->assertEquals('/relativebase/', Director::baseURL()); // Non-absolute url - $this->assertEquals('http://www.example.org/relativebase/', Director::absoluteBaseURL()); - $this->assertEquals('http://www.example.org/relativebase/sub-page/', Director::absoluteURL('', Director::REQUEST)); - $this->assertEquals('http://www.example.org/relativebase/', Director::absoluteURL('', Director::BASE)); - $this->assertEquals('http://www.example.org/', Director::absoluteURL('', Director::ROOT)); - $this->assertEquals( - 'http://www.example.org/relativebase/sub-page/subfolder/test', - Director::absoluteURL('subfolder/test', Director::REQUEST) - ); - $this->assertEquals( - 'http://www.example.org/subfolder/test', - Director::absoluteURL('subfolder/test', Director::ROOT) - ); - $this->assertEquals( - 'http://www.example.org/relativebase/subfolder/test', - Director::absoluteURL('subfolder/test', Director::BASE) - ); + $this->assertEquals('/relativebase/', Director::baseURL()); + $this->assertEquals("http://www.somesite.com/relativebase{$slash}", Director::absoluteBaseURL()); + $this->assertEquals( + "http://www.somesite.com/relativebase/subfolder/test{$slash}", + Director::absoluteURL('subfolder/test') + ); - // absolute base URLs - you should end them in a / - Director::config()->set('alternate_base_url', 'http://www.example.org/'); - $_SERVER['REQUEST_URI'] = "http://www.example.org/sub-page/"; - $this->assertEquals('/', Director::baseURL()); // Non-absolute url - $this->assertEquals('http://www.example.org/', Director::absoluteBaseURL()); - $this->assertEquals('http://www.example.org/sub-page/', Director::absoluteURL('', Director::REQUEST)); - $this->assertEquals('http://www.example.org/', Director::absoluteURL('', Director::BASE)); - $this->assertEquals('http://www.example.org/', Director::absoluteURL('', Director::ROOT)); - $this->assertEquals( - 'http://www.example.org/sub-page/subfolder/test', - Director::absoluteURL('subfolder/test', Director::REQUEST) - ); - $this->assertEquals( - 'http://www.example.org/subfolder/test', - Director::absoluteURL('subfolder/test', Director::ROOT) - ); - $this->assertEquals( - 'http://www.example.org/subfolder/test', - Director::absoluteURL('subfolder/test', Director::BASE) - ); + // absolute base URLS with subdirectory - You should end them in a / + Director::config()->set('alternate_base_url', 'http://www.example.org/relativebase/'); + $_SERVER['REQUEST_URI'] = 'http://www.example.org/relativebase/sub-page/'; + $this->assertEquals('/relativebase/', Director::baseURL()); // Non-absolute url + $this->assertEquals("http://www.example.org/relativebase{$slash}", Director::absoluteBaseURL()); + $this->assertEquals("http://www.example.org/relativebase/sub-page{$slash}", Director::absoluteURL('', Director::REQUEST)); + $this->assertEquals("http://www.example.org/relativebase{$slash}", Director::absoluteURL('', Director::BASE)); + $this->assertEquals("http://www.example.org{$slash}", Director::absoluteURL('', Director::ROOT)); + $this->assertEquals( + "http://www.example.org/relativebase/sub-page/subfolder/test{$slash}", + Director::absoluteURL('subfolder/test', Director::REQUEST) + ); + $this->assertEquals( + "http://www.example.org/subfolder/test{$slash}", + Director::absoluteURL('subfolder/test', Director::ROOT) + ); + $this->assertEquals( + "http://www.example.org/relativebase/subfolder/test{$slash}", + Director::absoluteURL('subfolder/test', Director::BASE) + ); + + // absolute base URLs - you should end them in a / + Director::config()->set('alternate_base_url', 'http://www.example.org/'); + $_SERVER['REQUEST_URI'] = "http://www.example.org/sub-page/"; + $this->assertEquals('/', Director::baseURL()); // Non-absolute url + $this->assertEquals("http://www.example.org{$slash}", Director::absoluteBaseURL()); + $this->assertEquals("http://www.example.org/sub-page{$slash}", Director::absoluteURL('', Director::REQUEST)); + $this->assertEquals("http://www.example.org{$slash}", Director::absoluteURL('', Director::BASE)); + $this->assertEquals("http://www.example.org{$slash}", Director::absoluteURL('', Director::ROOT)); + $this->assertEquals( + "http://www.example.org/sub-page/subfolder/test{$slash}", + Director::absoluteURL('subfolder/test', Director::REQUEST) + ); + $this->assertEquals( + "http://www.example.org/subfolder/test{$slash}", + Director::absoluteURL('subfolder/test', Director::ROOT) + ); + $this->assertEquals( + "http://www.example.org/subfolder/test{$slash}", + Director::absoluteURL('subfolder/test', Director::BASE) + ); + } } /** diff --git a/tests/php/Control/HTTPTest.php b/tests/php/Control/HTTPTest.php index 5f748771c..b81281555 100644 --- a/tests/php/Control/HTTPTest.php +++ b/tests/php/Control/HTTPTest.php @@ -244,36 +244,41 @@ class HTTPTest extends FunctionalTest */ public function testAbsoluteURLsCSS() { - $this->withBaseURL( - 'http://www.silverstripe.org/', - function () { + foreach ([true, false] as $withTrailingSlash) { + // The results should be the same with both settings, since files should never have trailing slashes. + Controller::config()->set('add_trailing_slash', $withTrailingSlash); - // background-image - // Note that using /./ in urls is absolutely acceptable - $this->assertEquals( - '
' . 'Content
', - HTTP::absoluteURLs('
Content
') - ); + $this->withBaseURL( + 'http://www.silverstripe.org/', + function () { - // background - $this->assertEquals( - '
Content
', - HTTP::absoluteURLs('
Content
') - ); + // background-image + // Note that using /./ in urls is absolutely acceptable + $this->assertEquals( + '
' . 'Content
', + HTTP::absoluteURLs('
Content
') + ); - // list-style-image - $this->assertEquals( - '
Content
', - HTTP::absoluteURLs('
Content
') - ); + // background + $this->assertEquals( + '
Content
', + HTTP::absoluteURLs('
Content
') + ); - // list-style - $this->assertEquals( - '
Content
', - HTTP::absoluteURLs('
Content
') - ); - } - ); + // list-style-image + $this->assertEquals( + '
Content
', + HTTP::absoluteURLs('
Content
') + ); + + // list-style + $this->assertEquals( + '
Content
', + HTTP::absoluteURLs('
Content
') + ); + } + ); + } } /** @@ -281,70 +286,75 @@ class HTTPTest extends FunctionalTest */ public function testAbsoluteURLsAttributes() { - $this->withBaseURL( - 'http://www.silverstripe.org/', - function () { - //empty links - $this->assertEquals( - 'test', - HTTP::absoluteURLs('test') - ); + foreach ([true, false] as $withTrailingSlash) { + Controller::config()->set('add_trailing_slash', $withTrailingSlash); + $slash = $withTrailingSlash ? '/' : ''; - $this->assertEquals( - 'test', - HTTP::absoluteURLs('test') - ); + $this->withBaseURL( + 'http://www.silverstripe.org/', + function () use ($slash) { + //empty links + $this->assertEquals( + "test", + HTTP::absoluteURLs('test') + ); - //relative - $this->assertEquals( - 'test', - HTTP::absoluteURLs('test') - ); - $this->assertEquals( - 'test', - HTTP::absoluteURLs('test') - ); + $this->assertEquals( + "test", + HTTP::absoluteURLs('test') + ); - // links - $this->assertEquals( - 'SS Blog', - HTTP::absoluteURLs('SS Blog') - ); + //relative + $this->assertEquals( + "test", + HTTP::absoluteURLs('test') + ); + $this->assertEquals( + "test", + HTTP::absoluteURLs('test') + ); - // background - // Note that using /./ in urls is absolutely acceptable - $this->assertEquals( - '
' . 'SS Blog
', - HTTP::absoluteURLs('
SS Blog
') - ); + // links + $this->assertEquals( + "SS Blog", + HTTP::absoluteURLs('SS Blog') + ); - //check dot segments - // Assumption: dots are not removed - //if they were, the url should be: http://www.silverstripe.org/abc - $this->assertEquals( - 'Test', - HTTP::absoluteURLs('Test') - ); + // background + // Note that using /./ in urls is absolutely acceptable + $this->assertEquals( + '
' . 'SS Blog
', + HTTP::absoluteURLs('
SS Blog
') + ); - // image - $this->assertEquals( - '', - HTTP::absoluteURLs('') - ); + //check dot segments + // Assumption: dots are not removed + //if they were, the url should be: http://www.silverstripe.org/abc + $this->assertEquals( + "Test", + HTTP::absoluteURLs('Test') + ); - // link - $this->assertEquals( - '', - HTTP::absoluteURLs('') - ); + // image + $this->assertEquals( + '', + HTTP::absoluteURLs('') + ); - // Test special characters are retained - $this->assertEquals( - 'password reset link', - HTTP::absoluteURLs('password reset link') - ); - } - ); + // link + $this->assertEquals( + '', + HTTP::absoluteURLs('') + ); + + // Test special characters are retained + $this->assertEquals( + "password reset link", + HTTP::absoluteURLs('password reset link') + ); + } + ); + } } /** diff --git a/tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php b/tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php index f605b0e07..0798ca594 100644 --- a/tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php +++ b/tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php @@ -2,44 +2,28 @@ namespace SilverStripe\Control\Tests\Middleware; +use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\Middleware\CanonicalURLMiddleware; +use SilverStripe\Core\Environment; use SilverStripe\Dev\SapphireTest; class CanonicalURLMiddlewareTest extends SapphireTest { - /** - * Stub middleware class, partially mocked - * - * @var CanonicalURLMiddleware - */ - protected $middleware; - - protected function setUp(): void - { - parent::setUp(); - - /** @var CanonicalURLMiddleware $middleware */ - $this->middleware = $this->getMockBuilder(CanonicalURLMiddleware::class) - ->setMethods(['getRedirect']) - ->getMock(); - - $this->middleware->expects($this->any())->method('getRedirect')->willReturn(false); - - $this->middleware->setForceBasicAuthToSSL(true); - } public function testHttpsIsForcedForBasicAuth() { - $this->middleware->expects($this->once())->method('getRedirect'); + $middleware = $this->getMockedMiddleware(); + $middleware->expects($this->once())->method('getRedirect'); $request = new HTTPRequest('GET', '/'); + $request->addHeader('host', 'www.example.com'); $mockResponse = (new HTTPResponse) ->addHeader('WWW-Authenticate', 'basic') ->setStatusCode(401); - $result = $this->middleware->process($request, function () use ($mockResponse) { + $result = $middleware->process($request, function () use ($mockResponse) { return $mockResponse; }); @@ -50,15 +34,17 @@ class CanonicalURLMiddlewareTest extends SapphireTest public function testMiddlewareDelegateIsReturnedWhenBasicAuthRedirectIsDisabled() { - $this->middleware->expects($this->once())->method('getRedirect'); - $this->middleware->setForceBasicAuthToSSL(false); + $middleware = $this->getMockedMiddleware(); + $middleware->expects($this->once())->method('getRedirect'); + $middleware->setForceBasicAuthToSSL(false); $request = new HTTPRequest('GET', '/'); + $request->addHeader('host', 'www.example.com'); $mockResponse = (new HTTPResponse) ->addHeader('WWW-Authenticate', 'basic') ->setStatusCode(401); - $result = $this->middleware->process($request, function () use ($mockResponse) { + $result = $middleware->process($request, function () use ($mockResponse) { return $mockResponse; }); $this->assertSame($mockResponse, $result, 'Response returned verbatim with auto redirect disabled'); @@ -66,12 +52,14 @@ class CanonicalURLMiddlewareTest extends SapphireTest public function testMiddlewareDelegateIsReturnedWhenNoBasicAuthIsPresent() { - $this->middleware->expects($this->once())->method('getRedirect'); + $middleware = $this->getMockedMiddleware(); + $middleware->expects($this->once())->method('getRedirect'); $request = new HTTPRequest('GET', '/'); + $request->addHeader('host', 'www.example.com'); $mockResponse = (new HTTPResponse)->addHeader('Foo', 'bar'); - $result = $this->middleware->process($request, function () use ($mockResponse) { + $result = $middleware->process($request, function () use ($mockResponse) { return $mockResponse; }); @@ -80,19 +68,125 @@ class CanonicalURLMiddlewareTest extends SapphireTest public function testGetForceBasicAuthToSSL() { - $this->middleware->setForceBasicAuthToSSL(null); + $middleware = $this->getMockedMiddleware(); + $middleware->setForceBasicAuthToSSL(null); - $this->middleware->setForceSSL(true); - $this->assertTrue($this->middleware->getForceBasicAuthToSSL(), 'Default falls over to forceSSL'); + $middleware->setForceSSL(true); + $this->assertTrue($middleware->getForceBasicAuthToSSL(), 'Default falls over to forceSSL'); - $this->middleware->setForceSSL(false); - $this->assertFalse($this->middleware->getForceBasicAuthToSSL(), 'Default falls over to forceSSL'); + $middleware->setForceSSL(false); + $this->assertFalse($middleware->getForceBasicAuthToSSL(), 'Default falls over to forceSSL'); - $this->middleware->setForceBasicAuthToSSL(true); - $this->assertTrue($this->middleware->getForceBasicAuthToSSL(), 'Explicitly set is returned'); + $middleware->setForceBasicAuthToSSL(true); + $this->assertTrue($middleware->getForceBasicAuthToSSL(), 'Explicitly set is returned'); - $this->middleware->setForceBasicAuthToSSL(false); - $this->middleware->setForceSSL(true); - $this->assertFalse($this->middleware->getForceBasicAuthToSSL(), 'Explicitly set is returned'); + $middleware->setForceBasicAuthToSSL(false); + $middleware->setForceSSL(true); + $this->assertFalse($middleware->getForceBasicAuthToSSL(), 'Explicitly set is returned'); + } + + public function testRedirectTrailingSlash() + { + $testScenarios = [ + [ + 'forceRedirect' => true, + 'addTrailingSlash' => true, + 'requestHasSlash' => true, + ], + [ + 'forceRedirect' => true, + 'addTrailingSlash' => true, + 'requestHasSlash' => false, + ], + [ + 'forceRedirect' => true, + 'addTrailingSlash' => false, + 'requestHasSlash' => true, + ], + [ + 'forceRedirect' => true, + 'addTrailingSlash' => false, + 'requestHasSlash' => false, + ], + [ + 'forceRedirect' => false, + 'addTrailingSlash' => true, + 'requestHasSlash' => true, + ], + [ + 'forceRedirect' => false, + 'addTrailingSlash' => true, + 'requestHasSlash' => false, + ], + [ + 'forceRedirect' => false, + 'addTrailingSlash' => false, + 'requestHasSlash' => true, + ], + [ + 'forceRedirect' => false, + 'addTrailingSlash' => false, + 'requestHasSlash' => false, + ], + ]; + foreach ($testScenarios as $scenario) { + $forceRedirect = $scenario['forceRedirect']; + $addTrailingSlash = $scenario['addTrailingSlash']; + $requestHasSlash = $scenario['requestHasSlash']; + + $middleware = $this->getMockedMiddleware(false); + + $middleware->setEnforceTrailingSlashConfig($forceRedirect); + Controller::config()->set('add_trailing_slash', $addTrailingSlash); + + $requestSlash = $requestHasSlash ? '/' : ''; + $requestURL = "/about-us{$requestSlash}"; + + Environment::setEnv('REQUEST_URI', $requestURL); + $request = new HTTPRequest('GET', $requestURL); + $request->setScheme('https'); + $request->addHeader('host', 'www.example.com'); + $mockResponse = (new HTTPResponse) + ->setStatusCode(200); + + $result = $middleware->process($request, function () use ($mockResponse) { + return $mockResponse; + }); + + $noRedirect = !$forceRedirect || ($addTrailingSlash && $requestHasSlash) || (!$addTrailingSlash && !$requestHasSlash); + if ($noRedirect) { + $this->assertNull($result->getHeader('Location'), 'No location header should be added'); + $this->assertEquals(200, $result->getStatusCode(), 'No redirection should be made'); + } else { + $this->assertEquals(301, $result->getStatusCode(), 'Responses should be redirected to include/omit trailing slash'); + if ($addTrailingSlash) { + $this->assertStringEndsWith('/', $result->getHeader('Location'), 'Trailing slash should be added'); + } else { + $this->assertStringEndsNotWith('/', $result->getHeader('Location'), 'Trailing slash should be removed'); + } + } + } + } + + private function getMockedMiddleware($mockGetRedirect = true): CanonicalURLMiddleware + { + $mockedMethods = ['isEnabled']; + if ($mockGetRedirect) { + $mockedMethods[] = 'getRedirect'; + } + + /** @var CanonicalURLMiddleware $middleware */ + $middleware = $this->getMockBuilder(CanonicalURLMiddleware::class) + ->setMethods($mockedMethods) + ->getMock(); + + $middleware->expects($this->any())->method('isEnabled')->willReturn(true); + if ($mockGetRedirect) { + $middleware->expects($this->any())->method('getRedirect')->willReturn(false); + } + + $middleware->setForceBasicAuthToSSL(true); + + return $middleware; } } diff --git a/tests/php/Control/RSS/RSSFeedTest.php b/tests/php/Control/RSS/RSSFeedTest.php index 45d772ac5..79ed01516 100644 --- a/tests/php/Control/RSS/RSSFeedTest.php +++ b/tests/php/Control/RSS/RSSFeedTest.php @@ -24,7 +24,7 @@ class RSSFeedTest extends SapphireTest $rssFeed = new RSSFeed($list, "http://www.example.com", "Test RSS Feed", "Test RSS Feed Description"); $content = $rssFeed->outputToBrowser(); - $this->assertStringContainsString('http://www.example.org/item-a/', $content); + $this->assertStringContainsString('http://www.example.org/item-a', $content); $this->assertStringContainsString('http://www.example.com/item-b.html', $content); $this->assertStringContainsString('http://www.example.com/item-c.html', $content); @@ -60,9 +60,9 @@ class RSSFeedTest extends SapphireTest public function testLinkEncoding() { $list = new ArrayList(); - $rssFeed = new RSSFeed($list, "http://www.example.com/?param1=true¶m2=true", "Test RSS Feed"); + $rssFeed = new RSSFeed($list, "http://www.example.com?param1=true¶m2=true", "Test RSS Feed"); $content = $rssFeed->outputToBrowser(); - $this->assertStringContainsString('http://www.example.com/?param1=true&param2=true', $content); + $this->assertStringContainsString('http://www.example.com?param1=true&param2=true', $content); } public function testRSSFeedWithShortcode() diff --git a/tests/php/Forms/FormFactoryTest.php b/tests/php/Forms/FormFactoryTest.php index 8cac9edc4..6ab9d9942 100644 --- a/tests/php/Forms/FormFactoryTest.php +++ b/tests/php/Forms/FormFactoryTest.php @@ -56,7 +56,7 @@ class FormFactoryTest extends SapphireTest $previewLink = $form->Fields()->fieldByName('PreviewLink'); $this->assertInstanceOf(LiteralField::class, $previewLink); $this->assertEquals( - 'Preview', + 'Preview', $previewLink->getContent() ); diff --git a/tests/php/Forms/HTMLEditor/TinyMCECombinedGeneratorTest.php b/tests/php/Forms/HTMLEditor/TinyMCECombinedGeneratorTest.php index 229b31f28..6425bf6ba 100644 --- a/tests/php/Forms/HTMLEditor/TinyMCECombinedGeneratorTest.php +++ b/tests/php/Forms/HTMLEditor/TinyMCECombinedGeneratorTest.php @@ -62,7 +62,7 @@ class TinyMCECombinedGeneratorTest extends SapphireTest $this->assertMatchesRegularExpression('#_tinymce/tinymce-testconfig-[0-9a-z]{10,10}#', $generator->generateFilename($c)); $content = $generator->generateContent($c); $this->assertStringContainsString( - "var baseURL = baseTag.length ? baseTag[0].baseURI : 'http://www.mysite.com/basedir/';\n", + "var baseURL = baseTag.length ? baseTag[0].baseURI : 'http://www.mysite.com/basedir';\n", $content ); // Main script file diff --git a/tests/php/ORM/PaginatedListTest.php b/tests/php/ORM/PaginatedListTest.php index e2ade0d48..a77a4950d 100644 --- a/tests/php/ORM/PaginatedListTest.php +++ b/tests/php/ORM/PaginatedListTest.php @@ -344,7 +344,8 @@ class PaginatedListTest extends SapphireTest public function testFirstLink() { $list = new PaginatedList(new ArrayList()); - $this->assertStringContainsString('start=0', $list->FirstLink()); + $link = $list->FirstLink(); + $this->assertStringContainsString('start=0', $link); } public function testFirstLinkContainsCurrentGetParameters() diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php index 8ab99e0cc..ec3d67233 100644 --- a/tests/php/Security/SecurityTest.php +++ b/tests/php/Security/SecurityTest.php @@ -332,7 +332,7 @@ class SecurityTest extends FunctionalTest $this->get( Config::inst()->get(Security::class, 'logout_url'), null, - ['Referer' => Director::absoluteBaseURL() . 'testpage'] + ['Referer' => Controller::join_links(Director::absoluteBaseURL(), 'testpage')] ); /* Make sure the user is still logged in */ @@ -388,11 +388,11 @@ class SecurityTest extends FunctionalTest $response = $this->doTestLoginForm( 'noexpiry@silverstripe.com', '1nitialPassword', - Director::absoluteBaseURL() . 'testpage' + Controller::join_links(Director::absoluteBaseURL(), 'testpage') ); // for some reason the redirect happens to a relative URL $this->assertMatchesRegularExpression( - '/^' . preg_quote(Director::absoluteBaseURL() ?? '', '/') . 'testpage/', + '/^' . preg_quote(Controller::join_links(Director::absoluteBaseURL(), 'testpage'), '/') . '/', $response->getHeader('Location'), "Internal absolute BackURLs work when passed through to login form" ); diff --git a/tests/php/Security/SecurityTokenTest.php b/tests/php/Security/SecurityTokenTest.php index df2cee25c..eb01bc3b9 100644 --- a/tests/php/Security/SecurityTokenTest.php +++ b/tests/php/Security/SecurityTokenTest.php @@ -123,14 +123,14 @@ class SecurityTokenTest extends SapphireTest { $t = new SecurityToken(); - $url = 'http://absolute.tld/action/'; + $url = 'http://absolute.tld/action'; $this->assertEquals( sprintf('%s?%s=%s', $url, $t->getName(), $t->getValue()), $t->addToUrl($url), 'Urls without existing GET parameters' ); - $url = 'http://absolute.tld/?getparam=1'; + $url = 'http://absolute.tld?getparam=1'; $this->assertEquals( sprintf('%s&%s=%s', $url, $t->getName(), $t->getValue()), $t->addToUrl($url),