From 16c3750b154795e06ae0df1d319703dd88674e1e Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Mon, 20 Jun 2016 12:05:33 -0400 Subject: [PATCH] Don't check extensions before the first non compressed extension (#493) Currently, we extract all the extensions from an image path and report back that the image is invalid if *any* of the extensions is not a valida extension, however this can cause trouble with images including information between dots that are not strictly extensions. For example: ``` elementaryos-0.3.2-stable-i386.20151209.iso ``` Etcher will consider `20151209` to be an invalid extension and therefore will prevent such image from being selected. As a way to allow these corner cases but will make use of our enforced check controls, the validation routine will only consider extensions starting from the first non compressed extension. This PR also includes logic to show a nice GUI dialog saying that the image is invalid in order to provide a much better experience than just silently failing. Fixes: https://github.com/resin-io/etcher/issues/492 Signed-off-by: Juan Cruz Viotti --- lib/gui/app.js | 13 ++++- lib/gui/models/supported-formats.js | 66 ++++++++++++---------- lib/gui/os/dialog/services/dialog.js | 26 ++++++--- package.json | 2 +- tests/gui/models/supported-formats.spec.js | 38 ++++++++++--- 5 files changed, 95 insertions(+), 50 deletions(-) diff --git a/lib/gui/app.js b/lib/gui/app.js index e7bda8bf..cd862721 100644 --- a/lib/gui/app.js +++ b/lib/gui/app.js @@ -108,6 +108,14 @@ app.controller('AppController', function( this.tooltipModal = TooltipModalService; this.success = true; + this.handleError = function(error) { + OSDialogService.showError(error); + + // Also throw it so it gets displayed in DevTools + // and its reported by TrackJS. + throw error; + }; + if (UpdateNotifierService.shouldCheckForUpdates()) { AnalyticsService.logEvent('Checking for updates'); @@ -147,7 +155,7 @@ app.controller('AppController', function( OSWindowProgressService.set(state.progress); }); - DriveScannerService.start(2000).on('error', OSDialogService.showError).on('scan', function(drives) { + DriveScannerService.start(2000).on('error', self.handleError).on('scan', function(drives) { // Cover the case where you select a drive, but then eject it. if (self.selection.hasDrive() && !_.find(drives, self.selection.isCurrentDrive)) { @@ -189,6 +197,7 @@ app.controller('AppController', function( this.selectImage = function(image) { if (!SupportedFormatsModel.isSupportedImage(image.path)) { + OSDialogService.showError('Invalid image', `${image.path} is not a supported image type.`); AnalyticsService.logEvent('Invalid image', image); return; } @@ -297,7 +306,7 @@ app.controller('AppController', function( } self.writer.resetState(); - OSDialogService.showError(error); + self.handleError(error); }) .finally(OSWindowProgressService.clear); }; diff --git a/lib/gui/models/supported-formats.js b/lib/gui/models/supported-formats.js index 1545061d..4061a572 100644 --- a/lib/gui/models/supported-formats.js +++ b/lib/gui/models/supported-formats.js @@ -22,6 +22,7 @@ const angular = require('angular'); const _ = require('lodash'); +const path = require('path'); const imageStream = require('etcher-image-stream'); const MODULE_NAME = 'Etcher.Models.SupportedFormats'; const SupportedFormats = angular.module(MODULE_NAME, []); @@ -30,23 +31,22 @@ SupportedFormats.service('SupportedFormatsModel', function() { let self = this; /** - * @summary Check if a file type is a compressed format + * @summary Build an extension list getter from a type * @function * @private * - * @param {Object} fileType - file type - * @returns {Boolean} whether the file type is a compressed format + * @param {String} type - file type + * @returns {Function} extension list getter * * @example - * if (isCompressedFileType({ - * extension: 'zip', - * type: 'compressed' - * })) { - * console.log('This is a compressed file type'); - * } + * const extensions = getExtensionsFromTypeGetter('archive')(); */ - const isCompressedFileType = function(fileType) { - return fileType.type === 'compressed'; + const getExtensionsFromTypeGetter = function(type) { + return function() { + return _.map(_.filter(imageStream.supportedFileTypes, function(fileType) { + return fileType.type === type; + }), 'extension'); + }; }; /** @@ -60,9 +60,7 @@ SupportedFormats.service('SupportedFormatsModel', function() { * console.log('We support the ' + extension + ' compressed file format'); * }); */ - this.getCompressedExtensions = function() { - return _.map(_.filter(imageStream.supportedFileTypes, isCompressedFileType), 'extension'); - }; + this.getCompressedExtensions = getExtensionsFromTypeGetter('compressed'); /** * @summary Get non compressed extensions @@ -75,9 +73,20 @@ SupportedFormats.service('SupportedFormatsModel', function() { * console.log('We support the ' + extension + ' file format'); * }); */ - this.getNonCompressedExtensions = function() { - return _.map(_.reject(imageStream.supportedFileTypes, isCompressedFileType), 'extension'); - }; + this.getNonCompressedExtensions = getExtensionsFromTypeGetter('image'); + + /** + * @summary Get archive extensions + * @function + * @public + * + * @returns {String[]} archive extensions + * + * SupportedFormatsModel.getArchiveExtensions().forEach(function(extension) { + * console.log('We support the ' + extension + ' file format'); + * }); + */ + this.getArchiveExtensions = getExtensionsFromTypeGetter('archive'); /** * @summary Get all supported extensions @@ -107,25 +116,20 @@ SupportedFormats.service('SupportedFormatsModel', function() { * } */ this.isSupportedImage = function(image) { + const extension = path.extname(image).slice(1); - // We roll our own extension detection system instead of - // using `path.extname()` since that function will only - // return the last extension, while we're interested - // to check every possible extension of an image path, - // like `.tar.gz`. + if (_.some([ + _.includes(self.getNonCompressedExtensions(), extension), + _.includes(self.getArchiveExtensions(), extension) + ])) { + return true; + } - const firstDotIndex = _.get(/(\.)[\w\.]+$/.exec(image), 'index'); - - // An image without an extension is not considered a valid image - if (!firstDotIndex) { + if (!_.includes(self.getCompressedExtensions(), extension)) { return false; } - const extensions = image.slice(firstDotIndex + 1).split('.'); - - return _.every(extensions, function(extension) { - return _.includes(self.getAllExtensions(), extension); - }); + return self.isSupportedImage(path.basename(image, `.${extension}`)); }; }); diff --git a/lib/gui/os/dialog/services/dialog.js b/lib/gui/os/dialog/services/dialog.js index 49152727..5c2b9101 100644 --- a/lib/gui/os/dialog/services/dialog.js +++ b/lib/gui/os/dialog/services/dialog.js @@ -81,25 +81,33 @@ module.exports = function($q, SupportedFormatsModel) { * @function * @public * - * @param {Error} error - error + * @param {(Error|String)} error - error + * @param {String} [description] - error description * * @example * OSDialogService.showError(new Error('Foo Bar')); + * + * @example + * OSDialogService.showError(new Error('Foo Bar'), 'A custom description'); + * + * @example + * OSDialogService.showError('Foo Bar', 'An error happened!'); */ - this.showError = function(error) { + this.showError = function(error, description) { error = error || {}; // Try to get as most information as possible about the error // rather than falling back to generic messages right away. - const title = error.message || error.code || 'An error ocurred'; - const message = error.stack || JSON.stringify(error) || ''; + const title = _.attempt(function() { + if (_.isString(error)) { + return error; + } + return error.message || error.code || 'An error ocurred'; + }); + + const message = description || error.stack || JSON.stringify(error) || ''; electron.remote.dialog.showErrorBox(title, message); - - // Also throw it so it gets displayed in DevTools - // and its reported by TrackJS. - throw error; - }; }; diff --git a/package.json b/package.json index 5e76aab5..7e1e1ce1 100644 --- a/package.json +++ b/package.json @@ -60,7 +60,7 @@ "chalk": "^1.1.3", "drivelist": "^3.2.0", "electron-is-running-in-asar": "^1.0.0", - "etcher-image-stream": "^2.0.0", + "etcher-image-stream": "^2.1.0", "etcher-image-write": "^5.0.1", "flexboxgrid": "^6.3.0", "is-elevated": "^1.0.0", diff --git a/tests/gui/models/supported-formats.spec.js b/tests/gui/models/supported-formats.spec.js index 7f1b35b9..b4af5b72 100644 --- a/tests/gui/models/supported-formats.spec.js +++ b/tests/gui/models/supported-formats.spec.js @@ -23,7 +23,7 @@ describe('Browser: SupportedFormats', function() { it('should return the supported compressed extensions', function() { const extensions = SupportedFormatsModel.getCompressedExtensions(); - m.chai.expect(extensions).to.deep.equal([ 'zip', 'gz', 'bz2', 'xz' ]); + m.chai.expect(extensions).to.deep.equal([ 'gz', 'bz2', 'xz' ]); }); }); @@ -37,12 +37,22 @@ describe('Browser: SupportedFormats', function() { }); + describe('.getArchiveExtensions()', function() { + + it('should return the supported archive extensions', function() { + const extensions = SupportedFormatsModel.getArchiveExtensions(); + m.chai.expect(extensions).to.deep.equal([ 'zip' ]); + }); + + }); + describe('.getAllExtensions()', function() { - it('should return the union of .getCompressedExtensions and .getNonCompressedExtensions', function() { + it('should return the union of all compressed, uncompressed, and archive extensions', function() { + const archiveExtensions = SupportedFormatsModel.getArchiveExtensions(); const compressedExtensions = SupportedFormatsModel.getCompressedExtensions(); const nonCompressedExtensions = SupportedFormatsModel.getNonCompressedExtensions(); - const expected = _.union(compressedExtensions, nonCompressedExtensions); + const expected = _.union(archiveExtensions, compressedExtensions, nonCompressedExtensions); const extensions = SupportedFormatsModel.getAllExtensions(); m.chai.expect(extensions).to.deep.equal(expected); }); @@ -62,15 +72,29 @@ describe('Browser: SupportedFormats', function() { }); it('should return true if the extension is included in .getAllExtensions()', function() { - const supportedExtensions = SupportedFormatsModel.getAllExtensions(); - const imagePath = '/path/to/foo.' + _.first(supportedExtensions); + const nonCompressedExtension = _.first(SupportedFormatsModel.getNonCompressedExtensions()); + const imagePath = '/path/to/foo.' + nonCompressedExtension; + const isSupported = SupportedFormatsModel.isSupportedImage(imagePath); + m.chai.expect(isSupported).to.be.true; + }); + + it('should not consider an extension before a non compressed extension', function() { + const nonCompressedExtension = _.first(SupportedFormatsModel.getNonCompressedExtensions()); + const imagePath = '/path/to/foo.1234.' + nonCompressedExtension; const isSupported = SupportedFormatsModel.isSupportedImage(imagePath); m.chai.expect(isSupported).to.be.true; }); it('should return true if the extension is supported and the file name includes dots', function() { - const supportedExtensions = SupportedFormatsModel.getAllExtensions(); - const imagePath = '/path/to/foo.1.2.3-bar.' + _.first(supportedExtensions); + const nonCompressedExtension = _.first(SupportedFormatsModel.getNonCompressedExtensions()); + const imagePath = '/path/to/foo.1.2.3-bar.' + nonCompressedExtension; + const isSupported = SupportedFormatsModel.isSupportedImage(imagePath); + m.chai.expect(isSupported).to.be.true; + }); + + it('should return true if the extension is only a supported archive extension', function() { + const archiveExtension = _.first(SupportedFormatsModel.getArchiveExtensions()); + const imagePath = '/path/to/foo.' + archiveExtension; const isSupported = SupportedFormatsModel.isSupportedImage(imagePath); m.chai.expect(isSupported).to.be.true; });