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

Raise error on unsupported redirects, log supported redirects #2524

Merged
merged 3 commits into from May 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 32 additions & 5 deletions github/Requester.py
Expand Up @@ -386,7 +386,7 @@ def _refresh_token_if_needed(self) -> None:
if not self.__installation_authorization:
return
if self._must_refresh_token():
logging.debug("Refreshing access token")
self._logger.debug("Refreshing access token")
self._refresh_token()

def _refresh_token(self) -> None:
Expand Down Expand Up @@ -617,7 +617,30 @@ def __requestRaw(self, cnx, verb, url, requestHeaders, input):
return self.__requestRaw(original_cnx, verb, url, requestHeaders, input)

if status == 301 and "location" in responseHeaders:
o = urllib.parse.urlparse(responseHeaders["location"])
location = responseHeaders["location"]
o = urllib.parse.urlparse(location)
if o.scheme != self.__scheme:
raise RuntimeError(
f"Github server redirected from {self.__scheme} protocol to {o.scheme}, "
f"please correct your Github server URL via base_url: Github(base_url=...)"
)
if o.hostname != self.__hostname:
raise RuntimeError(
f"Github server redirected from host {self.__hostname} to {o.hostname}, "
f"please correct your Github server URL via base_url: Github(base_url=...)"
)
if o.path == url:
port = ":" + str(self.__port) if self.__port is not None else ""
requested_location = f"{self.__scheme}://{self.__hostname}{port}{url}"
raise RuntimeError(
f"Requested {requested_location} but server redirected to {location}, "
f"you may need to correct your Github server URL "
f"via base_url: Github(base_url=...)"
)
Comment on lines +622 to +639
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it may be fixing a potential security vulnerability where credentials could be leaked to the server being redirected to? Do you agree?

This would be similar to this vulnerability in curl: https://curl.se/docs/CVE-2022-27774.html

Choose a reason for hiding this comment

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

I discovered an infinite loop this PR fixes with a self hosted Github Enterprise Server (GHES) instance, it was not due to a vulnerability. Our appliance is reachable under several host names but the default GHES option is to send a 302/301 redirect if the Host header doesn't match the expected value. This can be seen if you attempt to connect to www.github.com with the simple reproducer:

from github import Github

# Github Enterprise with custom hostname
g = Github(base_url="https://www.github.com/api/v3",
#                            ^^^^ notice the www.
           login_or_token="doesn't matter what it is",)
print(g.get_user().name)

I welcome the RuntimeErrors seen here because this happens periodically and is hard to remember that the infinite loop is caused by a mismatched host name value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this fix is not trying to fix a vulnerability. Such redirects caused an infinite loop and a non-descriptive error. So they now fail on the first response and explain the situation.

PyGithub redirects any 301 response and sends the headers to the new location, but it only considered the path of the new location. So it reused the original hostname and port and protocol, so did not leak anything. Now it only redirects if hostname and port and protocol are identical, so there is no functional change (except for better error messages in the error case).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me!

if self._logger.isEnabledFor(logging.INFO):
self._logger.info(
f"Following Github server redirection from {url} to {o.path}"
)
return self.__requestRaw(original_cnx, verb, o.path, requestHeaders, input)

return status, responseHeaders, output
Expand Down Expand Up @@ -671,10 +694,14 @@ def __createConnection(self):

return self.__connection

def __log(self, verb, url, requestHeaders, input, status, responseHeaders, output):
@property
def _logger(self):
if self.__logger is None:
self.__logger = logging.getLogger(__name__)
if self.__logger.isEnabledFor(logging.DEBUG):
return self.__logger

