fix: make gix-sec identity check on Windows similar to Git for Windows

[Sebastian Thiel (Byron)]

Related to gitbutlerapp/gitbutler#13278 and based Codex analysis.

Tasks

• refackview
• get Eliah's review

---

gix-sec is not doing the same Windows ownership check as Git.

Comparison

• gix-sec reads the path owner SID with GetNamedSecurityInfoW(...), but then compares it to the token’s TokenOwner SID, not the current user SID. See identity.rs, especially lines 177-214.
• Upstream Git reads the path owner SID, then compares it to the process TokenUser SID. See compat/mingw.c. In your local upstream checkout, get_current_user_sid() uses TokenUser, and there is also a TokenLinkedToken fallback for limited admin tokens at compat/mingw.c.
• Git for Windows windows/main still uses TokenUser, not TokenOwner; it just dropped the linked-token fallback. I compared windows/main:compat/mingw.c, and the ownership function there is still fundamentally Git’s model, not gix-sec’s.

There are two concrete bugs in gix-sec’s Windows path:

• It compares folder_owner against token_owner, where Git compares path owner against current user.
• Its Administrators exception checks IsWellKnownSid(token_owner, WinBuiltinAdministratorsSid), but Git checks whether the path owner SID is WinBuiltinAdministratorsSid.

That second one matters a lot: if the repository is owned by BUILTIN\\Administrators, Git will accept it for an admin user, but gix-sec only accepts it if the token’s default owner SID also happens to be Administrators.

Why Issue 13278 behaves differently

• In gix, repo-local config is filtered by trust: config/mod.rs and repository/config/mod.rs.
• If the repo stays at reduced trust, [remote "origin"] from .git/config is ignored, which explains “The remote named origin did not exist”.
• Git instead decides repo safety at the repository level in setup.c, and for this case its ownership logic likely passed, so git push could still read origin.

So the likely explanation for issue #13278 is:

• Git and gix disagreed on whether the repo or .git/config was “owned by the current user”.
• The disagreement comes from gix-sec using TokenOwner and checking the Administrators exemption against the wrong SID.
• Once gix marked the repo config untrusted, it filtered out the remote section, which surfaced as “origin did not exist”.

The issue comment you referenced matches that diagnosis: gitbutlerapp/gitbutler#13278 (comment)

If you want, I can turn this into a minimal patch for gix-sec that matches Git’s Windows logic more closely.

[Sebastian Thiel (Byron)]

Hi Eliah Kagan (@EliahKagan) , is this something you could test?
If not, or if it's going to be a while, please let me know so and I will probably just merge this as CI indicates it's not completely broken or crashing, while being a chance that it works better than before.

Thank you 🙏.

[Copilot]

Pull request overview

This PR updates gix-sec’s Windows ownership/identity check to more closely match Git for Windows’ behavior when determining whether a path is “owned by the current user”, which affects repository trust decisions.

Changes:

• Switch comparison from token TokenOwner to TokenUser when matching the path owner SID.
• Adjust the Administrators-group exception to check whether the path owner is BUILTIN\Administrators, then validate membership accordingly.
• Add a limited-token (TokenElevationTypeLimited) path that uses TokenLinkedToken to test membership against the elevated token.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 39e37482d6

ℹ️ 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 (@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 (@codex) address that feedback".

[Eliah Kagan (EliahKagan)]

Hi Eliah Kagan (@EliahKagan) , is this something you could test?

Sebastian Thiel (@Byron) Thanks for looping me in! I think that by the end of tomorrow I can manage either to have tested it or to know whatever is in the way of me testing it (and thus have a timeframe estimate on testing).

If I haven't told you more related to this by, say, Wednesday, then that's almost certainly unintentional and I recommend reminding me. (To be clear, you can also ping or otherwise contact me about stuff related to this before then, too, and any number of times, if there's anything of potential interest. I don't plan to put this off until then; that's just a point where I might've forgotten to give you an update if you haven't heard from me.)

[Sebastian Thiel (Byron)]

Thanks so much! I will put this into draft to prevent accidental merges.

[Eliah Kagan (EliahKagan)]

I think the concept here is directionally correct, and I suspect will end up leading to significant improvements even beyond the motivating issue in GitButler. I furthermore suspect this specific PR can be made to work properly; I do not recommend closing it.

But this is definitely not ready yet. I tested a few significant cases. One case I tested is where a member of the Administrators group who is running with UAC enabled and running gix with their filtered token (i.e., not elevated), who accesses a repository owned by the Administrators group.

This might be the specific case in the gitbutlerapp/gitbutler#13278, though there is no information in that issue about any folders' specific ownership or the way (e.g., with respect to UAC) that the command was run, unless I missed it.

Git permits this (it would refuse to read the config if it didn't consider this to be a case where the member of the Administrators group owns the relevant path or paths):

C:\Users\ek\src\ownership-test [main]> git config -l --local
core.repositoryformatversion=0
core.filemode=false
core.bare=false
core.logallrefupdates=true
core.ignorecase=true

In contrast, gix from the most release gitoxide crate release treats the local directory as untrusted (it will read but not use config it considers untrusted, and gix config shows that the local config is untrusted):

C:\Users\ek\src\ownership-test [main]> gix config
...

# From '.\.git\config' (Local, untrusted)
[core]
        repositoryformatversion = 0
        filemode = false
        bare = false
        logallrefupdates = true
        ignorecase = true

...

Since that strongly resembles the situation described in the linked GitButler bug report, and this PR was created with that as its motivation, I expected that it would treat the configuration as trusted. Instead, what happens is that it has a fatal error and doesn't do anything:

C:\Users\ek\src\ownership-test [main]> C:\Users\ek\source\repos\gitoxide\target\debug\gix config
Error: Could not determine trust level for path '.\.git'.

Caused by:
    Couldn't acquire token information

This is a quick initial review. The above finding is enough that I definitely recommend against merging this PR at this time or without changes. However, there remain a number of important cases I have not tested. Furthermore, it looks like there are some cases where this does work better than what we currently have--it's not merely based on theoretical considerations that I say this is directionally correct. But the cases I've tested that work better do not resemble the GitButler situation in any way.

I hope to figure out and/or give more details soon.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

TOKEN_USER::User.Sid points into the buffer returned by GetTokenInformation. Here user_info is first a Vec<u8>, then immediately shadowed by a by-value TOKEN_USER, which drops the Vec<u8> at the shadowing point. That makes token_user a dangling pointer before it’s passed to EqualSid, which is UB and can misclassify ownership. Keep the Vec<u8> alive for at least as long as token_user is used (e.g., bind it to a different name and only shadow after you no longer need the SID pointer, or keep the comparison in a scope that retains the buffer).

Suggested change

	let user_info = ptr::read_unaligned(user_info.as_ptr().cast::<TOKEN_USER>());
	let token_user = user_info.User.Sid;
	let token_user_info = ptr::read_unaligned(user_info.as_ptr().cast::<TOKEN_USER>());
	let token_user = token_user_info.User.Sid;

[Sebastian Thiel (Byron)]

Thanks so much for taking a look! It's also very encouraging to hear that you think this PR could actually improve correctness and bring us closer to parity with Git for Windows.

The fact that it now can fail is probably due to API misuse + the lack of actual testing on the platform (while also not covering enough cases on CI), so I don't know how to properly address this without actually doing development on Windows and having a set of test-cases ready.

That said, I've added more context to all possible failures. So it should now be clearer what exactly didn't work when it doesn't work. The auto-review comments were also addressed, so maybe that helps (but probably not)

(Maybe what I could do is to try to use codex in full agent mode to set up these test cases in a VM)