Fix security issues, simplify app setup/updates, fix calibrate and some minor issues#446
Fix security issues, simplify app setup/updates, fix calibrate and some minor issues#446base47 wants to merge 14 commits intoactuallymentor:mainfrom
Conversation
… timeouts only, tolerate stderr warnings
|
@actuallymentor ready for review |
| const line = util.format(...messages) + "\n"; | ||
| await fs.appendFile( `${ HOME }/.battery/gui.log`, line, 'utf8' ) |
There was a problem hiding this comment.
This change allows to have a nice output like:
Update details: {
stdout: '☑️ No updates found\n' +
'☑️ The existing battery visudo file is what it should be for version v1.3.2\n',
stderr: ''
}
not only in terminal but in gui.log as well. Version 1.3.2 logs stdout only. Btw, stdout and stderr are logged for errors as well, unless we decide to ignore resolved/rejected value.
| async function set_initial_interface() { | ||
|
|
||
| log( "Starting tray app" ) | ||
| log('\n===\n=== Starting tray app\n===\n') |
There was a problem hiding this comment.
Makes the new app session more visible in the gui.log
| local color=$1 | ||
|
|
||
| # Check whether user can run color changes without password (required for backwards compatibility) | ||
| if sudo -n smc -k ACLC -r &>/dev/null; then |
There was a problem hiding this comment.
This check is not needed anymore. The smc command just reads smc changing nothing
battery.sh
Outdated
| # Temporarily support the 'silent' setting for backward compatibility with older UI versions. | ||
| if [[ "$setting" == "silent" ]]; then | ||
| sudo -n $battery_binary update_silent | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Old GUI version actually works if the new battery.sh is already installed, but it cannot be used to update from current version. It will install the new battery script using current battery executable, but the next time Old GUI starts user will get an error alert. I'm gonna need one more commit to prevent it.
| $battery_binary maintain stop | ||
|
|
||
| # Disable charge blocker if enabled | ||
| $battery_binary adapter on |
There was a problem hiding this comment.
Not needed here, charging is restored below, don't do it twice.
| sudo chmod 755 $binfolder/smc | ||
| sudo chmod +x $binfolder/smc | ||
| echo "[ 3 ] Make sure $binfolder exists and owned by root" | ||
| sudo install -d -m 755 -o root -g wheel "$binfolder" |
There was a problem hiding this comment.
install is just a system utility which does the same job as cp/chown/chmod commands we deleted above and below.
|
|
||
| touch $logfile | ||
| sudo chown $calling_user $logfile | ||
| sudo chmod 755 $logfile |
There was a problem hiding this comment.
Only executables and folders require 755. Others are good with just 644 which is read/write for owner and read for everyone else.
|
|
||
| sudo chown $calling_user $binfolder/battery | ||
| # Fix permissions for 'create_daemon' action | ||
| echo "[ 7 ] Fix ownership and permissions for $(dirname "$launch_agent_plist")" |
There was a problem hiding this comment.
A user once had a root ownership on ~/Library/LaunchAgents folder (#420 (comment)). Not sure how that could happen, but lets make installer capable of fixing it.
setup.sh
Outdated
| echo "[ 6 ] Setting up visudo declarations" | ||
| sudo $batteryfolder/battery.sh visudo $USER | ||
| echo "[ 8 ] Setup visudo configuration" | ||
| sudo $binfolder/battery visudo $calling_user |
There was a problem hiding this comment.
visudo does not need $calling_user argument anymore. Will remove.
| mkdir -p $batteryfolder | ||
| curl -sS -o $batteryfolder/battery.sh https://raw.githubusercontent.com/actuallymentor/battery/main/battery.sh | ||
| echo -n "[ 1 ] Allocating temp folder: " | ||
| tempfolder="$(mktemp -d)" |
There was a problem hiding this comment.
mktemp is a standard system utility for temporary directory allocation, used extensively on macOS. The system owns the directory and will remove it at some point on startup if we don’t remove it ourselves.
aac6db3 to
acb0eac
Compare
|
@actuallymentor You asked to tag you when PR is ready. Now it is. |
Primary goal of this PR is to fix the #443 .
All changes in bash scripts and GUI (including updates from current version 1.3.2 using GUI and Terminal) were tested on Sequoia 15.7.3 and Tahoe 26.2 (which is the latest).
During the update GUI users will be asked to reinstall background executables. Terminal users can just run
battery updateas usual. In both cases user password will be required.First of all let me start with assuring you that this PR is not vibe-coded. Every change I made have a reason, I'm aware of each modification and understand what they do. Now, let’s look at the changes from a bird’s-eye perspective
Security improvements
Battery limiter requires root previleges to set the values of SMC keys which control macbook charging. That means that maintainers have to pay a special attention to app security, specifically to privilage escalation attacks. This PR addresses the following attack scenarios:
Setup/Update functionality in battery.sh
Since the battery background executables are not writable by the user, root privileges are required for updates. To keep updates silent on each startup of the GUI app, this PR adds a passwordless 'update_silent' action to the battery.sh script (via visudo config, same way as we already do for smc). This same action is also used by the 'battery update' action, so updates from the Terminal are also passwordless. Normally, with this PR, a user will have to enter a password twice: during installation of the background binaries and during uninstall.
Users no longer need to use the
battery visudoaction, even though I suspect few of them ever did. Theupdate_silentaction executes battery visudo after each update. Even if no updates are available,update_silentstill invokesbattery visudoand ensures that the ownership and permissions of the battery files are intact. Thevisudoaction does not overwrite sudoers config on every invocation. It writes it only if it is missing or outdated, so it is OK to invoke it on each update.In the past some users had an issues related to the ownership of battery config folder or battery daemon plist, because they invoked
battery visudoor other actions with sudo. I did it many times myself. With this PR thevisudoaction uses system allocated cache folder to store its temp file and not touching config folder at all. Additionally, the PR adds some guards to make sure some actions are not executed with sudo by mistake.Setup/Update functionality in app/modules/battery.js
This functionality is located in
initialize_batteryfunction. This PR simplifies the process to a simple condition:The decision about whether to install or update is based on:
If any of the conditions fail, the GUI app triggers reinstallation of background components asking for user password.
Silent update is launched otherwise.
The time app checks for updates (when user sees the "Updating..." message in menubar) is significantly reduced with this PR comparing to current version 1.3.2, even though I'm not sure why.
exec_async() and other shell execution helpers in app/modules/battery.js
Existing exec_async related functionality makes it difficult to maintain the app and introduces some bugs:
With this PR:
Other changes
--helpand--versionactions (cli support --help #433). Because they are trivial to implement, and it’s sometimes annoying to run battery help with its long output and scroll back just to find out which version is installed.adapteraction. Before the fix,battery adapter onwould actually disable the adapter.battery chargekilled the maintenance process, whilebattery dischargedid not (otherwise maintenance process would kill itself with --force-discharge option). With this PR, both "charge" and "discharge" actions kill maintenance process, but only when they are called by user from Terminal. This prevents interference between the two charge-controlling tasks. These actions wouldn't work otherwise. Additionally, since they stop maintenance, both actions now restore it on completion, again only when run directly from the Terminal. This change is implemented using environment variable flag, so affected clients (if any) can easily adapt to the new behavior.calibrateaction. It was severely broken and was trying to run calibration in a background process for no reason. With this PR, it runs in a Terminal window. Similar to the charge/discharge actions, it terminates the maintenance process and restores it upon completion. The opposite is true as well: if the user launches maintenance in the Terminal or by starting the GUI menubar app, the calibration process is terminated. We would need to implement some kind of confirmations one day. Tested it once and now coconutBattery reports that battery health increased by 2%. Maybe it was worth it.