From 75589099f1d632a3771b24fdc4f6201f70f434a1 Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford <7019101+cwillisf@users.noreply.github.com> Date: Fri, 21 Aug 2020 13:58:35 -0700 Subject: [PATCH 1/2] fix MAS file overwrite by deleting existing file From the code comments: If the file exists the browser will first download to a temp file then rename to the userChosenPath. The MAS sandbox allows accessing userChosenPath but not the temp file, so overwriting fails on MAS. Deleting the file first could be considered risky but works around the sandbox problem. Security bookmarks might work to fix the problem but they're only supported by async showSaveDialog. Since we need to use showSaveDialogSync (see WARNING below) this workaround might be the best option. --- src/main/index.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/main/index.js b/src/main/index.js index fe98e1a..7174ce5 100644 --- a/src/main/index.js +++ b/src/main/index.js @@ -237,6 +237,14 @@ const createMainWindow = () => { } const userChosenPath = dialog.showSaveDialogSync(window, options); if (userChosenPath) { + // If the file exists the browser will first download to a temp file then rename to the userChosenPath. + // The MAS sandbox allows accessing userChosenPath but not the temp file, so overwriting fails on MAS. + // Deleting the file first could be considered risky but works around the sandbox problem. + // Security bookmarks might work to fix the problem but they're only supported by async showSaveDialog. + // Since we need to use showSaveDialogSync (see WARNING below) this workaround might be the best option. + if (fs.existsSync(userChosenPath)) { + fs.unlinkSync(userChosenPath); + } // WARNING: `setSavePath` on this item is only valid during the `will-download` event. Calling the async // version of `showSaveDialog` means the event will finish before we get here, so `setSavePath` will be // ignored. For that reason we need to call `showSaveDialogSync` above. From e9e3f06289bbfb98cf6b88499069ff8b337d755d Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford <7019101+cwillisf@users.noreply.github.com> Date: Mon, 24 Aug 2020 20:20:37 -0700 Subject: [PATCH 2/2] save files more safely (temp then move) --- package-deps.json | 1 + package-lock.json | 6 ++--- package.json | 1 + src/main/index.js | 65 +++++++++++++++++++++++++++++++---------------- 4 files changed, 48 insertions(+), 25 deletions(-) diff --git a/package-deps.json b/package-deps.json index 90ec61e..4d7d90a 100644 --- a/package-deps.json +++ b/package-deps.json @@ -14,6 +14,7 @@ "eslint-config-scratch": "^6.0.0", "eslint-plugin-import": "^2.20.0", "eslint-plugin-react": "^7.20.0", + "fs-extra": "^9.0.1", "mkdirp": "^1.0.4", "nets": "^3.2.0", "rimraf": "^3.0.2", diff --git a/package-lock.json b/package-lock.json index 419c6b7..a6bad58 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7513,9 +7513,9 @@ } }, "fs-extra": { - "version": "9.0.0", - "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-9.0.0.tgz", - "integrity": "sha512-pmEYSk3vYsG/bF651KPUXZ+hvjpgWYw/Gc7W9NFUe3ZVLczKKWIij3IKpOrQcdw4TILtibFslZ0UmR8Vvzig4g==", + "version": "9.0.1", + "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-9.0.1.tgz", + "integrity": "sha512-h2iAoN838FqAFJY2/qVpzFXy+EBxfVE220PalAqQLDVsFOHLJrZvut5puAbCdNv6WJk+B8ihI+k0c7JK5erwqQ==", "dev": true, "requires": { "at-least-node": "^1.0.0", diff --git a/package.json b/package.json index 255f9dd..d732934 100644 --- a/package.json +++ b/package.json @@ -68,6 +68,7 @@ "eslint-plugin-jest": "^22.14.1", "eslint-plugin-react": "^7.20.0", "file-loader": "2.0.0", + "fs-extra": "^9.0.1", "get-float-time-domain-data": "0.1.0", "get-user-media-promise": "1.1.4", "gh-pages": "github:rschamp/gh-pages#publish-branch-to-subfolder", diff --git a/src/main/index.js b/src/main/index.js index 7174ce5..229d4a4 100644 --- a/src/main/index.js +++ b/src/main/index.js @@ -1,5 +1,5 @@ import {BrowserWindow, Menu, app, dialog, ipcMain, systemPreferences} from 'electron'; -import fs from 'fs'; +import fs from 'fs-extra'; import path from 'path'; import {URL} from 'url'; @@ -223,9 +223,9 @@ const createMainWindow = () => { }); const webContents = window.webContents; - webContents.session.on('will-download', (ev, item) => { - const isProjectSave = getIsProjectSave(item); - const itemPath = item.getFilename(); + webContents.session.on('will-download', (willDownloadEvent, downloadItem) => { + const isProjectSave = getIsProjectSave(downloadItem); + const itemPath = downloadItem.getFilename(); const baseName = path.basename(itemPath); const extName = path.extname(baseName); const options = { @@ -236,30 +236,51 @@ const createMainWindow = () => { options.filters = [getFilterForExtension(extNameNoDot)]; } const userChosenPath = dialog.showSaveDialogSync(window, options); + // this will be falsy if the user canceled the save if (userChosenPath) { - // If the file exists the browser will first download to a temp file then rename to the userChosenPath. - // The MAS sandbox allows accessing userChosenPath but not the temp file, so overwriting fails on MAS. - // Deleting the file first could be considered risky but works around the sandbox problem. - // Security bookmarks might work to fix the problem but they're only supported by async showSaveDialog. - // Since we need to use showSaveDialogSync (see WARNING below) this workaround might be the best option. - if (fs.existsSync(userChosenPath)) { - fs.unlinkSync(userChosenPath); - } + const userBaseName = path.basename(userChosenPath); + const tempPath = path.join(app.getPath('temp'), userBaseName); + // WARNING: `setSavePath` on this item is only valid during the `will-download` event. Calling the async // version of `showSaveDialog` means the event will finish before we get here, so `setSavePath` will be // ignored. For that reason we need to call `showSaveDialogSync` above. - item.setSavePath(userChosenPath); - if (isProjectSave) { - const newProjectTitle = path.basename(userChosenPath, extName); - webContents.send('setTitleFromSave', {title: newProjectTitle}); + downloadItem.setSavePath(tempPath); - // "setTitleFromSave" will set the project title but GUI has already reported the telemetry event - // using the old title. This call lets the telemetry client know that the save was actually completed - // and the event should be committed to the event queue with this new title. - telemetry.projectSaveCompleted(newProjectTitle); - } + downloadItem.on('done', async (doneEvent, doneState) => { + try { + if (doneState !== 'completed') { + // The download was canceled or interrupted. Cancel the telemetry event and delete the file. + throw new Error(`save ${doneState}`); // "save cancelled" or "save interrupted" + } + await fs.move(tempPath, userChosenPath, {overwrite: true}); + if (isProjectSave) { + const newProjectTitle = path.basename(userChosenPath, extName); + webContents.send('setTitleFromSave', {title: newProjectTitle}); + + // "setTitleFromSave" will set the project title but GUI has already reported the telemetry + // event using the old title. This call lets the telemetry client know that the save was + // actually completed and the event should be committed to the event queue with this new title. + telemetry.projectSaveCompleted(newProjectTitle); + } + } catch (e) { + if (isProjectSave) { + telemetry.projectSaveCanceled(); + } + // don't clean up until after the message box to allow troubleshooting / recovery + await dialog.showMessageBox(window, { + type: 'error', + message: `Save failed:\n${userChosenPath}`, + detail: e.message + }); + fs.exists(tempPath).then(exists => { + if (exists) { + fs.unlink(tempPath); + } + }); + } + }); } else { - item.cancel(); + downloadItem.cancel(); if (isProjectSave) { telemetry.projectSaveCanceled(); }