From b61d6dcd57577b0345af7a69e51da409305e1957 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 13 Nov 2015 10:51:13 +1300 Subject: [PATCH] [ss-2015-027]: FIX HtmlEditorField_Toolbar#viewfile not whitelisting URLs --- .../Field_types/03_HTMLEditorField.md | 12 +++ forms/HtmlEditorField.php | 100 ++++++++++++++---- tests/forms/HtmlEditorFieldToolbarTest.php | 71 +++++++++++++ tests/forms/HtmlEditorFieldToolbarTest.yml | 9 ++ 4 files changed, 171 insertions(+), 21 deletions(-) create mode 100644 tests/forms/HtmlEditorFieldToolbarTest.php create mode 100644 tests/forms/HtmlEditorFieldToolbarTest.yml diff --git a/docs/en/02_Developer_Guides/03_Forms/Field_types/03_HTMLEditorField.md b/docs/en/02_Developer_Guides/03_Forms/Field_types/03_HTMLEditorField.md index 13b276dde..b18938ca5 100644 --- a/docs/en/02_Developer_Guides/03_Forms/Field_types/03_HTMLEditorField.md +++ b/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 diff --git a/forms/HtmlEditorField.php b/forms/HtmlEditorField.php index fad081a74..4bc987e9b 100644 --- a/forms/HtmlEditorField.php +++ b/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); diff --git a/tests/forms/HtmlEditorFieldToolbarTest.php b/tests/forms/HtmlEditorFieldToolbarTest.php new file mode 100644 index 000000000..6b661936d --- /dev/null +++ b/tests/forms/HtmlEditorFieldToolbarTest.php @@ -0,0 +1,71 @@ +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'); + } + +} \ No newline at end of file diff --git a/tests/forms/HtmlEditorFieldToolbarTest.yml b/tests/forms/HtmlEditorFieldToolbarTest.yml new file mode 100644 index 000000000..164c30844 --- /dev/null +++ b/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