Skip to content

Commit

Permalink
Do not transform requestHeaders when logging (#1965)
Browse files Browse the repository at this point in the history
Requester.__log() sanitizes the headers of the request so that
authentication details are not logged, but this has the side effect of
meaning that future requests that use the same Requester object will
fail. Usually, this is perfectly fine, since almost every method will
only make one request -- where this falls down is when we make another
after a redirect. Make a copy of the requestHeaders, and sanitize those.

Fixes #1959
  • Loading branch information
s-t-e-v-e-n-k committed Jun 2, 2021
1 parent ed7d0fe commit 1265747
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 5 deletions.
11 changes: 6 additions & 5 deletions github/Requester.py
Expand Up @@ -629,17 +629,18 @@ def __log(self, verb, url, requestHeaders, input, status, responseHeaders, outpu
if self.__logger is None:
self.__logger = logging.getLogger(__name__)
if self.__logger.isEnabledFor(logging.DEBUG):
headersForRequest = requestHeaders.copy()
if "Authorization" in requestHeaders:
if requestHeaders["Authorization"].startswith("Basic"):
requestHeaders[
headersForRequest[
"Authorization"
] = "Basic (login and password removed)"
elif requestHeaders["Authorization"].startswith("token"):
requestHeaders["Authorization"] = "token (oauth token removed)"
headersForRequest["Authorization"] = "token (oauth token removed)"
elif requestHeaders["Authorization"].startswith("Bearer"):
requestHeaders["Authorization"] = "Bearer (jwt removed)"
headersForRequest["Authorization"] = "Bearer (jwt removed)"
else: # pragma no cover (Cannot happen, but could if we add an authentication method => be prepared)
requestHeaders[
headersForRequest[
"Authorization"
] = "(unknown auth removed)" # pragma no cover (Cannot happen, but could if we add an authentication method => be prepared)
self.__logger.debug(
Expand All @@ -648,7 +649,7 @@ def __log(self, verb, url, requestHeaders, input, status, responseHeaders, outpu
self.__scheme,
self.__hostname,
url,
requestHeaders,
headersForRequest,
input,
status,
responseHeaders,
Expand Down
16 changes: 16 additions & 0 deletions tests/Logging_.py
Expand Up @@ -198,3 +198,19 @@ def testLoggingWithBaseUrl(self):
}
output = '{"type":"User","html_url":"https://github.com/jacquev6","login":"jacquev6","followers":14,"company":"Criteo","created_at":"2010-07-09T06:10:06Z","email":"vincent@vincent-jacques.net","hireable":false,"avatar_url":"https://secure.gravatar.com/avatar/b68de5ae38616c296fa345d2b9df2225?d=https://a248.e.akamai.net/assets.github.com%2Fimages%2Fgravatars%2Fgravatar-user-420.png","public_gists":3,"bio":"","following":29,"name":"Vincent Jacques","blog":"http://vincent-jacques.net","gravatar_id":"b68de5ae38616c296fa345d2b9df2225","id":327146,"public_repos":13,"location":"Paris, France","url":"https://api.github.com/users/jacquev6"}'
self.assertLogging("GET", url, requestHeaders, responseHeaders, output)

def testLoggingDoesNotModifyRequestHeaders(self):
# Recorded replay data already sanitizes Authorization headers, so we
# need to go under the covers
requestHeaders = {"Authorization": "thisisnotatoken"}
responseHeaders = {"status": "200 OK"}
github.Github()._Github__requester._Requester__log(
"GET",
"http://example.com",
requestHeaders,
None,
200,
responseHeaders,
None,
)
self.assertEqual(requestHeaders["Authorization"], "thisisnotatoken")

0 comments on commit 1265747

Please sign in to comment.