From 95101fcbc6505c1d351caa1e5573d8a0f8a51177 Mon Sep 17 00:00:00 2001
From: Elliot Sawyer <elliot@silverstripe.com>
Date: Mon, 14 Jul 2014 13:54:40 +1200
Subject: [PATCH] Disallow javascript protocol in IFrameURL using a whitelist
 of allowed schemes. Includes unit testing and a docblock to IFramePage
 validate function to explain exceptions and return types

---
 code/IFramePage.php      | 20 ++++++++++++++++++++
 lang/en.yml              |  2 ++
 tests/IFramePageTest.php | 41 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/code/IFramePage.php b/code/IFramePage.php
index 20b1cb2..cc16cfd 100644
--- a/code/IFramePage.php
+++ b/code/IFramePage.php
@@ -73,6 +73,26 @@ class IFramePage extends Page {
 
 		return $style;
 	}
+
+	/**
+	 * Ensure that the IFrameURL is a valid url and prevents XSS
+	 * 
+	 * @throws ValidationException
+	 * @return ValidationResult
+	 */
+	public function validate() {
+		$result = parent::validate();
+
+		//whitelist allowed URL schemes
+		$allowed_schemes = array('http', 'https');
+		if($matches = parse_url($this->IFrameURL)) {
+			if(isset($matches['scheme']) && !in_array($matches['scheme'], $allowed_schemes)) {
+				$result->error(_t('IFramePage.VALIDATION.BANNEDURLSCHEME', "This URL scheme is not allowed."));
+			}
+		}
+
+		return $result;
+	}
 }
 
 class IFramePage_Controller extends Page_Controller {
diff --git a/lang/en.yml b/lang/en.yml
index bf2ecc2..deb35bd 100644
--- a/lang/en.yml
+++ b/lang/en.yml
@@ -3,6 +3,8 @@ en:
     DESCRIPTION: 'Embeds an iframe into the body of the page.'
     PLURALNAME: 'Base Pages'
     SINGULARNAME: 'I Frame Page'
+    VALIDATION:
+      BANNEDURLSCHEME: 'This URL scheme is not allowed.'
   IframePage:
     ExternalNote: 'Please note the following section of content is possibly being delivered from an external source (IFRAME in HTML terms), and may present unusual experiences for screen readers.'
     Loading: 'Loading content...'
diff --git a/tests/IFramePageTest.php b/tests/IFramePageTest.php
index 9a407e9..8c893d6 100644
--- a/tests/IFramePageTest.php
+++ b/tests/IFramePageTest.php
@@ -33,4 +33,45 @@ class IFramePageTest extends SapphireTest {
 		$iframe->FixedWidth = '200';
 		$this->assertContains('width: 200px', $iframe->getStyle(), 'Fixed width is settable');
 	}
+
+	function testAllowedUrls() {
+		$iframe = new IFramePage();
+
+		$tests = array(
+			'allowed' => array(
+				'http://anything',
+				'https://anything',
+				'page',
+				'sub-page/link',
+				'page/link',
+				'page.html',
+				'page.htm',
+				'page.phpissoawesomewhywouldiuseanythingelse',
+				'//url.com/page',
+				'/root/page/link',
+				'http://intranet:8888',
+				'http://javascript:8080',
+				'http://username:password@hostname/path?arg=value#anchor'
+			),
+			'banned' => array(
+				'javascript:alert',
+				'tel:0210001234',
+				'ftp://url',
+				'ssh://1.2.3.4',
+				'ssh://url.com/page'
+			)
+		);
+
+		foreach($tests['allowed'] as $url) {
+			$iframe->IFrameURL = $url;
+			$iframe->write();
+			$this->assertContains($iframe->IFrameURL, $url);
+		}
+
+		foreach($tests['banned'] as $url) {
+			$iframe->IFrameURL = $url;
+			$this->setExpectedException('ValidationException');
+			$iframe->write();
+		}
+	}
 }