Fix responses lost to decompression, add alpn_protocols to request() - #78
Open
liquidsec wants to merge 1 commit into
Open
Fix responses lost to decompression, add alpn_protocols to request()#78liquidsec wants to merge 1 commit into
liquidsec wants to merge 1 commit into
Conversation
Two problems found while building against real targets. A response that declared Content-Encoding but carried no body was thrown away entirely. gzip and brotli correctly report there's no stream to read, and the `?` turned that into a transport error, so the caller couldn't tell the host apart from an unreachable one. Since we ask for `gzip, deflate, br` on every request, any bodyless response carrying a Content-Encoding hit this: bodyless redirects, HEAD responses (which echo the entity headers of the GET they mirror), and 304s (which carry the headers a 200 would). Those last two are correct server behavior, not edge cases. `request_batch` and `request_batch_stream` were affected too, since they share `parse_response`. `max_body_size` made it worse in two ways. It truncated the compressed bytes mid-stream, and the resulting decode failure discarded the whole response, so the cap couldn't be used with compressed bodies at all. And it only ever bounded the bytes read off the wire, never the inflated result, so a 291KB response could expand to 300MB resident, well past the 10MiB default. So `decompress` now takes the cap and reads incrementally: an empty body is empty whatever the header claims, output is bounded at every layer, and a stream that breaks partway keeps what inflated instead of failing. More broadly, a body that won't decode no longer costs you the response. Whatever the reason, the status line and headers arrived cleanly and the raw bytes are what the server actually sent, so they're handed back undecoded with a note in the debug log. Discarding the response instead looks identical to an unreachable host from the caller's side, which throws away far more than a body we can't read. That matters most for the tool this is for: unexpected bytes aren't grounds for dropping evidence. `read_body` already bounds those bytes, so returning them can't exceed the cap. Content-Encoding is an ordered list, but only an exact match on the whole header value was recognized, so `gzip, br` and the `x-gzip` alias fell through and handed back a still-compressed body as if it were content. Those are now parsed as a list and undone in reverse. A coding we can't undo returns the body untouched, since decoding the layers beneath it would only produce nonsense. Note this narrows what `max_body_size` returns. A caller setting a small cap on a large compressed body used to receive the whole thing inflated and now gets it truncated to the cap, which is what the parameter says it does. Separately, `RequestConfig.alpn_protocols` already existed and the direct-connection path already honored it, but the pooled path hardcoded an h2-first offer and never looked at the config, and the pyo3 signature for `request()` didn't accept the parameter at all. So from Python there was no way to keep a request off HTTP/2. That matters when a server puts a connection-specific header in an HTTP/2 response: RFC 9113 8.2.2 forbids it, hyper kills the stream with PROTOCOL_ERROR, and the response is lost even though the same server answers cleanly over HTTP/1.1. The only workaround was passing `resolve_ip` to divert onto the direct path, which is a DNS-pinning parameter doing protocol selection. The h2-first default is unchanged, and different offers already can't share a pooled connection since `TlsKey` includes `alpn_protocols`. Also gitignores local agent settings and compiled extension modules. A build left at the repo root shadows the installed package, because pytest puts the rootdir on sys.path, so a stale one silently gets tested instead of what you built.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two issues found while building badmerge against real targets, plus one adjacent problem found while fixing them.
Responses were being discarded over undecodable bodies
A response that declared
Content-Encodingbut carried no body was thrown away entirely. gzip and brotli correctly report there's no stream to read, and the?turned that into a transport error, so the caller couldn't tell the host apart from an unreachable one. We ask forgzip, deflate, bron every request, so any bodyless response carrying aContent-Encodinghit it: bodyless redirects,HEADresponses (which echo the entity headers of theGETthey mirror), and304s (which carry the headers a200would). The last two are required server behavior, not broken edges.request_batchandrequest_batch_streamwere affected too, since they shareparse_response.max_body_sizemade it worse twice over. It cut the compressed bytes mid-stream and the resulting decode failure discarded the whole response, so the cap was unusable with compressed bodies. And it only ever bounded what came off the wire, never the inflated result, so a 291KB response could expand to 300MB resident, 28x past the 10MiB default.decompressnow takes the cap and reads incrementally: an empty body is empty whatever the header claims, output is bounded at every layer, and a stream that breaks partway keeps what inflated.More broadly, a body that won't decode no longer costs you the response. Whatever the reason, the status line and headers arrived cleanly and the raw bytes are what the server actually sent, so they come back undecoded with a note in the debug log. Discarding the response looks identical to an unreachable host from the caller's side, which throws away much more than a body we can't read.
Content-Encodinglists and thex-gzipaliasOnly an exact match on the whole header value was recognized, so
gzip, brandx-gzipfell through and handed back a still-compressed body as if it were content. Those are now parsed as the ordered list they are and undone in reverse. A coding we can't undo returns the body untouched, since peeling the layers beneath it would only produce nonsense.alpn_protocolswas unreachable fromrequest()RequestConfig.alpn_protocolsalready existed and the direct-connection path already honored it, but the pooled path hardcoded an h2-first offer and never consulted the config, and the pyo3 signature didn't accept the parameter at all. So there was no way to keep a request off HTTP/2 from Python.That matters when a server puts a connection-specific header in an HTTP/2 response. RFC 9113 8.2.2 forbids it, hyper kills the stream with
PROTOCOL_ERROR, and the response is lost even though the same server answers cleanly over HTTP/1.1. The only workaround was passingresolve_ipto divert onto the direct path, which is a DNS-pinning parameter doing protocol selection.The h2-first default is unchanged, so this is additive. Different offers already can't share a pooled connection, since
TlsKeyincludesalpn_protocols.Behavior changes worth a look
max_body_sizenow caps the decompressed body. A caller setting a small cap on a large compressed body used to receive the whole thing inflated and now gets it truncated to the cap, which is what the parameter says it does.except RuntimeErrorfallbacks aroundrequest()will take a different branch.Also
Version bumped to 0.10.0, and compiled extension modules are gitignored. A build left at the repo root shadows the installed package, because pytest puts the rootdir on
sys.path, so a stale one silently gets tested instead of what you built.