From c4981fe359a8186904b773cc4c9def0bb1df7e52 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 | 7 +- src/Control/HTTP.php | 2 +- .../Middleware/CanonicalURLMiddleware.php | 103 +++++++- 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 | 219 +++++++++--------- tests/php/Control/HTTPTest.php | 2 +- tests/php/Control/RSS/RSSFeedTest.php | 2 +- tests/php/Forms/FormFactoryTest.php | 2 +- tests/php/Security/SecurityTest.php | 6 +- tests/php/Security/SecurityTokenTest.php | 2 +- 16 files changed, 443 insertions(+), 134 deletions(-) diff --git a/src/Control/Controller.php b/src/Control/Controller.php index fa3ae3f3666..a69b269f6d6 100644 --- a/src/Control/Controller.php +++ b/src/Control/Controller.php @@ -89,6 +89,11 @@ public function __construct() $this->setResponse(HTTPResponse::create()); } + /** + * 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; + /** * Initialisation function that is run before any action on the controller is called. * @@ -648,6 +653,7 @@ public static function join_links($arg = null) 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 @@ public static function join_links($arg = null) } } + $result = static::normaliseTrailingSlash($result); + if ($queryargs) { $result .= '?' . http_build_query($queryargs ?? []); } @@ -669,6 +677,52 @@ public static function join_links($arg = null) 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 cd63dc5e351..fe7fd66f7b8 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -400,13 +400,13 @@ public static function absoluteURL(string $url, string $relativeParent = self::B { // 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 @@ -878,10 +878,11 @@ public static function fileExists($file) */ 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 4ccf5d7b7f3..94bed387037 100644 --- a/src/Control/HTTP.php +++ b/src/Control/HTTP.php @@ -172,7 +172,7 @@ public static function setGetVar($varname, $varvalue, $currentURL = null, $separ $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; } diff --git a/src/Control/Middleware/CanonicalURLMiddleware.php b/src/Control/Middleware/CanonicalURLMiddleware.php index 976f33ca653..34a697f6022 100644 --- a/src/Control/Middleware/CanonicalURLMiddleware.php +++ b/src/Control/Middleware/CanonicalURLMiddleware.php @@ -4,11 +4,12 @@ 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 @@ * - 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. + */ + private static bool $force_trailing_slash = true; + + /** + * If force_trailing_slash is enabled, this is the list of paths that are ignored + */ + private static array $force_trailing_slash_ignore_paths = [ + 'admin/', + 'dev/', + ]; + + /** + * If force_trailing_slash is enabled, this is the list of user agents that are ignored + */ + private static array $force_trailing_slash_ignore_user_agents = []; /** * Set if we should redirect to WWW @@ -214,6 +236,7 @@ protected function getRedirect(HTTPRequest $request) // 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 +251,16 @@ protected function getRedirect(HTTPRequest $request) $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 +340,72 @@ protected function requiresSSL(HTTPRequest $request) 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 (self::config()->uninherited('force_trailing_slash') !== 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) self::config()->uninherited('force_trailing_slash_ignore_paths'); + 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) self::config()->uninherited('force_trailing_slash_ignore_user_agents'); + 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 +454,7 @@ public function setEnabledEnvs($enabledEnvs) protected function isEnabled() { // At least one redirect must be enabled - if (!$this->getForceWWW() && !$this->getForceSSL()) { + if (!$this->getForceWWW() && !$this->getForceSSL() && self::config()->uninherited('force_trailing_slash') !== true) { return false; } diff --git a/src/Control/RequestHandler.php b/src/Control/RequestHandler.php index 2052b01bae8..2c47efa6de6 100644 --- a/src/Control/RequestHandler.php +++ b/src/Control/RequestHandler.php @@ -561,7 +561,7 @@ public function Link($action = null) // 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 b6bd389d169..5c380a7bf6f 100644 --- a/src/Dev/DebugView.php +++ b/src/Dev/DebugView.php @@ -135,8 +135,9 @@ public function Breadcrumbs() $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 c8ae71a6817..203030fc42e 100644 --- a/src/Dev/TaskRunner.php +++ b/src/Dev/TaskRunner.php @@ -76,7 +76,7 @@ public function index() 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 80379fc5046..8e85d8e7430 100644 --- a/src/Forms/HTMLEditor/TinyMCEConfig.php +++ b/src/Forms/HTMLEditor/TinyMCEConfig.php @@ -651,7 +651,7 @@ protected function getConfig() $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 61e74affe9b..c74766515d3 100644 --- a/src/View/SSViewer.php +++ b/src/View/SSViewer.php @@ -825,7 +825,8 @@ public function setTemplateFile($type, $file) */ 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 34559386e9b..ee1222dda08 100644 --- a/tests/php/Control/ControllerTest.php +++ b/tests/php/Control/ControllerTest.php @@ -59,7 +59,7 @@ protected function setUp(): void 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 @@ public function testUndefinedActions() public function testAllowedActions() { - $response = $this->get("UnsecuredController/"); + $response = $this->get("UnsecuredController"); $this->assertEquals( 200, $response->getStatusCode(), @@ -115,7 +115,7 @@ public function testAllowedActions() '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 @@ public function testAllowedActions() '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 @@ public function testAllowedActions() "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 @@ public function testAllowedActions() ); $this->logInAs('admin'); - $response = $this->get('IndexSecuredController/'); + $response = $this->get('IndexSecuredController'); $this->assertEquals( 200, $response->getStatusCode(), @@ -276,18 +276,41 @@ public function testJoinLinks() { /* 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 @@ public function testJoinLinks() ); /* 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 @@ public function testRedirectBackByBackUrl() ); // 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 85a0979b185..bd97877e73c 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 @@ public function testFileExists() 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/", 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")')); + // 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")')); + } } 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/"; - - $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') - ); - - // 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) - ); - - // 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) - ); + foreach ([true, false] as $withTrailingSlash) { + Controller::config()->set('add_trailing_slash', $withTrailingSlash); + $slash = $withTrailingSlash ? '/' : ''; + + // 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/"; + + $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 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 5f748771c7d..a1b13879efa 100644 --- a/tests/php/Control/HTTPTest.php +++ b/tests/php/Control/HTTPTest.php @@ -307,7 +307,7 @@ function () { // links $this->assertEquals( - 'SS Blog', + 'SS Blog', HTTP::absoluteURLs('SS Blog') ); diff --git a/tests/php/Control/RSS/RSSFeedTest.php b/tests/php/Control/RSS/RSSFeedTest.php index 45d772ac54a..1e86e87cc38 100644 --- a/tests/php/Control/RSS/RSSFeedTest.php +++ b/tests/php/Control/RSS/RSSFeedTest.php @@ -24,7 +24,7 @@ public function testRSSFeed() $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); diff --git a/tests/php/Forms/FormFactoryTest.php b/tests/php/Forms/FormFactoryTest.php index 8cac9edc463..6ab9d99427c 100644 --- a/tests/php/Forms/FormFactoryTest.php +++ b/tests/php/Forms/FormFactoryTest.php @@ -56,7 +56,7 @@ public function testVersionedForm() $previewLink = $form->Fields()->fieldByName('PreviewLink'); $this->assertInstanceOf(LiteralField::class, $previewLink); $this->assertEquals( - 'Preview', + 'Preview', $previewLink->getContent() ); diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php index 8ab99e0cc7b..ec3d6723331 100644 --- a/tests/php/Security/SecurityTest.php +++ b/tests/php/Security/SecurityTest.php @@ -332,7 +332,7 @@ public function testLogout() $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 @@ public function testExternalBackUrlRedirectionDisallowed() $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 df2cee25c47..cb8f8ae19da 100644 --- a/tests/php/Security/SecurityTokenTest.php +++ b/tests/php/Security/SecurityTokenTest.php @@ -123,7 +123,7 @@ public function testAddToUrl() { $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),