Stop following redirects, so the key never goes to the host they name

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/<client version> (<application>)`. 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.

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,17 +41,22 @@ 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"),
                 "user_agent": self.headers.get("User-Agent"),
                 "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()

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: