BUGFIX: Improved RedirectorPage's handling of invalid configuration options to prevent infinite loops and segfaults

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@63939 467b73ca-7a2a-4603-9d3b-597d59a354a9

core/model/RedirectorPage.php

@@ -33,13 +33,54 @@ class RedirectorPage extends Page {
 		}		
 	}
 	
+	/**
+	 * Return the the link that should be used for this redirector page, in navigation, etc.
+	 * If the redirectorpage has been appropriately configured, then it will return the redirection
+	 * destination, to prevent unnecessary 30x redirections.  However, if it's misconfigured, then
+	 * it will return a link to itself, which will then display an error message. 
+	 */
 	function Link() {
+		if($link = $this->redirectionLink()) return $link;
+		else return $this->regularLink();
+	}
+	
+	/**
+	 * Return the normal link directly to this page.  Once you visit this link, a 30x redirection
+	 * will take you to your final destination.
+	 */
+	function regularLink() {
+		return parent::Link();
+	}
+	
+	/**
+	 * Return the link that we should redirect to.
+	 * Only return a value if there is a legal redirection destination.
+	 */
+	function redirectionLink() {
 		if($this->RedirectionType == 'External') {
-			return Convert::raw2att($this->ExternalURL);
+			if($this->ExternalURL) {
+				return Convert::raw2att($this->ExternalURL);
+			}
+			
 		} else {
 			$linkTo = $this->LinkToID ? DataObject::get_by_id("SiteTree", $this->LinkToID) : null;
+
 			if($linkTo) {
-				return $linkTo->Link();
+				// We shouldn't point to ourselves - that would create an infinite loop!  Return null since we have a
+				// bad configuration
+				if($this->ID == $linkTo->ID) {
+					return null;
+			
+				// If we're linking to another redirectorpage then just return the URLSegment, to prevent a cycle of redirector
+				// pages from causing an infinite loop.  Instead, they will cause a 30x redirection loop in the browser, but
+				// this can be handled sufficiently gracefully by the browser.
+				} elseif($linkTo instanceof RedirectorPage) {
+					return $linkTo->regularLink();
+
+				// For all other pages, just return the link of the page.
+				} else {
+					return $linkTo->Link();
+				}
 			}
 		}
 	}
@@ -108,17 +149,10 @@ class RedirectorPage extends Page {
  */
 class RedirectorPage_Controller extends Page_Controller {
 	function init() {
-		if($this->RedirectionType == 'External') {
+		if($link = $this->redirectionLink()) {
-			if($this->ExternalURL) {
+			Director::redirect($link, 301);
-				Director::redirect($this->ExternalURL);
-			}
-		} else {
-			$linkTo = DataObject::get_by_id("SiteTree", $this->LinkToID);
-			if($linkTo) {
-				Director::redirect($linkTo->Link(), 301);
-			}
 		}
-		
+
 		parent::init();
 	}
 	
@@ -126,9 +160,11 @@ class RedirectorPage_Controller extends Page_Controller {
 	 * If we ever get this far, it means that the redirection failed.
 	 */
 	function index() {
-		return "<p>" .
+		return array(
+			"Content" => "<p>" .
 			_t('RedirectorPage.HASBEENSETUP', 'A redirector page has been set up without anywhere to redirect to.') .
-			"</p>";
+			"</p>"
+		);
 	}
 }
 ?>

tests/RedirectorPageTest.php

@@ -0,0 +1,48 @@
+<?php
+
+class RedirectorPageTest extends FunctionalTest {
+	static $fixture_file = 'sapphire/tests/RedirectorPageTest.yml';
+	static $use_draft_site = true;
+	
+	function testGoodRedirectors() {
+		/* For good redirectors, the final destination URL will be returned */
+		$this->assertEquals("http://www.google.com", $this->objFromFixture('RedirectorPage','goodexternal')->Link());
+		$this->assertEquals(Director::baseURL() . "redirection-dest/", $this->objFromFixture('RedirectorPage','goodinternal')->redirectionLink());
+		$this->assertEquals(Director::baseURL() . "redirection-dest/", $this->objFromFixture('RedirectorPage','goodinternal')->Link());
+	}
+
+	function testEmptyRedirectors() {
+		/* If a redirector page is misconfigured, then its link method will just return the usual URLSegment-generated value */
+		$page1 = $this->objFromFixture('RedirectorPage','badexternal');
+		$this->assertEquals(Director::baseURL() . 'bad-external/', $page1->Link());
+
+		/* An error message will be shown if you visit it */
+		$content = $this->get(Director::makeRelative($page1->Link()))->getBody();
+		$this->assertContains("A redirector page has been set up without anywhere to redirect to.", $content);
+
+		/* This also applies for internal links */
+		$page2 = $this->objFromFixture('RedirectorPage','badinternal');
+		$this->assertEquals(Director::baseURL() . 'bad-internal/', $page2->Link());
+		$content = $this->get(Director::makeRelative($page2->Link()))->getBody();
+		$this->assertContains("A redirector page has been set up without anywhere to redirect to.", $content);
+	}
+
+	function testReflexiveAndTransitiveInternalRedirectors() {
+		/* Reflexive redirectors are those that point to themselves.  They should behave the same as an empty redirector */
+		$page = $this->objFromFixture('RedirectorPage','reflexive');
+		$this->assertEquals(Director::baseURL() . 'reflexive/', $page->Link());
+		$content = $this->get(Director::makeRelative($page->Link()))->getBody();
+		$this->assertContains("A redirector page has been set up without anywhere to redirect to.", $content);
+
+		/* Transitive redirectors are those that point to another redirector page.  They should send people to the URLSegment
+		 * of the destination page - the middle-stop, so to speak.  That should redirect to the final destination */
+		$page = $this->objFromFixture('RedirectorPage','transitive');
+		$this->assertEquals(Director::baseURL() . 'good-internal/', $page->Link());
+		
+		$this->autoFollowRedirection = false;
+		$response = $this->get(Director::makeRelative($page->Link()));
+		$this->assertEquals(Director::baseURL() . "redirection-dest/", $response->getHeader("Location"));
+	}
+}
+
+?>

tests/RedirectorPageTest.yml

@@ -0,0 +1,34 @@
+Page:
+   dest:
+      Title: Redirection Dest
+      URLSegment: redirection-dest
+
+RedirectorPage:
+   goodexternal:
+      Title: Good External
+      URLSegment: good-external
+      RedirectionType: External
+      ExternalURL: http://www.google.com
+   goodinternal:
+      Title: Good Internal
+      URLSegment: good-internal
+      RedirectionType: Internal:
+      LinkTo: =>Page.dest
+   badexternal:
+      Title: Bad External
+      RedirectionType: External
+      URLSegment: bad-external
+   badinternal:
+      Title: Bad Internal
+      RedirectionType: Internal
+      URLSegment: bad-internal
+   reflexive:
+      Title: Reflexive
+      RedirectionType: Internal
+      LinkTo: =>RedirectorPage.reflexive
+      URLSegment: reflexive 
+   transitive:
+      Title: Transitive
+      RedirectionType: Internal
+      LinkTo: =>RedirectorPage.goodinternal
+      URLSegment: transitive