pip doesn't properly parse git URL if branch name contains @ or #

[phdru]

Description

Trying pip install git+https://example.com/repository@branch fails if branch contains characters @ or #, even percent-encoded.

Expected behavior

pip must parse percent-encoded special characters in branch name, split the branch name from the URL, clone the repository and checkout the named branch with special characters decoded. I.e.

pip install https://example.com/repository@master%40test

must clone https://example.com/repository and checkout master@test branch. The same for # character %-encoded as %23.

pip version

Any; tested with 21.1.3

Python version

Any; tested with Python 3.9

OS

Any; tested with Debian 10 buster

How to Reproduce

Here is a test program test-pip-git that creates a repository, tries pip download and cleanups:

#! /bin/sh
set -e

PERCENT_ENCODING=0
while getopts p: opt; do
    case $opt in
        p ) PERCENT_ENCODING="${OPTARG:-1}" ;;
    esac
done
shift `expr $OPTIND - 1`

if [ -z "$1" ]; then
    echo "Usage: $0 [-p1|2] test_char" >&2
    exit 1
fi

TEST_CHAR1="$1"
if [ $PERCENT_ENCODING -ge 1 ]; then

    py_ver=`python -c "import sys; print(sys.version_info[0])"`
    if [ $py_ver -eq 2 ]; then
        percent_encode() {
            python -c "import urllib; print(urllib.quote('$1'))"
        }
    elif [ $py_ver -eq 3 ]; then
        percent_encode() {
            python -c "import urllib.parse; print(urllib.parse.quote('$1'))"
        }
    else
        echo "Unknown python version" >&1
        exit 1
    fi
    TEST_CHAR2=`percent_encode "$1"`
    if [ $PERCENT_ENCODING -eq 2 ]; then
        TEST_CHAR2=`percent_encode "$TEST_CHAR2"`
    fi
else
    TEST_CHAR2="$1"
fi

rm -rf test-pip-git-repo test-pip-git-spec-char-0.0.1.zip
git init test-pip-git-repo
cd test-pip-git-repo

echo test >test
git add test
git commit -m test

git branch -M master # to fixed name
git checkout -b test${TEST_CHAR1}test # new branch

cat >setup.py <<EOF
#!/usr/bin/env python

from setuptools import setup

setup(
    name='test_pip_git_spec_char',
    version='0.0.1',
    description='Test pip+git+special characters',
    author='Oleg Broytman',
    author_email='phd@phdru.name',
    keywords=['pip', 'git', '@', '!', '#', '/'],
    platforms='Any',
)
EOF

git add setup.py
git commit -m setup.py
git checkout master # make test branch non-current

cd ..
pip download git+file://`pwd`/test-pip-git-repo@test${TEST_CHAR2}test | grep '\(clone\|checkout\)' || : # ignore errors

rm -rf test-pip-git-repo test-pip-git-spec-char-0.0.1.zip

Output

./test-pip-git @

Running command git clone -q file:///home/phd/tmp/test-pip-git-repo@master /tmp/pip-req-build-v1v16zoe
fatal: '/home/phd/tmp/test-pip-git-repo@master' does not appear to be a git repository

pip clones incorrect repository test-pip-git-repo@master; the repo must be test-pip-git-repo.

./test-pip-git -p1 @

Running command git clone -q file:///home/phd/tmp/test-pip-git-repo@master /tmp/pip-req-build-__0b6wh5
fatal: '/home/phd/tmp/test-pip-git-repo@master' does not appear to be a git repository

The same incorrect repo.

./test-pip-git \!

Running command git clone -q file:///home/phd/tmp/test-pip-git-repo /tmp/pip-req-build-fsnq5vt_
Running command git checkout -b 'master!test' --track 'origin/master!test'

Just a test with another less special character. pip clones correct repository test-pip-git-repo and checks out correct branch master!test. Doesn't even require %-encoding. Test passed!

