-
-
Notifications
You must be signed in to change notification settings - Fork 371
Fix Windows trust-check failure on 9P network shares by handling ERROR_INVALID_FUNCTION #2129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…UNCTION Co-authored-by: Byron <[email protected]>
Co-authored-by: Byron <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the reasons described in #2128 (comment), I think the approach here of falling back to Ok(false)
for the relevant error code is good.
However, as described in more detail in the "Implications for PR #2129" section of that comment, I think the commit with the fix should--perhaps when rebasing anyway to add fix:
--be reworded to avoid describing the currently broken case as "mounted paths" or "mounted drives," because:
- Shares generally work; it is only 9P shares that do not.
- Whether a mapped drive letter is used to access the share has no impact.
gix-sec/tests/sec/identity.rs
Outdated
fn test_trust_behavior_scenarios() { | ||
// Test 1: Current directory (should work) | ||
let current_dir = std::env::current_dir().unwrap(); | ||
println!("Testing current directory: {:?}", current_dir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Don't println
in tests, use assertions with third arguments instead, like assert_eq!(a, b, "format {} string")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 0d664ac. Replaced all println statements with proper assertions using third arguments for better error messages.
gix-sec/tests/sec/identity.rs
Outdated
println!("Testing trust derivation from path ownership:"); | ||
match gix_sec::Trust::from_path_ownership(¤t_dir) { | ||
Ok(trust) => println!(" Trust level: {:?}", trust), | ||
Err(e) => println!(" Error: {}", e), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Remove this, makes no sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the redundant Trust::from_path_ownership test in commit b2dca1b. The test was duplicating functionality already covered by the is_path_owned_by_current_user test above it.
@EliahKagan I think my review will remove the worst bits of this PR, but I am happy that maybe there is something salvageable. Could you take it from here? |
Co-authored-by: Byron <[email protected]>
Thank you for the clarification. I've updated the commit message to be more specific about 9P network shares rather than general "mounted paths" or "mounted drives", since the issue is specific to certain network filesystems like 9P shares and not all mounted drives. |
…ewer Co-authored-by: Byron <[email protected]>
Fixes issue #2128 where gitoxide fails to open repositories on Windows when they are accessed via 9P network shares.
Problem
On Windows, when a repository is located on certain network filesystems (particularly 9P shares), the
GetNamedSecurityInfoW
Windows API function fails with "Incorrect function" (error code 1 /ERROR_INVALID_FUNCTION
). This causes gitoxide to completely fail when trying to determine repository trust level, preventing the repository from being opened at all.Solution
Instead of failing hard when
GetNamedSecurityInfoW
returnsERROR_INVALID_FUNCTION
, the code now gracefully handles this case by:ERROR_INVALID_FUNCTION
error conditionOk(false)
(indicating the path is not owned by the current user)Changes
Core fix in
gix-sec/src/identity.rs
:ERROR_INVALID_FUNCTION
to Windows API importsis_path_owned_by_current_user()
to specifically catch this errorEnhanced test coverage:
Security Considerations
This change maintains the security model by defaulting to the safer "reduced trust" state when ownership cannot be determined, rather than assuming trust or failing completely. Other error conditions continue to cause hard failures as expected.
The fix ensures Windows users can access repositories on 9P network shares while preserving gitoxide's security guarantees.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.