Skip to content

Fix responses lost to decompression, add alpn_protocols to request() - #78

Open
liquidsec wants to merge 1 commit into
devfrom
fix-decompression-and-alpn
Open

Fix responses lost to decompression, add alpn_protocols to request()#78
liquidsec wants to merge 1 commit into
devfrom
fix-decompression-and-alpn

Conversation

@liquidsec

Copy link
Copy Markdown
Collaborator

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-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. We ask for gzip, deflate, br on every request, so any bodyless response carrying a Content-Encoding hit it: bodyless redirects, HEAD responses (which echo the entity headers of the GET they mirror), and 304s (which carry the headers a 200 would). The last two are required server behavior, not broken edges. request_batch and request_batch_stream were affected too, since they share parse_response.

max_body_size made 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.

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.

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-Encoding lists and the x-gzip alias

Only an exact match on the whole header value was recognized, so gzip, br and x-gzip fell 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_protocols was unreachable from request()

RequestConfig.alpn_protocols already 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 passing resolve_ip to 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 TlsKey includes alpn_protocols.

Behavior changes worth a look

  • max_body_size now 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.
  • A body that doesn't match its declared encoding used to raise and now returns undecoded. Callers with except RuntimeError fallbacks around request() 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant