FIX Don't redirect admin URLs regardless of trailing slash (#10781)

src/Control/Middleware/CanonicalURLMiddleware.php

@@ -412,7 +412,10 @@ class CanonicalURLMiddleware implements HTTPMiddleware
         $paths = (array) $this->getEnforceTrailingSlashConfigIgnorePaths();
         if (!empty($paths)) {
             foreach ($paths as $path) {
-                if (str_starts_with(trim($path, '/'), trim($requestPath, '/'))) {
+                if (str_starts_with(
+                    $this->trailingSlashForComparison($requestPath),
+                    $this->trailingSlashForComparison($path)
+                )) {
                     return false;
                 }
             }
@@ -439,6 +442,15 @@ class CanonicalURLMiddleware implements HTTPMiddleware
         return true;
     }
 
+    /**
+     * Ensure a string has a trailing slash to that we can use str_starts_with and compare
+     * paths like admin/ with administration/ and get a correct result.
+     */
+    private function trailingSlashForComparison(string $path): string
+    {
+        return trim($path, '/') . '/';
+    }
+
     /**
      * @return int
      */

tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php

@@ -85,86 +85,114 @@ class CanonicalURLMiddlewareTest extends SapphireTest
         $this->assertFalse($middleware->getForceBasicAuthToSSL(), 'Explicitly set is returned');
     }
 
-    public function testRedirectTrailingSlash()
+    public function provideRedirectTrailingSlash()
     {
-        $testScenarios = [
+        $testScenarios = [];
+        foreach ([true, false] as $forceRedirect) {
+            foreach ([true, false] as $addTrailingSlash) {
+                foreach ([true, false] as $requestHasSlash) {
+                    $testScenarios[] = [
+                        $forceRedirect,
+                        $addTrailingSlash,
+                        $requestHasSlash,
+                    ];
+                }
+            }
+        }
+        return $testScenarios;
+    }
+
+    /**
+     * @dataProvider provideRedirectTrailingSlash
+     */
+    public function testRedirectTrailingSlash(bool $forceRedirect, bool $addTrailingSlash, bool $requestHasSlash)
+    {
+        Controller::config()->set('add_trailing_slash', $addTrailingSlash);
+
+        $noRedirect = !$forceRedirect || ($addTrailingSlash && $requestHasSlash) || (!$addTrailingSlash && !$requestHasSlash);
+        $middleware = $this->getMockedMiddleware(false);
+        $middleware->setEnforceTrailingSlashConfig($forceRedirect);
+
+        $requestSlash = $requestHasSlash ? '/' : '';
+        $requestURL = "/about-us{$requestSlash}";
+
+        $this->performRedirectTest($requestURL, $middleware, !$noRedirect, $addTrailingSlash);
+    }
+
+    private function performRedirectTest(string $requestURL, CanonicalURLMiddleware $middleware, bool $shouldRedirect, bool $addTrailingSlash)
+    {
+        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;
+        });
+
+        if (!$shouldRedirect) {
+            $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');
+            }
+        }
+    }
+
+    public function provideRedirectTrailingSlashIgnorePaths()
+    {
+        return [
             [
-                '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' => true,
-                'addTrailingSlash' => false,
+                'requestHasSlash' => true,
+            ],
+            [
+                'addTrailingSlash' => true,
                 'requestHasSlash' => false,
             ],
         ];
-        foreach ($testScenarios as $scenario) {
+    }
-            $forceRedirect = $scenario['forceRedirect'];
-            $addTrailingSlash = $scenario['addTrailingSlash'];
-            $requestHasSlash = $scenario['requestHasSlash'];
 
-            $middleware = $this->getMockedMiddleware(false);
+    /**
+     * @dataProvider provideRedirectTrailingSlashIgnorePaths
+     */
+    public function testRedirectTrailingSlashIgnorePaths(bool $addTrailingSlash, bool $requestHasSlash)
+    {
+        Controller::config()->set('add_trailing_slash', $addTrailingSlash);
 
-            $middleware->setEnforceTrailingSlashConfig($forceRedirect);
+        $middleware = $this->getMockedMiddleware(false);
-            Controller::config()->set('add_trailing_slash', $addTrailingSlash);
+        $middleware->setEnforceTrailingSlashConfig(true);
 
-            $requestSlash = $requestHasSlash ? '/' : '';
+        $requestSlash = $requestHasSlash ? '/' : '';
-            $requestURL = "/about-us{$requestSlash}";
+        $noRedirectPaths = [
+            "/admin{$requestSlash}",
+            "/admin/graphql{$requestSlash}",
+            "/dev/tasks/my-task{$requestSlash}",
+        ];
+        $allowRedirectPaths = [
+            "/administration{$requestSlash}",
+            "/administration/more-path{$requestSlash}",
+        ];
 
-            Environment::setEnv('REQUEST_URI', $requestURL);
+        foreach ($noRedirectPaths as $path) {
-            $request = new HTTPRequest('GET', $requestURL);
+            $this->performRedirectTest($path, $middleware, false, $addTrailingSlash);
-            $request->setScheme('https');
+        }
-            $request->addHeader('host', 'www.example.com');
+        foreach ($allowRedirectPaths as $path) {
-            $mockResponse = (new HTTPResponse)
+            $this->performRedirectTest($path, $middleware, $addTrailingSlash !== $requestHasSlash, $addTrailingSlash);
-                ->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');
-                }
-            }
         }
     }