[ss-2015-027]: FIX HtmlEditorField_Toolbar#viewfile not whitelisting URLs

docs/en/02_Developer_Guides/03_Forms/Field_types/03_HTMLEditorField.md

@@ -218,6 +218,18 @@ To refresh a oEmbed cache, append `?flush=1` to a URL.
 
 To disable oEmbed usage, set the `Oembed.enabled` configuration property to "false".
 
+## Limiting oembed URLs
+
+HtmlEditorField can have whitelists set on both the scheme (default http & https) and domains allowed when
+inserting files for use with oembed.
+
+This is performed through the config variables `HtmlEditorField_Toolbar::$fileurl_scheme_whitelist` and
+`HtmlEditorField_Toolbar::$fileurl_domain_whitelist`.
+
+Setting these configuration variables to empty arrays will disable the whitelist. Setting them to an array of
+lower case strings will require the scheme or domain respectively to exactly match one of those strings (no
+wildcards are currently supported).
+
 ### Doctypes
 
 Since TinyMCE generates markup, it needs to know which doctype your documents will be rendered in. You can set this 

forms/HtmlEditorField.php

@@ -449,40 +449,98 @@ class HtmlEditorField_Toolbar extends RequestHandler {
 		return $form;
 	}
 
+	/**
+	 * @config
+	 * @var array - list of allowed schemes (no wildcard, all lower case) or empty to allow all schemes
+	 */
+	private static $fileurl_scheme_whitelist = array('http', 'https');
+
+	/**
+	 * @config
+	 * @var array - list of allowed domains (no wildcard, all lower case) or empty to allow all domains
+	 */
+	private static $fileurl_domain_whitelist = array();
+
+	protected function viewfile_getLocalFileByID($id) {
+		$file = DataObject::get_by_id('File', $id);
+
+		if ($file && $file->canView()) return array($file, $file->RelativeLink());
+		return array(null, null);
+	}
+
+	protected function viewfile_getLocalFileByURL($fileUrl) {
+		$filteredUrl = Director::makeRelative($fileUrl);
+		$filteredUrl = preg_replace('/_resampled\/[^-]+-/', '', $filteredUrl);
+
+		$file = File::get()->filter('Filename', $filteredUrl)->first();
+
+		if ($file && $file->canView()) return array($file, $filteredUrl);
+		return array(null, null);
+	}
+
+	protected function viewfile_getRemoteFileByURL($fileUrl) {
+		$scheme = strtolower(parse_url($fileUrl, PHP_URL_SCHEME));
+		$allowed_schemes = self::config()->fileurl_scheme_whitelist;
+
+		if (!$scheme || ($allowed_schemes && !in_array($scheme, $allowed_schemes))) {
+			$exception = new SS_HTTPResponse_Exception("This file scheme is not included in the whitelist", 400);
+			$exception->getResponse()->addHeader('X-Status', $exception->getMessage());
+			throw $exception;
+		}
+
+		$domain = strtolower(parse_url($fileUrl, PHP_URL_HOST));
+		$allowed_domains = self::config()->fileurl_domain_whitelist;
+
+		if (!$domain || ($allowed_domains && !in_array($domain, $allowed_domains))) {
+			$exception = new SS_HTTPResponse_Exception("This file hostname is not included in the whitelist", 400);
+			$exception->getResponse()->addHeader('X-Status', $exception->getMessage());
+			throw $exception;
+		}
+
+		return array(
+			new File(array(
+				'Title' => basename($fileUrl),
+				'Filename' => $fileUrl
+			)),
+			$fileUrl
+		);
+	}
+
 	/**
 	 * View of a single file, either on the filesystem or on the web.
 	 */
 	public function viewfile($request) {
+		$file = null;
+		$url = null;
 
 		// 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.
-		if($url = $request->getVar('FileURL')) {
-			if(Director::is_absolute_url($url) && !Director::is_site_url($url)) {
-				$url = $url;
-				$file = new File(array(
-					'Title' => basename($url),
-					'Filename' => $url
-				));
-			} else {
-				$url = Director::makeRelative($request->getVar('FileURL'));
-				$url = preg_replace('/_resampled\/[^-]+-/', '', $url);
-				$file = File::get()->filter('Filename', $url)->first();
-				if(!$file) $file = new File(array(
-					'Title' => basename($url),
-					'Filename' => $url
-				));
+		if($fileUrl = $request->getVar('FileURL')) {
+			// If this isn't an absolute URL, or is, but is to this site, try and get the File object
+			// that is associated with it
+			if(!Director::is_absolute_url($fileUrl) || Director::is_site_url($fileUrl)) {
+				list($file, $url) = $this->viewfile_getLocalFileByURL($fileUrl);
 			}
-		} elseif($id = $request->getVar('ID')) {
-			$file = DataObject::get_by_id('File', $id);
-			$url = $file->RelativeLink();
-		} else {
-			throw new LogicException('Need either "ID" or "FileURL" parameter to identify the file');
+			// If this is an absolute URL, but not to this site, use as an oembed source (after whitelisting URL)
+			else {
+				list($file, $url) = $this->viewfile_getRemoteFileByURL($fileUrl);
+			}
+		}
+		// Or we could have been passed an ID directly
+		elseif($id = $request->getVar('ID')) {
+			list($file, $url) = $this->viewfile_getLocalFileByID($id);
+		}
+		// Or we could have been passed nothing, in which case panic
+		else {
+			throw new SS_HTTPResponse_Exception('Need either "ID" or "FileURL" parameter to identify the file', 400);
 		}
 
 		// 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
-		if($file && $file->appCategory() == 'image' && Director::is_site_url($url)) {
+		if (!$file || !$url) {
+			throw new SS_HTTPResponse_Exception('Unable to find file to view', 404);
+		} elseif($file->appCategory() == 'image' && Director::is_site_url($url)) {
 			$fileWrapper = new HtmlEditorField_Image($url, $file);
 		} elseif(!Director::is_site_url($url)) {
 			$fileWrapper = new HtmlEditorField_Embed($url, $file);

tests/forms/HtmlEditorFieldToolbarTest.php

@@ -0,0 +1,71 @@
+<?php
+
+class HtmlEditorFieldToolbarTest_Toolbar extends HtmlEditorField_Toolbar {
+	public function viewfile_getLocalFileByID($id) {
+		return parent::viewfile_getLocalFileByID($id);
+	}
+
+	public function viewfile_getLocalFileByURL($fileUrl) {
+		return parent::viewfile_getLocalFileByURL($fileUrl);
+	}
+
+	public function viewfile_getRemoteFileByURL($fileUrl) {
+		return parent::viewfile_getRemoteFileByURL($fileUrl);
+	}
+}
+
+class HtmlEditorFieldToolbarTest extends SapphireTest {
+
+	protected static $fixture_file = 'HtmlEditorFieldToolbarTest.yml';
+
+	protected function getToolbar() {
+		return new HtmlEditorFieldToolbarTest_Toolbar(null, '/');
+	}
+
+	public function setUp() {
+		parent::setUp();
+
+		Config::nest();
+		Config::inst()->update('HtmlEditorField_Toolbar', 'fileurl_scheme_whitelist', array('http'));
+		Config::inst()->update('HtmlEditorField_Toolbar', 'fileurl_domain_whitelist', array('example.com'));
+	}
+
+	public function tearDown() {
+		Config::unnest();
+
+		parent::tearDown();
+	}
+
+	public function testValidLocalReference() {
+		list($file, $url) = $this->getToolbar()->viewfile_getLocalFileByURL('folder/subfolder/example.pdf');
+		$this->assertEquals($this->objFromFixture('File', 'example_file'), $file);
+	}
+
+	public function testInvalidLocalReference() {
+		list($file, $url) = $this->getToolbar()->viewfile_getLocalFileByURL('folder/subfolder/missing.pdf');
+		$this->assertNull($file);
+	}
+
+	public function testValidScheme() {
+		list($file, $url) = $this->getToolbar()->viewfile_getRemoteFileByURL('http://example.com/test.pdf');
+		$this->assertInstanceOf('File', $file);
+		$this->assertEquals($file->Filename, '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->assertInstanceOf('File', $file);
+		$this->assertEquals($file->Filename, 'http://example.com/test.pdf');
+	}
+
+	/** @expectedException SS_HTTPResponse_Exception */
+	public function testInvalidDomain() {
+		list($file, $url) = $this->getToolbar()->viewfile_getRemoteFileByURL('http://evil.com/test.pdf');
+	}
+
+}

tests/forms/HtmlEditorFieldToolbarTest.yml

@@ -0,0 +1,9 @@
+File:
+  example_file:
+    Name: example.pdf
+    Filename: folder/subfolder/example.pdf
+
+Image:
+  example_image:
+    Name: HTMLEditorFieldTest_example.jpg
+    Filename: tests/forms/images/HTMLEditorFieldTest_example.jpg