install.sh: do not assume sudo consent when there is no terminal#7435
install.sh: do not assume sudo consent when there is no terminal#7435LeoBorcherding wants to merge 9 commits intounslothai:mainunslothai/unsloth:mainfrom LeoBorcherding:fix-7307-p7-no-tty-sudo-installLeoBorcherding/unsloth:fix-7307-p7-no-tty-sudo-installCopy head branch name to clipboard
Conversation
…lothai#7307 P7) _smart_apt_install printed an "Accept? [Y/n]" prompt, and when /dev/tty was unreadable it set REPLY=y and escalated anyway. Every sudo call in that branch redirects stdin from /dev/null, so on any host where sudo needs a password the install died on sudo's own error rather than the actionable message the no-sudo path already prints. Containers, CI and locked-down corporate machines hit this. Probe with `sudo -n true` first. If there is no terminal to prompt on and sudo would need a password, exit with the missing packages and the exact command to run, matching the no-sudo path. Passwordless sudo still escalates unattended, which is the one case where that is legitimate, and says so in the log. With a readable /dev/tty the behaviour is unchanged, and the prompt now only prints when something can actually answer it. Extend tests/sh/test_apt_distro_prompt.sh to drive the real function across all four TTY/sudo combinations, rewriting /dev/tty to a fixture path the same way the existing cases rewrite /etc/os-release. Against the old install.sh five of these assertions fail. Register the file in studio-backend-ci.yml's shell suite, which did not run it before.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66bb45dba9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…7307) Codex review follow-ups on the no-TTY sudo escalation guard. `test -r /dev/tty` only reads the device node's permission bits. Inside containers and systemd units those bits look fine while open() fails with ENXIO, so the guard still fell through to a prompt nobody could answer. _can_read_tty() does a real open. The subshell is load-bearing: in dash a failed redirection on the special builtin `:` exits the script. `sudo -n true` proves only that `true` is allowed. Under a command-specific rule like `NOPASSWD: /usr/bin/apt-get` it is the wrong question in both directions. _sudo_runs_unattended() asks the sudoers policy about the exact argument vectors we are about to elevate, via `sudo -n -l --`, which checks without running and fails instead of prompting. Tests cover both: a NOPASSWD-on-trivia-but-not-apt-get sudoers stub, and a readable-but-unopenable /dev/tty faked with a unix socket (skipped where the platform cannot produce that shape).
|
Addressed both review points in P1, tty probe. Replaced P2, sudo probe. Replaced Tests in The earlier @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b70f900791
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _sudo_runs_unattended() { | ||
| sudo -n -l -- "$@" >/dev/null 2>&1 |
There was a problem hiding this comment.
Probe authentication instead of command authorization
On a no-TTY host where apt-get is permitted but still has the PASSWD tag, sudo -n -l -- apt-get ... can return 0 even though executing the command noninteractively fails: list mode checks whether the command is authorized, not whether its execution is passwordless. This matches sudo --help, which describes -l as “list user's privileges or check a specific command” and -n only as disabling prompts; with a sudoers entry using Options: authenticate, I confirmed the list command returned 0 while sudo -n apt-get ... returned 1. Consequently this helper sets _sudo_unattended=true, and the later sudo calls with /dev/null still produce the raw authentication failure this change intends to prevent. Determine whether the matched entry has !authenticate rather than relying on the sudo -l exit status, and make the stub reproduce real list-mode semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 774ff5d. The list-mode probe is gone; the only remaining mention of it is the comment at install.sh:724 recording why it was abandoned. Head asks the question by running the real commands with -n.
Your reading matches sudo(8), which defines -n as "Avoid prompting the user for input of any kind. If a password is required for the command to run, sudo will display an error message and exit", while -l only lists privileges. Authorization and authentication are separate questions, and only execution answers the second one.
Verified across the tty and sudoers matrix, including the authorized-but-PASSWD case you describe: head declines and escalates nothing.
Codex follow-up. `sudo -n -l -- apt-get ...` answers authorization, not authentication: on a host where apt-get is permitted but still carries the PASSWD tag, list mode exits 0 while the actual run needs a password, so the guard reported unattended and the escalation died exactly as unslothai#7307 described. Inferring the answer from list output means parsing for `!authenticate`, which is human-readable text that varies by sudo version. Drop the inference. In the no-terminal branch, run the real commands with `sudo -n`: -n never prompts, so it cannot block on a closed stdin, and its exit status is the question we were trying to answer. If it is refused, print the actionable manual command as before. The terminal branch is unchanged: prompt, then plain sudo, which may ask for a password because someone is there to type it. The test stub now models sudo properly (-n refuses and runs nothing when a password is needed) instead of special-casing the probe's argv.
Resolves the conflict in .github/workflows/studio-backend-ci.yml. Main replaced the hardcoded shell-test allowlist with auto-discovery over tests/sh/test_*.sh plus an explicit skip list, guarded by tests/studio/test_ci_shell_suite_coverage.py. That covers what this branch's allowlist entry was for, so the entry is dropped and main's version of the step is taken unchanged. test_apt_distro_prompt.sh is picked up by the glob. install.sh and tests/sh/test_apt_distro_prompt.sh are unchanged by this merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdf0ff5237
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo " sudo likely needs a password here and there is no terminal to" | ||
| echo " enter one on, so this cannot be done unattended." |
There was a problem hiding this comment.
Distinguish apt failures from sudo authentication failures
When sudo is passwordless but either apt-get command fails because of a bad repository, network outage, dpkg lock, or package conflict, this same branch reports that sudo likely needs a password and directs the user to rerun the commands manually. sudo --help defines -n only as “non-interactive mode, no prompts are used,” so a nonzero result can be the elevated command's own exit status rather than an authentication failure. Preserve or neutrally report the apt failure instead of diagnosing it as a missing password.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Reproduced both ways: with a nonexistent package and with a simulated DNS failure, sudo authenticated fine, apt-get returned 100, and the branch still told the user that a password was probably needed.
Fixed in 33f0c4d. Your reasoning matches sudo(8), which says the exit status "is the exit status of the program that was executed" when the command runs, and 1 for an authentication or configuration failure, so the status alone does not separate the two cases. The message now reports both possibilities and points at the real error rather than guessing:
Either sudo needs a password here, or apt-get itself
failed; see the error above. With no terminal to
authenticate on, this cannot be done unattended.
The underlying apt error was already reaching the user on stderr; it was only the summary line that was wrong.
| if sudo -n apt-get update -y </dev/null && | ||
| sudo -n apt-get install -y $_STILL_MISSING </dev/null; then |
There was a problem hiding this comment.
Reject cached credentials before unattended elevation
On hosts where an authentication timestamp applies globally or otherwise remains valid for this headless process, sudo -n succeeds for a PASSWD-tagged apt-get command, so a recent unrelated sudo authentication is treated as “passwordless sudo” and packages are installed without an Accept response. The new direct invocation is fresh evidence beyond the earlier authorization-probe issue: sudo --help defines -n only as “non-interactive mode, no prompts are used” and separately exposes -k to invalidate a timestamp. Require an actual NOPASSWD rule, bypass cached credentials, or require explicit unattended opt-in before executing these elevated commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this was reachable. Fixed in 33f0c4d by adding -k to both probes, so they succeed only under a real NOPASSWD rule:
if sudo -n -k apt-get update -y </dev/null &&
sudo -n -k apt-get install -y $_STILL_MISSING </dev/null; thenOn the side effect you would rightly worry about: per sudo(8), -k used with a command only makes sudo "ignore the user's cached credentials" for that invocation and "will not update the user's cached credentials". It does not clear the timestamp the way bare sudo -k does, so an interactive session in another terminal does not have to re-authenticate afterwards.
Reproduced before and after with a sudoers model carrying a valid timestamp for a PASSWD-tagged apt-get: previously it installed with nobody having answered the prompt, now it declines and runs nothing as root. Covered by a new cached mode in tests/sh/test_apt_distro_prompt.sh, which fails against the previous commit.
…rd for apt failures Two review findings on the headless escalation branch. A cached authentication timestamp from an earlier, unrelated elevation made `-n` succeed for a PASSWD-tagged apt-get, so packages installed with nobody having answered the prompt. Add `-k` so the probe ignores the timestamp and only a real NOPASSWD rule counts as passwordless. Per sudo(8), `-k` alongside a command ignores the cached credentials for that invocation and "will not update the user's cached credentials", so an interactive session elsewhere does not have to re-authenticate afterwards. A nonzero status from the elevated apt-get was reported as "likely needs a password" even when sudo had authenticated fine and apt itself failed on a bad repository, a dpkg lock or a network outage. sudo returns the command's own exit status when the command runs, so the two cases are not distinguishable from the status alone. Report both possibilities and point at the real error. tests/sh/test_apt_distro_prompt.sh: teach the sudo stub about -k, add a cached mode, and assert both behaviours. The three new assertions fail against the previous commit.
_can_read_tty proves the device opens, not that anyone is there to answer. A read that hits EOF still fell back to REPLY=y and escalated, so the branch that does have a terminal kept the behaviour this change removes from the branch that does not. A drained or half-closed terminal reached it. Default to n instead, which is what the post-install autostart prompt at the bottom of this file already does on the same condition. Enter still means yes: that is a successful read of an empty line, not a failed read. tests/sh/test_apt_distro_prompt.sh: add an eof tty fixture, which opens normally and returns EOF immediately. Both new assertions fail against the previous commit.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…tus claim Comment-only. The earlier note said a nonzero status from the elevated apt-get was not distinguishable from the status alone; sudo(8) is more specific than that. sudo exits 1 on an authentication or configuration failure and passes the command's own status through when the command runs, while apt-get(8) returns 100 on error, so the two usually are distinguishable. sudo also exits 1 when the command cannot be executed, which is why the message still states both causes rather than naming one.
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Comment-only pass over the branch's own comments in both files. Same intent, fewer lines: drop restatement, keep the parts a reader cannot derive from the code (why test -r is the wrong probe, why the subshell around the redirection is load-bearing under dash, what -k buys over -n, and why a nonzero status does not by itself name the cause). Verified to touch nothing but comments and blank lines.
Addresses Problem 7 of #7307.
Problem
_smart_apt_install()ininstall.shprints a consent prompt, then decides theanswer itself when there is no terminal:
Two things go wrong when
/dev/ttyis unreadable:sudowith stdin redirected from/dev/null, so on any hostwhere sudo needs a password, sudo cannot prompt and dies with its own raw
error.
The result is the worst of both paths. The script neither degrades gracefully nor
explains itself, even though the no-sudo branch a few lines below already prints
exactly the message the user needs. This hits containers, CI, and locked-down
corporate machines. It also prints
Accept? [Y/n]into logs where nothing cananswer it.
Fix
Probe with
sudo -n true, which succeeds only when no password is required.the exact command to run, mirroring the existing no-sudo message, instead of
invoking sudo and letting it fail.
where unattended escalation is legitimate, and it now logs why it proceeded.
/dev/tty: unchanged. The prompt is simply moved inside thebranch so it only prints when something can answer it.
TAURI_MODEalready had the right shape for this (tauri_log "NEED_SUDO"; exit 2,letting the Rust caller handle elevation). This gives the headless shell path
equivalent honesty.
Tests
Extended
tests/sh/test_apt_distro_prompt.sh, which already covers this function.It drives the real
_smart_apt_installacross all four TTY/sudo combinations,rewriting
/dev/ttyto a fixture path the same way the existing cases rewrite/etc/os-release, withsudo,apt-get,command -vand_is_pkg_installedstubbed. 20 assertions pass.
Against the previous
install.sh, five of the new assertions fail. The capturedoutput is the bug itself:
An unanswerable prompt, then escalation despite
sudo -nreporting that apassword is required. The
tty presentandno sudo binaryassertions pass bothbefore and after, confirming those paths are untouched.
This file was not in
studio-backend-ci.yml's shell-test allowlist, so it neverran in CI. Registered it there, since the coverage is otherwise inert. It is
hermetic: it stubs every external command and only reads
install.shviased.