fix: use absolute path when invoking once in install script#14
fix: use absolute path when invoking once in install script#14kevinmcconnell merged 1 commit intobasecamp:mainfrom
Conversation
`sudo` resets PATH to a restricted set defined by secure_path in `/etc/sudoers`
Running `sudo once` gives below error:
```
Installing background service...
sudo: once: command not found
```
Fix by introducing a `ONCE_BIN` variable that is set to the existing
path if `once` is already installed, or to `"${INSTALL_DIR}/once"` after
a fresh install. This avoids PATH resolution issues under `sudo` and
correctly handles the case where `once` was previously installed to a
non-standard location.
Closes basecamp#13
There was a problem hiding this comment.
Pull request overview
This PR updates the install script to invoke once via an absolute path (stored in a new ONCE_BIN variable) so that sudo calls don’t fail due to secure_path PATH restrictions, addressing issue #13.
Changes:
- Introduce
ONCE_BINand set it to either the existingoncelocation or${INSTALL_DIR}/onceafter installation. - Use
ONCE_BINfor background service installation and for subsequentrun_onceexecution across Docker modes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "once is already installed at ${ONCE_BIN}" | ||
| return |
There was a problem hiding this comment.
i thin this is overly paranoid for this context.
This is an install script that the user is already running (with curl | sh). If an attacker can place a malicious once earlier in $PATH, they likely already have enough access to do damage without this vector.
it wouldn't hurt to add a simple check like [[ "$ONCE_BIN" = /* ]], to ensure it's an absolute path, but it's not a real security concern.
| sg docker -c "${ONCE_BIN} ${install_flag}" </dev/tty | ||
| ;; |
There was a problem hiding this comment.
ONCE_BIN will either be the output of command -v once (which returns a clean path) or /usr/local/bin/once. unless user intentionally modifies where once is installed to a path with spaces and run this script again, this isn't really an issue
|
hey @kevinmcconnell thanks for starting a review, imo, the reviews from copilot are too defensive and paranoid for this context, and adds unnecessary complexity to the script. |
|
@baggiiiie yes, agreed. Sometimes it flags things that are genuinely useful, but I don’t think this is one of those times :) I think using the absolute path, as you have here, is the right fix. Will get it merged soon. Thanks for the contribution! |
sudoresets PATH to a restricted set defined by secure_path in/etc/sudoersRunningsudo oncegives below error:Fix by introducing a
ONCE_BINvariable that is set to the existing path ifonceis already installed, or to"${INSTALL_DIR}/once"after a fresh install. This avoids PATH resolution issues undersudoand correctly handles the case whereoncewas previously installed to a non-standard location.Closes #13