Add a bunch of missing urllib.parse.quote calls

[ExplodingCabbage]

There were previously lots of places in PyGithub where public methods take a string value and interpolate it directly into a URL without URL-encoding it. This results in a bunch of bugs; for instance, trying to get a branch with a # in its name fails.

It also conceivably introduces some kind of security issue somewhere, since anywhere that an application is passing a user-provided value to one of these methods, a user can exploit this to cause a request to be sent to a malformed URL or to add arbitrary additional path segments to the URL after the interpolated value or to add an arbitrary query string (although I have not been able to think of a realistic exploit based on this, and am not confident that one really exists).

To try to fix this, I did a regex search across the whole codebase for

f"{self.url}/.+{

which finds expressions like

f"{self.url}/labels/{label}"

or

f"{self.url}/comments/{id}"

and audited each such expression. If the value being interpolated into the URL was already being explicitly URL-encoded in the method, I left it alone. If it was guaranteed via an assert isinstance(...) check to be a number, I left it alone. If it was the ._identity property of a PyGithub object whose _identity is guaranteed not to contain any special characters in need of URL-encoding, I left it alone. In all other cases, it was an arbitrary string, and in those cases, I URL-encoded it. I did this even in cases where the argument represented something like a GitHub username which cannot legally contain any special characters that need URL-encoding, for a couple of reasons:

• there might be a security benefit to doing so in those cases, for the reason given above
• if somebody tries to look up an illegal username like foo?bar, it still seems like the correct thing for the library to do is to faithfully encode that username and send it to the API, and get back an appropriate error from GitHub, rather than sending a malformed URL or a URL where ?bar is treated as a query string (which might produce a seemingly-nonsensical error that complains about user foo not existing, or worse, might actually return existing user foo).

I've manually tested my change to check it fixes getting a branch by name when the branch name contains a # character. Here's an attempt to get such a branch without this change:

>>> from github import Github
>>> g = Github()
>>> repo = g.get_repo('ExplodingCabbage/PyGithub')
>>> repo.get_branch('foo#bar')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mark/PyGithub/github/Repository.py", line 1643, in get_branch
    "GET", f"{self.url}/branches/{branch}"
  File "/home/mark/PyGithub/github/Requester.py", line 355, in requestJsonAndCheck
    verb, url, parameters, headers, input, self.__customConnection(url)
  File "/home/mark/PyGithub/github/Requester.py", line 378, in __check
    raise self.__createException(status, responseHeaders, output)
github.GithubException.GithubException: 404 {"message": "Branch not found", "documentation_url": "https://docs.github.com/rest/reference/repos#get-a-branch"}

And here's an attempt with this change:

>>> from github import Github
>>> g = Github()
>>> repo = g.get_repo('ExplodingCabbage/PyGithub')
>>> repo.get_branch('foo#bar')
Branch(name="foo#bar")

I haven't tested anything else, and indeed am not familiar with several of the API calls I've touched in this PR. I'd appreciate if a maintainer familiar with them could review my changes sceptically, and maybe test them if needed.

Note that this is kind of a breaking change, since some users may have worked around this bug by calling urllib.parse.quote on values in their own code before passing them to PyGithub, and such values will now get doubly encoded. As such, this should be called out as a breaking change in the change notes.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ExplodingCabbage]

Go away, stale bot.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ExplodingCabbage]

Any chance of this getting reviewed and merged, @s-t-e-v-e-n-k? It's a bugfix with possible security implications.

(I would be happy to leave for you to handle it whenever you get to it, but, well, by using the stale bot you're not leaving me that choice; I have to either nag you about it every few months or have it be automatically thrown away.)

[EnricoMi]

This is very interesting, thanks for the thorough explanation.

Can you please rerun the regexp with latest main HEAD and also consider the following regexp: f".*/[{]?

Those urls can also be constructed without self.url, which are then completed in Requester.__makeAbsoluteUrl.

[codecov-commenter]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (a48c37f) 96.73% compared to head (5eb0e0a) 96.68%.

Files	Patch %	Lines
github/Repository.py	87.09%	4 Missing ⚠️
github/Team.py	25.00%	3 Missing ⚠️
github/GithubIntegration.py	71.42%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1976      +/-   ##
==========================================
- Coverage   96.73%   96.68%   -0.05%     
==========================================
  Files         142      142              
  Lines       14499    14559      +60     
==========================================
+ Hits        14025    14077      +52     
- Misses        474      482       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ExplodingCabbage]

Okay, I've now:

1. resolved 2.5 years of merge conflicts
2. done a search with the regex you suggested and added a bunch more urllib.parse.quote calls
3. fixed a test failure

Should be ready for another round of review!

[EnricoMi]

Amazing work, thanks for the effort!