Validate a caller-supplied extensions["timeout"] - #1189
Conversation
The mapping reaches the transport unchanged and its values are handed to `socket.settimeout()`, so a non-numeric value raised a stdlib TypeError from inside the connect, read or write path rather than a clear error from httpx2. Refs pydantic#1162
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/httpx2/httpx2/_client.py">
<violation number="1" location="src/httpx2/httpx2/_client.py:125">
P3: A boolean slips through the int/float check because `bool` subclasses `int`, so `extensions['timeout'] = {"read": True}` passes validation and becomes a 1-second timeout instead of raising. This contradicts the PR's goal of rejecting non-timeout values. Explicitly exclude `bool` from the accepted types.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "Use `Timeout(...).as_dict()` to build one." | ||
| ) | ||
| for name, value in timeout.items(): | ||
| if value is not None and not isinstance(value, (int, float)): |
There was a problem hiding this comment.
P3: A boolean slips through the int/float check because bool subclasses int, so extensions['timeout'] = {"read": True} passes validation and becomes a 1-second timeout instead of raising. This contradicts the PR's goal of rejecting non-timeout values. Explicitly exclude bool from the accepted types.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/_client.py, line 125:
<comment>A boolean slips through the int/float check because `bool` subclasses `int`, so `extensions['timeout'] = {"read": True}` passes validation and becomes a 1-second timeout instead of raising. This contradicts the PR's goal of rejecting non-timeout values. Explicitly exclude `bool` from the accepted types.</comment>
<file context>
@@ -113,6 +113,19 @@ class UseClientDefault:
+ "Use `Timeout(...).as_dict()` to build one."
+ )
+ for name, value in timeout.items():
+ if value is not None and not isinstance(value, (int, float)):
+ raise TypeError(f"extensions['timeout'][{name!r}] must be a number or None, got {type(value).__name__}.")
+
</file context>
There was a problem hiding this comment.
I'd leave this one. The guard admits exactly what socket.settimeout admits, which is the point of it: nothing that works today starts raising. bool is in that set, and it is reachable without the extensions dict at all, since Timeout(5.0, read=True).as_dict() returns {'read': True, ...} and the value goes straight through. So excluding it here would only make the two paths disagree.
The behaviour is worse than a 1s read though, and that part is worth knowing: settimeout(False) sets the socket non-blocking rather than raising. Happy to file that separately against Timeout if you think it's worth rejecting bools on both paths.
Request shallow-copies extensions, so request.extensions["timeout"] is the mapping the caller passed in and the equality was x == x. Snapshot it with deepcopy first, which also pins that the new validator does not mutate its argument.
Summary
extensions["timeout"]reaches the transport unchanged and its values land insocket.settimeout(), so patchingconnect_tcpfixes the traceback in the issue but not the rest of it: put aTimeoutinreadorwriteand you get the same TypeError out of_backends/sync.py:126and:137. I put the check where the caller's mapping is accepted instead, so it covers every key and both thebuild_requestandsend()paths.I went with raising rather than coercing. Coercing means guessing which attribute was meant, and the
getattr(timeout, "connect", None)version turns any other object intoNone, which is no timeout at all rather than an error. Can switch it if you'd prefer.The guard accepts
int,floatandNone, which is whatsocket.settimeoutitself accepts, so nothing that works today starts failing (DecimalandFractionalready raise there). The extensions doc already shows this as a mapping of floats, so there was nothing to change there.This leaves httpcore2 alone, so using httpcore2 directly still reaches the stdlib error. That is why it is
Refsand notFixes.Refs #1162
Checklist