test: avoid http.client for malformed header checks #35647

Fixes #35632.

This updates the malformed whitespace header checks in interface_http.py to send exact raw HTTP requests instead of using http.client to construct and parse deliberately invalid requests.

The failing case is platform-sensitive: the server can correctly send HTTP/1.1 400 and close the connection, while Python on Windows may surface the close as ConnectionAbortedError before http.client returns an HTTPResponse. Sending raw bytes and checking the raw status line matches the pattern already used by nearby malformed-request tests and keeps the assertion focused on the server behavior.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35647.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

ACK 084095a7ac379c1617a410127e7eb63b986ec034

Good fix — bypassing http.client for these two cases eliminates the Windows-specific race where getresponse() throws ConnectionAbortedError before reading the 400 response. This matches the pattern already used in nearby tests (e.g. check_idle_timeout).

<details> <summary>I'd suggest refactoring the two cases into a local <code>assert_rejects_bad_header</code> helper — if more whitespace variants are added in the future, each one becomes a two-liner. Also, RFC 7230 §3.2.6 defines field-names as tokens excluding all whitespace (both <code>SP</code> and <code>HTAB</code>); the refactor makes it trivial to add the <code>HTAB</code> case for completeness.</summary>

Suggested diff at 669c2dcab0 (on top of 084095a7ac).

--- a/test/functional/interface_http.py
+++ b/test/functional/interface_http.py
@@ -552,20 +552,27 @@ class HTTPBasicsTest (BitcoinTestFramework):
 
     def check_whitespace_in_headers(self):
         self.log.info("Check that requests with whitespace in headers are rejected")
-        # Extra whitespace before colon in header.
-        conn = BitcoinHTTPConnection(self.node)
         body = '{"method": "getbestblockhash"}'
-        raw = (
-            f"POST / HTTP/1.1\r\n"
-            f"Host: {conn.url.hostname}\r\n"
-            f"Authorization : Basic {str_to_b64str(conn.authpair)}\r\n"
-            f"Content-Length: {len(body)}\r\n"
-            f"\r\n"
-            f"{body}"
-        ).encode("ascii")
-        conn.send_raw(raw)
-        response = conn.recv_raw().decode()
-        assert response.startswith("HTTP/1.1 400")
+
+        def assert_rejects_bad_header(conn, header_line):
+            raw = (
+                f"POST / HTTP/1.1\r\n"
+                f"Host: {conn.url.hostname}\r\n"
+                f"{header_line}\r\n"
+                f"Content-Length: {len(body)}\r\n"
+                f"\r\n"
+                f"{body}"
+            ).encode("ascii")
+            conn.send_raw(raw)
+            assert conn.recv_raw().decode().startswith("HTTP/1.1 400")
+
+        # Extra whitespace (space) before colon in header field-name.
+        conn = BitcoinHTTPConnection(self.node)
+        assert_rejects_bad_header(conn, f"Authorization : Basic {str_to_b64str(conn.authpair)}")
+
+        # Extra whitespace (tab) before colon in header field-name.
+        conn = BitcoinHTTPConnection(self.node)
+        assert_rejects_bad_header(conn, f"Authorization\t: Basic {str_to_b64str(conn.authpair)}")
 
         # Extra whitespace at start of new line.
         # "line folding" as defined in
@@ -573,17 +580,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
         # is considered unsafe and is explicitly deprecated in
         # https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4
         conn = BitcoinHTTPConnection(self.node)
-        raw = (
-            f"POST / HTTP/1.1\r\n"
-            f"Host: {conn.url.hostname}\r\n"
-            f"Authorization: Basic \r\n {str_to_b64str(conn.authpair)}\r\n"
-            f"Content-Length: {len(body)}\r\n"
-            f"\r\n"
-            f"{body}"
-        ).encode("ascii")
-        conn.send_raw(raw)
-        response = conn.recv_raw().decode()
-        assert response.startswith("HTTP/1.1 400")
+        assert_rejects_bad_header(conn, f"Authorization: Basic \r\n {str_to_b64str(conn.authpair)}")
 
 
 if __name__ == '__main__':

</details>

Thanks, good point. I refactored the repeated raw-request/assertion pattern into a local helper and added the HTAB-before-colon case for completeness.

ACK 8d29ace9f2d5d3b12e73e061ac576577245b56c9

utACK 8d29ace9f2d5d3b12e73e061ac576577245b56c9 (I couldn't verify the Windows flake myself.)

nit: the commits could be squashed into a single one.

Force-pushed a clean single-commit branch on top of current master.

The diff is still limited to test/functional/interface_http.py and keeps the previously requested helper refactor plus both HTAB cases:

• whitespace before :: Authorization\t: Basic ...
• folded whitespace: Authorization: Basic \r\n\t...

Verified compare state after the push: ahead_by: 1, behind_by: 0, total_commits: 1.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/28718398451/job/85241060956} _{LLM reason (✨ experimental): CI failed due to Python lint errors in test/functional/interface_http.py (missing trailing newline; also incorrect executable permission for the shebang file).}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

I don't think this is the right approach. It looks to me like the intermittent issue is a result of the same race condition described (and addressed) this way:

https://github.com/bitcoin/bitcoin/blob/116b68e0263f853b9fbe24794ca4c75d2aa32aa3/test/functional/interface_http.py#L336-L346

I also don't really understand the title. You're using BitcoinHTTPConnection which wraps http.client right?

I don't think this is the right approach. It looks to me like the intermittent issue is a result of the same race condition described (and addressed) this way:

I agree that changing the test reader may avoid the symptom but it does not make the 400 reliably observable to a normal HTTP client.

I can also refer from the discussion below when try/catch was introduced #34772 (review)

The oversized-body is a different case the client may still be uploading 32 to 40 MB when the server rejects it. It is reasonable for sending and receiving to race there.

The server detects the invalid header and produces a 400 Bad Request which is okay

Invalid header field-name contains whitespace
HTTPResponse (status code: 400 size: 129) added to send buffer
Sent 129 bytes
Disconnecting HTTP client
......
ConnectionAbortedError: [WinError 10053]

so the problem might be the ordering of socket teardown coz the server sends the response and closes immediately while unread request bytes still remains and so on windows, that close may produce a TCP reset which prevents Python from observing the response. here is parse error path https://github.com/bitcoin/bitcoin/blob/ee61b11a9e7cba73bc3e51b3c7b01de78a5a6655/src/httpserver.cpp#L1004-L1015

Then DisconnectClients() at the end of the same I/O-loop iteration: https://github.com/bitcoin/bitcoin/blob/ee61b11a9e7cba73bc3e51b3c7b01de78a5a6655/src/httpserver.cpp#L1060

Even if Send() reports all 129 bytes accepted by the kernel, that doesn’t mean the client app has read them before the reset.

based on RFC 9112 §9.6 it describes this problem and the lingering-close mitigation

The whitespace header is a small request sent normally for a standard client should be able to receive the 400.

The parse-error path is stricter in the wrong direction based on my understanding currently the sequence is

1 detect parse error
-> 2 WriteReply(400 or 413)
-> 3 m_disconnect = true
-> 4 DisconnectClients() closes (same I/O loop turn)
-> 5 unread request bytes may remain
-> 6 Windows TCP RST → client never sees the 400

perhaps the sequence should be in this order:

1 detect parse error
-> queue 400/413 with Connection: close
-> 2 flush response through the normal I/O loop
-> 3 briefly read/discard remaining request input
-> 4 close after client EOF or a bounded timeout

Disclaimer i used llm to research

Closing for now. The CI is red for two weeks now, without any update and it is unclear if the author violates https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md