pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.24k stars 137 forks source link

electron-builder: Add '--no-sandbox' launch arg for Linux build targets #1029

Closed DeeDeeG closed 3 months ago

DeeDeeG commented 3 months ago

Fixes Dev Tools not being able to be opened, among other subtle issue.

This is a frequently reported bug, and we try to apply this workaround elsewhere, it's just that the .desktop file that electron-builder generates skips the workaround we have applie e.g. in the /us/bin/pulsar launcher script.

Identify the Bug

Appears to apply to:

May apply to:

Description of the Change

Specify the --no-sandbox flag in our linux config for electron-builder.

This adds a --no-sandbox flag to the .desktop file installed by the .deb installer, allowing Dev Tools to work as expected with our older (pre-13) version of Electron on newer Linux distros when launching Pulsar from the OS's graphical app launcher interface.

This is the same workaround we already do in our launcher bash script /usr/bin/pulsar. Just applying the workaround here too, so we are consistently applying the workaround for all the ways a user has available to launch Pulsar (I hope this gets all of them), at least for our officially supported releases. (This probably doesn't affect the community-supported FlatPak.)

Alternate Designs

Possible Drawbacks

Having no sandboxing in place is presumably a bona-fide security downgrade.

However, Atom's security posture, which we have inherited, has relied mostly on trusting package authors -- packages are able to do essentially arbitrary code execution by way of the NodeJS and Atom APIs. "Sandboxing" against untrusted code seems to me to be patching a whole in the wall while the gate is wide open. A good idea, but also not a comprehensive solution for the threat model of Atom/Pulsar, IMO. Meanwhile, and for what it's worth, browsing arbitrary websites is not particularly easy in Atom/Pulsar, if it can indeed be done. Thankfully, most packages don't appear to do stuff over the network, once they've been installed. But there is no guarantee of this, to my knowledge.

Regardless of whether that assessment of the threat model is perfect or not, we have users who ask for the Dev Tools to be available, and this is the workaround needed to do it.

NOTE: We have applied this workaround for a while now via the bash launcher script /usr/bin/pulsar. This is just being consistent by applying it to the GUI launcher as well.

Verification Process

Ran CI with this change on my personal fork of Pulsar. Using the binaries built with this change, on a VM running a Ubuntu 24.04 Live session, after installing the .deb, Pulsar was launchable from the OS's GUI app launcher and Dev Tools worked fine.

(Without this change, there were errors, and the app wouldn't launch at all from the OS's GUI launcher, if I recall correctly. But running pulsar from the terminal worked fine, with or without this PR's change, as pulsar in the terminal calls the bash launcher script /usr/bin/pulsar, which has the --no-sandbox flag applied already, from well before this PR.)

Release Notes

Fixed --no-sandbox flag not being applied to the .desktop launcher on Linux (Fixes Dev Tools)

DeeDeeG commented 3 months ago

Alright, I can confirm from some further testing this is working well for Fedora as well. More precisely:

In a VirtualBox VM with a Fedora Workstation 40 x64 Live guest, I tried the .rpm from the latest x64 Rolling release (1.118.2024061815) and from my fork build here: https://github.com/DeeDeeG/pulsar/releases/tag/fix-Linux-desktop-file-tag1.

After installing the .rpm with changes matching this PR, and launching Pulsar from the GNOME "Activities" view/app drawer, I can confirm that Pulsar was launched with --no-sandbox in the arguments (as verified with htop while filtering the process name for the string "pulsar"). It was able to open Dev Tools, and consistently launched successfully. ✅

The .rpm from our latest Rolling release without this PR's changes, when similarly launched from GNOME Activities/app drawer view, Pulsar would not consistently launch (some sort of error?). When it did launch, Dev Tools were not openable. Trying to open Dev Tools silently does nothing. ❌

In both cases with or without this PR's changes, running pulsar form the terminal worked fine. Pulsar consistently launched successfully, and Dev Tools could be opened. This is expected. ✅

So, that seems to confirm that the fix is valid in the .rpm/for Fedora and presumably for RHEL-alike distros as well. 👍

(Notes on what wasn't tested: Apparently Suse can install .rpms as well, I think? Not sure if it works there, but I don't see why not. Also I only tested on GNOME, since it's the default desktop in both Ubuntu 24.04 and Fedora Workstation 40. But hey, I think .desktop files are an open standard that should be respected by big the major DEs. Right? Right???)

DeeDeeG commented 3 months ago

Was able to test the AppImage. There is no difference from before/after this change.

It would seem there's no provision for adding CLI flags to the underlying app when putting together an AppImage bundle, or else our version of electron-builder doesn't take advantage of it, if there is such a mechanism.

Oh well, at least it's no different and no worse than before for the AppImage crowd, even if it means they still need to apply the flag themselves somehow as the workaround, given what we're shipping. (This is the best state I'm aware of being able to ship the AppImage in at the moment, with regards to this "failure to launch" / "No Dev Tools" issue.)


Tested the AppImages of the same releases/tags as mentioned in my previous comment just above.

In a VirtualBox VM with an Ubuntu 24.04 x64 Live guest, I tried the .AppImage from the latest x64 Rolling release (1.118.2024061815) and from my fork build here: https://github.com/DeeDeeG/pulsar/releases/tag/fix-Linux-desktop-file-tag1.


Testing notes: Need to install libfuse2 or there is an error message about needing FUSE to run the AppImage. There is no --no-sandbox flag applied "out of the box", either with or without this change. The flag can be manually specified from the terminal, such as by running ./Pulsar-1.118.2024062015.AppImage --no-sandbox, and that does work. Same as before this change. All good there 👍.

Without the --no-sandbox flag, the app doesn't launch, with an error shown in the terminal if launching from the terminal:

[4985:0621/222832.334120:FATAL:credentials.cc(123)] Check failed: . : Permission denied (13)
Trace/breakpoint trap (core dumped)

A web search leads to here, indicating this needs either the --no-sandbox flag, or adding an AppArmor exception for the app, or changing to a different kernel (?????) in order to workaround the error: 1, 2, 3, 4, 5, 6.

(There is no hint if launching the AppImage from the GUI, the app just silently/invisibly exits immediately. Although, if I recall correctly, it does briefly show in task managers such as htop for just a moment, before exiting.)

DeeDeeG commented 3 months ago

No notes about the .tar.gz, since, y'know... it's not a launcher! Just a bare executable! Users can install it somewhere and/or add the launcher script to their PATH manually if they want, I guess.

And with that, that's all the four types of launcher/binaries we offer on Linux. All either improved (.deb, .rpm) or left no different than before (and therefore no worse) (.AppImage and .tar.gz). I'm satisfied that makes for an improvement for now.

With the review-approve, I'm merging this. Thanks again!