To add a bit more context: I followed the general approach mentioned by @Byron and used by gitoxide, which is a for loop that iterates over each character, with memory of the previous two characters seen. This is faster than the naive approach (since we minimize the amount of times we iterate over the refname string), at the cost of some readability.

For comparison, here's the naive approach, where the logic separation matches the docs (one rule per if condition):

def _check_ref_name_valid_naive(ref_path: PathLike) -> None:
    # Based on https://git-scm.com/docs/git-check-ref-format/
    if any([component.startswith(".") or component.endswith(".lock") for component in ref_path.split("/")]):
        raise ValueError(f"Invalid reference '{ref_path}': components cannot start with '.' or end with '.lock'")
    elif ".." in str(ref_path):
        raise ValueError(f"Invalid reference '{ref_path}': references cannot contain '..'")
    elif any([ord(c) < 32 or ord(c) == 127 or c in [" ", "~", "^", ":"] for c in ref_path]):
        raise ValueError(
            f"Invalid reference '{ref_path}': references cannot contain ASCII control characters, spaces, tildes (~), carets (^) or colons (:)"
        )
    elif any([c in ["?", "*", "["] for c in ref_path]):
        raise ValueError(
            f"Invalid reference '{ref_path}': references cannot contain question marks (?), asterisks (*) or open brackets ([)"
        )
    elif ref_path.startswith("/") or ref_path.endswith("/") or "//" in ref_path:
        raise ValueError(f"Invalid reference '{ref_path}': references cannot start or end with '/', or contain '//")
    elif ref_path.endswith("."):
        raise ValueError(f"Invalid reference '{ref_path}': references cannot end with '.'")
    elif "@{" in ref_path:
        raise ValueError(f"Invalid reference '{ref_path}': references cannot contain '@{{'")
    elif ref_path == "@":
        raise ValueError(f"Invalid reference '{ref_path}': references cannot be '@'")
    elif "\\" in ref_path:
        raise ValueError(f"Invalid reference '{ref_path}': references cannot contain '\\'")

The naive approach is IMO more readable, but around half as fast as the one in the PR. Although, for reference, in my MacBook M1 Pro, for a refname 25 characters long:

• Time to validate using approach in PR: 2.48us
• Time to validate using naive approach: 4.69us

So we are talking about minimal amounts either way. I'll leave the choice of which algorithm to use up to the maintainers.

It looks like CI improved and now that the PR was merged, it failed CI due to a lint: https://github.com/gitpython-developers/GitPython/actions/runs/6271134895/job/17030195508#step:4:122 . A quick fix will be appreciated.

Edit: I quickly fixed it myself - it seems like sometimes I forget that I am still able to edit text, despite it being python.

@Byron Because the forthcoming 3.1.37 release that will include this patch will be a security fix, either the existing advisory/CVE should be updated with a correction (both a note and the version change), or a new CVE should be created for the variant of the vulnerability reported at #1644 (comment). I am not sure which of those things should be done here.

Usually I would lean toward regarding such things as new bugs meriting new advisories/CVEs, which is also what I see more often. But I do not know that that's the best approach here, because the variant of the exploit where an absolute (or otherwise non-relative) path is used does seem to match the description in the summary section of CVE-2023-41040 even though it doesn't resemble any of the examples. To be clear, I don't mean that this situation is necessarily ambiguous, but instead that I do not have the knowledge and experience to know how it ought to be handled. Either way, this need not delay the release, of course.

