From c911068acc520af0afebe18f82029ae23fd647af Mon Sep 17 00:00:00 2001 From: Daniel McCoy Stephenson Date: Sat, 7 Nov 2026 19:49:06 -0700 Subject: [PATCH] Stop following redirects, so the key never goes to the host they name urlopen follows 301/302/303 by re-sending the Authorization header to the Location target as an empty GET. Reports now go through an opener whose redirect handler declines every redirect, so the 3xx reaches the existing non-201 DEBUG line with its own status, as trace-client-java does. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++++ tests/test_trace_client.py | 28 +++++++++++++++++++++++++++- trace_client/trace_client.py | 17 ++++++++++++++++- 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6956fcc..c2a84fc 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,10 @@ key or value is `None` is left out, and every other key and value is sent as a string. An event with a blank `name` is not sent at all. The server assigns the timestamp. A `201` is success; anything else is logged at `DEBUG` and dropped. +Redirects are not followed: a `3xx` answer is logged at `DEBUG` with its own +status and the report is dropped, so the key is never re-sent to whatever host +a `Location` header names. Point `base_url` at the server's final address. + The `User-Agent` is `trace-client-python/ ()`. Any character of the application's name that a header cannot carry (outside printable Latin-1) becomes `?` there; `application` in the body is always the exact name. diff --git a/tests/test_trace_client.py b/tests/test_trace_client.py index b9aa1ed..3d3e7bd 100644 --- a/tests/test_trace_client.py +++ b/tests/test_trace_client.py @@ -19,6 +19,7 @@ from unittest import mock from trace_client import MAX_TAG_LENGTH, MAX_TAGS, TraceClient, __version__, environment_opts_out +from trace_client import trace_client as trace_client_module _ENV_VARS = ("TRACE_USAGE_REPORTING", "DO_NOT_TRACK") @@ -27,6 +28,7 @@ class _Capture: def __init__(self): self.requests = [] self.reply_status = 201 + self.location = None # sent as a Location header when set self.arrived = threading.Event() self.release = threading.Event() self.release.set() # by default answer at once @@ -39,6 +41,7 @@ def do_POST(self): body = self.rfile.read(length) capture.release.wait(10) capture.requests.append({ + "method": self.command, "path": self.path, "authorization": self.headers.get("Authorization"), "content_type": self.headers.get("Content-Type"), @@ -46,10 +49,14 @@ def do_POST(self): "body": body.decode("utf-8"), }) self.send_response(capture.reply_status) + if capture.location is not None: + self.send_header("Location", capture.location) self.send_header("Content-Length", "0") self.end_headers() capture.arrived.set() + do_GET = do_POST # a followed 301/302/303 arrives as a GET + def log_message(self, *args): # keep test output quiet pass @@ -198,6 +205,25 @@ def test_a_success_status_other_than_201_is_logged_as_not_delivered(self): "only a 201 is success: %s" % [r.getMessage() for r in self.log]) self.assertTrue(all(r.levelno == logging.DEBUG for r in self.log)) + def test_a_redirect_is_logged_and_dropped_never_followed_with_the_key(self): + target = _Capture() + target_server = _server(target) + self.addCleanup(target_server.shutdown) + # Another port and another host name: what a redirect elsewhere looks like. + self.capture.location = "http://localhost:%d/elsewhere" % target_server.server_address[1] + for status in (301, 302, 303, 307, 308): + self.log.clear() + self.capture.reply_status = status + client = TraceClient(self.base_url, "MyGame", "1.2.3", key="secret-key") + client.report("startup") + client.close() # joins the sender, so its log line is written by now + self.assertEqual([], target.requests, + "a %d must not carry the key to the host it names" % status) + self.assertTrue(any("answered %d" % status in r.getMessage() for r in self.log), + "the log names the redirect that lost the event: %s" + % [r.getMessage() for r in self.log]) + self.assertTrue(all(r.levelno == logging.DEBUG for r in self.log)) + def test_an_error_answer_whose_body_cannot_be_read_still_logs_its_status(self): class Unreadable(urllib.error.HTTPError): def read(self, *args): @@ -206,7 +232,7 @@ def read(self, *args): answer = Unreadable(self.base_url + "/api/metrics", 503, "Service Unavailable", {}, None) crashes = [] with mock.patch.object(threading, "excepthook", crashes.append), \ - mock.patch.object(urllib.request, "urlopen", side_effect=answer): + mock.patch.object(trace_client_module._OPENER, "open", side_effect=answer): client = TraceClient(self.base_url, "MyGame", "1.2.3", key="k") client.report("startup") # must not raise client.close() diff --git a/trace_client/trace_client.py b/trace_client/trace_client.py index ba13e8f..b57041a 100644 --- a/trace_client/trace_client.py +++ b/trace_client/trace_client.py @@ -57,6 +57,21 @@ _NOT_IN_A_HEADER = re.compile(r"[^\x20-\x7e\xa0-\xff]") +class _RedirectsNotFollowed(urllib.request.HTTPRedirectHandler): + """Declines every redirect, so the redirect itself is the answer. Following + one would re-send the key to whatever host ``Location`` names, and a 301, + 302 or 303 would turn the POST into an empty GET that loses the event + anyway. trace-client-java does not follow redirects either.""" + + def redirect_request(self, req, fp, code, msg, headers, newurl): + return None + + +# Every report goes through this opener rather than urlopen: urlopen follows +# redirects, and install_opener would change the host program's urlopen too. +_OPENER = urllib.request.build_opener(_RedirectsNotFollowed) + + def environment_opts_out(environ: Optional[Mapping[str, str]] = None) -> bool: """Whether the environment asks for usage reporting to be off, via ``TRACE_USAGE_REPORTING=off`` or ``DO_NOT_TRACK=1``. Only the listed @@ -322,7 +337,7 @@ def _send(self, body: bytes) -> None: "Authorization": "Bearer " + self._key, "User-Agent": _user_agent(self._application), }) - with urllib.request.urlopen(request, timeout=self.TIMEOUT_SECONDS) as response: + with _OPENER.open(request, timeout=self.TIMEOUT_SECONDS) as response: status = response.status response.read() except urllib.error.HTTPError as answered: