darwin: fix "$HOME is not owned by you" warning #429
Conversation
WalkthroughRefactored elevation command construction in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 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 |
NotAShelf
changed the title
NotAShelf
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![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)
src/commands.rs (1)
416-460: LGTM: Well-structured helper method.The
build_sudo_partsmethod cleanly separates the elevation command construction logic. The implementation correctly handles elevation program selection, sudo -A flag, and environment variable passing.Minor note: Line 433 binds
_askpassbut only checks for its existence. Consider usingstd::env::var("NH_SUDO_ASKPASS").is_ok()directly if the value isn't needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands.rs (1)
src/checks.rs (1)
new(338-347)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
🔇 Additional comments (4)
src/commands.rs (4)
25-27: LGTM: Formatting improvement.The multi-line formatting improves readability without changing behavior.
33-35: LGTM: Consistent formatting.The formatting matches the pattern used in
get_cached_passwordabove.
468-500: LGTM: Cleaner elevation command construction.The refactoring eliminates shell command parsing by directly composing command parts using
build_sudo_parts. This addresses the escaping issues mentioned in the PR comments (e.g., empty HOME from escaped quotes) and provides a more robust elevation mechanism.
894-904: Test coverage may need adjustment.The tests correctly verify the current implementation (HOME="" unconditionally on macOS). However, if the HOME setting logic is adjusted to only apply to elevated commands on macOS (as suggested in the earlier comment), these tests should be updated to cover both:
- Non-elevated commands: HOME should have actual value on all platforms.
- Elevated commands: HOME="" on macOS, no HOME on other platforms.
Also applies to: 939-947, 1000-1010
|
@NotAShelf can we give this pr a try? |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/commands.rs (1)
1000-1010: Test expectations don't match the new HOME logic.Same issue as in the previous tests: the test expects
HOME=""on macOS for a non-elevated command, but the new logic only setsHOME=""for elevated commands on macOS.Apply this diff to fix the test expectations:
- if cfg!(target_os = "macos") { - // macOS sets HOME to "" in Nix environment - assert!( - matches!(cmd.env_vars.get("HOME"), Some(EnvAction::Set(val)) if val.is_empty()) - ); - } else { - // Other OSes should have the actual HOME value - assert!( - matches!(cmd.env_vars.get("HOME"), Some(EnvAction::Set(val)) if val == "/test/home") - ); - } + // Non-elevated commands should have HOME from environment on all platforms + assert!( + matches!(cmd.env_vars.get("HOME"), Some(EnvAction::Set(val)) if val == "/test/home") + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands.rs (1)
src/checks.rs (1)
new(338-347)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: treewide-checks
🔇 Additional comments (2)
src/commands.rs (2)
299-305: LGTM! Addresses the past review comment correctly.The HOME="" setting is now correctly scoped to elevated commands on macOS only. This aligns with the PR objectives to fix the "$HOME is not owned by you" warning and addresses the concern raised in the past review comment.
416-460: Solid refactor to eliminate escaping issues.The new
build_sudo_partshelper returnsVec<String>instead of constructing anExecobject, which avoids the shell escaping issues mentioned in the PR comments that caused malformed environment variables (empty HOME). The logic correctly mirrorsbuild_sudo_cmdwhile producing unescaped string parts suitable forstd::process::Command::new().args().
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands.rs (1)
492-497: SUDO_ASKPASS inconsistency still present (duplicate issue from past review).The
SUDO_ASKPASSenvironment variable is only set when the strategy is exactlyForce("sudo"), butbuild_sudo_parts()(line 432) adds the-Aflag whenever the resolved program is sudo, regardless of strategy (Auto,Prefer, orForce).This inconsistency means:
ElevationStrategy::AutoorPreferthat resolves to sudo withNH_SUDO_ASKPASSset →-Aflag added butSUDO_ASKPASSenv NOT set- This will cause sudo to fail with "sudo: no askpass program specified"
Apply this diff to check the resolved program name instead of the strategy variant:
- if let Some(ElevationStrategy::Force("sudo")) = cmd_builder.elevate.as_ref() - { + // Set SUDO_ASKPASS environment variable when using sudo + let sudo_parts = cmd_builder.build_sudo_parts()?; + // Check if first part is sudo by examining the resolved program + if sudo_parts[0].contains("sudo") { if let Ok(askpass) = std::env::var("NH_SUDO_ASKPASS") { std_cmd.env("SUDO_ASKPASS", askpass); } } + + // Use the sudo_parts we already built + let mut std_cmd = std::process::Command::new(&sudo_parts[0]); + if sudo_parts.len() > 1 { + std_cmd.args(&sudo_parts[1..]); + } - - let mut sudo_parts = cmd_builder.build_sudo_parts()?; - - // Add the target executable and arguments - sudo_parts.push(current_exe.to_string_lossy().to_string()); - let args: Vec<String> = std::env::args().skip(1).collect(); - sudo_parts.extend(args); - - let mut std_cmd = std::process::Command::new(&sudo_parts[0]); - if sudo_parts.len() > 1 { - std_cmd.args(&sudo_parts[1..]); - }Note: You'll need to add the executable and args to sudo_parts before the SUDO_ASKPASS check, or restructure to avoid building sudo_parts twice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands.rs (1)
src/checks.rs (1)
new(338-347)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: treewide-checks
🔇 Additional comments (1)
src/commands.rs (1)
416-460: LGTM! Clean helper design for building elevation command parts.The
build_sudo_parts()helper cleanly separates the elevation command construction into reusable parts. ReturningVec<String>provides flexibility for assembling commands withstd::process::Commandinself_elevate_cmd.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/commands.rs (2)
416-460: LGTM! Consider consolidating with build_sudo_cmd if possible.The
build_sudo_partshelper correctly constructs elevation command parts as aVec<String>, which avoids the shell escaping issues mentioned in the PR discussion. The logic for resolving the program, adding the-Aflag, and building environment parts is sound.Minor observation: There's some code duplication between
build_sudo_partsand the existingbuild_sudo_cmdmethod (lines 365-414). Ifbuild_sudo_cmdis being phased out in favor of the new approach, consider adding a comment or deprecation notice. Otherwise, consider whether the common logic could be extracted to reduce duplication.
480-497: Consider more robust SUDO_ASKPASS handling.The implementation works correctly but has a fragile check at line 493. The condition
sudo_parts[1] == "-A"assumes the-Aflag is always at index 1 when present. While this matches the currentbuild_sudo_partsimplementation, it could break if that method is refactored to add other flags or arguments before-A.Apply this diff for a more robust approach:
- // check if using SUDO_ASKPASS - if sudo_parts[1] == "-A" { + // Set SUDO_ASKPASS if using sudo with -A flag + if sudo_parts.contains(&"-A".to_string()) { if let Ok(askpass) = std::env::var("NH_SUDO_ASKPASS") { std_cmd.env("SUDO_ASKPASS", askpass); } }Alternatively, you could check the resolved program name and
NH_SUDO_ASKPASSdirectly:// Set SUDO_ASKPASS when using sudo with askpass let elevation_program = cmd_builder .elevate .as_ref() .unwrap() .resolve()?; if elevation_program.file_name().and_then(|n| n.to_str()) == Some("sudo") { if let Ok(askpass) = std::env::var("NH_SUDO_ASKPASS") { std_cmd.env("SUDO_ASKPASS", askpass); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands.rs (1)
src/checks.rs (1)
new(338-347)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: treewide-checks
🔇 Additional comments (1)
src/commands.rs (1)
299-305: LGTM! Verify the fix resolves the warning on macOS.The conditional
self.elevate.is_some() && cfg!(target_os = "macos")correctly appliesHOME=""only to elevated commands on macOS, which should resolve the "$HOME is not owned by you" warning per the linked Nix source and PR objectives.Please verify that running
nh clean allon macOS no longer produces the warning. If possible, test both elevated and non-elevated commands to ensure HOME handling is correct in both cases.
|
Sorry for the delay, I've been a little occupied with other projects that are demanding my attention. If you could just rebase this on the current master, I'll try to review this week before I prepare for the 4.3 release. I apologize in advance if I'm not able to review in time, there is an immediate health concern that might take me out of commission for some time. But I'm sure other Darwin users would appreciate a rebased branch to use your fork for a while if I can merge it in time. |
Parsifa1
force-pushed
the
fix-home-var
branch
2 times, most recently
from
6b74395 to
ce30a6b
Compare
|
i can't fix the failing check... |
|
I did the rebase :) I've probably been a bit busy lately as well and most likely won't have time to respond to the review :( |
Parsifa1
force-pushed
the
fix-home-var
branch
4 times, most recently
from
ad2bd85 to
ce30a6b
Compare
|
there are some error in #451, so there will have one failed check :( |
|
Checks failing are fine. GH actions are archaic, and they're usually not my metric for a merge.> I'll try to review the PR this week, perhaps after Monday. Thank you for rebasing :) |
Performance Comparison
|
Performance Comparison
|
|
Nice, the CI is finally working. I don't know why it insists that there's a regression, but it seems to do that for all PRs so I'll assume it's on my end. I'll try to review this tomorrow after work. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/commands.rs (1)
537-542: Prefer safer bounds checking for the askpass flag.The index-based check
sudo_parts[1] == "-A"is fragile and tightly couples this code to the internal ordering ofbuild_sudo_parts. While currently safe (minimum length is 2), future modifications could silently break this assumption.Consider using safer access:
- // check if using SUDO_ASKPASS - if sudo_parts[1] == "-A" { + // check if using SUDO_ASKPASS + if sudo_parts.get(1).map_or(false, |s| s == "-A") { if let Ok(askpass) = std::env::var("NH_SUDO_ASKPASS") { std_cmd.env("SUDO_ASKPASS", askpass); } }Alternatively, consider returning a struct from
build_sudo_partsthat explicitly indicates whether askpass is enabled, making the intent clearer and the code more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: profile
- GitHub Check: Build NH on Linux
🔇 Additional comments (2)
src/commands.rs (2)
344-350: macOS HOME handling looks correct.The conditional properly restricts
HOME=""to elevated commands on macOS only, which aligns with the PR objective to fix the ownership warning. The Nix source reference supports this approach.
461-505: LGTM! Helper function correctly constructs elevation command parts.The
build_sudo_partshelper properly mirrors the elevation logic, constructs the command parts without shell escaping issues, and maintains the same environment handling asbuild_sudo_cmd.
2 participants
Sanity Checking
nix fmtto format my Nix codecargo fmtto format my Rust codecargo clippyand fixed any new linter warnings.logic
description.
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinclose #314
Add a 👍 reaction to pull requests you find important.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.