Raise error on unsupported redirects, log supported redirects #2524
Conversation
EnricoMi
force-pushed
the
raise-on-unsupported-redirections
branch
from
f1f2bf9
to
f3f4b67
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #2524 +/- ##
=========================================
Coverage ? 98.77%
=========================================
Files ? 117
Lines ? 11716
Branches ? 0
=========================================
Hits ? 11573
Misses ? 143
Partials ? 0
☔ View full report in Codecov by Sentry. |
| 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=...)" | ||
| ) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
JLLeitschuh
force-pushed
the
raise-on-unsupported-redirections
branch
from
f3f4b67
to
b1827e9
Compare
|
Feel free to merge |
|
Thanks! |
Redirects that do not change the url path but other pieces like host name or protocol schema end up in an endless loop because only the path part of the new location is redirected to: #2447 (comment)
This identifies situations where hostname, protocol or port number change in a redirect. Instead of redirecting to the new location, an error is raised because the base url should be corrected. Otherwise, each request will be redirected, duplicating the traffic and calls.
Further, any redirection is logged to info level. There have been many issues where knowing a redirect happened (without enabling debug level) would have helped investigating and understanding.