Skip to content

client: validate the content-type of Connect streaming responses - #282

Draft
iainmcgin wants to merge 1 commit into
mainfrom
fix/275
Draft

client: validate the content-type of Connect streaming responses#282
iainmcgin wants to merge 1 commit into
mainfrom
fix/275

Conversation

@iainmcgin

Copy link
Copy Markdown
Collaborator

A Connect-protocol streaming client never looked at the response content-type. The gRPC and gRPC-Web arms of make_server_stream have been gated through check_grpc_response_head since #266, and the Connect unary path has its own inline check, but the Protocol::Connect arm of make_server_stream (behind call_server_stream and BidiStream) and parse_connect_client_stream_response (behind call_client_stream) constructed the stream or read the body whatever the header said. Connect and gRPC share envelope framing, so a 200 application/grpc+proto reply was decoded as Connect and its data envelopes handed to the caller as messages until the body ended without END_STREAM (internal), and a proxy's 200 text/html error page was parsed as an envelope header and reported as resource_exhausted ("message size 1752460652 exceeds limit"). connect-go's connectStreamingClientConn.validateResponse rejects both with unknown before reading a byte.

This adds validate_connect_response_content_type, one classifier for all three Connect shapes, called on the 200 path of both streaming parsers before the stream is constructed or the body read (a mismatched reply is no longer buffered up to max_message_size either). HTTP-status precedence is unchanged: a non-200 Connect reply still parses its JSON error body first. A content-type that is a Connect type for the shape (application/connect+* for a stream, application/proto / application/json for unary) but not the configured codec's is internal; anything else is unknown — connect-go's rule for streams. The rejection carries the response headers, and call_server_stream / call_client_stream / BidiRecvHalf::message document the gate under # Errors.

A missing content-type header stays accepted. connect-go rejects it, but the gRPC validator here accepts it for proxy compatibility and the unary Connect path always has; the cost, stated on the classifier, is that a header-less non-Connect body still fails on its framing.

The unary Connect check now shares the classifier. Two observable consequences: its message gains the (expected …) suffix the gRPC message already had, and it strips media-type parameters and matches exactly instead of by prefix, so application/protobuf for a proto client — accepted by the old starts_with("application/proto") — now fails with unknown. The changelog fragment names that signature.

Tests: the streaming matrix tests gain Protocol::Connect rows; the row that pinned the gap is gone; the status-precedence test gains a Connect 502 text/html case whose JSON error body is still parsed; the unary classification is pinned directly; a new call_client_stream test drives text/html, application/grpc+proto and application/connect+json through the real call path.

Fixes #275

Deferred review findings

  • Media-type comparison is case-sensitive, as in connect-go, the gRPC validator and Protocol::detect_from_content_type; a crate-wide decision.
  • The classifier duplicates the header-fetch / parameter-strip prelude of validate_grpc_response_content_type; sharing it means touching the gRPC validator, deliberately left alone here.
  • A non-UTF-8 content-type renders as empty in the message (pre-existing in the gRPC validator).
  • is_streaming: bool is a positional bool at three private call sites; the test module now has two canned-200 transports that could merge; client-stream non-200 precedence is pinned only through make_server_stream.

Gates: clippy 1.95.0 -D warnings clean; cargo test --workspace 938 passed; cargo test -p connectrpc --no-default-features 514 passed; rustdoc -Dwarnings clean; nightly fmt check clean; task conformance:test-client-connect-only 2580 passed, 0 failed.

A Connect-protocol streaming client never looked at the response
content-type. The gRPC arms of make_server_stream are gated through
check_grpc_response_head and the Connect unary path has its own inline
check, but the Connect arm of make_server_stream (call_server_stream and
BidiStream) and parse_connect_client_stream_response (call_client_stream)
constructed the stream or read the body whatever the header said. Connect
and gRPC share envelope framing, so a 200 application/grpc+proto reply was
decoded as Connect and its data envelopes handed to the caller as messages
until the body ended without END_STREAM, and a proxy's 200 text/html error
page was parsed as a five-byte envelope header and reported as
resource_exhausted, a code a retry policy treats as back-off rather than
"the peer is not speaking Connect".

Add validate_connect_response_content_type, one classifier for all three
Connect shapes, and call it on the 200 path of both streaming parsers
before the stream is constructed or the body read. HTTP-status precedence
is unchanged: a non-200 Connect reply still parses its JSON error body
first, so the gate only ever sees 200s. A Connect content-type for the
shape but the wrong codec is internal; anything else is unknown. The
rejection carries the response headers. A missing header stays accepted,
as on the gRPC and unary paths; connect-go rejects it, but an intermediary
that strips the header would otherwise fail an otherwise well-formed call.

The unary Connect check now shares the classifier instead of being a third
copy. Its message gains the "(expected ...)" suffix the gRPC message has,
and it strips media-type parameters and matches exactly instead of by
prefix, so application/protobuf for a proto client is now rejected.

Tests: the two streaming matrix tests gain Connect rows and lose the grpc
in their names, the row pinning the gap is gone, the HTTP-status
precedence test gains a Connect case whose JSON error body is still
parsed, the unary classification is pinned directly, and a new
call_client_stream test drives text/html, application/grpc+proto and
application/connect+json through the real call path.

Fixes #275

Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
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.

client: validate the content-type of Connect-protocol streaming responses

1 participant