Skip to content
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

Use --git-dir to avoid issues with CVE-2022-24765 mitigation #708

Merged
merged 3 commits into from
Jun 21, 2022

Conversation

chrisburr
Copy link
Contributor

This is a possible solution to #707 by explicitly passing --git-dir=$PWD/.git (which bypasses auto-discovery and therefore the vulnerability).

I've tried to add a test for this in b8dcd7a. Obviously it has to change ownership to a non-existant user so it needs to use sudo but I don't particularly like it. I can refactor if you have better ideas or drop the test entirely if you'd prefer.

Copy link
Contributor

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the parameters are needed everywhere, let's consider a helper method that calls git subcommands

def is_dirty(self):
out, _, _ = self.do_ex("git status --porcelain --untracked-files=no")
out, _, _ = self.do_ex(
f"git --git-dir={quote(self._git_dir)} "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the list form here

@@ -92,6 +94,42 @@ def test_parse_call_order(wd):
git.parse(str(wd.cwd), git.DEFAULT_DESCRIBE)


@pytest.mark.issue("https://github.com/pypa/setuptools_scm/issues/707")
def test_not_owner(wd):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's mark this as xfail run=False for now

@henryiii
Copy link
Contributor

If the parameters are needed everywhere, let's consider a helper method that calls git subcommands

I almost suggested this! Something like self._do_git(*cmds). FYI, I've used this method to fix a CMakeLists.txt git usage (unrelated to setuptools_scm) in GooFit after seeing it here. Thanks!

@chrisburr
Copy link
Contributor Author

Thanks for the review @RonnyPfannschmidt.

I think these changes have also fixed the issue with the Windows CI in 2e54cc2.

@henryiii
Copy link
Contributor

henryiii commented May 8, 2022

Gentle reminder, this is broken on some systems using newer git until this patch is made. (cibuildwheel should be fine by matching ownership, but multibuild still has mismatched owners).

chrisjbillington added a commit to rpanderson/workflow-sandbox that referenced this pull request Jun 20, 2022
Due to this issue:

pypa/manylinux#1309

Can likely remove the "pre-build command" once the following is merged
and released in setuptools_scm:

pypa/setuptools-scm#708

Bumping version of actions/checkout may or may not be necessary

Bumping other versions not necessary but why not
@RonnyPfannschmidt
Copy link
Contributor

i just merged this, will be part of the next release

@RonnyPfannschmidt RonnyPfannschmidt merged commit e261b6e into pypa:main Jun 21, 2022
@chrisburr chrisburr deleted the fix-git-perm-issue branch June 21, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants