From ce02d2a1c47e07d62fad7f60ff6b15172830a2c1 Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:26:40 +0200 Subject: [PATCH 1/3] Two harness checkers whose population was narrower than their claim (#1598, #1584) #1598 -- the `$1`-shim gate scanned `tests/test_*.py`, so a shim in `_gitshim.py`, `_git_decline.py`, `conftest.py` or `tests/fixtures/` returned the same clean zero. It now reads every Python module under `tests/`, recursively. The widened population produces exactly one hit, `_gitshim.py`'s `while`/`shift` dispatcher, which is the correct implementation of the pattern and is exempted by its shape rather than its filename -- `$1` inside a loop that consumes arguments has no position to slide to, the same grammar argument #1412 used for an option at `$1`. Naming the file would have re-created the allowlist #1412 deleted. The module-level POSIX `pytestmark` also came off, onto the five tests that actually run a `/bin/sh` shim: it was skipping the class gate and every rule test on Windows, all of them pure AST and regex work. #1584 -- the suite-wide netblock documented its blind spot as one thing and six in-process egress routes stayed open behind the sentence. `connect_ex`, `sendto`, `gethostbyname`, `gethostbyname_ex`, `gethostbyaddr` and `getnameinfo` all left an armed context, and `gethostbyname` came back with a live-resolved address. All are refused now, with `sendmsg`. The fix is not the longer list: `_netblock.ROUTES` / `SOCKET_ROUTES` classify every callable `socket` and `socket.socket` expose, a new one arrives red, and the four routes no in-process patch can reach are named in `BEYOND_THE_PROCESS` rather than counted as blocked. Co-Authored-By: Max --- changelog.d/1584.fixed.md | 2 + changelog.d/1598.fixed.md | 1 + docs/contributing.md | 8 +- tests/_netblock.py | 277 ++++++++++++++++++-- tests/conftest.py | 17 +- tests/test_git_shim_subcommand_1206.py | 177 ++++++++++++- tests/test_netblock_egress_register_1584.py | 227 ++++++++++++++++ tests/test_transport_seam_enforced_1341.py | 13 +- 8 files changed, 682 insertions(+), 40 deletions(-) create mode 100644 changelog.d/1584.fixed.md create mode 100644 changelog.d/1598.fixed.md create mode 100644 tests/test_netblock_egress_register_1584.py diff --git a/changelog.d/1584.fixed.md b/changelog.d/1584.fixed.md new file mode 100644 index 00000000..af402529 --- /dev/null +++ b/changelog.d/1584.fixed.md @@ -0,0 +1,2 @@ +- [#1584](https://github.com/Digital-Process-Tools/claude-supertool/issues/1584) The suite-wide network block stated its blind spot as one thing — "it binds `socket` in the pytest process only" — and six in-process egress routes stayed open behind that sentence. Measured from inside an armed context, `connect_ex`, `sendto`, `gethostbyname`, `gethostbyname_ex`, `gethostbyaddr` and `getnameinfo` all walked out, and `gethostbyname` returned a live-resolved address. All six are refused now, along with `sendmsg`. Nothing in the suite took them, so no green was ever wrong — the boundary was. +- The fix is not a longer list, because a longer list is read exactly as trustingly as a short one. `tests/_netblock.py` now carries a register classifying **every callable the `socket` module and the `socket.socket` type expose** as patched, reached-via-a-patched-route, local, inert, or open-with-a-reason, and `tests/test_netblock_egress_register_1584.py` goes red when a name arrives that nobody has classified. The four routes no in-process patch can reach — a child process, a C extension calling libc directly, a descriptor connected before the guard was armed, and `os.read`/`os.write` on such a descriptor — are named rather than counted as blocked. diff --git a/changelog.d/1598.fixed.md b/changelog.d/1598.fixed.md new file mode 100644 index 00000000..6bc4de6f --- /dev/null +++ b/changelog.d/1598.fixed.md @@ -0,0 +1 @@ +- [#1598](https://github.com/Digital-Process-Tools/claude-supertool/issues/1598) The suite's `$1`-shim gate scanned `tests/test_*.py`, so a shim in a helper module returned the same clean zero. It now reads every Python module under `tests/`, recursively — `_gitshim.py`, `_git_decline.py`, `conftest.py` and everything in `tests/fixtures/` were never opened, and a *shared* shim is exactly what a helper module holds. The widened population produces one hit, `tests/_gitshim.py`'s `while`/`shift` dispatcher, which is the correct implementation of the pattern; it is exempted by its **shape** rather than by its filename, because `$1` inside a loop that consumes arguments is the argument under the cursor and has no position to slide to. Naming the file would have re-created the allowlist [#1412](https://github.com/Digital-Process-Tools/claude-supertool/issues/1412) deleted, and would bless every shim that file ever acquires. diff --git a/docs/contributing.md b/docs/contributing.md index 65452e60..5fcd3f0e 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -504,9 +504,13 @@ The same trap one layer up: an op with both a `gql` and a `gql_safe` needs both **`block_outbound` is already armed for every test — you do not opt in ([#1341](https://github.com/Digital-Process-Tools/claude-supertool/issues/1341)).** `tests/conftest.py` applies it in an autouse fixture across the whole suite, so a missed or misaimed stub fails at the socket instead of contacting a third party and passing on the reply. Until #1341 it was a per-file fixture you had to remember, which is the same shape as a linter nobody runs: the three leaks #1312 found were all in files that had not added it. -`tests/_netblock.py` refuses any non-loopback `connect` or `getaddrinfo` and names the host and what to stub. Loopback and `AF_UNIX` stay open, so `test_http_bounds.py` and the `claude-channel` suites are unaffected — those bind their own servers and the process under test is the one that answered. +`tests/_netblock.py` refuses any non-loopback `connect`, `connect_ex`, `sendto`, `sendmsg`, `getaddrinfo`, `getnameinfo`, `gethostbyname`, `gethostbyname_ex` or `gethostbyaddr`, and names the host and what to stub. Loopback and `AF_UNIX` stay open, so `test_http_bounds.py` and the `claude-channel` suites are unaffected — those bind their own servers and the process under test is the one that answered. -**What it does not cover, so you do not read its green as more than it is:** it binds `socket` in the pytest process only. A test that shells out to `supertool.py` or a preset gets an unpatched child, and nothing there is blocked. That is the same blind spot the static grep has for a *missing* stub — #1312 measured a grep for transport tokens at 2 of 3 live leaks against the socket recorder's 3 of 3, because the third leak contained no transport token at all. +**What it does not cover is a register, not a sentence ([#1584](https://github.com/Digital-Process-Tools/claude-supertool/issues/1584)).** This paragraph used to say "it binds `socket` in the pytest process only" and stop, which reads as a closed list. It was not one: `connect_ex`, `gethostbyname`, `gethostbyname_ex`, `gethostbyaddr`, `getnameinfo` and a UDP `sendto` all left an armed context, and `gethostbyname` came back with a live-resolved address. Nothing in the suite took them — the #1312 class is `urllib`, which routes through `getaddrinfo` and `connect` — so no green was ever wrong; the boundary was. + +`_netblock.ROUTES` and `_netblock.SOCKET_ROUTES` now classify **every callable the `socket` module and the `socket.socket` type expose** as `PATCHED`, `VIA`, `LOCAL`, `INERT` or `OPEN`, and `tests/test_netblock_egress_register_1584.py` fails when a name arrives that nobody has classified. Read the register rather than this paragraph: the register is derived from the running interpreter and this paragraph is not. + +`_netblock.BEYOND_THE_PROCESS` names the four routes no in-process patch can reach — a child process (a test that shells out to `supertool.py` or a preset gets an unpatched interpreter), a C extension calling libc directly, a descriptor connected before the guard was armed, and `os.read`/`os.write` on such a descriptor. That first one is the same blind spot the static grep has for a *missing* stub — #1312 measured a grep for transport tokens at 2 of 3 live leaks against the socket recorder's 3 of 3, because the third leak contained no transport token at all. Why this is a rule and not a nicety: a live call makes the leg a statement about somebody else's DNS and redirect policy. #1312 was filed off a red on PR #1302, whose diff was one test file and one markdown table; Hashnode had started answering 301, `_http.urlopen` correctly refused the off-origin hop, and the test rendered that correct refusal as a product defect. diff --git a/tests/_netblock.py b/tests/_netblock.py index cc5dd630..bcf67c93 100644 --- a/tests/_netblock.py +++ b/tests/_netblock.py @@ -15,6 +15,24 @@ Loopback and AF_UNIX are allowed on purpose: `test_http_bounds.py` and the `claude-channel` suites bind real servers on `127.0.0.1` / a unix socket, and those are hermetic -- the process under test is the one that answered. + +**Its coverage is a register, not a sentence (#1584).** Until then this guard +patched `connect` and `getaddrinfo`, and both `conftest.py` and +`docs/contributing.md` stated its limit as exactly one thing -- "it binds +`socket` in the pytest process only". That reads as exhaustive and was not: +`connect_ex`, `gethostbyname`, `gethostbyname_ex`, `gethostbyaddr`, +`getnameinfo` and a UDP `sendto` all walked out of an armed context, and +`gethostbyname` came back with a live-resolved address. Nothing in the suite +took them, which is why it was low and not a leak. + +The lesson is not "patch six more names". A longer list is still a list, and +the next reader trusts it exactly as much as the one-line version. So `ROUTES` +and `SOCKET_ROUTES` below classify **every callable the `socket` module and the +`socket.socket` type expose**, `tests/test_netblock_egress_register_1584.py` +fails when one of them arrives unclassified, and a route that cannot be covered +is `OPEN` with its reason next to it rather than absent. That is this repo's +third state applied to the guard's own boundary: `blocked`, `local`, and *I +cannot see this one, and here is why*. """ from __future__ import annotations @@ -54,34 +72,253 @@ def _refuse(what: str, target: Any) -> None: ) -def block_outbound(monkeypatch: pytest.MonkeyPatch) -> None: - """Block non-loopback sockets for the duration of one test. +# --------------------------------------------------------------------------- +# The register (#1584). What this guard covers, derived from the interpreter. +# --------------------------------------------------------------------------- + +#: Refused here unless the destination is loopback or AF_UNIX. +PATCHED = "patched" +#: Reaches a `PATCHED` route to do its work, so it is refused there. Recorded +#: rather than omitted: "covered because something else covers it" is a claim, +#: and an implementation change can quietly stop making it true. +VIA = "via a patched route" +#: Inbound, or local to this machine. There is no remote destination to refuse. +LOCAL = "local or inbound" +#: Byte-order and address-format arithmetic. Touches nothing. +INERT = "no I/O" +#: Reaches the network and this guard cannot see it. The third state, and the +#: reason is in `OPEN_ROUTES` -- an entry without one is a shorter list, not a +#: boundary. +OPEN = "open" + +#: Every callable `socket` exposes. A name the interpreter grows and nobody has +#: classified fails `test_netblock_egress_register_1584.py` rather than sitting +#: inside a guard whose prose still says "connect and getaddrinfo". +ROUTES = { + "getaddrinfo": PATCHED, + "getnameinfo": PATCHED, + "gethostbyname": PATCHED, + "gethostbyname_ex": PATCHED, + "gethostbyaddr": PATCHED, + "create_connection": VIA, + "getfqdn": VIA, + "create_server": LOCAL, + "socketpair": LOCAL, + "close": LOCAL, + "gethostname": LOCAL, + "sethostname": LOCAL, + "getprotobyname": LOCAL, + "getservbyname": LOCAL, + "getservbyport": LOCAL, + "has_dualstack_ipv6": LOCAL, + "if_indextoname": LOCAL, + "if_nameindex": LOCAL, + "if_nametoindex": LOCAL, + "recv_fds": LOCAL, + "send_fds": LOCAL, + "getdefaulttimeout": INERT, + "setdefaulttimeout": INERT, + "htonl": INERT, + "htons": INERT, + "ntohl": INERT, + "ntohs": INERT, + "inet_aton": INERT, + "inet_ntoa": INERT, + "inet_ntop": INERT, + "inet_pton": INERT, + "CMSG_LEN": INERT, + "CMSG_SPACE": INERT, + "dup": OPEN, + "fromfd": OPEN, + "fromshare": OPEN, +} + +#: Every method `socket.socket` exposes, same rule. +SOCKET_ROUTES = { + "connect": PATCHED, + "connect_ex": PATCHED, + "sendto": PATCHED, + "sendmsg": PATCHED, + "send": VIA, + "sendall": VIA, + "sendfile": VIA, + "accept": LOCAL, + "bind": LOCAL, + "listen": LOCAL, + "close": LOCAL, + "detach": LOCAL, + "dup": LOCAL, + "fileno": LOCAL, + "makefile": LOCAL, + "shutdown": LOCAL, + "recv": LOCAL, + "recv_into": LOCAL, + "recvfrom": LOCAL, + "recvfrom_into": LOCAL, + "recvmsg": LOCAL, + "recvmsg_into": LOCAL, + "sendmsg_afalg": LOCAL, + "family": INERT, + "proto": INERT, + "type": INERT, + "timeout": INERT, + "gettimeout": INERT, + "settimeout": INERT, + "getblocking": INERT, + "setblocking": INERT, + "getsockopt": INERT, + "setsockopt": INERT, + "get_inheritable": INERT, + "set_inheritable": INERT, + "getpeername": INERT, + "getsockname": INERT, + "share": OPEN, +} + +#: Why each `OPEN` route is open. Every one of them is the same shape: a +#: descriptor whose `connect` happened somewhere this guard was not watching, +#: so wrapping it in a `socket` object afterwards inherits nothing. +OPEN_ROUTES = { + "dup": "duplicates a descriptor that may already be connected -- the " + "connect this guard would have refused happened before the wrap.", + "fromfd": "builds a socket around an arbitrary descriptor, including one " + "a child process or a C extension connected.", + "fromshare": "Windows: rebuilds a socket another process shared, so the " + "connect happened in a process this guard never patched.", + "share": "Windows: hands this socket to another process, which is not " + "running an armed pytest and is not bound by anything here.", +} + +#: Names that legitimately do not exist on every platform or Python. A name +#: missing here is a route that cannot be taken, which is the safe direction; +#: what the register must never carry is a name no Python has. +PLATFORM_OPTIONAL = frozenset({ + "CMSG_LEN", "CMSG_SPACE", "sethostname", "fromshare", "share", + "if_indextoname", "if_nameindex", "if_nametoindex", + "recv_fds", "send_fds", "sendmsg", "sendmsg_afalg", + "recvmsg", "recvmsg_into", "sendfile", +}) + +_MODULE_PATCHES = tuple( + n for n, c in ROUTES.items() if c == PATCHED and hasattr(socket, n)) +_METHOD_PATCHES = tuple( + n for n, c in SOCKET_ROUTES.items() + if c == PATCHED and hasattr(socket.socket, n)) + +#: What `block_outbound` actually replaces on this interpreter. Derived, so the +#: register cannot claim a route the code does not patch: on Windows there is +#: no `socket.socket.sendmsg` at all, and a hand-maintained list would have +#: read as coverage there. +PATCHES = _MODULE_PATCHES + _METHOD_PATCHES + +#: What no in-process patch can reach, stated as the several things it is +#: rather than the one thing `conftest.py` used to call it (#1584). +BEYOND_THE_PROCESS = ( + "A child process. A test that shells out to `supertool.py` or a preset " + "gets an unpatched interpreter, and nothing there is blocked.", + "A C extension, or any library calling libc's `connect` directly. The " + "`socket` module is a Python-level name and rebinding it binds nobody " + "who does not go through it.", + "A descriptor connected before this guard was armed, or received over " + "AF_UNIX with SCM_RIGHTS. The connect happened where the guard was not; " + "`fromfd`, `dup`, `share` and `fromshare` are the in-module doors to it.", + "Reads and writes on such a descriptor through `os.read` / `os.write` " + "rather than a socket object, which reach no method this file replaces.", +) + + +def _is_local(host: Any) -> bool: + """True for an address that is the test process talking to itself. - Both `getaddrinfo` and `connect` are covered. Either alone leaves a hole: - a name lookup is already egress and already blocks the runner, and an - address literal reaches `connect` without ever resolving anything. + `bytes` on purpose: urllib passes `str`, but a caller further down the + stack may not, and `str(b"127.0.0.1")` is `"b'127.0.0.1'"` -- which would + block loopback and read exactly like a real egress finding. """ - orig_connect = socket.socket.connect - orig_getaddrinfo = socket.getaddrinfo + if isinstance(host, (bytes, bytearray)): + host = host.decode("ascii", errors="replace") + return str(host) in LOOPBACK - def _is_local(host: Any) -> bool: - # `bytes` on purpose: urllib passes `str`, but a caller further down the - # stack may not, and `str(b"127.0.0.1")` is `"b'127.0.0.1'"` -- which - # would block loopback and read exactly like a real egress finding. - if isinstance(host, (bytes, bytearray)): - host = host.decode("ascii", errors="replace") - return str(host) in LOOPBACK + +def _address_is_local(sock: "socket.socket", address: Any) -> bool: + """AF_UNIX has no destination to protect; otherwise read the host out.""" + if _AF_UNIX is not None and sock.family == _AF_UNIX: + return True + host = address[0] if isinstance(address, tuple) else address + return _is_local(host) + + +def block_outbound(monkeypatch: pytest.MonkeyPatch) -> None: + """Block non-loopback egress for the duration of one test. + + The routes patched here are exactly the names `ROUTES` and `SOCKET_ROUTES` + classify `PATCHED`, and a test asserts that correspondence rather than + trusting it -- a guard whose register drifts from its patches documents + coverage it does not have, which is the defect this file exists for. + + Why each is needed, since "connect and getaddrinfo" once read as complete: + a name lookup is already egress and already blocks the runner; an address + literal reaches `connect` without resolving anything; `connect_ex` returns + an errno instead of raising, so it never touched the patched `connect`; a + UDP `sendto` carries its destination per-datagram and never connects at + all; and the four `gethost*`/`getnameinfo` resolvers reach the resolver + without going through `getaddrinfo`. + """ + orig = {name: getattr(socket, name) for name in _MODULE_PATCHES} + orig_methods = {name: getattr(socket.socket, name) for name in _METHOD_PATCHES} def _connect(self: socket.socket, address: Any) -> Any: - host = address[0] if isinstance(address, tuple) else address - if (_AF_UNIX is not None and self.family == _AF_UNIX) or _is_local(host): - return orig_connect(self, address) + if _address_is_local(self, address): + return orig_methods["connect"](self, address) _refuse("an outbound connect", address) + def _connect_ex(self: socket.socket, address: Any) -> Any: + # Returns an errno rather than raising, so it reached none of the + # arms `connect` goes through and left the block as a plain `0`. + if _address_is_local(self, address): + return orig_methods["connect_ex"](self, address) + _refuse("an outbound connect_ex", address) + + def _sendto(self: socket.socket, *args: Any) -> Any: + # `sendto(data, address)` and `sendto(data, flags, address)`: the + # destination is the last positional either way, and a datagram never + # connects, so nothing else in this file would have seen it. + if args and _address_is_local(self, args[-1]): + return orig_methods["sendto"](self, *args) + _refuse("an outbound datagram", args[-1] if args else None) + + def _sendmsg(self: socket.socket, buffers: Any, ancdata: Any = (), + flags: int = 0, address: Any = None) -> Any: + if address is None or _address_is_local(self, address): + return orig_methods["sendmsg"](self, buffers, ancdata, flags, address) + _refuse("an outbound datagram", address) + def _getaddrinfo(host: Any, port: Any, *args: Any, **kwargs: Any) -> Any: if host is not None and not _is_local(host): _refuse("a DNS lookup", (host, port)) - return orig_getaddrinfo(host, port, *args, **kwargs) + return orig["getaddrinfo"](host, port, *args, **kwargs) + + def _one_arg_resolver(name: str): + def _resolve(host: Any, *args: Any, **kwargs: Any) -> Any: + if host is not None and not _is_local(host): + _refuse("a DNS lookup", host) + return orig[name](host, *args, **kwargs) + return _resolve + + def _getnameinfo(sockaddr: Any, *args: Any, **kwargs: Any) -> Any: + host = sockaddr[0] if isinstance(sockaddr, tuple) else sockaddr + if not _is_local(host): + _refuse("a reverse DNS lookup", sockaddr) + return orig["getnameinfo"](sockaddr, *args, **kwargs) - monkeypatch.setattr(socket.socket, "connect", _connect) - monkeypatch.setattr(socket, "getaddrinfo", _getaddrinfo) + replacements = { + "getaddrinfo": _getaddrinfo, + "getnameinfo": _getnameinfo, + "gethostbyname": _one_arg_resolver("gethostbyname"), + "gethostbyname_ex": _one_arg_resolver("gethostbyname_ex"), + "gethostbyaddr": _one_arg_resolver("gethostbyaddr"), + } + for name, replacement in replacements.items(): + monkeypatch.setattr(socket, name, replacement) + for name, replacement in (("connect", _connect), ("connect_ex", _connect_ex), + ("sendto", _sendto), ("sendmsg", _sendmsg)): + monkeypatch.setattr(socket.socket, name, replacement) diff --git a/tests/conftest.py b/tests/conftest.py index 4add6045..d0665593 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -134,11 +134,18 @@ # found 3 of 3. The one it alone caught contained no transport token at all, # because it was a missing stub rather than a wrong one. # -# Blind spot, stated here rather than discovered later: this binds `socket` in -# the pytest process only. A test that shells out to `supertool.py` or a preset -# gets an unpatched child. Loopback and AF_UNIX stay open on purpose — -# `test_http_bounds.py` and the `claude-channel` suites bind real servers on -# 127.0.0.1 and those are hermetic. +# Blind spot — and it is several things, not the one thing this comment used to +# name (#1584). "It binds `socket` in the pytest process only" read as a closed +# list, and it was not: `connect_ex`, four `gethost*`/`getnameinfo` resolvers +# and a UDP `sendto` all walked out of an armed context until #1584 patched +# them. The boundary now lives in `_netblock.ROUTES` / `SOCKET_ROUTES`, which +# classify every callable `socket` exposes, and in `BEYOND_THE_PROCESS`, which +# states the four things no in-process patch can reach. A register goes red +# when a route arrives unclassified; a sentence does not. +# +# Loopback and AF_UNIX stay open on purpose — `test_http_bounds.py` and the +# `claude-channel` suites bind real servers on 127.0.0.1 and those are +# hermetic. # # Tolerated rather than assumed, exactly as `_paths` and `_env` above. _netblock = None diff --git a/tests/test_git_shim_subcommand_1206.py b/tests/test_git_shim_subcommand_1206.py index c5a2de01..bd4f55a2 100644 --- a/tests/test_git_shim_subcommand_1206.py +++ b/tests/test_git_shim_subcommand_1206.py @@ -37,7 +37,12 @@ class gate, because this one was found by tripping over it rather than by TESTS = Path(__file__).resolve().parent -pytestmark = pytest.mark.skipif(os.name == "nt", reason="POSIX /bin/sh shim") +#: Only the tests that actually *run* a `/bin/sh` shim need this. It used to be +#: a module-level `pytestmark`, which also skipped the class gate and every +#: rule test -- all of them pure AST and regex work with no shell in sight. A +#: platform mark that reaches past what it is about reports coverage the leg +#: does not have, which is the same defect the gate below exists for. +posix_shim = pytest.mark.skipif(os.name == "nt", reason="POSIX /bin/sh shim") def _repo(tmp_path: Path) -> Path: @@ -62,6 +67,7 @@ def _run(bindir: Path, *args: str, timeout: float = 30): env=dict(os.environ, PATH=str(bindir))) +@posix_shim @pytest.mark.parametrize("prefix", [ ["--literal-pathspecs"], ["--no-optional-locks"], @@ -82,6 +88,7 @@ def test_a_global_flag_before_the_subcommand_still_reaches_the_failing_shim( assert "unable to read index file" in r.stderr +@posix_shim def test_the_shim_is_reached_through_the_dash_C_form_the_suite_actually_uses( tmp_path: Path ) -> None: @@ -93,6 +100,7 @@ def test_the_shim_is_reached_through_the_dash_C_form_the_suite_actually_uses( assert r.returncode == 128, repr((r.returncode, r.stdout, r.stderr)) +@posix_shim def test_the_stalling_shim_also_matches_past_a_global_flag(tmp_path: Path) -> None: """Same defect, the other builder: it must still hang, not fall through.""" d = t705._bindir(tmp_path) @@ -101,6 +109,7 @@ def test_the_stalling_shim_also_matches_past_a_global_flag(tmp_path: Path) -> No _run(d, "--literal-pathspecs", "status", "--porcelain", timeout=3) +@posix_shim def test_the_timeout_suite_shim_matches_past_a_global_flag(tmp_path: Path) -> None: """`test_git_timeout_disclosure_650` builds the same shape separately.""" bindir = Path(t650._failing_git_path(tmp_path, "diff")) @@ -109,6 +118,7 @@ def test_the_timeout_suite_shim_matches_past_a_global_flag(tmp_path: Path) -> No assert "fatal: shim" in r.stderr +@posix_shim def test_a_subcommand_named_only_as_an_argument_is_not_intercepted( tmp_path: Path ) -> None: @@ -218,6 +228,49 @@ def cannot_be_a_git_subcommand_test(text: str) -> bool: return bool(operands) and all(o.startswith("-") for o in operands) +#: A shell loop that consumes its arguments. `$1` inside one is "the argument +#: currently under the cursor", not "the first argument" -- the loop has already +#: walked past every global flag by the time the word is read, so there is no +#: position for it to slide to. Non-greedy to the *first* `done`, so a second +#: loop later in the same literal is a second span rather than one long one. +_LOOP_BODY = re.compile(r"(?:^|[\s;&|(])(?:while|until|for)\b.*?\bdone\b", re.S) + +#: Every construct `_looks_like_the_class` counts as testing `$1`. Kept as one +#: tuple so the span check below cannot drift from the detection above it. +_TESTS_DOLLAR_ONE = ( + _CASE_ON_DOLLAR_ONE, + _COMPARED_WITH_DOLLAR_ONE, + _COMPARED_WITH_DOLLAR_ONE_REVERSED, +) + + +def _shifts_through_argv(text: str) -> bool: + """True when every `$1` test in TEXT sits inside a loop that shifts. + + This is the #1598 exemption, and it is a statement about git's grammar in + the same way #1412's option rule is -- not about which file the code is in. + `tests/_gitshim.py`'s dispatcher is the one correct implementation of the + pattern this gate hunts, and the widened population makes it the gate's + single hit. Naming the file would re-create the allowlist #1412 deleted, + and would bless every shim that file ever acquires; naming the *shape* + exempts the construct wherever anybody writes it. + + Deliberately narrow in three ways, each pinned by a test below. A loop with + no `shift` consumes nothing, so `$1` is still the first word. A `$1` test + outside the loop is not covered by a dispatcher that happens to sit above + it. And an empty set of tests is not "all of them inside a loop" -- there + is nothing here to exempt, and `_looks_like_the_class` has already decided + that case. + """ + spans = [m.span() for m in _LOOP_BODY.finditer(text) if "shift" in m.group(0)] + if not spans: + return False + positions = [m.start() for rx in _TESTS_DOLLAR_ONE for m in rx.finditer(text)] + return bool(positions) and all( + any(start <= p < end for start, end in spans) for p in positions + ) + + def _looks_like_the_class(text: str) -> bool: """The gate's entire rule for one string constant: tested, and not exempt. @@ -228,15 +281,34 @@ def _looks_like_the_class(text: str) -> bool: """ text = _unquoted(text) tested = _CASE_ON_DOLLAR_ONE.search(text) or _operands_compared_with_dollar_one(text) - return bool(tested) and not cannot_be_a_git_subcommand_test(text) + if not tested: + return False + return not ( + cannot_be_a_git_subcommand_test(text) or _shifts_through_argv(text) + ) + +def _modules_scanned(root: Path = TESTS) -> list: + """Every Python module under `tests/`, not only the `test_*.py` ones. -def _sh_literals_matching_dollar_one(): + Until #1598 this was `glob("test_*.py")`, and the gate's zero was read as + "no shim in the suite decides a subcommand from `$1`" when it meant "no + shim in the files whose names begin with `test_`". `tests/_gitshim.py`, + `tests/_git_decline.py`, `tests/conftest.py` and everything under + `tests/fixtures/` were never opened -- and a *shared* shim is exactly the + thing that lives in a helper module. Recursive, because `tests/fixtures/` + holds real modules and a directory is not a boundary the defect respects. + """ + return [p for p in sorted(root.rglob("*.py")) + if "__pycache__" not in p.parts + and p.name not in _FIRST_ARG_IS_HONEST + and p.name != Path(__file__).name] + + +def _sh_literals_matching_dollar_one(root: Path = TESTS): """Every string constant in the suite that tests `$1` inside a shim.""" hits = [] - for path in sorted(TESTS.glob("test_*.py")): - if path.name in _FIRST_ARG_IS_HONEST or path.name == Path(__file__).name: - continue + for path in _modules_scanned(root): try: tree = ast.parse(path.read_text(encoding="utf-8")) except SyntaxError: # pragma: no cover - a broken fixture module @@ -251,10 +323,11 @@ def _sh_literals_matching_dollar_one(): if isinstance(v, ast.Constant) and isinstance(v.value, str) ) if text and _looks_like_the_class(text): + name = path.relative_to(root).as_posix() # An f-string is walked as the JoinedStr and again as each of # its Constant parts, so the same shim would otherwise be named # twice and the list would read as a longer class than it is. - hit = (path.name, node.lineno, text.strip()[:70]) + hit = (name, node.lineno, text.strip()[:70]) if not any(h[:2] == hit[:2] for h in hits): hits.append(hit) return hits @@ -413,9 +486,93 @@ def test_the_narrowing_did_not_just_move_the_hole_into_the_allowlist() -> None: def test_the_scan_still_reads_every_test_module_it_did_before() -> None: """A rule can also be narrowed by quietly reading fewer files.""" - scanned = [p.name for p in sorted(TESTS.glob("test_*.py")) - if p.name not in _FIRST_ARG_IS_HONEST - and p.name != Path(__file__).name] + scanned = [p.name for p in _modules_scanned()] assert len(scanned) > 500, len(scanned) assert "test_status_swallowed_705.py" in scanned assert "test_hook_interpreter_windows_1401_1402.py" in scanned + + +# --------------------------------------------------------------------------- +# #1598 -- the population the gate scans, and the shape that exempts the one +# correct implementation of the pattern. +# --------------------------------------------------------------------------- + + +def test_the_scan_reads_every_python_module_in_the_suite_not_only_test_files( +) -> None: + """`tests/*.py` is the claim; `tests/test_*.py` was the population (#1598). + + A shared shim belongs in a helper module -- that is what a helper module is + for -- and every helper, `conftest.py` and every fixture module returned the + gate's same clean zero because none of them was ever opened. + """ + scanned = {p.relative_to(TESTS).as_posix() for p in _modules_scanned()} + for name in ("_gitshim.py", "_git_decline.py", "conftest.py", + "fixtures/mock_mcp_server.py"): + assert name in scanned, (name, len(scanned)) + + +def test_a_shim_planted_in_a_helper_module_is_reported(tmp_path: Path) -> None: + """The gate's zero has to mean "I looked there", not "I did not look". + + Held against a directory rather than the tree, because the tree is clean by + construction: a scan that only ever runs over a passing population cannot + tell a widened glob from a narrow one. + """ + (tmp_path / "_helper_shim.py").write_text( + """SHIM = 'if [ "$1" = "status" ]; then exit 128; fi'""" + os.linesep, + encoding="utf-8", + ) + (tmp_path / "conftest.py").write_text( + """SHIM = 'case "$1" in diff) exit 1 ;; esac'""" + os.linesep, + encoding="utf-8", + ) + found = {name for name, _line, _text in _sh_literals_matching_dollar_one(tmp_path)} + assert found == {"_helper_shim.py", "conftest.py"}, found + + +def test_a_fixture_module_below_tests_is_reached_too(tmp_path: Path) -> None: + """`tests/fixtures/` holds real modules, so `glob` is not deep enough.""" + sub = tmp_path / "fixtures" / "resolve" + sub.mkdir(parents=True) + (sub / "helper.py").write_text( + """SHIM = 'if [ "$1" = "stash" ]; then sleep 30; fi'""" + os.linesep, + encoding="utf-8", + ) + found = {name for name, _line, _text in _sh_literals_matching_dollar_one(tmp_path)} + assert found == {"fixtures/resolve/helper.py"}, found + + +def test_the_canonical_dispatcher_is_exempt_by_its_shape_not_by_its_filename( +) -> None: + """`_gitshim`'s `while`/`shift` loop is the *correct* implementation. + + It is the one hit the widened population produces, and it must not be + answered by putting `_gitshim.py` back on a filename list: #1412 removed + that list on purpose, so that the judgement stops depending on where the + code sits. `$1` inside a loop that shifts is "the argument being examined", + not "the first argument" -- there is no position for it to slide to, which + is the same grammar argument #1412 used for an option at `$1`. + """ + import _gitshim + + assert not _looks_like_the_class(_gitshim._SUBCOMMAND_FUNCTION) + assert not _looks_like_the_class( + _gitshim.dispatch_on_subcommand("status", "exit 128", "/usr/bin/git") + ) + assert "_gitshim.py" not in _FIRST_ARG_IS_HONEST + + +@pytest.mark.parametrize("shim", [ + # A dispatcher loop above does not bless a first-argument decision below it. + 'while [ $# -gt 0 ]; do case "$1" in -*) shift ;; *) break ;; esac; done' + '\nif [ "$1" = "status" ]; then exit 128; fi\n', + # A loop that never shifts is not walking argv; `$1` stays the first word. + 'while true; do if [ "$1" = "status" ]; then exit 128; fi; done', + # `for` over a fixed list shifts nothing either. + 'for x in a b; do if [ "$1" = "diff" ]; then exit 1; fi; done', +]) +def test_the_dispatcher_exemption_does_not_bless_a_first_argument_decision( + shim: str, +) -> None: + assert _looks_like_the_class(shim), shim diff --git a/tests/test_netblock_egress_register_1584.py b/tests/test_netblock_egress_register_1584.py new file mode 100644 index 00000000..108d3bc2 --- /dev/null +++ b/tests/test_netblock_egress_register_1584.py @@ -0,0 +1,227 @@ +"""#1584 -- the suite-wide netblock's coverage boundary, measured not asserted. + +`_netblock.block_outbound` is the sole suite-wide egress gate since #1341, and +until this file it patched `socket.socket.connect` and `socket.getaddrinfo` and +nothing else. Both `conftest.py` and `docs/contributing.md` documented its limit +as exactly **one** thing -- "it binds `socket` in the pytest process only" -- +which reads as a closed list. Measured from inside an armed context it was not: + + connect blocked + connect_ex NOT BLOCKED -> 0 + getaddrinfo blocked + gethostbyname NOT BLOCKED -> '104.20.23.154' + udp sendto NOT BLOCKED -> 1 + +`gethostbyname` returned a live-resolved address: real DNS egress from inside +the block. Three more the issue did not name -- `gethostbyname_ex`, +`getnameinfo` and `gethostbyaddr` -- were open the same way, which is the +argument against the fix being "patch three more names": a longer list is still +a list, and the next reader trusts it exactly as much. + +So the boundary is a register rather than a sentence. `_netblock.ROUTES` +classifies **every callable the `socket` module exposes** on the running +interpreter, and the tests below fail when a name appears that nobody has +classified. A route that cannot be covered is `OPEN` and carries its reason; +that is the third state, not a shorter list. +""" +from __future__ import annotations + +import socket +import tempfile +from pathlib import Path + +import pytest + +import _netblock + + +TEST_NET_1 = "192.0.2.1" # RFC 5737, guaranteed not routed. +#: RFC 6761 reserves `.invalid`, so a red leg here cannot leave the building +#: even when the route it is probing is genuinely open. +NOWHERE = "no-such-host.invalid" + + +# --------------------------------------------------------------------------- +# The routes, held against the armed context conftest already installs. +# --------------------------------------------------------------------------- + + +def _udp(): + s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + s.settimeout(0.2) + return s + + +def _tcp(): + s = socket.socket() + s.settimeout(0.2) + return s + + +@pytest.mark.parametrize("name,call", [ + ("connect", lambda: _tcp().connect((TEST_NET_1, 80))), + ("connect_ex", lambda: _tcp().connect_ex((TEST_NET_1, 80))), + ("sendto", lambda: _udp().sendto(b"x", (TEST_NET_1, 53))), + pytest.param( + "sendmsg", lambda: _udp().sendmsg([b"x"], [], 0, (TEST_NET_1, 53)), + marks=pytest.mark.skipif( + not hasattr(socket.socket, "sendmsg"), + reason="no socket.socket.sendmsg on this platform (Windows)"), + ), + ("getaddrinfo", lambda: socket.getaddrinfo(NOWHERE, 80)), + ("gethostbyname", lambda: socket.gethostbyname(NOWHERE)), + ("gethostbyname_ex", lambda: socket.gethostbyname_ex(NOWHERE)), + ("gethostbyaddr", lambda: socket.gethostbyaddr(TEST_NET_1)), + ("getnameinfo", lambda: socket.getnameinfo((TEST_NET_1, 80), 0)), +]) +def test_every_route_classified_as_patched_actually_refuses(name, call) -> None: + """The autouse gate is already armed; these are held against it directly. + + A route that raises anything other than `OutboundBlocked` reached the + network stack -- `gaierror` for a name that does not resolve is still the + resolver having been asked, and on a route that *is* open the same call + returns an address. + """ + register = dict(_netblock.ROUTES, **_netblock.SOCKET_ROUTES) + assert register[name] == _netblock.PATCHED, name + with pytest.raises(_netblock.OutboundBlocked): + call() + + +@pytest.mark.parametrize("call", [ + lambda: socket.getaddrinfo("127.0.0.1", 0), + lambda: socket.gethostbyname("localhost"), + lambda: socket.getnameinfo(("127.0.0.1", 80), 0), +]) +def test_loopback_is_still_open_on_the_widened_routes(call) -> None: + """Widening the block must not close the hermetic servers the suite binds. + + `test_http_bounds.py` and the `claude-channel` suites answer themselves on + `127.0.0.1` and AF_UNIX; a guard that refused those would be trading a + quiet leak for a loud false finding. + + Only `OutboundBlocked` is a failure here. A runner whose reverse lookup for + `127.0.0.1` fails raises `gaierror`, which is that runner's resolver and not + this guard -- asserting "did not raise at all" would turn a host + configuration into a finding about the diff. + """ + try: + call() + except _netblock.OutboundBlocked: + raise + except OSError: + pass + + +def test_an_af_unix_datagram_is_not_refused() -> None: + """AF_UNIX has no destination the guard could be protecting. + + Not on `tmp_path`: `sun_path` is 104 bytes on macOS and pytest's per-test + directory alone is longer than that, so the bind fails with an `OSError` + that reads exactly like the refusal this test exists to rule out. + """ + if not hasattr(socket, "AF_UNIX"): + pytest.skip("AF_UNIX is POSIX-only") + tmp = Path(tempfile.mkdtemp(prefix="nb")) + peer = tmp / "s" + if len(str(peer).encode()) > 100: + pytest.skip("sun_path is too short for this temporary directory") + try: + server = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM) + server.bind(str(peer)) + except (OSError, AttributeError) as exc: + # Windows has `AF_UNIX` but no datagram support for it, and refuses + # here with an `OSError` that is not this guard talking. + pytest.skip("AF_UNIX datagrams unavailable: {0}".format(exc)) + client = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM) + try: + client.sendto(b"x", str(peer)) + assert server.recv(8) == b"x" + finally: + client.close() + server.close() + + +# --------------------------------------------------------------------------- +# The register: the boundary has to be derived from the interpreter, not typed. +# --------------------------------------------------------------------------- + + +def _module_callables() -> set: + return {n for n in dir(socket) + if not n.startswith("_") + and callable(getattr(socket, n)) + and not isinstance(getattr(socket, n), type)} + + +def _socket_methods() -> set: + return {n for n in dir(socket.socket) if not n.startswith("_")} + + +def test_no_socket_callable_is_left_unclassified() -> None: + """A route that arrives in a later Python goes red rather than unseen. + + This is the whole point of the register and the one thing a prose note + cannot do: `recv_fds` and `send_fds` both arrived after the guard's own + defect class was filed, and either could have been an egress route nobody + re-read the sentence for. + """ + unclassified = _module_callables() - set(_netblock.ROUTES) + assert unclassified == set(), sorted(unclassified) + + +def test_no_socket_method_is_left_unclassified() -> None: + unclassified = _socket_methods() - set(_netblock.SOCKET_ROUTES) + assert unclassified == set(), sorted(unclassified) + + +def test_the_register_only_classifies_names_that_exist() -> None: + """A register that outlives its names records coverage it does not have. + + Tolerant of the platform-conditional ones on purpose -- `sethostname` and + the `if_*` family are not everywhere -- because a name absent here is a + route that cannot be taken, which is the safe direction. What is not + tolerated is a name in the register that no Python has. + """ + stale = set(_netblock.ROUTES) - _module_callables() - _netblock.PLATFORM_OPTIONAL + assert stale == set(), sorted(stale) + + +def test_every_patched_route_is_one_block_outbound_actually_patches() -> None: + """The register is derived from the code, not maintained alongside it. + + Intersected with what this interpreter has, because Windows has no + `socket.socket.sendmsg`: claiming it there would be the register doing + exactly what the prose note did, one layer in. + """ + declared = {n for n, c in _netblock.ROUTES.items() if c == _netblock.PATCHED} + declared |= {n for n, c in _netblock.SOCKET_ROUTES.items() + if c == _netblock.PATCHED} + available = _module_callables() | _socket_methods() + assert declared & available == set(_netblock.PATCHES), ( + sorted((declared & available) ^ set(_netblock.PATCHES))) + assert declared - available <= _netblock.PLATFORM_OPTIONAL, ( + sorted(declared - available - _netblock.PLATFORM_OPTIONAL)) + + +def test_every_open_route_carries_a_reason() -> None: + """`OPEN` is the third state; without a reason it is just a shorter list.""" + for name, reason in _netblock.OPEN_ROUTES.items(): + assert reason.strip(), name + named = set(_netblock.OPEN_ROUTES) + classified = {n for n, c in _netblock.ROUTES.items() if c == _netblock.OPEN} + classified |= {n for n, c in _netblock.SOCKET_ROUTES.items() + if c == _netblock.OPEN} + assert named == classified, sorted(named ^ classified) + + +def test_the_uncoverable_routes_are_stated_rather_than_counted_as_blocked( +) -> None: + """The blind spot was one sentence and is at least four things (#1584). + + Fixed at four so that adding a fifth is a decision somebody records here + rather than a line appended to prose nobody re-reads. + """ + assert len(_netblock.BEYOND_THE_PROCESS) == 4, _netblock.BEYOND_THE_PROCESS + for reason in _netblock.BEYOND_THE_PROCESS: + assert reason.strip() diff --git a/tests/test_transport_seam_enforced_1341.py b/tests/test_transport_seam_enforced_1341.py index 0c99b586..286028fe 100644 --- a/tests/test_transport_seam_enforced_1341.py +++ b/tests/test_transport_seam_enforced_1341.py @@ -32,9 +32,16 @@ The blind spot, stated rather than discovered later --------------------------------------------------- -This blocks sockets **in the pytest process only**. A test that shells out to -`supertool.py` or a preset as a subprocess is not covered -- the child has its -own unpatched `socket` module. Loopback and `AF_UNIX` stay open on purpose: +"In the pytest process only" was this file's whole disclosure, and it read as +exhaustive when it was one item of several (#1584). The measured boundary is +`_netblock.ROUTES` / `SOCKET_ROUTES` -- every callable the `socket` module and +the `socket.socket` type expose, classified, with +`tests/test_netblock_egress_register_1584.py` going red on one that arrives +unclassified -- and `_netblock.BEYOND_THE_PROCESS`, which names the four +routes no in-process patch reaches at all. A child process is the first of +those four: it has its own unpatched `socket` module. + +Loopback and `AF_UNIX` stay open on purpose: `test_http_bounds.py` and the `claude-channel` suites bind real servers on `127.0.0.1`, and those are hermetic because the process under test is the one that answered. From bbbf8387014aeeb2f58605209aff295b15ff1cb7 Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:40:06 +0200 Subject: [PATCH 2/3] review: derive the netblock patch loop from the register, not a written-out list The reviewer's finding, and it would have reddened every Windows leg rather than a socket test: `block_outbound` computed `_MODULE_PATCHES` / `_METHOD_PATCHES` with a `hasattr` filter -- precisely because Windows has no `socket.socket.sendmsg` -- and then iterated a hardcoded four-name tuple anyway. `monkeypatch.setattr` with `raising=True` on an absent attribute raises `AttributeError` out of an autouse fixture, so every test in the suite. Pinned by a test that shortens the derived tuple and asserts the untouched method stayed untouched; against the previous loop that assertion is False. `delattr` cannot simulate the platform here -- `socket.socket.sendmsg` is inherited from `_socket.socket` -- which is why the pin is on the tuple. Also the reviewer's second finding: a line in `test_transport_seam_enforced_1341.py` still named `connect`/`getaddrinfo` as the whole boundary, one paragraph above the rewritten disclosure. That is the #1584 defect reproduced next to its own fix. Co-Authored-By: Max --- tests/_netblock.py | 21 +++++++++++---- tests/test_netblock_egress_register_1584.py | 29 +++++++++++++++++++++ tests/test_transport_seam_enforced_1341.py | 5 +++- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/tests/_netblock.py b/tests/_netblock.py index bcf67c93..0dae39f7 100644 --- a/tests/_netblock.py +++ b/tests/_netblock.py @@ -310,6 +310,12 @@ def _getnameinfo(sockaddr: Any, *args: Any, **kwargs: Any) -> Any: _refuse("a reverse DNS lookup", sockaddr) return orig["getnameinfo"](sockaddr, *args, **kwargs) + # Keyed off `_MODULE_PATCHES` / `_METHOD_PATCHES` rather than written out, + # and that is not tidiness. Those two are `hasattr`-filtered because + # Windows has no `socket.socket.sendmsg`, and a hand-written loop would + # have called `monkeypatch.setattr` on a name that is not there -- + # `AttributeError` out of an autouse fixture, so every test on every + # Windows leg, not only the ones that touch a socket. replacements = { "getaddrinfo": _getaddrinfo, "getnameinfo": _getnameinfo, @@ -317,8 +323,13 @@ def _getnameinfo(sockaddr: Any, *args: Any, **kwargs: Any) -> Any: "gethostbyname_ex": _one_arg_resolver("gethostbyname_ex"), "gethostbyaddr": _one_arg_resolver("gethostbyaddr"), } - for name, replacement in replacements.items(): - monkeypatch.setattr(socket, name, replacement) - for name, replacement in (("connect", _connect), ("connect_ex", _connect_ex), - ("sendto", _sendto), ("sendmsg", _sendmsg)): - monkeypatch.setattr(socket.socket, name, replacement) + method_replacements = { + "connect": _connect, + "connect_ex": _connect_ex, + "sendto": _sendto, + "sendmsg": _sendmsg, + } + for name in _MODULE_PATCHES: + monkeypatch.setattr(socket, name, replacements[name]) + for name in _METHOD_PATCHES: + monkeypatch.setattr(socket.socket, name, method_replacements[name]) diff --git a/tests/test_netblock_egress_register_1584.py b/tests/test_netblock_egress_register_1584.py index 108d3bc2..014d1552 100644 --- a/tests/test_netblock_egress_register_1584.py +++ b/tests/test_netblock_egress_register_1584.py @@ -204,6 +204,35 @@ def test_every_patched_route_is_one_block_outbound_actually_patches() -> None: sorted(declared - available - _netblock.PLATFORM_OPTIONAL)) +def test_block_outbound_patches_only_names_this_interpreter_has() -> None: + """The patch loop must iterate the derived tuples, not a written-out list. + + `_MODULE_PATCHES` / `_METHOD_PATCHES` are `hasattr`-filtered because + Windows has no `socket.socket.sendmsg`. A loop that names the four methods + inline passes everywhere the author can run it and calls + `monkeypatch.setattr` on a missing attribute on Windows -- `AttributeError` + out of an autouse fixture, so every test on every Windows leg rather than + the ones that touch a socket. + """ + if "sendmsg" not in _netblock._METHOD_PATCHES: + pytest.skip("this interpreter already has no sendmsg") + # Not `delattr`: `socket.socket.sendmsg` is inherited from `_socket.socket` + # and cannot be removed from the subclass, so the platform is simulated on + # the derived tuple -- which is the thing the loop is required to read. + original = socket.socket.sendmsg + shortened = tuple(n for n in _netblock._METHOD_PATCHES if n != "sendmsg") + with pytest.MonkeyPatch.context() as mp: + mp.setattr(_netblock, "_METHOD_PATCHES", shortened) + with pytest.MonkeyPatch.context() as inner: + _netblock.block_outbound(inner) + assert socket.socket.sendmsg is original, ( + "block_outbound replaced a method that is not in " + "_METHOD_PATCHES, so it is reading a written-out list; on " + "Windows that same list names an attribute that is not there") + with pytest.raises(_netblock.OutboundBlocked): + _tcp().connect((TEST_NET_1, 80)) + + def test_every_open_route_carries_a_reason() -> None: """`OPEN` is the third state; without a reason it is just a shorter list.""" for name, reason in _netblock.OPEN_ROUTES.items(): diff --git a/tests/test_transport_seam_enforced_1341.py b/tests/test_transport_seam_enforced_1341.py index 286028fe..14dfe0cb 100644 --- a/tests/test_transport_seam_enforced_1341.py +++ b/tests/test_transport_seam_enforced_1341.py @@ -20,7 +20,10 @@ ----------------------------- `tests/conftest.py` arms `_netblock.block_outbound` for **every** test. A stub against the wrong name no longer succeeds quietly against a third-party host; it -fails at `connect`/`getaddrinfo`, naming the destination and what to stub. +fails at the socket, naming the destination and what to stub. *Which* names is +`_netblock.PATCHES`, derived from the register -- writing the list here is how +the disclosure below came to describe a boundary two thirds narrower than the +one it had. #1312 measured both methods over 559 test modules. A static grep for transport tokens found 2 of the 3 live leaks. The socket recorder found 3 of 3 -- the one From 5d276da4d0a1f3976a427ec3e186e4787faf2c47 Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Fri, 14 Aug 2026 06:18:27 +0200 Subject: [PATCH 3/3] The egress register could only see the platform it ran on, so a Windows-only route was classified nowhere (#1584) `socket.socket.ioctl` exists under `MS_WINDOWS` only. The register's population is `dir(socket.socket)` on the running interpreter -- right for a name a later Python grows, blind by construction to a name another platform has -- so it was in neither register, invisible to every POSIX author and every POSIX leg, and red on all four Windows legs one PR after the register shipped. Classified `OPEN`, not `PATCHED`. `WSAIoctl` addresses no peer, but `SIO_RCVALL` puts the interface into promiscuous receive, and the traffic that then arrives at `recv` carries no destination for a destination-refusing guard to check -- indistinguishable there from the loopback bytes the suite is entitled to. It is not patched rather than not patchable: refusing it means deciding per control code on a platform nobody who maintains this file can run, and a guess would be coverage claimed rather than had. `BEYOND_THE_PROCESS` stays at four; `ioctl` is inside the process, it is a route this guard declines, which is what `OPEN` is for. `OPEN_ROUTES`' header claimed all its entries were the connected-descriptor shape; `ioctl` is a second shape and says so rather than being filed under it. One name is not the fix, though -- a register that learns one platform-only name per release is the sentence it replaced. `PLATFORM_ONLY` states the `MS_WINDOWS` names (`ioctl`, `share`, `fromshare`) and a parametrised test asserts each is classified and present exactly on its own platform: absent here, present on the Windows legs. Only win32 is listed, and deliberately -- the macOS and Linux populations are derived green on every PR, so they are observed; Windows was the one platform no green leg had ever agreed with. Windows claims here are reasoned from CPython's `Modules/socketmodule.c`, not observed: this was written on macOS. The presence half of the new test is what converts them, on the leg that can. Adjacent, same file: `test_the_register_only_classifies_names_that_exist` checked `ROUTES` against the interpreter and left `SOCKET_ROUTES` unchecked, so a method name no Python has could sit there and read as coverage -- the same half-derived population as the defect this file exists for. Widened; verified non-vacuous by injecting a bogus name (`['wsarecvmsg']` reported, `[]` as shipped). RED before the fix, matching the Windows legs: test_every_platform_only_route_is_classified[ioctl] AssertionError: ioctl Co-Authored-By: Max --- changelog.d/1584.fixed.md | 1 + docs/contributing.md | 2 + tests/_netblock.py | 50 +++++++++++++++++++-- tests/conftest.py | 7 +++ tests/test_netblock_egress_register_1584.py | 44 ++++++++++++++++++ 5 files changed, 100 insertions(+), 4 deletions(-) diff --git a/changelog.d/1584.fixed.md b/changelog.d/1584.fixed.md index af402529..fc01ab96 100644 --- a/changelog.d/1584.fixed.md +++ b/changelog.d/1584.fixed.md @@ -1,2 +1,3 @@ - [#1584](https://github.com/Digital-Process-Tools/claude-supertool/issues/1584) The suite-wide network block stated its blind spot as one thing — "it binds `socket` in the pytest process only" — and six in-process egress routes stayed open behind that sentence. Measured from inside an armed context, `connect_ex`, `sendto`, `gethostbyname`, `gethostbyname_ex`, `gethostbyaddr` and `getnameinfo` all walked out, and `gethostbyname` returned a live-resolved address. All six are refused now, along with `sendmsg`. Nothing in the suite took them, so no green was ever wrong — the boundary was. - The fix is not a longer list, because a longer list is read exactly as trustingly as a short one. `tests/_netblock.py` now carries a register classifying **every callable the `socket` module and the `socket.socket` type expose** as patched, reached-via-a-patched-route, local, inert, or open-with-a-reason, and `tests/test_netblock_egress_register_1584.py` goes red when a name arrives that nobody has classified. The four routes no in-process patch can reach — a child process, a C extension calling libc directly, a descriptor connected before the guard was armed, and `os.read`/`os.write` on such a descriptor — are named rather than counted as blocked. +- A register derived from the running interpreter can only see the platform it runs on, and #1584's could not see `socket.socket.ioctl` — Windows-only, classified nowhere, red on all four Windows legs of the next PR while macOS and Linux stayed green. It is `OPEN` now with its reason: it addresses no peer, but `SIO_RCVALL` puts the interface into promiscuous receive, and the traffic that then arrives at `recv` has no destination for a destination-refusing guard to check. `_netblock.PLATFORM_ONLY` states the Windows-only names so the register stops learning one of them per release, and a test asserts each is absent on POSIX and present on Windows — the presence half is observed by the Windows legs, not by whoever wrote the list. diff --git a/docs/contributing.md b/docs/contributing.md index 5fcd3f0e..2b494e64 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -510,6 +510,8 @@ The same trap one layer up: an op with both a `gql` and a `gql_safe` needs both `_netblock.ROUTES` and `_netblock.SOCKET_ROUTES` now classify **every callable the `socket` module and the `socket.socket` type expose** as `PATCHED`, `VIA`, `LOCAL`, `INERT` or `OPEN`, and `tests/test_netblock_egress_register_1584.py` fails when a name arrives that nobody has classified. Read the register rather than this paragraph: the register is derived from the running interpreter and this paragraph is not. +Derived from the *running* interpreter, which is where that check ends and `_netblock.PLATFORM_ONLY` starts. `socket.socket.ioctl` exists only on Windows, so no POSIX author and no POSIX leg could see it: it was classified nowhere and went red on all four Windows legs one PR after the register shipped. The names CPython guards with `MS_WINDOWS` are listed there and classified above, and a test asserts each is present exactly on its own platform — an absence on macOS and Linux, a presence on the Windows legs, which is what turns a claim read out of CPython's source into an observation. `ioctl` itself is `OPEN`, not `PATCHED`: it addresses no peer, but `SIO_RCVALL` puts the interface into promiscuous receive, after which a plain `recv` returns traffic between other hosts that this guard cannot tell from loopback. + `_netblock.BEYOND_THE_PROCESS` names the four routes no in-process patch can reach — a child process (a test that shells out to `supertool.py` or a preset gets an unpatched interpreter), a C extension calling libc directly, a descriptor connected before the guard was armed, and `os.read`/`os.write` on such a descriptor. That first one is the same blind spot the static grep has for a *missing* stub — #1312 measured a grep for transport tokens at 2 of 3 live leaks against the socket recorder's 3 of 3, because the third leak contained no transport token at all. Why this is a rule and not a nicety: a live call makes the leg a statement about somebody else's DNS and redirect policy. #1312 was filed off a red on PR #1302, whose diff was one test file and one markdown table; Hashnode had started answering 301, `_http.urlopen` correctly refused the off-origin hop, and the test rendered that correct refusal as a product defect. diff --git a/tests/_netblock.py b/tests/_netblock.py index 0dae39f7..ad255c76 100644 --- a/tests/_netblock.py +++ b/tests/_netblock.py @@ -33,6 +33,10 @@ is `OPEN` with its reason next to it rather than absent. That is this repo's third state applied to the guard's own boundary: `blocked`, `local`, and *I cannot see this one, and here is why*. + +A derived population sees one platform, though, and `PLATFORM_ONLY` below is +where that stops being silent: `socket.socket.ioctl` is Windows-only, so it was +in no register and no POSIX leg could notice (#1642). """ from __future__ import annotations @@ -172,12 +176,15 @@ def _refuse(what: str, target: Any) -> None: "set_inheritable": INERT, "getpeername": INERT, "getsockname": INERT, + "ioctl": OPEN, "share": OPEN, } -#: Why each `OPEN` route is open. Every one of them is the same shape: a -#: descriptor whose `connect` happened somewhere this guard was not watching, -#: so wrapping it in a `socket` object afterwards inherits nothing. +#: Why each `OPEN` route is open. Four of them are one shape: a descriptor whose +#: `connect` happened somewhere this guard was not watching, so wrapping it in a +#: `socket` object afterwards inherits nothing. `ioctl` is a second shape and is +#: spelled out rather than filed under the first (#1642) -- "they are all the +#: same thing" is how the next name gets waved through. OPEN_ROUTES = { "dup": "duplicates a descriptor that may already be connected -- the " "connect this guard would have refused happened before the wrap.", @@ -187,18 +194,53 @@ def _refuse(what: str, target: Any) -> None: "connect happened in a process this guard never patched.", "share": "Windows: hands this socket to another process, which is not " "running an armed pytest and is not bound by anything here.", + "ioctl": "Windows: `WSAIoctl`. It addresses no peer -- but `SIO_RCVALL` " + "puts the interface into promiscuous receive, and after it a " + "plain `recv` returns traffic between other hosts. This guard " + "refuses destinations, and that traffic has none: at `recv` it " + "is indistinguishable from the loopback bytes the suite is " + "entitled to. Not patched rather than not patchable -- refusing " + "it means deciding per control code on a platform nobody who " + "maintains this file can run, and a guess there would be " + "coverage claimed rather than had.", } #: Names that legitimately do not exist on every platform or Python. A name #: missing here is a route that cannot be taken, which is the safe direction; #: what the register must never carry is a name no Python has. PLATFORM_OPTIONAL = frozenset({ - "CMSG_LEN", "CMSG_SPACE", "sethostname", "fromshare", "share", + "CMSG_LEN", "CMSG_SPACE", "sethostname", "fromshare", "share", "ioctl", "if_indextoname", "if_nameindex", "if_nametoindex", "recv_fds", "send_fds", "sendmsg", "sendmsg_afalg", "recvmsg", "recvmsg_into", "sendfile", }) +#: Routes that exist on exactly one platform family, with the platform that has +#: them. The register's population is derived from the running interpreter, +#: which is the right way to catch a name a later Python grows -- and is blind +#: by construction to a name only another platform has. `socket.socket.ioctl` +#: arrived exactly that way (#1642): classified nowhere, invisible to every +#: POSIX author and to the macOS and Linux legs, and red on all four Windows +#: legs one PR after the register shipped. A register that learns one +#: platform-only name per release is the sentence it replaced, one layer in. +#: +#: So the names CPython guards with `MS_WINDOWS` in `Modules/socketmodule.c` are +#: stated here and classified above. That claim is **reasoned from CPython's +#: source, not observed** -- nobody who can edit this file can run Windows -- +#: and `test_a_platform_only_route_is_present_exactly_on_its_platform` is what +#: converts it into an observation on whichever leg does run there. It also +#: fails here, on POSIX, if one of these ever shows up where it should not. +#: +#: Only `win32` is listed, and that is not an oversight: the macOS and Linux +#: populations are derived on every green leg of every PR, so they are observed +#: rather than reasoned. Windows was the one platform whose population no green +#: leg had ever agreed with. +PLATFORM_ONLY = { + "ioctl": "win32", + "share": "win32", + "fromshare": "win32", +} + _MODULE_PATCHES = tuple( n for n, c in ROUTES.items() if c == PATCHED and hasattr(socket, n)) _METHOD_PATCHES = tuple( diff --git a/tests/conftest.py b/tests/conftest.py index d0665593..b9f56a53 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -143,6 +143,13 @@ # states the four things no in-process patch can reach. A register goes red # when a route arrives unclassified; a sentence does not. # +# It goes red on the leg that has the route, though, and that is a boundary of +# its own: the population is `dir(socket)` on the running interpreter, so a +# Windows-only name is invisible to every POSIX author and to every POSIX leg. +# `socket.socket.ioctl` arrived that way and reddened four Windows legs one PR +# after the register shipped. `_netblock.PLATFORM_ONLY` states the win32 names +# so they are classified before the leg finds them (#1642). +# # Loopback and AF_UNIX stay open on purpose — `test_http_bounds.py` and the # `claude-channel` suites bind real servers on 127.0.0.1 and those are # hermetic. diff --git a/tests/test_netblock_egress_register_1584.py b/tests/test_netblock_egress_register_1584.py index 014d1552..ade5be0a 100644 --- a/tests/test_netblock_egress_register_1584.py +++ b/tests/test_netblock_egress_register_1584.py @@ -27,6 +27,7 @@ from __future__ import annotations import socket +import sys import tempfile from pathlib import Path @@ -35,6 +36,14 @@ import _netblock +#: Parametrised rather than looped, so a failure names the route in the test id. +#: Asserted non-empty at collection because a `parametrize` over an empty list +#: is a green leg that ran nothing -- this repo's own defect class, in the file +#: whose subject is that defect class. +PLATFORM_ONLY_NAMES = sorted(_netblock.PLATFORM_ONLY) +assert PLATFORM_ONLY_NAMES, "PLATFORM_ONLY is empty: these tests would run nothing" + + TEST_NET_1 = "192.0.2.1" # RFC 5737, guaranteed not routed. #: RFC 6761 reserves `.invalid`, so a red leg here cannot leave the building #: even when the route it is probing is genuinely open. @@ -185,6 +194,11 @@ def test_the_register_only_classifies_names_that_exist() -> None: """ stale = set(_netblock.ROUTES) - _module_callables() - _netblock.PLATFORM_OPTIONAL assert stale == set(), sorted(stale) + # `SOCKET_ROUTES` was outside this check until #1642 -- the same half-derived + # shape as the bug that file exists for, one register over: a method name no + # Python has could sit there forever and read as coverage. + stale = set(_netblock.SOCKET_ROUTES) - _socket_methods() - _netblock.PLATFORM_OPTIONAL + assert stale == set(), sorted(stale) def test_every_patched_route_is_one_block_outbound_actually_patches() -> None: @@ -233,6 +247,36 @@ def test_block_outbound_patches_only_names_this_interpreter_has() -> None: _tcp().connect((TEST_NET_1, 80)) +@pytest.mark.parametrize("name", PLATFORM_ONLY_NAMES) +def test_every_platform_only_route_is_classified(name) -> None: + """The population is derived, so it cannot see another platform's names. + + `ioctl` is Windows-only, was in no register, and went red on all four + Windows legs of PR #1642 while macOS and Linux stayed green -- the derived + check working exactly as designed and arriving one platform at a time, + which is the register learning its own boundary from CI. `PLATFORM_ONLY` + states those names ahead of the leg; this is what makes stating them + load-bearing rather than a comment. + """ + register = dict(_netblock.ROUTES, **_netblock.SOCKET_ROUTES) + assert name in register, name + assert name in _netblock.PLATFORM_OPTIONAL, name + + +@pytest.mark.parametrize("name", PLATFORM_ONLY_NAMES) +def test_a_platform_only_route_is_present_exactly_on_its_platform(name) -> None: + """Turns a reasoned claim into an observed one, on whichever leg can. + + Not vacuous here: on macOS and Linux this asserts every `win32` name is + absent, which is the half a POSIX author can check. On a Windows leg the + same assertion runs the other way and observes the presence that this + file, written on macOS, could only reason about from CPython's source. + """ + expected = sys.platform.startswith(_netblock.PLATFORM_ONLY[name]) + present = hasattr(socket, name) or hasattr(socket.socket, name) + assert present == expected, (name, sys.platform, present) + + def test_every_open_route_carries_a_reason() -> None: """`OPEN` is the third state; without a reason it is just a shorter list.""" for name, reason in _netblock.OPEN_ROUTES.items():