Merge pull request #5434 from open-sausages/pulls/4.0/fix-htmleditorf…

…ield-whitelist [ss-2015-027] FIX HtmlEditorField_Toolbar#viewfile not whitelisting URLs
silverstripe · May 3, 2016 · db6af33 · db6af33
2 parents a7f5ef7 + 70480f5
commit db6af33
Show file tree

Hide file tree

Showing 3 changed files with 220 additions and 18 deletions.
diff --git a/forms/htmleditor/HtmlEditorField.php b/forms/htmleditor/HtmlEditorField.php
@@ -413,37 +413,120 @@ public function MediaForm() {
 		return $form;
 	}
 
+	/**
+	 * List of allowed schemes (no wildcard, all lower case) or empty to allow all schemes
+	 *
+	 * @config
+	 * @var array
+	 */
+	private static $fileurl_scheme_whitelist = array('http', 'https');
+
+	/**
+	 * List of allowed domains (no wildcard, all lower case) or empty to allow all domains
+	 *
+	 * @config
+	 * @var array
+	 */
+	private static $fileurl_domain_whitelist = array();
+
+	/**
+	 * Find local File dataobject given ID
+	 *
+	 * @param int $id
+	 * @return array
+	 */
+	protected function viewfile_getLocalFileByID($id) {
+		/** @var File $file */
+		$file = DataObject::get_by_id('File', $id);
+		if ($file && $file->canView()) {
+			return array($file, $file->getURL());
+		}
+		return [null, null];
+	}
+
+	/**
+	 * Get remote File given url
+	 *
+	 * @param string $fileUrl Absolute URL
+	 * @return array
+	 * @throws SS_HTTPResponse_Exception
+	 */
+	protected function viewfile_getRemoteFileByURL($fileUrl) {
+		if(!Director::is_absolute_url($fileUrl)) {
+			throw $this->getErrorFor(_t(
+				"HtmlEditorField_Toolbar.ERROR_ABSOLUTE",
+				"Only absolute urls can be embedded"
+			));
+		}
+		$scheme = strtolower(parse_url($fileUrl, PHP_URL_SCHEME));
+		$allowed_schemes = self::config()->fileurl_scheme_whitelist;
+		if (!$scheme || ($allowed_schemes && !in_array($scheme, $allowed_schemes))) {
+			throw $this->getErrorFor(_t(
+				"HtmlEditorField_Toolbar.ERROR_SCHEME",
+				"This file scheme is not included in the whitelist"
+			));
+		}
+		$domain = strtolower(parse_url($fileUrl, PHP_URL_HOST));
+		$allowed_domains = self::config()->fileurl_domain_whitelist;
+		if (!$domain || ($allowed_domains && !in_array($domain, $allowed_domains))) {
+			throw $this->getErrorFor(_t(
+				"HtmlEditorField_Toolbar.ERROR_HOSTNAME",
+				"This file hostname is not included in the whitelist"
+			));
+		}
+		return [null, $fileUrl];
+	}
+
+	/**
+	 * Prepare error for the front end
+	 *
+	 * @param string $message
+	 * @param int $code
+	 * @return SS_HTTPResponse_Exception
+	 */
+	protected function getErrorFor($message, $code = 400) {
+		$exception = new SS_HTTPResponse_Exception($message, $code);
+		$exception->getResponse()->addHeader('X-Status', $message);
+		return $exception;
+	}
+
 	/**
 	 * View of a single file, either on the filesystem or on the web.
 	 *
+	 * @throws SS_HTTPResponse_Exception
 	 * @param SS_HTTPRequest $request
 	 * @return string
 	 */
 	public function viewfile($request) {
-		// TODO Would be cleaner to consistently pass URL for both local and remote files,
-		// but GridField doesn't allow for this kind of metadata customization at the moment.
 		$file = null;
-		if($url = $request->getVar('FileURL')) {
-			// URLS should be used for remote resources (not local assets)
-			$url = Director::absoluteURL($url);
+		$url = null;
+		// Get file and url by request method
+		if($fileUrl = $request->getVar('FileURL')) {
+			// Get remote url
+			list($file, $url) = $this->viewfile_getRemoteFileByURL($fileUrl);
 		} elseif($id = $request->getVar('ID')) {
-			// Use local dataobject
-			$file = DataObject::get_by_id('File', $id);
-			if(!$file) {
-				throw new InvalidArgumentException("File could not be found");
-			}
-			$url = $file->getURL();
-			if(!$url) {
-				return $this->httpError(404, 'File not found');
-			}
+			// Or we could have been passed an ID directly
+			list($file, $url) = $this->viewfile_getLocalFileByID($id);
 		} else {
-			throw new LogicException('Need either "ID" or "FileURL" parameter to identify the file');
+			// Or we could have been passed nothing, in which case panic
+			throw $this->getErrorFor(_t(
+				"HtmlEditorField_Toolbar.ERROR_ID",
+				'Need either "ID" or "FileURL" parameter to identify the file'
+			));
+		}
+
+		// Validate file exists
+		if(!$url) {
+			throw $this->getErrorFor(_t(
+				"HtmlEditorField_Toolbar.ERROR_NOTFOUND",
+				'Unable to find file to view'
+			));
 		}
 
 		// Instanciate file wrapper and get fields based on its type
 		// Check if appCategory is an image and exists on the local system, otherwise use oEmbed to refference a
 		// remote image
-		$fileCategory = File::get_app_category(File::get_file_extension($url));
+		$fileCategory = $this->getFileCategory($url, $file);
 		switch($fileCategory) {
 			case 'image':
 			case 'image/supported':
@@ -456,10 +539,12 @@ public function viewfile($request) {
 				// Only remote files can be linked via o-embed
 				// {@see HtmlEditorField_Toolbar::getAllowedExtensions())
 				if($file) {
-					throw new InvalidArgumentException(
+					throw $this->getErrorFor(_t(
+						"HtmlEditorField_Toolbar.ERROR_OEMBED_REMOTE",
 						"Oembed is only compatible with remote files"
-					);
+					));
 				}
+
 				// Other files should fallback to oembed
 				$fileWrapper = new HtmlEditorField_Embed($url, $file);
 				break;
@@ -472,10 +557,28 @@ public function viewfile($request) {
 		))->renderWith($this->templateViewFile);
 	}
 
+	/**
+	 * Guess file category from either a file or url
+	 *
+	 * @param string $url
+	 * @param File $file
+	 * @return string
+	 */
+	protected function getFileCategory($url, $file) {
+		if($file) {
+			return $file->appCategory();
+		}
+		if($url) {
+			return File::get_app_category(File::get_file_extension($url));
+		}
+		return null;
+	}
+
 	/**
 	 * Find all anchors available on the given page.
 	 *
 	 * @return array
+	 * @throws SS_HTTPResponse_Exception
 	 */
 	public function getanchors() {
 		$id = (int)$this->getRequest()->getVar('PageID');

diff --git a/tests/forms/HtmlEditorFieldToolbarTest.php b/tests/forms/HtmlEditorFieldToolbarTest.php
@@ -0,0 +1,81 @@
+<?php
+
+class HtmlEditorFieldToolbarTest_Toolbar extends HtmlEditorField_Toolbar {
+	public function viewfile_getLocalFileByID($id) {
+		return parent::viewfile_getLocalFileByID($id);
+	}
+
+	public function viewfile_getRemoteFileByURL($fileUrl) {
+		return parent::viewfile_getRemoteFileByURL($fileUrl);
+	}
+}
+
+class HtmlEditorFieldToolbarTest extends SapphireTest {
+
+	protected static $fixture_file = 'HtmlEditorFieldToolbarTest.yml';
+
+	/**
+	 * @return HtmlEditorFieldToolbarTest_Toolbar
+	 */
+	protected function getToolbar() {
+		return new HtmlEditorFieldToolbarTest_Toolbar(null, '/');
+	}
+
+	public function setUp() {
+		parent::setUp();
+
+		Config::inst()->update('HtmlEditorField_Toolbar', 'fileurl_scheme_whitelist', array('http'));
+		Config::inst()->update('HtmlEditorField_Toolbar', 'fileurl_domain_whitelist', array('example.com'));
+
+		// Filesystem mock
+		AssetStoreTest_SpyStore::activate(__CLASS__);
+
+		// Load up files
+		/** @var File $file1 */
+		$file1 = $this->objFromFixture('File', 'example_file');
+		$file1->setFromString(str_repeat('x', 1000), $file1->Name);
+		$file1->write();
+
+		/** @var Image $image1 */
+		$image1 = $this->objFromFixture('Image', 'example_image');
+		$image1->setFromLocalFile(
+			__DIR__ . '/images/HTMLEditorFieldTest-example.jpg',
+			'folder/subfolder/HTMLEditorFieldTest_example.jpg'
+		);
+		$image1->write();
+	}
+
+	public function testValidLocalReference() {
+		/** @var File $exampleFile */
+		$exampleFile = $this->objFromFixture('File', 'example_file');
+		$expectedUrl = $exampleFile->AbsoluteLink();
+		Config::inst()->update('HtmlEditorField_Toolbar', 'fileurl_domain_whitelist', array(
+			'example.com',
+			strtolower(parse_url($expectedUrl, PHP_URL_HOST))
+		));
+
+		list($file, $url) = $this->getToolbar()->viewfile_getRemoteFileByURL($exampleFile->AbsoluteLink());
+		$this->assertEquals($expectedUrl, $url);
+	}
+
+	public function testValidScheme() {
+		list($file, $url) = $this->getToolbar()->viewfile_getRemoteFileByURL('http://example.com/test.pdf');
+		$this->assertEquals($url, 'http://example.com/test.pdf');
+	}
+
+	/** @expectedException SS_HTTPResponse_Exception */
+	public function testInvalidScheme() {
+		list($file, $url) = $this->getToolbar()->viewfile_getRemoteFileByURL('nosuchscheme://example.com/test.pdf');
+	}
+
+	public function testValidDomain() {
+		list($file, $url) = $this->getToolbar()->viewfile_getRemoteFileByURL('http://example.com/test.pdf');
+		$this->assertEquals($url, 'http://example.com/test.pdf');
+	}
+
+	/** @expectedException SS_HTTPResponse_Exception */
+	public function testInvalidDomain() {
+		list($file, $url) = $this->getToolbar()->viewfile_getRemoteFileByURL('http://evil.com/test.pdf');
+	}
+
+}
diff --git a/tests/forms/HtmlEditorFieldToolbarTest.yml b/tests/forms/HtmlEditorFieldToolbarTest.yml
@@ -0,0 +1,18 @@
+Folder:
+  folder1:
+    Name: folder
+    Title: folder
+  folder2:
+    Name: subfolder
+    Title: subfolder
+    Parent: =>Folder.folder1
+
+File:
+  example_file:
+    Name: example.pdf
+    Parent: =>Folder.folder2
+
+Image:
+  example_image:
+    Name: HTMLEditorFieldTest_example.jpg
+    Parent: =>Folder.folder2