Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use renameSync instead of mv #8706

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

beyondkmp
Copy link
Collaborator

@beyondkmp beyondkmp commented Nov 23, 2024

fix #8698

Copy link

changeset-bot bot commented Nov 23, 2024

🦋 Changeset detected

Latest commit: d790230

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
electron-updater Patch

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

Copy link

@imconnorngl imconnorngl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We desperately need this fix. Our Linux users are having to manually move the App images our auto updater downloads

@beyondkmp beyondkmp marked this pull request as draft November 24, 2024 01:40
@@ -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)
Copy link
Collaborator

@mmaietta mmaietta Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Get the installer path, ensuring spaces are escaped on Linux
// 1. Check if downloadedUpdateHelper is not null
// 2. Check if downloadedUpdateHelper.file is not null
// 3. If both checks pass:
// a. If the platform is Linux, replace spaces with '\ ' for shell compatibility
// b. If the platform is not Linux, use the original path
// 4. If any check fails, set installerPath to null
const installerPath =
downloadedUpdateHelper && downloadedUpdateHelper.file ? (process.platform === "linux" ? downloadedUpdateHelper.file.replace(/ /g, "\\ ") : downloadedUpdateHelper.file) : null

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const cmd = ["pacman", "-U", "--noconfirm", options.installerPath]
this.spawnSyncLog(sudo, [`${wrapper}/bin/bash`, "-c", `'${cmd.join(" ")}'${wrapper}`])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linux AppImage update unable to move downloaded file
4 participants