From 0db49b76fa57acd02152203fc39fd76e163e9181 Mon Sep 17 00:00:00 2001 From: boutell Date: Tue, 23 Jan 2024 16:02:10 -0500 Subject: [PATCH] fix oversights re: cleanup route --- CHANGELOG.md | 6 ++++ i18n/en.json | 2 +- lib/methods/import.js | 30 +++++++++++++++---- test/index.js | 23 ++++++++------ ui/apos/apps/index.js | 9 ++---- .../components/AposDuplicateImportModal.vue | 11 +++---- 6 files changed, 53 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e70b039..b33d964d 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/i18n/en.json b/i18n/en.json index 0b796520..a9c3b571 100644 --- a/i18n/en.json +++ b/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", diff --git a/lib/methods/import.js b/lib/methods/import.js index 72ff5584..0a06c6ea 100644 --- a/lib/methods/import.js +++ b/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); } + }; }; diff --git a/test/index.js b/test/index.js index 14b70899..8552a72a 100644 --- a/test/index.js +++ b/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,9 +703,14 @@ 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) } }); @@ -713,7 +718,7 @@ describe('@apostrophecms/import-export', function () { 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: [ diff --git a/ui/apos/apps/index.js b/ui/apos/apps/index.js index df4fe004..b30ed73d 100644 --- a/ui/apos/apps/index.js +++ b/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' }); } } diff --git a/ui/apos/components/AposDuplicateImportModal.vue b/ui/apos/components/AposDuplicateImportModal.vue index 1acbb4b2..189b3d17 100644 --- a/ui/apos/components/AposDuplicateImportModal.vue +++ b/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 }