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 <[email protected]>
silverstripe · Jan 12, 2023 · ec8040b · ec8040b
1 parent 6d45425
commit ec8040b
Show file tree

Hide file tree

Showing 16 changed files with 546 additions and 227 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -135,8 +135,9 @@ public function Breadcrumbs()
         $pathLinks = [];
         foreach ($parts as $part) {
             if ($part != '') {
-                $pathPart .= "$part/";
-                $pathLinks[] = "<a href=\"$base$pathPart\">$part</a>";
+                $pathPart = Controller::join_links($pathPart, $part);
+                $href = Controller::join_links($base, $pathPart);
+                $pathLinks[] = "<a href=\"$href\">$part</a>";
             }
         }
         return implode('&nbsp;&rarr;&nbsp;', $pathLinks);

diff --git 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
@@ -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
@@ -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('/<!DOCTYPE[^>]+xhtml/i', $contentGeneratedSoFar ?? '')) {