(Sorry if you're already on top of the CVE/advisory matter and this comment is just noise.)

Thanks for the hint, it's appreciated!

I think it's fair to say that I am not on top of CVEs and that I have no intention to be - even though this sounds harsh it's just the current reality. But thus far members of the community picked up the necessary work around CVEs which I definitely appreciate if this would keep happening.

A new release was created: https://pypi.org/project/GitPython/3.1.37/

Given the 3.1.37 release title ("3.1.37 - a proper fix CVE-2023-41040") I'm thinking this is intuitively being regarded as fixing the originally reported vulnerability, so perhaps that advisory should be updated, rather than a new one created? I am still not sure.

As noted in #1638 (comment), you (@Byron) could update the local advisory. If you do, a PR could then also be opened on https://github.com/github/advisory-database (where github/advisory-database#2690 was opened) to change the global advisory accordingly. I don't know if there's anything else that would need to be done.

@stsewd Do you have any opinion about what ought to be done here? Would you have any objection to the local advisory being edited this way? Would you instead prefer that this variant, where an absolute path is used, be regarded as a related but separate vulnerability altogether? (I know @Byron can edit the advisory, but I wanted to check in case you had an opinion on this.)

One source of my hesitancy here is that I think a new CVE may still be needed in this kind of situation. That seems common (courtesy of this SO answer).

As noted in #1638 (comment), you (@Byron) could update the local advisory. If you do, a PR could then also be opened on https://github.com/github/advisory-database (where github/advisory-database#2690 was opened) to change the global advisory accordingly. I don't know if there's anything else that would need to be done.

A good point - I am still getting used to advisories and the local ones are indeed editable. So that one has been adjusted. I kindly ask somebody else to create a PR for the global database though - it seems GitHub makes it hard/impossible to the use web interface for that.

One source of my hesitancy here is that I think a new CVE may still be needed in this kind of situation.

To me, CVEs are good to create a far-reaching 'ping' to users of GitPython. Some might see it earlier than the new release. To me it's the question on how much time one wants to spend to create such a ping, and judging from the CVE's I have seen, it's quite expensive.

A good point

Thanks, but I'm not actually sure if it was a good point. Maybe a new advisory ought to have been, or ought to be, created. I really don't know the proper thing to do here.

If there is an uproar because of how this was handled, it will be possible to undo changes to the local CVE and create a new one. So I think nothing is lost, and I think it's OK to chose less expensive options in dealing with this.

Hi, a new CVE/advisory is usually created for this type of situation, and in the description you can put something like "this was a due to an incomplete fix of [link to the other CVE]". I don't oppose to edit the current one, but I guess editing doesn't have the same "ping to everyone to upgrade" effect as a new one.

Hi, a new CVE/advisory is usually created for this type of situation, and in the description you can put something like "this was a due to an incomplete fix of [link to the other CVE]". I don't oppose to edit the current one, but I guess editing doesn't have the same "ping to everyone to upgrade" effect as a new one.

Thanks!

@Byron Based on this, and also what I am now seeing is the recent history of this practice being followed for GitPython in CVE-2022-24439/CVE-2023-40267, I recommend making a new advisory. Maybe there is some way I can help with this?

However, if for any reason you would still prefer this route not be taken, then I can definitely go ahead and open a PR to update the global advisory with the version change. (I am unsure if that would cause Dependabot to notify users of the security update or not, but I imagine that, if it would not, then a reviewer on the PR would mention that.)

But thus far members of the community picked up the necessary work around CVEs which I definitely appreciate if this would keep happening.

I have three ideas of what I could do, but I don't know what, if any of them, would help or be wanted. This depends, in part, on what takes up the time for you.

1. If the issue is drafting the text of the advisory, I can write a draft and propose that, here, to you. (I considered doing that for this comment, but I figured it would be better to ask first.) You would still have to create the advisory and request the CVE in the same way as you did for CVE-2023-41040.
2. If the issue is the process after that, then I might be able to actually request the CVE. Although GitHub is a CNA, I don't think they provide a way to request a CVE except by a maintainer and through the interface you have used. MITRE is a CNA and I've heard of non-maintainers requesting CVEs from them successfully. However, I am unsure if they would accept such a request from me, because I have no specific connection to this vulnerability (I did not discover, report, analyze, fix, or integrate a fix for it). In addition, if I make the request, then I would first want to ask you some questions about how a situation would arise where someone could exploit this vulnerability without otherwise already being able to open files outside the local repository's .git directory, to ensure that I would be able to stand fully by any statements I would make in the request and afterwards. Given that, I am unsure to what extent this option would save you effort.
3. Combination of 1 and 2: I could draft a new advisory, and you could create and publish the new advisory based on my draft (with any modifications you deem appropriate) via the GitHub interface, but not request a CVE for it. Even at this point something would have been achieved, I believe, because within the GitHub ecosystem (e.g., for Dependabot), I think alerts would be generated once it makes its way into the GitHub Advisory Database. Then I could attempt to request a CVE from some CNA, which if/when assigned could be associated with the advisory.

@Byron Based on this, and also what I am now seeing is the recent history of this practice being followed for GitPython in CVE-2022-24439/CVE-2023-40267, I recommend making a new advisory. Maybe there is some way I can help with this?

However, if for any reason you would still prefer this route not be taken, then I can definitely go ahead and open a PR to update the global advisory with the version change. (I am unsure if that would cause Dependabot to notify users of the security update or not, but I imagine that, if it would not, then a reviewer on the PR would mention that.)

Let's try something: updating version numbers is much cheaper than creating a new 'follow-up' CVE, for all sides actually. One could ask in the PR of the version change if notifications will be sent, and if unknown, @stsewd could probably help to tell as well.

If no notification is sent, you could create a new CVE - you would be able to do this here in GitPython and from there it can be elevated, along with requesting a global CVE for it - this is easily done through the maintainer interface. The rest we can take from there should it come to that.

Let's try something: updating version numbers is much cheaper than creating a new 'follow-up' CVE, for all sides actually.

Sounds good; I will do this.

I noticed in the local advisory that, while 3.1.37 is given for patched versions, <=3.1.34 is still given for affected versions. I recommend changing that <=3.1.34 here to <=3.1.36 for consistency, and I'll specify <=3.1.36 in my proposed change to the global advisory. But if that is no the case and you want that specified differently, please let me know and I'll do differently (or amend the PR if I have already made it).

One could ask in the PR of the version change if notifications will be sent

I'm making the PR through the structured "Suggest improvements" template, in which every field is pretty specific. I'll either include it somewhere if it fits, or otherwise try and add it into the created PR or add a comment with it.

If no notification is sent, you could create a new CVE - you would be able to do this here in GitPython and from there it can be elevated, along with requesting a global CVE for it - this is easily done through the maintainer interface. The rest we can take from there should it come to that.

Thanks for telling me about that. That is much nicer than the particular specific I had suggested might be used. Of course, I'll still save that for if the above proves insufficient, as you say.

I noticed in the local advisory that, while 3.1.37 is given for patched versions, <=3.1.34 is still given for affected versions. I recommend changing that <=3.1.34 here to <=3.1.36 for consistency, and I'll specify <=3.1.36 in my proposed change to the global advisory. But if that is no the case and you want that specified differently, please let me know and I'll do differently (or amend the PR if I have already made it).

Thanks for the head's up, that's an oversight that is now corrected.

And thanks again for your help!

No problem! I've submitted the proposed edit to the global advisory in PR github/advisory-database#2753.

github/advisory-database#2753 has been merged and the global GitHub advisory for CVE-2023-41040 has thus been updated.

Further update: The change to the global advisory has caused Dependabot security alerts to be raised, as desired. For example, dmvassallo/EmbeddingScratchwork#248 is a PR opened automatically to resolve a new Dependabot security alert in a project where GitPython had already been upgraded to the previously listed version. Note that this does not necessarily apply for tools that are less closely coupled to the GitHub ecosystem, and I don't know, for example, if any new Renovatebot PRs will be generated.