From c337c8fa454366384bec12e889e4dd371c0a67f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 20 Sep 2017 12:32:41 +0200 Subject: [PATCH 1/4] Theming: Handle errors on uploaded files properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../lib/Controller/ThemingController.php | 23 ++++++++++- .../Controller/ThemingControllerTest.php | 41 +++++++++++-------- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index 06c2c430b7..e73fc16b20 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -207,11 +207,32 @@ class ThemingController extends Controller { } $newLogo = $this->request->getUploadedFile('uploadlogo'); $newBackgroundLogo = $this->request->getUploadedFile('upload-login-background'); + $error = null; + $phpFileUploadErrors = array( + 0 => $this->l10n->t('There is no error, the file uploaded with success'), + 1 => $this->l10n->t('The uploaded file exceeds the upload_max_filesize directive in php.ini'), + 2 => $this->l10n->t('The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form'), + 3 => $this->l10n->t('The uploaded file was only partially uploaded'), + 4 => $this->l10n->t('No file was uploaded'), + 6 => $this->l10n->t('Missing a temporary folder'), + 7 => $this->l10n->t('Failed to write file to disk.'), + 8 => $this->l10n->t('A PHP extension stopped the file upload.'), + ); if (empty($newLogo) && empty($newBackgroundLogo)) { + $error = $this->l10n->t('No file uploaded'); + } + if (!empty($newLogo) && array_key_exists('error', $newLogo) && $newLogo['error'] !== UPLOAD_ERR_OK) { + $error = $phpFileUploadErrors[$newLogo['error']]; + } + if (!empty($newBackgroundLogo) && array_key_exists('error', $newBackgroundLogo) && $newBackgroundLogo['error'] !== UPLOAD_ERR_OK) { + $error = $phpFileUploadErrors[$newBackgroundLogo['error']]; + } + + if ($error !== null) { return new DataResponse( [ 'data' => [ - 'message' => $this->l10n->t('No file uploaded') + 'message' => $error ] ], Http::STATUS_UNPROCESSABLE_ENTITY diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index c03eccb6ee..96a742cfa3 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -131,8 +131,9 @@ class ThemingControllerTest extends TestCase { $this->l10n ->expects($this->once()) ->method('t') - ->with($message) - ->willReturn($message); + ->will($this->returnCallback(function($str) { + return $str; + })); $this->scssCacher ->expects($this->once()) ->method('getCachedSCSS') @@ -183,8 +184,9 @@ class ThemingControllerTest extends TestCase { $this->l10n ->expects($this->once()) ->method('t') - ->with($message) - ->willReturn($message); + ->will($this->returnCallback(function($str) { + return $str; + })); $expected = new DataResponse( [ @@ -215,10 +217,11 @@ class ThemingControllerTest extends TestCase { ->with('upload-login-background') ->willReturn(null); $this->l10n - ->expects($this->once()) + ->expects($this->any()) ->method('t') - ->with('No file uploaded') - ->willReturn('No file uploaded'); + ->will($this->returnCallback(function($str) { + return $str; + })); $expected = new DataResponse( [ @@ -282,6 +285,7 @@ class ThemingControllerTest extends TestCase { 'tmp_name' => $tmpLogo, 'type' => 'text/svg', 'name' => 'logo.svg', + 'error' => 0, ]); $this->request ->expects($this->at(2)) @@ -289,10 +293,11 @@ class ThemingControllerTest extends TestCase { ->with('upload-login-background') ->willReturn(null); $this->l10n - ->expects($this->once()) + ->expects($this->any()) ->method('t') - ->with('Saved') - ->willReturn('Saved'); + ->will($this->returnCallback(function($str) { + return $str; + })); $file = $this->createMock(ISimpleFile::class); @@ -357,12 +362,14 @@ class ThemingControllerTest extends TestCase { 'tmp_name' => $tmpLogo, 'type' => 'text/svg', 'name' => 'logo.svg', + 'error' => 0, ]); $this->l10n - ->expects($this->once()) + ->expects($this->any()) ->method('t') - ->with('Saved') - ->willReturn('Saved'); + ->will($this->returnCallback(function($str) { + return $str; + })); $file = $this->createMock(ISimpleFile::class); $folder = $this->createMock(ISimpleFolder::class); @@ -425,12 +432,14 @@ class ThemingControllerTest extends TestCase { 'tmp_name' => $tmpLogo, 'type' => 'text/svg', 'name' => 'logo.svg', + 'error' => 0, ]); $this->l10n - ->expects($this->once()) + ->expects($this->any()) ->method('t') - ->with('Unsupported image type') - ->willReturn('Unsupported image type'); + ->will($this->returnCallback(function($str) { + return $str; + })); $folder = $this->createMock(ISimpleFolder::class); $this->appData From c0d104087b28316281bcda65262e03856b89bed7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 20 Sep 2017 13:33:42 +0200 Subject: [PATCH 2/4] Theming: Check valid image format also for logo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/theming/lib/Controller/ThemingController.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index e73fc16b20..e6aa3a380b 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -248,6 +248,18 @@ class ThemingController extends Controller { if (!empty($newLogo)) { $target = $folder->newFile('logo'); + $supportedFormats = ['image/jpeg', 'image/png', 'image/gif', 'image/svg+xml', 'text/svg']; + if (!in_array($newLogo['type'], $supportedFormats)) { + return new DataResponse( + [ + 'data' => [ + 'message' => $this->l10n->t('Unsupported image type'), + ], + 'status' => 'failure', + ], + Http::STATUS_UNPROCESSABLE_ENTITY + ); + } $target->putContent(file_get_contents($newLogo['tmp_name'], 'r')); $this->themingDefaults->set('logoMime', $newLogo['type']); $name = $newLogo['name']; From 9651c1abffa0134b11b7637de76ceb4e3ffae79e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 21 Sep 2017 11:35:40 +0200 Subject: [PATCH 3/4] Theming: Fix message/loading display on errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/theming/css/settings-admin.css | 1 + apps/theming/js/settings-admin.js | 2 ++ 2 files changed, 3 insertions(+) diff --git a/apps/theming/css/settings-admin.css b/apps/theming/css/settings-admin.css index b32ed189ce..7270ec59b8 100644 --- a/apps/theming/css/settings-admin.css +++ b/apps/theming/css/settings-admin.css @@ -78,6 +78,7 @@ form.uploadButton { #theming_settings_msg { vertical-align: middle; + border-radius: 3px; } #theming-preview-logo { diff --git a/apps/theming/js/settings-admin.js b/apps/theming/js/settings-admin.js index d9e66284d1..44a799a19b 100644 --- a/apps/theming/js/settings-admin.js +++ b/apps/theming/js/settings-admin.js @@ -141,6 +141,7 @@ $(document).ready(function () { fail: function (e, response){ OC.msg.finishedError('#theming_settings_msg', response._response.jqXHR.responseJSON.data.message); $('label#uploadlogo').addClass('icon-upload').removeClass('icon-loading-small'); + $('#theming_settings_loading').hide(); } }; var uploadParamsLogin = { @@ -159,6 +160,7 @@ $(document).ready(function () { fail: function (e, response){ $('label#upload-login-background').removeClass('icon-loading-small').addClass('icon-upload'); OC.msg.finishedError('#theming_settings_msg', response._response.jqXHR.responseJSON.data.message); + $('#theming_settings_loading').hide(); } }; From f15e85c4f50b6c9e77742e15f8291a8628a28ef7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 21 Sep 2017 11:36:27 +0200 Subject: [PATCH 4/4] Theming: Add tests for mimetype and upload error checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../lib/Controller/ThemingController.php | 23 +-- .../Controller/ThemingControllerTest.php | 162 +++++++++++++++++- 2 files changed, 170 insertions(+), 15 deletions(-) diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index e6aa3a380b..ccc2634ec1 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -208,16 +208,16 @@ class ThemingController extends Controller { $newLogo = $this->request->getUploadedFile('uploadlogo'); $newBackgroundLogo = $this->request->getUploadedFile('upload-login-background'); $error = null; - $phpFileUploadErrors = array( - 0 => $this->l10n->t('There is no error, the file uploaded with success'), - 1 => $this->l10n->t('The uploaded file exceeds the upload_max_filesize directive in php.ini'), - 2 => $this->l10n->t('The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form'), - 3 => $this->l10n->t('The uploaded file was only partially uploaded'), - 4 => $this->l10n->t('No file was uploaded'), - 6 => $this->l10n->t('Missing a temporary folder'), - 7 => $this->l10n->t('Failed to write file to disk.'), - 8 => $this->l10n->t('A PHP extension stopped the file upload.'), - ); + $phpFileUploadErrors = [ + UPLOAD_ERR_OK => $this->l10n->t('There is no error, the file uploaded with success'), + UPLOAD_ERR_INI_SIZE => $this->l10n->t('The uploaded file exceeds the upload_max_filesize directive in php.ini'), + UPLOAD_ERR_FORM_SIZE => $this->l10n->t('The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form'), + UPLOAD_ERR_PARTIAL => $this->l10n->t('The uploaded file was only partially uploaded'), + UPLOAD_ERR_NO_FILE => $this->l10n->t('No file was uploaded'), + UPLOAD_ERR_NO_TMP_DIR => $this->l10n->t('Missing a temporary folder'), + UPLOAD_ERR_CANT_WRITE => $this->l10n->t('Failed to write file to disk.'), + UPLOAD_ERR_EXTENSION => $this->l10n->t('A PHP extension stopped the file upload.'), + ]; if (empty($newLogo) && empty($newBackgroundLogo)) { $error = $this->l10n->t('No file uploaded'); } @@ -233,7 +233,8 @@ class ThemingController extends Controller { [ 'data' => [ 'message' => $error - ] + ], + 'status' => 'failure', ], Http::STATUS_UNPROCESSABLE_ENTITY ); diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 96a742cfa3..e964e886e5 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -229,6 +229,56 @@ class ThemingControllerTest extends TestCase { [ 'message' => 'No file uploaded', ], + 'status' => 'failure', + ], + Http::STATUS_UNPROCESSABLE_ENTITY + ); + + $this->assertEquals($expected, $this->themingController->updateLogo()); + } + + public function testUpdateLogoInvalidMimeType() { + $this->request + ->expects($this->at(0)) + ->method('getParam') + ->with('backgroundColor') + ->willReturn(false); + $this->request + ->expects($this->at(1)) + ->method('getUploadedFile') + ->with('uploadlogo') + ->willReturn([ + 'tmp_name' => 'logo.pdf', + 'type' => 'application/pdf', + 'name' => 'logo.pdf', + 'error' => 0, + ]); + $this->request + ->expects($this->at(2)) + ->method('getUploadedFile') + ->with('upload-login-background') + ->willReturn(null); + $this->l10n + ->expects($this->any()) + ->method('t') + ->will($this->returnCallback(function($str) { + return $str; + })); + + $folder = $this->createMock(ISimpleFolder::class); + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('images') + ->willReturn($folder); + + $expected = new DataResponse( + [ + 'data' => + [ + 'message' => 'Unsupported image type', + ], + 'status' => 'failure' ], Http::STATUS_UNPROCESSABLE_ENTITY ); @@ -261,13 +311,17 @@ class ThemingControllerTest extends TestCase { public function dataUpdateImages() { return [ - [false], - [true] + ['image/jpeg', false], + ['image/jpeg', true], + ['image/gif'], + ['image/png'], + ['image/svg+xml'], + ['text/svg'], ]; } /** @dataProvider dataUpdateImages */ - public function testUpdateLogoNormalLogoUpload($folderExists) { + public function testUpdateLogoNormalLogoUpload($mimeType, $folderExists=true) { $tmpLogo = \OC::$server->getTempManager()->getTemporaryFolder() . '/logo.svg'; $destination = \OC::$server->getTempManager()->getTemporaryFolder(); @@ -283,7 +337,7 @@ class ThemingControllerTest extends TestCase { ->with('uploadlogo') ->willReturn([ 'tmp_name' => $tmpLogo, - 'type' => 'text/svg', + 'type' => $mimeType, 'name' => 'logo.svg', 'error' => 0, ]); @@ -461,6 +515,106 @@ class ThemingControllerTest extends TestCase { $this->assertEquals($expected, $this->themingController->updateLogo()); } + public function dataPhpUploadErrors() { + return [ + [UPLOAD_ERR_INI_SIZE, 'The uploaded file exceeds the upload_max_filesize directive in php.ini'], + [UPLOAD_ERR_FORM_SIZE, 'The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form'], + [UPLOAD_ERR_PARTIAL, 'The uploaded file was only partially uploaded'], + [UPLOAD_ERR_NO_FILE, 'No file was uploaded'], + [UPLOAD_ERR_NO_TMP_DIR, 'Missing a temporary folder'], + [UPLOAD_ERR_CANT_WRITE, 'Failed to write file to disk.'], + [UPLOAD_ERR_EXTENSION, 'A PHP extension stopped the file upload.'], + ]; + } + + /** + * @dataProvider dataPhpUploadErrors + */ + public function testUpdateLogoLoginScreenUploadWithInvalidImageUpload($error, $expectedErrorMessage) { + $this->request + ->expects($this->at(0)) + ->method('getParam') + ->with('backgroundColor') + ->willReturn(false); + $this->request + ->expects($this->at(1)) + ->method('getUploadedFile') + ->with('uploadlogo') + ->willReturn(null); + $this->request + ->expects($this->at(2)) + ->method('getUploadedFile') + ->with('upload-login-background') + ->willReturn([ + 'tmp_name' => '', + 'type' => 'text/svg', + 'name' => 'logo.svg', + 'error' => $error, + ]); + $this->l10n + ->expects($this->any()) + ->method('t') + ->will($this->returnCallback(function($str) { + return $str; + })); + + $expected = new DataResponse( + [ + 'data' => + [ + 'message' => $expectedErrorMessage, + ], + 'status' => 'failure' + ], + Http::STATUS_UNPROCESSABLE_ENTITY + ); + $this->assertEquals($expected, $this->themingController->updateLogo()); + } + + /** + * @dataProvider dataPhpUploadErrors + */ + public function testUpdateLogoUploadWithInvalidImageUpload($error, $expectedErrorMessage) { + $this->request + ->expects($this->at(0)) + ->method('getParam') + ->with('backgroundColor') + ->willReturn(false); + $this->request + ->expects($this->at(1)) + ->method('getUploadedFile') + ->with('uploadlogo') + ->willReturn([ + 'tmp_name' => '', + 'type' => 'text/svg', + 'name' => 'logo.svg', + 'error' => $error, + ]); + $this->request + ->expects($this->at(2)) + ->method('getUploadedFile') + ->with('upload-login-background') + ->willReturn(null); + $this->l10n + ->expects($this->any()) + ->method('t') + ->will($this->returnCallback(function($str) { + return $str; + })); + + $expected = new DataResponse( + [ + 'data' => + [ + 'message' => $expectedErrorMessage + ], + 'status' => 'failure' + ], + Http::STATUS_UNPROCESSABLE_ENTITY + ); + $this->assertEquals($expected, $this->themingController->updateLogo()); + } + public function testUndo() { $this->l10n ->expects($this->once())