./test-pip-git \#

Running command git clone -q file:///home/phd/tmp/test-pip-git-repo /tmp/pip-req-build-i0wy10_1
ERROR: File "setup.py" not found

pip clones correct repository test-pip-git-repo but doesn't check out branch master#test. It just uses branch master and ignores everything after #.

./test-pip-git -p1 \#

Exactly the same problem.

Code of Conduct

• I agree to follow the PSF Code of Conduct.

[phdru]

Double percent-encoding (% is replaced with %25):

./test-pip-git -p2 @

Running command git clone -q file:///home/phd/tmp/test-pip-git-repo /tmp/pip-req-build-3hbwu2hb
WARNING: Did not find branch or tag 'master%40test', assuming revision or ref.
Running command git checkout -q master%40test
error: pathspec 'master%40test' did not match any file(s) known to git

./test-pip-git -p2 \#

Running command git clone -q file:///home/phd/tmp/test-pip-git-repo /tmp/pip-req-build-oapmtjix
WARNING: Did not find branch or tag 'master%23test', assuming revision or ref.
Running command git checkout -q master%23test
error: pathspec 'master%23test' did not match any file(s) known to git

This is very close — pip clones the proper repo and tries to checkout almost proper branch.

This suggests that the bug is triggered because pip splits the branch name off of the URL too late, after percent-decoding the URL. If I'm right I think the order should be changed: first urlsplit the URL, extract the branch name from the path, urljoin protocol, user name, password, host, port and path to reconstruct the URL without the branch and #egg fragment, git clone the URL, percent-decode branch name, directory and fragment (#egg=) and checkout the branch.

[uranusjr]

Thanks for looking into this! I took a quick look at the implementation and, well, let’s say the original code author probably didn’t think too hard before deciding on the syntax and implementation. Both @ and / are unfortunately perfectly valid characters in a Git branch name and the path part of the URL, so there are quite a few problematic edge cases:

• https://domain/@username/repo.git (implies default branch)
• https://domain/username/repo.git@branch
• https://domain/username/repo.git@branch/with/slash
• https://domain/username/repo.git@branch-with-#-a-hash

I think at this point, the only reasonable-ish approach is to require the user to escape @ in both the URL and the branch name, since / is quite common in both the path (obviously) and the branch (many people use patterns like stable/1.2.3), and pip should unescape when parsing the branch out. The parsing part is in VersionControl.get_url_rev_and_auth, so we should add a urllib.parse.unquote call somewhere in there. Would you be interested in working on a PR and some test cases to cover this? I would be happy to offer assistance.

[phdru]

Would you be interested in working on a PR and some test cases to cover this?

Yes but not immediately. I am busy with other projects and this bug is not of high priority for me; it's not even my bug, I reported it from https://stackoverflow.com/q/68529806/7976758 (I am phd there). Do not expect a pull request in the nearest future. I hope to find time some day.

[uranusjr]

No problem, we all have limited time 🙂 Looking forward to hearing back then!

[phdru]

I updated the script above to test slashes. The test passed. It seems slashes aren't special character for pip. Don't even need to be %-encoded.

[uranusjr]

Not surprising; / is a rather popular character in git branches, we would've heard complaints if it needs escaping.

[AbhinavOmprakash]

hi @uranusjr, since I have time, I would like to work on this, with your help.
I think it's possible to come up with a solution that does not involve escaping @. I tried implementing a solution, that passes the following test cases (which are taken from the ones you mentioned) also, the solution doesn't break any other tests.

