Change theme uploads to move & delete at end

- Currently theme uploads delete the existing theme before copying the new files into place
- If something goes wrong with the delete action, you will end up in a bad state
   - Some or all of the files may be deleted, but now Ghost won't try to put the new theme in place, instead returning an error
   - This leaves you with an invalid active theme and a broken site
- Unlike delete, move is a one-hit operation that succeeds or fails, there moving a theme is safer than deleting
- This updated code moves the old theme to a folder with the name [theme-name]-[uuid] before copying the new theme into place
- Even if this fails, the files should not be gone
- There's a cleanup operation to remove the theme backup at the end, but we don't care too much if this fails

core/frontend/services/themes/ThemeStorage.js

@@ -57,8 +57,26 @@ class ThemeStorage extends LocalFileStorage {
         };
     }
 
+    /**
+     * Rename a file / folder
+     *
+     *
+     * @param String fileName
+     */
+    rename(srcName, destName) {
+        let src = path.join(this.getTargetDir(), srcName);
+        let dest = path.join(this.getTargetDir(), destName);
+
+        return fs.move(src, dest);
+    }
+
+    /**
+     * Rename a file / folder
+     *
+     * @param String backupName
+     */
     delete(fileName) {
-        return fs.remove(path.join(this.storagePath, fileName));
+        return fs.remove(path.join(this.getTargetDir(), fileName));
     }
 }
 

core/frontend/services/themes/storage.js

@@ -12,6 +12,7 @@ const {i18n} = require('../../../server/lib/common');
 const logging = require('../../../shared/logging');
 const errors = require('@tryghost/errors');
 const debug = require('ghost-ignition').debug('api:themes');
+const ObjectID = require('bson-objectid');
 
 let themeStorage;
 
@@ -37,6 +38,7 @@ module.exports = {
     },
     setFromZip: (zip) => {
         const shortName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]);
+        const backupName = `${shortName}_${ObjectID()}`;
 
         // check if zip name is casper.zip
         if (zip.name === 'casper.zip') {
@@ -54,9 +56,9 @@ module.exports = {
                 return getStorage().exists(shortName);
             })
             .then((themeExists) => {
-                // CASE: delete existing theme
+                // CASE: move the existing theme to a backup folder
                 if (themeExists) {
-                    return getStorage().delete(shortName);
+                    return getStorage().rename(shortName, backupName);
                 }
             })
             .then(() => {
@@ -86,14 +88,20 @@ module.exports = {
             })
             .finally(() => {
                 // @TODO: we should probably do this as part of saving the theme
-                // CASE: remove extracted dir from gscan
-                // happens in background
+                // CASE: remove extracted dir from gscan happens in background
                 if (checkedTheme) {
                     fs.remove(checkedTheme.path)
                         .catch((err) => {
                             logging.error(new errors.GhostError({err: err}));
                         });
                 }
+
+                // CASE: remove the backup we created earlier
+                getStorage()
+                    .delete(backupName)
+                    .catch((err) => {
+                        logging.error(new errors.GhostError({err: err}));
+                    });
             });
     },
     destroy: function (themeName) {

test/api-acceptance/admin/themes_spec.js

@@ -118,20 +118,11 @@ describe('Themes API', function () {
                         tmpFolderContents.splice(index, 1);
                     }
                 });
-                tmpFolderContents.should.be.an.Array().with.lengthOf(10);
 
-                tmpFolderContents.should.eql([
-                    'broken-theme',
-                    'casper',
-                    'casper-1.4',
-                    'casper.zip',
-                    'invalid.zip',
-                    'test-theme',
-                    'test-theme-channels',
-                    'valid',
-                    'valid.zip',
-                    'warnings.zip'
-                ]);
+                // Note: at this point, the tmpFolder can legitimately still contain a valid_34324324 backup
+                // As it is deleted asynchronously
+                tmpFolderContents.should.containEql('valid');
+                tmpFolderContents.should.containEql('valid.zip');
 
                 // Check the Themes API returns the correct result
                 return ownerRequest
@@ -157,6 +148,16 @@ describe('Themes API', function () {
                 should.exist(addedTheme);
                 localUtils.API.checkResponse(addedTheme, 'theme');
                 addedTheme.active.should.be.false();
+
+                // Note: at this point, the API should not return a valid_34324324 backup folder as a theme
+                _.map(jsonResponse.themes, 'name').should.eql([
+                    'broken-theme',
+                    'casper',
+                    'casper-1.4',
+                    'test-theme',
+                    'test-theme-channels',
+                    'valid'
+                ]);
             });
     });