fix: use renameSync instead of mv #8706
Conversation
🦋 Changeset detectedLatest commit: 43213ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -93,7 +93,7 @@ export class AppImageUpdater extends BaseUpdater { | |||
| destination = path.join(path.dirname(appImageFile), path.basename(options.installerPath)) | |||
| } | |||
|
|
|||
| execFileSync("mv", ["-f", options.installerPath, destination]) | |||
| renameSync(options.installerPath, destination) | |||
There was a problem hiding this comment.
This doesn't seem like it would work due to installerPath being a space-escaped string for piping to the cmd shell. Right?
If the platform is Linux, replace spaces with '\ ' for shell compatibility
electron-builder/packages/electron-updater/src/BaseUpdater.ts
Lines 52 to 60 in 6a6bed4
There was a problem hiding this comment.
Yes, I found that this was the cause, so I changed it to draft.
I don't think it's handled well here - it shouldn't directly add /. Instead, it should be handled by each deb/rpm/pacman separately.
These linux install commands use string concatenation, so if malicious code is added to the name, it could lead to serious security vulnerabilities.
electron-builder/packages/electron-updater/src/PacmanUpdater.ts
Lines 34 to 35 in 6a6bed4
| const cmd = ["dpkg", "-i", options.installerPath, "||", "apt-get", "install", "-f", "-y"] | ||
| this.spawnSyncLog(sudo, [`${wrapper}/bin/bash`, "-c", `'${cmd.join(" ")}'${wrapper}`]) | ||
| this.spawnSyncLog(sudo, cmd) |
There was a problem hiding this comment.
Just to double check, was this tested? (Same for pacman and Rpm)
Pretty sure this will fail as my previous project required piping it to bash
| const cmd = ["dpkg", "-i", options.installerPath, "||", "apt-get", "install", "-f", "-y"] | ||
| this.spawnSyncLog(sudo, [`${wrapper}/bin/bash`, "-c", `'${cmd.join(" ")}'${wrapper}`]) | ||
| this.spawnSyncLog(sudo, cmd) |
There was a problem hiding this comment.
Imo, this way of installing local .deb in 2 steps with dpkg and apt-get install -f can be updated. Apt is now able to install local .deb and depends in a single command if the package argument is an absolute path to a .deb.
You may now do this.spawnSyncLog("sudo", ["apt-get", "install", "-y", options.installerPath]). This will avoid you to pipe commands, or to split it into 2 commands and checking return code.
fix #8698