From f7377865d44bd054c3a9765da1b7d89fff22dc12 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Thu, 7 Jun 2018 18:02:19 +1200 Subject: [PATCH] NEW: Ensure internal URLs are domain-relative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Domain relative URLs (i.e. those starting with “/“) are safer; less likely to break webcrawlers. Other parts of SilverStripe output URLs in this form by default. --- code/DocumentationManifest.php | 39 ++++++++++++++++-------- code/controllers/DocumentationViewer.php | 8 +++-- code/models/DocumentationEntity.php | 4 +-- code/models/DocumentationPage.php | 9 ++---- tests/DocumentationPageTest.php | 6 ++-- tests/DocumentationParserTest.php | 32 +++++++++---------- tests/DocumentationViewerTest.php | 12 ++++---- 7 files changed, 62 insertions(+), 48 deletions(-) diff --git a/code/DocumentationManifest.php b/code/DocumentationManifest.php index 3d83a61..a7c5098 100644 --- a/code/DocumentationManifest.php +++ b/code/DocumentationManifest.php @@ -418,14 +418,15 @@ class DocumentationManifest */ protected function stripLinkBase($link) { - return ltrim( - str_replace( - Config::inst()->get('DocumentationViewer', 'link_base'), - '', - $link - ), - '/' - ); + // Trim baseURL + $link = preg_replace('/^' . preg_quote(Director::baseURL(), '/') .'/', '', $link); + + // Trim link_base + if ($linkBase = Config::inst()->get('DocumentationViewer', 'link_base')) { + $link = preg_replace('/^' . preg_quote($linkBase, '/') .'\/?/', '', $link); + } + + return $link; } /** @@ -569,6 +570,19 @@ class DocumentationManifest return $output; } + /** + * Create a clean domain-relative URL form a docuetn URL + */ + protected function buildUrl($url) + { + return Controller::join_links( + Director::baseURL(), + Config::inst()->get('DocumentationViewer', 'link_base'), + $url, + '/' + ); + } + /** * Determine the next page from the given page. * @@ -589,7 +603,7 @@ class DocumentationManifest if ($grabNext && strpos($page['filepath'], $entityBase) !== false) { return new ArrayData( array( - 'Link' => Controller::join_links(Config::inst()->get('DocumentationViewer', 'link_base'), $url), + 'Link' => $this->buildUrl($url), 'Title' => $page['title'] ) ); @@ -600,7 +614,7 @@ class DocumentationManifest } elseif (!$fallback && strpos($page['filepath'], $filepath) !== false) { $fallback = new ArrayData( array( - 'Link' => Controller::join_links(Config::inst()->get('DocumentationViewer', 'link_base'), $url), + 'Link' => $this->buildUrl($url), 'Title' => $page['title'], 'Fallback' => true ) @@ -635,7 +649,7 @@ class DocumentationManifest if ($previousUrl) { return new ArrayData( array( - 'Link' => Controller::join_links(Config::inst()->get('DocumentationViewer', 'link_base'), $previousUrl), + 'Link' => $this->buildUrl($previousUrl), 'Title' => $previousPage['title'] ) ); @@ -684,7 +698,6 @@ class DocumentationManifest } $output = new ArrayList(); - $base = Config::inst()->get('DocumentationViewer', 'link_base'); $entityPath = $this->normalizeUrl($entityPath); $recordPath = $this->normalizeUrl($recordPath); $recordParts = explode('/', trim($recordPath, '/')); @@ -720,7 +733,7 @@ class DocumentationManifest $output->push( new ArrayData( array( - 'Link' => Controller::join_links($base, $url, '/'), + 'Link' => $this->buildUrl($url), 'Title' => $page['title'], 'LinkingMode' => $mode, 'Summary' => $page['summary'], diff --git a/code/controllers/DocumentationViewer.php b/code/controllers/DocumentationViewer.php index 807214e..95b33c3 100755 --- a/code/controllers/DocumentationViewer.php +++ b/code/controllers/DocumentationViewer.php @@ -62,7 +62,7 @@ class DocumentationViewer extends Controller implements PermissionProvider /** * @config * - * @var string same as the routing pattern set through Director::addRules(). + * @var string Site-relative root URL of documentation. Same as the routing pattern set through Director::addRules(). */ private static $link_base = 'dev/docs/'; @@ -567,6 +567,7 @@ class DocumentationViewer extends Controller implements PermissionProvider public function Link($action = '') { $link = Controller::join_links( + Director::baseURL(), Config::inst()->get('DocumentationViewer', 'link_base'), $this->getLanguage(), $action, @@ -786,7 +787,10 @@ class DocumentationViewer extends Controller implements PermissionProvider */ public function getDocumentationBaseHref() { - return Config::inst()->get('DocumentationViewer', 'link_base'); + return Controller::join_links( + Director::baseURL(), + Config::inst()->get('DocumentationViewer', 'link_base') + ); } /** diff --git a/code/models/DocumentationEntity.php b/code/models/DocumentationEntity.php index c4a4d66..95f3fb6 100755 --- a/code/models/DocumentationEntity.php +++ b/code/models/DocumentationEntity.php @@ -137,12 +137,14 @@ class DocumentationEntity extends ViewableData { if ($this->getIsDefaultEntity()) { $base = Controller::join_links( + Director::baseURL(), Config::inst()->get('DocumentationViewer', 'link_base'), $this->getLanguage(), '/' ); } else { $base = Controller::join_links( + Director::baseURL(), Config::inst()->get('DocumentationViewer', 'link_base'), $this->getLanguage(), $this->getKey(), @@ -150,8 +152,6 @@ class DocumentationEntity extends ViewableData ); } - $base = ltrim(str_replace('//', '/', $base), '/'); - if ($short && $this->stable) { return $base; } diff --git a/code/models/DocumentationPage.php b/code/models/DocumentationPage.php index eba6de6..2537d89 100755 --- a/code/models/DocumentationPage.php +++ b/code/models/DocumentationPage.php @@ -262,12 +262,9 @@ class DocumentationPage extends ViewableData */ public function Link($short = false) { - return ltrim( - Controller::join_links( - $this->entity->Link($short), - $this->getRelativeLink() - ), - '/' + return Controller::join_links( + $this->entity->Link($short), + $this->getRelativeLink() ); } diff --git a/tests/DocumentationPageTest.php b/tests/DocumentationPageTest.php index 8e72bca..e4d90c4 100755 --- a/tests/DocumentationPageTest.php +++ b/tests/DocumentationPageTest.php @@ -42,7 +42,7 @@ class DocumentationPageTest extends SapphireTest // single layer $this->assertEquals( - 'dev/docs/en/doctest/2.4/test/', + Director::baseURL() . 'dev/docs/en/doctest/2.4/test/', $page->Link(), 'The page link should have no extension and have a language' ); @@ -53,7 +53,7 @@ class DocumentationPageTest extends SapphireTest DOCSVIEWER_PATH . '/tests/docs/en/sort/' ); - $this->assertEquals('dev/docs/en/doctest/2.4/sort/', $page->Link()); + $this->assertEquals(Director::baseURL() . 'dev/docs/en/doctest/2.4/sort/', $page->Link()); $page = new DocumentationFolder( $this->entity, @@ -61,7 +61,7 @@ class DocumentationPageTest extends SapphireTest DOCSVIEWER_PATH . '/tests/docs/en/sort/1-basic.md' ); - $this->assertEquals('dev/docs/en/doctest/2.4/sort/basic/', $page->Link()); + $this->assertEquals(Director::baseURL() . 'dev/docs/en/doctest/2.4/sort/basic/', $page->Link()); } public function testGetBreadcrumbTitle() diff --git a/tests/DocumentationParserTest.php b/tests/DocumentationParserTest.php index 906d509..7bbef79 100755 --- a/tests/DocumentationParserTest.php +++ b/tests/DocumentationParserTest.php @@ -174,7 +174,7 @@ HTML; ); $this->assertContains( - '[link: subfolder index](dev/docs/en/documentationparsertest/2.4/subfolder/)', + '[link: subfolder index](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/subfolder/)', $result ); @@ -186,11 +186,11 @@ HTML; ); $this->assertContains( - '[link: subfolder index](dev/docs/en/documentationparsertest/2.4/subfolder/)', + '[link: subfolder index](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/subfolder/)', $result ); $this->assertContains( - '[link: subfolder page](dev/docs/en/documentationparsertest/2.4/subfolder/subpage/)', + '[link: subfolder page](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/subfolder/subpage/)', $result ); $this->assertContains( @@ -199,12 +199,12 @@ HTML; ); $this->assertContains( - '[link: with anchor](dev/docs/en/documentationparsertest/2.4/test/#anchor)', + '[link: with anchor](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/test/#anchor)', $result ); $this->assertContains( - '[link: relative anchor](dev/docs/en/documentationparsertest/2.4/test/#relative-anchor)', + '[link: relative anchor](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/test/#relative-anchor)', $result ); @@ -215,33 +215,33 @@ HTML; // @todo this should redirect to /subpage/ $this->assertContains( - '[link: relative](dev/docs/en/documentationparsertest/2.4/subfolder/subpage.md/)', + '[link: relative](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/subfolder/subpage.md/)', $result ); $this->assertContains( - '[link: absolute index](dev/docs/en/documentationparsertest/2.4/)', + '[link: absolute index](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/)', $result ); // @todo this should redirect to / $this->assertContains( - '[link: absolute index with name](dev/docs/en/documentationparsertest/2.4/index/)', + '[link: absolute index with name](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/index/)', $result ); $this->assertContains( - '[link: relative index](dev/docs/en/documentationparsertest/2.4/)', + '[link: relative index](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/)', $result ); $this->assertContains( - '[link: relative parent page](dev/docs/en/documentationparsertest/2.4/test/)', + '[link: relative parent page](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/test/)', $result ); $this->assertContains( - '[link: absolute parent page](dev/docs/en/documentationparsertest/2.4/test/)', + '[link: absolute parent page](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/test/)', $result ); @@ -251,27 +251,27 @@ HTML; ); $this->assertContains( - '[link: absolute index](dev/docs/en/documentationparsertest/2.4/)', + '[link: absolute index](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/)', $result ); $this->assertContains( - '[link: relative index](dev/docs/en/documentationparsertest/2.4/subfolder/)', + '[link: relative index](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/subfolder/)', $result ); $this->assertContains( - '[link: relative parent page](dev/docs/en/documentationparsertest/2.4/subfolder/subpage/)', + '[link: relative parent page](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/subfolder/subpage/)', $result ); $this->assertContains( - '[link: relative grandparent page](dev/docs/en/documentationparsertest/2.4/test/)', + '[link: relative grandparent page](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/test/)', $result ); $this->assertContains( - '[link: absolute page](dev/docs/en/documentationparsertest/2.4/test/)', + '[link: absolute page](' . Director::baseURL() . 'dev/docs/en/documentationparsertest/2.4/test/)', $result ); } diff --git a/tests/DocumentationViewerTest.php b/tests/DocumentationViewerTest.php index ef6b8bb..7aeb618 100755 --- a/tests/DocumentationViewerTest.php +++ b/tests/DocumentationViewerTest.php @@ -157,9 +157,9 @@ class DocumentationViewerTest extends FunctionalTest $response = $v->handleRequest(new SS_HTTPRequest('GET', 'en/doc_test/2.3/'), DataModel::inst()); $expected = array( - 'dev/docs/en/doc_test/2.3/sort/' => 'Sort', - 'dev/docs/en/doc_test/2.3/subfolder/' => 'Subfolder', - 'dev/docs/en/doc_test/2.3/test/' => 'Test' + Director::baseURL() . 'dev/docs/en/doc_test/2.3/sort/' => 'Sort', + Director::baseURL() . 'dev/docs/en/doc_test/2.3/subfolder/' => 'Subfolder', + Director::baseURL() . 'dev/docs/en/doc_test/2.3/test/' => 'Test' ); $actual = $v->getMenu()->first()->Children->map('Link', 'Title'); @@ -174,9 +174,9 @@ class DocumentationViewerTest extends FunctionalTest // menu should contain all the english entities $expected = array( - 'dev/docs/en/doc_test/2.4/' => 'Doc Test', - 'dev/docs/en/documentationvieweraltmodule1/' => 'DocumentationViewerAltModule1', - 'dev/docs/en/documentationvieweraltmodule2/' => 'DocumentationViewerAltModule2' + Director::baseURL() . 'dev/docs/en/doc_test/2.4/' => 'Doc Test', + Director::baseURL() . 'dev/docs/en/documentationvieweraltmodule1/' => 'DocumentationViewerAltModule1', + Director::baseURL() . 'dev/docs/en/documentationvieweraltmodule2/' => 'DocumentationViewerAltModule2' ); $this->assertEquals($expected, $v->getMenu()->map('Link', 'Title'));