@pytest.mark.parametrize(
    "url, expected",
    [
        (
            "git+https://domain/@username/repo.git",
            ("https://domain/username/repo.git", None, (None, None)),
        ),
        (
            "git+https://domain/@username/repo.git@branch",
            ("https://domain/username/repo.git", "branch", (None, None)),
        ),
        (
            "git+https://domain/@username/repo.git@br@nch",
            ("https://domain/username/repo.git", "br@nch", (None, None)),
        ),
        (
            "git+https://domain/@username/repo.git@br@nch#1.2.3",
            ("https://domain/username/repo.git", "br@nch#1.2.3", (None, None)),
        ),
        (
            "git+https://domain/username/repo.git@branch#1.2.3",
            ("https://domain/username/repo.git", "branch#1.2.3", (None, None)),
        ),
        (
            "git+https://domain/username/repo.git@branch/1.2.3",
            ("https://domain/username/repo.git", "branch/1.2.3", (None, None)),
        ),
    ],
)
def test_version_control__get_url_rev_and_auth_with_special_char_in_username_and_branch(url, expected):
    actual = VersionControl.get_url_rev_and_auth(url)
    assert actual == expected

the implementation is still hacky and needs to be cleaned up, but if you think it is viable then I could make a PR and we could work on it there.

    @classmethod
    def get_url_rev_and_auth(cls, url):
        # type: (str) -> Tuple[str, Optional[str], AuthInfo]
        """
        Parse the repository URL to use, and return the URL, revision,
        and auth info to use.

        Returns: (url, rev, (username, password)).
        """
        scheme, netloc, path, query, frag = urllib.parse.urlsplit(url)
        if "+" not in scheme:
            raise ValueError(
                "Sorry, {!r} is a malformed VCS url. "
                "The format is <vcs>+<protocol>://<url>, "
                "e.g. svn+http://myrepo/svn/MyApp#egg=MyApp".format(url)
            )
        # Remove the vcs prefix.
        scheme = scheme.split("+", 1)[1]
        netloc, user_pass = cls.get_netloc_and_auth(netloc, scheme)
        rev = None
        if "@" in path:
            path, rev = path.split("@", 1)
            # "." is valid in rev if its a version number.
            # test if "." is part of url not version number
            if re.findall(r"(?<!\d)\.(?!\d)", rev):
                if "@" in rev: # the second @ would be before a branch
                    rem_path, rev = rev.split("@",1)
                    path += rem_path

                else: 
                    # if there isn't an @ in rev. that means the first @ is used to denote a username
                    # so join the path and the revision, and strip the @
                    path = path+rev 
                    path = path.replace("@","") 
                    rev = None
                

            elif not rev:
                raise InstallationError(
                    "The URL {!r} has an empty revision (after @) "
                    "which is not supported. Include a revision after @ "
                    "or remove @ from the URL.".format(url)
                )

            if frag and rev:  
                # incase branch name has a '#' in it, it will be stored in frag
                # so append it to rev.
                if not "egg" in frag:
                    rev += "#"+frag

        url = urllib.parse.urlunsplit((scheme, netloc, path, query, ""))
        return url, rev, user_pass

[uranusjr]

# "." is valid in rev if its a version number.
# test if "." is part of url not version number
if re.findall(r"(?<!\d)\.(?!\d)", rev):

What does this part cover? My intuition is this is trying too hard to be smart and could end up being unintuitive, but this depends on the specific situations you want to solve.

if frag and rev:  
    # incase branch name has a '#' in it, it will be stored in frag
    # so append it to rev.
    if not "egg" in frag:
        rev += "#"+frag

This is definitely trying too hard to be smart. # is not a URL-safe character, so the user is expected to escape it if it's used in a branch name (and we should unescape it). I don't think we should do this.

[AbhinavOmprakash]

okay, we'll proceed with the escaping solution.

Just to clarify, both @ and # need to be escaped with \?

which would mean the edge cases you listed would look like this.

https://domain/\@username/repo.git (implies default branch)
https://domain/username/repo.git\@branch
https://domain/username/repo.git\@branch/with/slash
https://domain/username/repo.git\@branch-with-\#-a-hash

let me know if there's anything else I need to look for or consider.

[uranusjr]

Not \. Special characters in the URL need to be percent-encoded.