def __log(self, verb, url, requestHeaders, input, status, responseHeaders, output):
if self._logger.isEnabledFor(logging.DEBUG):
headersForRequest = requestHeaders.copy()
if "Authorization" in requestHeaders:
if requestHeaders["Authorization"].startswith("Basic"):
Expand All @@ -689,7 +716,7 @@ def __log(self, verb, url, requestHeaders, input, status, responseHeaders, outpu
headersForRequest[
"Authorization"
] = "(unknown auth removed)" # pragma no cover (Cannot happen, but could if we add an authentication method => be prepared)
self.__logger.debug(
self._logger.debug(
"%s %s://%s%s %s %s ==> %i %s %s",
verb,
self.__scheme,
Expand Down
4 changes: 4 additions & 0 deletions github/Requester.pyi
@@ -1,5 +1,6 @@
from collections import OrderedDict
from io import BufferedReader
from logging import Logger
from typing import Any, Callable, Dict, Iterator, Optional, Tuple, Union

from requests.models import Response
Expand Down Expand Up @@ -51,6 +52,7 @@ class HTTPSRequestsConnectionClass:
class Requester:
__installation_authorization: Optional[InstallationAuthorization] = ...
__app_auth: Optional[AppAuthentication] = ...
__logger: Logger
def DEBUG_ON_RESPONSE(
self, statusCode: int, responseHeader: Dict[str, str], data: str
) -> None: ...
Expand Down Expand Up @@ -85,6 +87,8 @@ class Requester:
headers: Dict[str, Any],
output: str,
) -> Any: ...
@property
def _logger(self) -> Logger: ...
def __log(
self,
verb: str,
Expand Down
18 changes: 14 additions & 4 deletions github/__init__.py
Expand Up @@ -72,12 +72,22 @@
from .InputGitAuthor import InputGitAuthor
from .InputGitTreeElement import InputGitTreeElement

# set log level to INFO for github
logger = logging.getLogger("github")
logger.setLevel(logging.INFO)
logger.addHandler(logging.StreamHandler())


def set_log_level(level: int):
"""
Set the log level of the github logger, e.g. set_log_level(logging.WARNING)
:param level: log level
"""
logger.setLevel(level)


def enable_console_debug_logging(): # pragma no cover (Function useful only outside test environment)
"""
This function sets up a very simple logging configuration (log everything on standard output) that is useful for troubleshooting.
"""

logger = logging.getLogger("github")
logger.setLevel(logging.DEBUG)
logger.addHandler(logging.StreamHandler())
set_log_level(logging.DEBUG)
11 changes: 11 additions & 0 deletions tests/ReplayData/Requester.testBaseUrlHostRedirection.txt
@@ -0,0 +1,11 @@
https
GET
www.github.com
None
/repos/PyGithub/PyGithub
{'User-Agent': 'PyGithub/Python'}
None
301
[('Content-Length', '0'), ('Location', 'https://github.com/repos/PyGithub/PyGithub')]


11 changes: 11 additions & 0 deletions tests/ReplayData/Requester.testBaseUrlPortRedirection.txt
@@ -0,0 +1,11 @@
https
GET
api.github.com
None
/repos/PyGithub/PyGithub
{'User-Agent': 'PyGithub/Python'}
None
301
[('Content-Length', '0'), ('Location', 'https://api.github.com:443/repos/PyGithub/PyGithub')]


22 changes: 22 additions & 0 deletions tests/ReplayData/Requester.testBaseUrlPrefixRedirection.txt
@@ -0,0 +1,22 @@
https
GET
api.github.com
None
/api/v3/repos/PyGithub/PyGithub
{'User-Agent': 'PyGithub/Python'}
None
301
[('Content-Length', '0'), ('Location', 'https://api.github.com/repos/PyGithub/PyGithub')]


https
GET
api.github.com
None
/repos/PyGithub/PyGithub
{'User-Agent': 'PyGithub/Python'}
None
200
[('Server', 'GitHub.com'), ('Date', 'Tue, 09 May 2023 08:03:55 GMT'), ('Content-Type', 'application/json; charset=utf-8'), ('Cache-Control', 'public, max-age=60, s-maxage=60'), ('Vary', 'Accept, Accept-Encoding, Accept, X-Requested-With'), ('ETag', 'W/"c4bbbf57b6ff21caac0b59dec0ae83b3b9e66a234db40e22ee60c1b26b771a3f"'), ('Last-Modified', 'Tue, 09 May 2023 07:44:21 GMT'), ('X-GitHub-Media-Type', 'github.v3; format=json'), ('x-github-api-version-selected', '2022-11-28'), ('Access-Control-Expose-Headers', 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'), ('Access-Control-Allow-Origin', '*'), ('Strict-Transport-Security', 'max-age=31536000; includeSubdomains; preload'), ('X-Frame-Options', 'deny'), ('X-Content-Type-Options', 'nosniff'), ('X-XSS-Protection', '0'), ('Referrer-Policy', 'origin-when-cross-origin, strict-origin-when-cross-origin'), ('Content-Security-Policy', "default-src 'none'"), ('Content-Encoding', 'gzip'), ('X-RateLimit-Limit', '60'), ('X-RateLimit-Remaining', '55'), ('X-RateLimit-Reset', '1683622021'), ('X-RateLimit-Resource', 'core'), ('X-RateLimit-Used', '5'), ('Accept-Ranges', 'bytes'), ('Transfer-Encoding', 'chunked'), ('X-GitHub-Request-Id', 'C22C:5529:6AEA2F:6BEA9A:6459FE6B')]
{"id":3544490,"node_id":"MDEwOlJlcG9zaXRvcnkzNTQ0NDkw","name":"PyGithub","full_name":"PyGithub/PyGithub","private":false,"owner":{"login":"PyGithub","id":11288996,"node_id":"MDEyOk9yZ2FuaXphdGlvbjExMjg4OTk2","avatar_url":"https://avatars.githubusercontent.com/u/11288996?v=4","gravatar_id":"","url":"https://api.github.com/users/PyGithub","html_url":"https://github.com/PyGithub","followers_url":"https://api.github.com/users/PyGithub/followers","following_url":"https://api.github.com/users/PyGithub/following{/other_user}","gists_url":"https://api.github.com/users/PyGithub/gists{/gist_id}","starred_url":"https://api.github.com/users/PyGithub/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PyGithub/subscriptions","organizations_url":"https://api.github.com/users/PyGithub/orgs","repos_url":"https://api.github.com/users/PyGithub/repos","events_url":"https://api.github.com/users/PyGithub/events{/privacy}","received_events_url":"https://api.github.com/users/PyGithub/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/PyGithub/PyGithub","description":"Typed interactions with the GitHub API v3","fork":false,"url":"https://api.github.com/repos/PyGithub/PyGithub","forks_url":"https://api.github.com/repos/PyGithub/PyGithub/forks","keys_url":"https://api.github.com/repos/PyGithub/PyGithub/keys{/key_id}","collaborators_url":"https://api.github.com/repos/PyGithub/PyGithub/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/PyGithub/PyGithub/teams","hooks_url":"https://api.github.com/repos/PyGithub/PyGithub/hooks","issue_events_url":"https://api.github.com/repos/PyGithub/PyGithub/issues/events{/number}","events_url":"https://api.github.com/repos/PyGithub/PyGithub/events","assignees_url":"https://api.github.com/repos/PyGithub/PyGithub/assignees{/user}","branches_url":"https://api.github.com/repos/PyGithub/PyGithub/branches{/branch}","tags_url":"https://api.github.com/repos/PyGithub/PyGithub/tags","blobs_url":"https://api.github.com/repos/PyGithub/PyGithub/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/PyGithub/PyGithub/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/PyGithub/PyGithub/git/refs{/sha}","trees_url":"https://api.github.com/repos/PyGithub/PyGithub/git/trees{/sha}","statuses_url":"https://api.github.com/repos/PyGithub/PyGithub/statuses/{sha}","languages_url":"https://api.github.com/repos/PyGithub/PyGithub/languages","stargazers_url":"https://api.github.com/repos/PyGithub/PyGithub/stargazers","contributors_url":"https://api.github.com/repos/PyGithub/PyGithub/contributors","subscribers_url":"https://api.github.com/repos/PyGithub/PyGithub/subscribers","subscription_url":"https://api.github.com/repos/PyGithub/PyGithub/subscription","commits_url":"https://api.github.com/repos/PyGithub/PyGithub/commits{/sha}","git_commits_url":"https://api.github.com/repos/PyGithub/PyGithub/git/commits{/sha}","comments_url":"https://api.github.com/repos/PyGithub/PyGithub/comments{/number}","issue_comment_url":"https://api.github.com/repos/PyGithub/PyGithub/issues/comments{/number}","contents_url":"https://api.github.com/repos/PyGithub/PyGithub/contents/{+path}","compare_url":"https://api.github.com/repos/PyGithub/PyGithub/compare/{base}...{head}","merges_url":"https://api.github.com/repos/PyGithub/PyGithub/merges","archive_url":"https://api.github.com/repos/PyGithub/PyGithub/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/PyGithub/PyGithub/downloads","issues_url":"https://api.github.com/repos/PyGithub/PyGithub/issues{/number}","pulls_url":"https://api.github.com/repos/PyGithub/PyGithub/pulls{/number}","milestones_url":"https://api.github.com/repos/PyGithub/PyGithub/milestones{/number}","notifications_url":"https://api.github.com/repos/PyGithub/PyGithub/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/PyGithub/PyGithub/labels{/name}","releases_url":"https://api.github.com/repos/PyGithub/PyGithub/releases{/id}","deployments_url":"https://api.github.com/repos/PyGithub/PyGithub/deployments","created_at":"2012-02-25T12:53:47Z","updated_at":"2023-05-09T07:44:21Z","pushed_at":"2023-05-09T07:33:55Z","git_url":"git://github.com/PyGithub/PyGithub.git","ssh_url":"git@github.com:PyGithub/PyGithub.git","clone_url":"https://github.com/PyGithub/PyGithub.git","svn_url":"https://github.com/PyGithub/PyGithub","homepage":"https://pygithub.readthedocs.io/","size":13824,"stargazers_count":5996,"watchers_count":5996,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"has_discussions":true,"forks_count":1628,"mirror_url":null,"archived":false,"disabled":false,"open_issues_count":258,"license":{"key":"lgpl-3.0","name":"GNU Lesser General Public License v3.0","spdx_id":"LGPL-3.0","url":"https://api.github.com/licenses/lgpl-3.0","node_id":"MDc6TGljZW5zZTEy"},"allow_forking":true,"is_template":false,"web_commit_signoff_required":false,"topics":["github","github-api","pygithub","python"],"visibility":"public","forks":1628,"open_issues":258,"watchers":5996,"default_branch":"master","temp_clone_token":null,"organization":{"login":"PyGithub","id":11288996,"node_id":"MDEyOk9yZ2FuaXphdGlvbjExMjg4OTk2","avatar_url":"https://avatars.githubusercontent.com/u/11288996?v=4","gravatar_id":"","url":"https://api.github.com/users/PyGithub","html_url":"https://github.com/PyGithub","followers_url":"https://api.github.com/users/PyGithub/followers","following_url":"https://api.github.com/users/PyGithub/following{/other_user}","gists_url":"https://api.github.com/users/PyGithub/gists{/gist_id}","starred_url":"https://api.github.com/users/PyGithub/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PyGithub/subscriptions","organizations_url":"https://api.github.com/users/PyGithub/orgs","repos_url":"https://api.github.com/users/PyGithub/repos","events_url":"https://api.github.com/users/PyGithub/events{/privacy}","received_events_url":"https://api.github.com/users/PyGithub/received_events","type":"Organization","site_admin":false},"network_count":1628,"subscribers_count":115}

11 changes: 11 additions & 0 deletions tests/ReplayData/Requester.testBaseUrlSchemeRedirection.txt
@@ -0,0 +1,11 @@
http
GET
api.github.com
None
/repos/PyGithub/PyGithub
{'User-Agent': 'PyGithub/Python'}
None
301
[('Content-Length', '0'), ('Location', 'https://api.github.com/repos/PyGithub/PyGithub')]