fix: sudo password prompt broken in setup command#65
Merged
Peyton-Spencer merged 6 commits intomainfrom Mar 25, 2026
Merged
Conversation
Foundation's Process class spawns child processes via posix_spawn without setting the child as the terminal foreground process group. When sudo -v tries to read the password from /dev/tty, it fails with "unable to read password: Input/output error" because the child process doesn't have proper terminal control. Replace with Darwin.system() which uses fork/exec with correct process group management, terminal control, and signal handling (SIGINT/SIGQUIT properly ignored in parent during wait). Co-Authored-By: Zest <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRequires running setup/teardown as root: CLI help now shows Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (mac-runner)"
participant Setup as "SetupWizard"
participant OS as "OS (geteuid / SUDO_USER / userdb)"
participant Config as "ConfigService"
CLI->>Setup: invoke "setup" / "setup --teardown"
Setup->>OS: check geteuid()
alt not root
Setup-->>CLI: print "Run with sudo mac-runner setup..." and exit
else root
Setup->>OS: read SUDO_USER
Setup->>Config: initialize (resolve App Support for SUDO_USER if present)
Config->>OS: (may) chown config dir to invokingUser
Setup->>OS: resolve mainUser (SUDO_USER or NSUserName)
alt mainUser == "root"
Setup-->>CLI: skip sudoers installation
else mainUser missing
Setup-->>CLI: print error and exit
else mainUser valid
Setup->>OS: install sudoers entry for mainUser
Setup-->>CLI: continue setup/teardown steps
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Swift marks Darwin.system() as unavailable, so add a minimal C helper module (CHelpers) that wraps system() for use from Swift. system() properly manages terminal foreground process groups via fork/exec, which is required for sudo's interactive password prompt. Foundation's Process uses posix_spawn without terminal group setup, causing "unable to read password: Input/output error". Co-Authored-By: Zest <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/CHelpers/chelpers.c (1)
4-5: This function could be narrowed to reflect its single dedicated purpose.The current wrapper (
run_system_command) serves only one call site with a constant string ("sudo -v"). Rather than expose a generic command-execution API, consider a dedicated helper likerun_sudo_validate()for clarity and to prevent accidental misuse.♻️ Suggested refactor (dedicated helper)
- int run_system_command(const char *command) { - return system(command); - } +int run_sudo_validate(void) { + return system("/usr/bin/sudo -v"); +}And update the header + Swift call accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CHelpers/chelpers.c` around lines 4 - 5, Replace the generic wrapper run_system_command(const char *command) with a dedicated helper run_sudo_validate() that calls system("sudo -v") internally; update the C header to expose run_sudo_validate instead of run_system_command and update the Swift FFI/import site to call run_sudo_validate(), removing the ability to pass arbitrary commands and clarifying the function's sole purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Services/UserIsolationService.swift`:
- Around line 285-286: The call to run_system_command("sudo -v") in
UserIsolationService.swift should use an absolute path to avoid PATH-based
privilege escalation; update the invocation in the authentication flow (the
run_system_command call used in the UserIsolationService authentication block)
to call "/usr/bin/sudo -v" instead of "sudo -v" so the system() call executes
the fixed binary path.
---
Nitpick comments:
In `@Sources/CHelpers/chelpers.c`:
- Around line 4-5: Replace the generic wrapper run_system_command(const char
*command) with a dedicated helper run_sudo_validate() that calls system("sudo
-v") internally; update the C header to expose run_sudo_validate instead of
run_system_command and update the Swift FFI/import site to call
run_sudo_validate(), removing the ability to pass arbitrary commands and
clarifying the function's sole purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 228a568a-3926-424b-b623-64c3736a03c7
📒 Files selected for processing (4)
Package.swiftSources/CHelpers/chelpers.cSources/CHelpers/include/chelpers.hSources/Services/UserIsolationService.swift
Replace the `authenticateAdministratorAccess()` flow (which used Foundation's Process to spawn `sudo -v` — broken due to terminal I/O issues) with a simple root check at the start of setup/teardown. Users now run `sudo mac-runner setup` instead of entering their password mid-flow. This is cleaner, more secure, and matches how Homebrew handles admin-required operations. Also fixes SUDO_USER detection for the sudoers entry — NSUserName() returns "root" under sudo, so we read SUDO_USER from the environment. Co-Authored-By: Zest <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Services/SetupWizard.swift (1)
104-106: Consider handling the edge case where the resolved user is "root".The SUDO_USER lookup is correct for the normal
sudo mac-runner setupflow. However, if someone logs in directly as root (not via sudo), SUDO_USER won't be set andNSUserName()returns "root", resulting in a sudoers entry for the root user—which is redundant.Consider adding a guard or warning:
💡 Optional: Add a sanity check for root user
// When running via sudo, NSUserName() returns "root". // SUDO_USER contains the actual invoking user. let mainUser = ProcessInfo.processInfo.environment["SUDO_USER"] ?? NSUserName() + if mainUser == "root" { + print("Warning: Could not determine invoking user. Sudoers entry will be created for 'root'.") + } try isolationService.installSudoersEntry(mainUsername: mainUser, serviceUsername: username)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Services/SetupWizard.swift` around lines 104 - 106, The resolved main user assigned to mainUser via ProcessInfo.processInfo.environment["SUDO_USER"] ?? NSUserName() can legitimately be "root" when someone is logged in as root; add a check after computing mainUser (the variable in SetupWizard.swift) that detects if mainUser == "root" and handle it by either skipping creation of the redundant sudoers entry (or emitting a clear warning and aborting the sudoers modification flow), so the code that writes the sudoers entry for mainUser is not executed for "root".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Services/SetupWizard.swift`:
- Around line 104-106: The resolved main user assigned to mainUser via
ProcessInfo.processInfo.environment["SUDO_USER"] ?? NSUserName() can
legitimately be "root" when someone is logged in as root; add a check after
computing mainUser (the variable in SetupWizard.swift) that detects if mainUser
== "root" and handle it by either skipping creation of the redundant sudoers
entry (or emitting a clear warning and aborting the sudoers modification flow),
so the code that writes the sudoers entry for mainUser is not executed for
"root".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b7b5870-7890-4cbb-a02a-318ea8a0b489
📒 Files selected for processing (2)
Sources/Services/CLIHandler.swiftSources/Services/SetupWizard.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/Services/CLIHandler.swift
Root already has full privileges — writing a sudoers entry for "root" is redundant. This handles the edge case where someone runs setup while logged in as root directly (no SUDO_USER set). Co-Authored-By: Zest <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Services/SetupWizard.swift`:
- Around line 104-110: Resolve and validate the invoking account before passing
it into installSudoersEntry: prefer deriving mainUser from SUDO_UID by looking
up the local account for that UID (fallback to SUDO_USER only if no SUDO_UID),
then verify the resolved name is a canonical local short username (single token,
matches POSIX short name rules and exists in the local user DB); if validation
fails, log/print an error and skip calling
isolationService.installSudoersEntry(mainUsername:..., serviceUsername:
username). Update the code paths that currently set mainUser from SUDO_USER and
the call site of installSudoersEntry so they use the validated/lookup result or
bail out on invalid input.
- Line 48: The success footer in SetupWizard still prints the teardown
instruction without sudo; update the print statement that emits "Run 'mac-runner
setup --teardown' to disable." to include sudo (i.e. "Run 'sudo mac-runner setup
--teardown' to disable.") — locate the print in the SetupWizard success/finish
path (the footer print) and change its string, and search for any other
occurrences of the same message in SetupWizard to keep them consistent.
- Around line 27-34: SetupWizard.runSetup() and runTeardown() elevate to root,
causing ConfigService's .applicationSupportDirectory to resolve under root's
home; capture the invoking user's identity (from the SUDO_USER env var) or
resolve and store the invoking user's Application Support path before checking
geteuid(), then pass that original-user home/path into ConfigService
initialization or its path resolution so
ConfigService.applicationSupportDirectory uses the invoking user's home rather
than /var/root; update SetupWizard to supply that stored path or user to
ConfigService constructors or to a new init parameter so all config I/O targets
the original user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02fbd75d-a08e-4f81-ae0f-09d6682eb0be
📒 Files selected for processing (1)
Sources/Services/SetupWizard.swift
Comment on lines
+27
to
+34
| // Require root — all setup operations need administrator privileges. | ||
| // Running with sudo ensures clean terminal I/O (no mid-flow password | ||
| // prompts) and a cached credential for all child sudo -n calls. | ||
| guard geteuid() == 0 else { | ||
| print("Error: setup requires administrator privileges.") | ||
| print("Run: sudo mac-runner setup") | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
config_file=$(fd 'ConfigService\.swift$' Sources | head -n1)
if [[ -z "${config_file:-}" ]]; then
echo "ConfigService.swift not found"
exit 1
fi
echo "== ConfigService symbols =="
rg -n -C3 --type=swift '\b(class|struct|actor)\s+ConfigService\b|\bfunc\s+(loadConfig|saveConfig)\b' Sources
echo
echo "== ConfigService implementation =="
sed -n '1,260p' "$config_file"
echo
echo "== Path / ownership signals =="
rg -n -C2 --type=swift \
'NSHomeDirectory\(|homeDirectoryForCurrentUser|applicationSupportDirectory|documentDirectory|libraryDirectory|SUDO_(USER|UID|HOME)|createFile|write\(|replaceItemAt|setAttributes|chown' \
"$config_file"Repository: omniaura/mac-runner
Length of output: 5856
Config I/O will target root instead of the invoking user.
When SetupWizard.runSetup() and runTeardown() force geteuid() == 0, ConfigService will resolve .applicationSupportDirectory under the root user's home (/var/root/Library/Application Support), not the invoking user's directory. This means setup writes configuration to the wrong location—the invoking user's config remains untouched, and later runs without sudo will read from an uninitialized config directory.
Pass the original user's identity (from SUDO_USER environment variable) to ConfigService so it can resolve paths under the correct home directory, or preserve the original user's Application Support directory path before forcing root privileges.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Services/SetupWizard.swift` around lines 27 - 34,
SetupWizard.runSetup() and runTeardown() elevate to root, causing
ConfigService's .applicationSupportDirectory to resolve under root's home;
capture the invoking user's identity (from the SUDO_USER env var) or resolve and
store the invoking user's Application Support path before checking geteuid(),
then pass that original-user home/path into ConfigService initialization or its
path resolution so ConfigService.applicationSupportDirectory uses the invoking
user's home rather than /var/root; update SetupWizard to supply that stored path
or user to ConfigService constructors or to a new init parameter so all config
I/O targets the original user.
- ConfigService: resolve invoking user's Application Support dir when running under sudo (was writing to /var/root/ instead of user's home) - SetupWizard: add missing `sudo` prefix to teardown instruction footer - SetupWizard: validate SUDO_USER is a real local account via dscl before writing it into the sudoers template Co-Authored-By: Zest <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Services/ConfigService.swift`:
- Around line 8-18: ConfigService currently points appSupport at the invoking
user's Application Support when running under sudo (see the appSupport
assignment) but still performs directory creation and config writes as root in
init() and save operations, leaving MacRunner/ and config.json root-owned;
update init() (and any saveConfig/save methods) to perform I/O as the invoking
user or to chown the created directory/files back to SUDO_USER: detect SUDO_USER
from ProcessInfo, determine that user's uid/gid, and either (a) perform
directory creation and file writes using that uid (e.g., temporarily set
effective UID for the operations) or (b) after creating/writing, call chown on
the created MacRunner directory and config.json to the invoking user's uid/gid
so subsequent non-sudo runs can update the config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6e7b9c5-1f56-4ea8-b6d1-5bc54705c135
📒 Files selected for processing (2)
Sources/Services/ConfigService.swiftSources/Services/SetupWizard.swift
4 tasks
Chown MacRunner's Application Support directory back to the invoking user after sudo-based setup so later non-sudo runs can still read and update config. Add focused tests for the sudo user path resolution helpers that support the ownership fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🎉 This PR is included in version 1.13.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
• Fix
sudo: unable to read password: Input/output errorinmac-runner setupandmac-runner setup --teardown• Replace Foundation
Processwith Csystem()for thesudo -vpre-auth step• Foundation's
posix_spawndoesn't set the child as the terminal foreground process group, sosudocan't interact with/dev/ttyfor the password promptRoot Cause
sudo -vreads the password directly from/dev/tty(not stdin). It needs the calling process to be in the terminal's foreground process group to dotcsetattr/tcgetattrfor echo suppression and secure password reading. Foundation'sProcessclass usesposix_spawnwhich doesn't set up terminal control properly, causing the I/O error.Darwin.system()(C'ssystem()) usesfork/execwith correct process group management, signal handling, and terminal control inheritance.Test plan
mac-runner setup— password prompt should workmac-runner setup --teardown— password prompt should workmakeAdministratorAuthenticationProcessstill passes (factory kept for test coverage)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests