Merge pull request #1 from apostrophecms/pro-5509

fix oversights re: cleanup route

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## UNRELEASED
 
+### Security
+
+* Fixed a security issue that allowed a correctly crafted
+HTTP request to delete arbitrary files and folders, subject to the permissions with which the Node.js
+process was run. No user account was required to exploit this issue. All users of this module should immediately run `npm update @apostrophecms/import-export` and deploy the latest version of this module. The module has been carefully audited for similar issues and best practices have been put in place to prevent any similar issue in future.
+
 ### Changes
 
 * Prefix routes and events to avoid conflicts with the old [`@apostrophecms/piece-type-importer`](https://github.com/apostrophecms/piece-type-importer) and [`@apostrophecms/piece-type-exporter`](https://github.com/apostrophecms/piece-type-exporter) modules.

i18n/en.json

@@ -15,7 +15,7 @@
   "exported": "Exported {{ count }} {{ type }}",
   "exporting": "Exporting {{ type }}...",
   "import": "Import {{ type }}",
-  "importCleanFailed": "The cleaning of the imported file on server failed: {{ exportPath }}",
+  "importCleanFailed": "The cleaning of the imported file on the server failed.",
   "importWithCurrentLocaleHeading": "Different locale detected",
   "importWithCurrentLocaleDescription": "This file was exported from the \"{{ docsLocale }}\" locale. Are you sure you want to import it into the \"{{ currentLocale }}\" locale?",
   "importDuplicateContinue": "Continue Import",

lib/methods/import.js

@@ -11,7 +11,7 @@ module.exports = self => {
       }
 
       const overrideLocale = self.apos.launder.boolean(req.body.overrideLocale);
-      let exportPath = self.apos.launder.string(req.body.exportPath);
+      let exportPath = await self.getExportPathById(self.apos.launder.string(req.body.exportPathId));
 
       let docs;
       let attachmentsInfo;
@@ -42,7 +42,7 @@ module.exports = self => {
               name: 'import-export-import-locale-differs',
               data: {
                 moduleName,
-                exportPath,
+                exportPathId: await self.getExportPathId(exportPath),
                 content: {
                   heading: req.t('aposImportExport:importWithCurrentLocaleHeading'),
                   description: req.t('aposImportExport:importWithCurrentLocaleDescription', {
@@ -124,7 +124,7 @@ module.exports = self => {
         duplicatedDocs,
         importedAttachments,
         type: moduleName,
-        exportPath,
+        exportPathId: await self.getExportPathId(exportPath),
         jobId,
         notificationId
       };
@@ -192,6 +192,7 @@ module.exports = self => {
 
       try {
         const exportPath = await format.input(file.path);
+        await self.setExportPathId(exportPath);
 
         return {
           ...await self.getFilesData(exportPath),
@@ -441,7 +442,7 @@ module.exports = self => {
 
     async overrideDuplicates(req) {
       const overrideLocale = self.apos.launder.boolean(req.body.overrideLocale);
-      const exportPath = self.apos.launder.string(req.body.exportPath);
+      const exportPath = await self.getExportPathById(self.apos.launder.string(req.body.exportPathId));
       const docIds = self.apos.launder.strings(req.body.docIds);
       const jobId = self.apos.launder.string(req.body.jobId);
       const importedAttachments = self.apos.launder.strings(req.body.importedAttachments);
@@ -514,7 +515,10 @@ module.exports = self => {
     },
 
     async cleanExport(req) {
-      const exportPath = self.apos.launder.string(req.body.exportPath);
+      const exportPath = await self.getExportPathById(self.apos.launder.string(req.body.exportPathId));
+      if (!exportPath) {
+        throw self.apos.error.invalid('no such export path');
+      }
       const jobId = self.apos.launder.string(req.body.jobId);
       const notificationId = self.apos.launder.string(req.body.notificationId);
 
@@ -528,6 +532,22 @@ module.exports = self => {
       }
 
       await self.cleanFile(exportPath);
+    },
+
+    async setExportPathId(path) {
+      const id = self.apos.util.generateId();
+      await self.apos.cache.set('exportPaths', id, path, 86400);
+      await self.apos.cache.set('exportPathIds', path, id, 86400);
+      return id;
+    },
+
+    async getExportPathById(id) {
+      return self.apos.cache.get('exportPaths', id);
+    },
+
+    async getExportPathId(path) {
+      return self.apos.cache.get('exportPathIds', path);
     }
+
   };
 };

test/index.js

@@ -18,7 +18,7 @@ describe('@apostrophecms/import-export', function () {
   let pageTgzPath;
   let cleanFile;
 
-  this.timeout(t.timeout);
+  this.timeout(60000);
 
   after(async function() {
     await cleanData([ attachmentPath, exportsPath ]);
@@ -288,7 +288,7 @@ describe('@apostrophecms/import-export', function () {
     const {
       duplicatedDocs,
       importedAttachments,
-      exportPath,
+      exportPathId,
       jobId,
       notificationId
     } = await importExportManager.import(req);
@@ -310,7 +310,7 @@ describe('@apostrophecms/import-export', function () {
     req.body = {
       docIds: duplicatedDocs.map(({ aposDocId }) => aposDocId),
       importedAttachments,
-      exportPath,
+      exportPathId,
       jobId,
       notificationId
     };
@@ -413,7 +413,7 @@ describe('@apostrophecms/import-export', function () {
     const {
       duplicatedDocs,
       importedAttachments,
-      exportPath,
+      exportPathId,
       jobId,
       notificationId
     } = await importExportManager.import(req);
@@ -438,7 +438,7 @@ describe('@apostrophecms/import-export', function () {
     req.body = {
       docIds: duplicatedDocs.map(({ aposDocId }) => aposDocId),
       importedAttachments,
-      exportPath,
+      exportPathId,
       jobId,
       notificationId
     };
@@ -502,7 +502,7 @@ describe('@apostrophecms/import-export', function () {
     const {
       duplicatedDocs,
       importedAttachments,
-      exportPath,
+      exportPathId,
       jobId,
       notificationId
     } = await importExportManager.import(req);
@@ -529,7 +529,7 @@ describe('@apostrophecms/import-export', function () {
         .filter(({ type }) => type !== '@apostrophecms/image')
         .map(({ aposDocId }) => aposDocId),
       importedAttachments,
-      exportPath,
+      exportPathId,
       jobId,
       notificationId
     };
@@ -703,17 +703,22 @@ describe('@apostrophecms/import-export', function () {
     });
 
     it('should import pieces with related documents from the extracted export path when provided', async function() {
+      const expectedPath = '/custom/extracted-export-path';
+      // Since we are mocking this and not really uploading a file, we have to
+      // manually call setExportPathId to establish a mapping to a safe
+      // unique identifier to share with the "browser"
+      await importExportManager.setExportPathId(expectedPath);
       const req = apos.task.getReq({
         body: {
-          exportPath: '/custom/extracted-export-path'
+          exportPathId: await importExportManager.getExportPathId(expectedPath)
         }
       });
 
       apos.modules['@apostrophecms/import-export'].readExportFile = async () => {
         throw new Error('should not have been called');
       };
       apos.modules['@apostrophecms/import-export'].getFilesData = async exportPath => {
-        assert.equal(exportPath, '/custom/extracted-export-path');
+        assert.equal(exportPath, expectedPath);
 
         return {
           docs: [

ui/apos/apps/index.js

@@ -36,7 +36,7 @@ export default () => {
         await apos.http.post(`${moduleAction}/import-export-import`, {
           body: {
             overrideLocale: true,
-            exportPath: event.exportPath
+            exportPathId: event.exportPathId
           }
         });
       } catch (error) {
@@ -53,15 +53,12 @@ export default () => {
     try {
       await apos.http.post('/api/v1/@apostrophecms/import-export/clean-export', {
         body: {
-          exportPath: event.exportPath
+          exportPathId: event.exportPathId
         }
       });
     } catch (error) {
       apos.notify('aposImportExport:importCleanFailed', {
-        type: 'warning',
-        interpolate: {
-          exportPath: event.exportPath
-        }
+        type: 'warning'
       });
     }
   }

ui/apos/components/AposDuplicateImportModal.vue

@@ -119,7 +119,7 @@ export default {
       type: Array,
       required: true
     },
-    exportPath: {
+    exportPathId: {
       type: String,
       required: true
     },
@@ -192,17 +192,14 @@ export default {
       try {
         await apos.http.post('/api/v1/@apostrophecms/import-export/clean-export', {
           body: {
-            exportPath: this.exportPath,
+            exportPathId: this.exportPathId,
             jobId: this.jobId,
             notificationId: this.notificationId
           }
         });
       } catch (error) {
         apos.notify('aposImportExport:importCleanFailed', {
-          type: 'warning',
-          interpolate: {
-            exportPath: this.exportPath
-          }
+          type: 'warning'
         });
       } finally {
         apos.bus.$emit('import-export-import-ended');
@@ -217,7 +214,7 @@ export default {
         body: {
           docIds: this.checked,
           importedAttachments: this.importedAttachments,
-          exportPath: this.exportPath,
+          exportPathId: this.exportPathId,
           jobId: this.jobId,
           overrideLocale: this.overrideLocale
         }