Skip to content

Interaction between 'safe.directory' and trust level and remotes. #1912

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

Open
TyberiusPrime opened this issue Mar 28, 2025 · 3 comments · Fixed by #1964
Open

Interaction between 'safe.directory' and trust level and remotes. #1912

TyberiusPrime opened this issue Mar 28, 2025 · 3 comments · Fixed by #1964
Assignees
Labels
acknowledged an issue is accepted as shortcoming to be fixed help wanted Extra attention is needed

Comments

@TyberiusPrime
Copy link

TyberiusPrime commented Mar 28, 2025

Current behavior 😯

(version 0.70.0)

When a .git is not owned by the current user,
but is listed in safe.directories,
open(repo).remote_names() returns an empty list.

bail_if_untrusted(true) does not seem to make a difference.

See jj-vcs/jj#6155

Expected behavior 🤔

If the directory is in safe.directories, I'd expect it to be trusted.
Or, if gitoxide doesn't read safe.directories, I'd expect it to fail if bail_if_untrusted is set.

Git behavior

Git lists the remotes in both cases.

Steps to reproduce 🕹

  1. git init owned; cd owned; git remote add origin https://github.com/GitoxideLabs/gitoxide; cd ..
  2. git init non_owned; cd non_owned; git remote add origin https://github.com/GitoxideLabs/gitoxide; cd ..; sudo chown -R root non_owned
fn main() {
    println!("using owned repo");
    {
        let repo_owned = gix::open("owned").expect("could not opon owned repo");
        let rn = repo_owned.remote_names();
        println!("found {} remotes", rn.len());
        for remote in rn {
            println!("remote: {}", remote);
        }
    }

    println!("\n\n");
    {
        println!("using non-owned repo, which is in .gitconfig/safe.directories");
        let repo_non_owned = gix::open("non_owned").expect("could not opon non-owned repo");
        let rn = repo_non_owned.remote_names();
        println!("found {} remotes", rn.len());
        for remote in rn {
            println!("remote: {}", remote);
        }
    }

    println!("\n\n");
    {
        println!(
            "using non-owned repo, which is in .gitconfig/safe.directories, + bail_if_untrusted"
        );
        let repo_non_owned =
            gix::open_opts("non_owned", gix::open::Options::default().bail_if_untrusted(true))
                .expect("could not opon non-owned repo wit bail-if-untrused");
        let rn = repo_non_owned.remote_names();
        println!("found {} remotes", rn.len());
        for remote in rn {
            println!("remote: {}", remote);
        }
    }
}
@Byron Byron added the acknowledged an issue is accepted as shortcoming to be fixed label Mar 29, 2025
@Byron Byron self-assigned this Mar 29, 2025
@Byron
Copy link
Member

Byron commented Mar 29, 2025

Thanks a lot for bringing this to my attention.

Interestingly just a couple of days ago I thought about this for no apparent reason and I wondered the same - does safeDirectories affect the current implementation in terms of trust?

And even though something is implemented in that regard (I'd have to check for details), I don't think that configuration files are affected enough, or it's a bug.

I put this on my list, but it's long and any help is appreciated.

@Byron Byron added the help wanted Extra attention is needed label Mar 29, 2025
Byron added a commit that referenced this issue Apr 21, 2025
This means that repo-local configuration that is considered safe, ideally with
`safe.directory=safe/dir/*` notation, will be usable for sensitive operations.
@Byron Byron mentioned this issue Apr 21, 2025
@Byron
Copy link
Member

Byron commented Apr 21, 2025

Even though this is fixed, I'd really like some help with figuring out a way to test this, on any platform.
Thus far I made the fix with manual tests only, so a regression is absolutely possible right now.

@Byron
Copy link
Member

Byron commented Apr 21, 2025

On another note, and something that might require a follow-up: configuration is only marked as safe if either one uses prefix-based directory specifications , like /this/is/safe/*, or if one provides the path to the configuration file in addition to the Git directory, as in [safe] directory = /path/to/.git and [safe] directory = /path/to/.git/config.

The latter is probably unintuitive if one wants to be euphemistic, or a bug if one wants to be realistic.
And now that I have written this it's hard to not fix it immediately. Now that I see this code I also realize that the implementation is currently incorrect compared to what Git does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged an issue is accepted as shortcoming to be fixed help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants