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: