From 9eacf0eee6620cb9be8c73b172f40cc4a44e3e2c Mon Sep 17 00:00:00 2001
From: Simon Erkelens <simonerkelens@silverstripe.com>
Date: Sun, 9 Jul 2017 17:42:36 +1200
Subject: [PATCH] Disable the ability to use reserved URL's

Fixes https://github.com/silverstripe/silverstripe-framework/issues/7139

Move check for disallowed rules to validURLSegment

Fix test
---
 code/Model/SiteTree.php          | 26 +++++++++++++++----
 tests/php/Model/SiteTreeTest.php | 44 ++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/code/Model/SiteTree.php b/code/Model/SiteTree.php
index 49d2c3ad..66a907a1 100755
--- a/code/Model/SiteTree.php
+++ b/code/Model/SiteTree.php
@@ -1559,6 +1559,22 @@ class SiteTree extends DataObject implements PermissionProvider, i18nEntityProvi
      */
     public function validURLSegment()
     {
+        $excludes = Director::config()->get('rules');
+        $excludes = array_keys($excludes);
+        $disallowedSegments = array_map(function ($key) {
+            $route = explode('/', $key);
+            if (!empty($route) && strpos($route[0], '$') === false) {
+                return $route[0];
+            }
+            return;
+        }, $excludes);
+
+        if (!$this->ParentID && in_array($this->URLSegment, $disallowedSegments)) {
+            // Default to '-2', onBeforeWrite takes care of further possible clashes
+            return false;
+        }
+
+
         if (self::config()->nested_urls && $parent = $this->Parent()) {
             if ($controller = ModelAsController::controller_for($parent)) {
                 if ($controller instanceof Controller && $controller->hasAction($this->URLSegment)) {
@@ -1611,17 +1627,17 @@ class SiteTree extends DataObject implements PermissionProvider, i18nEntityProvi
     public function generateURLSegment($title)
     {
         $filter = URLSegmentFilter::create();
-        $t = $filter->filter($title);
+        $filteredTitle = $filter->filter($title);
 
         // Fallback to generic page name if path is empty (= no valid, convertable characters)
-        if (!$t || $t == '-' || $t == '-1') {
-            $t = "page-$this->ID";
+        if (!$filteredTitle || $filteredTitle == '-' || $filteredTitle == '-1') {
+            $filteredTitle = "page-$this->ID";
         }
 
         // Hook for extensions
-        $this->extend('updateURLSegment', $t, $title);
+        $this->extend('updateURLSegment', $filteredTitle, $title);
 
-        return $t;
+        return $filteredTitle;
     }
 
     /**
diff --git a/tests/php/Model/SiteTreeTest.php b/tests/php/Model/SiteTreeTest.php
index 1ad29818..97382e30 100644
--- a/tests/php/Model/SiteTreeTest.php
+++ b/tests/php/Model/SiteTreeTest.php
@@ -46,6 +46,15 @@ class SiteTreeTest extends SapphireTest
         SiteTreeTest_DataObject::class,
     );
 
+    public function reservedSegmentsProvider()
+    {
+        return [
+            ['Admin', 'admin-2'],
+            ['Dev', 'dev-2'],
+            ['Robots in disguise', 'robots-in-disguise']
+        ];
+    }
+
     public function testCreateDefaultpages()
     {
             $remove = SiteTree::get();
@@ -101,6 +110,33 @@ class SiteTreeTest extends SapphireTest
         }
     }
 
+    /**
+     * Check if reserved URL's are properly appended with a number at top level
+     * @dataProvider reservedSegmentsProvider
+     */
+    public function testDisallowedURLGeneration($title, $urlSegment)
+    {
+        $page = Page::create(['Title' => $title]);
+        $id = $page->write();
+        $page = Page::get()->byID($id);
+        $this->assertEquals($urlSegment, $page->URLSegment);
+    }
+
+    /**
+     * Check if reserved URL's are not appended with a number on a child page
+     * It's okay to have a URL like domain.com/my-page/admin as it won't interfere with domain.com/admin
+     * @dataProvider reservedSegmentsProvider
+     */
+    public function testDisallowedChildURLGeneration($title, $urlSegment)
+    {
+        // Using the same dataprovider, strip out the -2 from the admin and dev segment
+        $urlSegment = str_replace('-2', '', $urlSegment);
+        $page = Page::create(['Title' => $title, 'ParentID' => 1]);
+        $id = $page->write();
+        $page = Page::get()->byID($id);
+        $this->assertEquals($urlSegment, $page->URLSegment);
+    }
+
     /**
      * Test that publication copies data to SiteTree_Live
      */
@@ -820,6 +856,14 @@ class SiteTreeTest extends SapphireTest
         $this->assertTrue($ceo->isSection());
     }
 
+    public function testURLSegmentReserved()
+    {
+        $siteTree = SiteTree::create(['URLSegment' => 'admin']);
+        $segment = $siteTree->validURLSegment();
+
+        $this->assertFalse($segment);
+    }
+
     public function testURLSegmentAutoUpdate()
     {
         $sitetree = new SiteTree();