Repository navigation
fix: use POSIX path rules on the remote and trust user private groups - #816
Conversation
…vate groups Resolve remote paths with POSIX rules so Windows clients accept absolute remote paths and do not write backslashes when joining paths or finding parents. Allow group-writable candidates only when the primary group is named after the user and the path belongs to that group. Continue to reject group-writable paths for shared primary groups and all world-writable paths. Co-authored-by: jie.yuan <yuanjielovejesus@gmail.com> Assisted-by: gpt-6.1-terra Assisted-by: claude-sonnet-5.5
 [Medium impact] The changes affect remote connection, path selection, and executable checks. The group-write exception needs to check group membership before this PR is safe to merge. FindingsFix with agent prompt### Issue 1
src/commands/up_remote.rs:2156-2157
Matching the primary group name to the username does not prove that the group is private. If another account belongs to `alice`, it can change an `alice:alice` executable with mode `0775`. The owner check still passes, and the new exception lets the probe run the changed executable as Alice. Verify that no other accounts belong to the group before allowing group writes, or keep rejecting group-writable candidates.
**How this was verified:** The probe checks group names and file ownership, then directly runs `"$p" --version` without checking whether other accounts belong to the group.
### Issue 2
src/commands/up_remote.rs:2876-2878
The new test assumes the temporary directory belongs to the user's primary group, but it only sets its mode. If `TMPDIR` points to a shared directory whose set-group-ID bit is set, the fixture inherits that directory's group. When the user's primary group matches their username, the test expects acceptance even though the probe correctly rejects the fixture's different group. This can fail tests for developers using shared temporary directories. Set the fixture's group explicitly or account for its actual group before choosing the expected result.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThe PR replaces local filesystem path rules with POSIX string helpers for remote settings and paths.
Reviews (1) · Last reviewed commit: "fix(up_remote): use POSIX path rules on ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ \"$(id -gn 2>/dev/null)\" = \"$(id -un)\" ]; then \ | ||
| unsafe=$(find \"$p\" \"$(dirname \"$p\")\" -prune \\( -perm -002 -o \\( -perm -020 ! -group \"$(id -g)\" \\) \\) -print 2>/dev/null); \ |
There was a problem hiding this comment.
Matching the primary group name to the username does not prove that the group is private. If another account belongs to alice, it can change an alice:alice executable with mode 0775. The owner check still passes, and the new exception lets the probe run the changed executable as Alice. Verify that no other accounts belong to the group before allowing group writes, or keep rejecting group-writable candidates.
How this was verified: The probe checks group names and file ownership, then directly runs "$p" --version without checking whether other accounts belong to the group.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/up_remote.rs
Line: 2156-2157
Comment:
**Shared groups are trusted**
Matching the primary group name to the username does not prove that the group is private. If another account belongs to `alice`, it can change an `alice:alice` executable with mode `0775`. The owner check still passes, and the new exception lets the probe run the changed executable as Alice. Verify that no other accounts belong to the group before allowing group writes, or keep rejecting group-writable candidates.
**How this was verified:** The probe checks group names and file ownership, then directly runs `"$p" --version` without checking whether other accounts belong to the group.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let dir = std::env::temp_dir().join(format!("orx-probe-group-{}", std::process::id())); | ||
| std::fs::create_dir(&dir).unwrap(); | ||
| std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o775)).unwrap(); |
There was a problem hiding this comment.
The new test assumes the temporary directory belongs to the user's primary group, but it only sets its mode. If TMPDIR points to a shared directory whose set-group-ID bit is set, the fixture inherits that directory's group. When the user's primary group matches their username, the test expects acceptance even though the probe correctly rejects the fixture's different group. This can fail tests for developers using shared temporary directories. Set the fixture's group explicitly or account for its actual group before choosing the expected result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/up_remote.rs
Line: 2876-2878
Comment:
**Test assumes the directory's group**
The new test assumes the temporary directory belongs to the user's primary group, but it only sets its mode. If `TMPDIR` points to a shared directory whose set-group-ID bit is set, the fixture inherits that directory's group. When the user's primary group matches their username, the test expects acceptance even though the probe correctly rejects the fixture's different group. This can fail tests for developers using shared temporary directories. Set the fixture's group explicitly or account for its actual group before choosing the expected result.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…rust-user-private-groups
Supersedes #357. See it's description
Assisted-by: gpt-6.1-terra
Assisted-by: claude-sonnet-5.5