Verify that a trailers-only response is well-formed#685
Merged
Conversation
jhump
commented
Feb 9, 2024
Comment on lines
-308
to
-310
| if connectErr, ok := asError(err); ok { | ||
| return connectErr | ||
| } |
Member
Author
There was a problem hiding this comment.
Not related, but I happened to notice that this was being done twice. (On line 312, these exact three lines are repeated. Oops, I think this is from a change of mine.)
Comment on lines
+706
to
+714
| if numBytes, err := discard(response.Body); err != nil { | ||
| err = wrapIfContextError(err) | ||
| if connErr, ok := asError(err); ok { | ||
| return connErr | ||
| } | ||
| return errorf(CodeInternal, "corrupt response: I/O error after trailers-only response: %w", err) | ||
| } else if numBytes > 0 { | ||
| return errorf(CodeInternal, "corrupt response: %d extra bytes after trailers-only response", numBytes) | ||
| } |
Member
Author
There was a problem hiding this comment.
This was largely copied from the handling of end-stream messages (in envelopeReader.Unmarshal), where we also want to verify that there is no subsequent response body data.
akshayjshah
approved these changes
Feb 12, 2024
Contributor
akshayjshah
left a comment
There was a problem hiding this comment.
Apart from some suggestions on the error text, this LGTM.
Co-authored-by: Akshay Shah <akshayjshah@users.noreply.github.com>
jhump
added a commit
that referenced
this pull request
Feb 16, 2024
This largely undoes a recent change to do more validation of trailers-only responses (#685), which disallows a body or trailers in what appeared to be a trailers-only response. In that change, a trailers-only response was identified by the presence of a "grpc-status" key in the headers. In this PR, a trailers-only response is instead defined by the lack of body and trailers (not the presence of a "grpc-status" header). This PR also tweaks some other error scenarios: * If trailers (or an end-stream message) is completely missing from a response, it's considered an `internal` error. But if trailers are present, but the "grpc-status" key is missing, it's considered an issue determining the status, which is an `unknown` error. * Similarly, if a response content-type doesn't appear to be the right protocol (like it may have come from a non-RPC server), the error code is now `unknown`. But if it looks like the right protocol but uses the wrong sub-format/codec, it's an `internal` error. * Note that in grpc-go, this behavior is also seen in the client, but this PR doesn't attempt to address that in the connect-go client. Instead, that change can be made when #689 is addressed. This PR also now makes connect-go more strict about the "compressed" flag in a streaming protocol when there was no compression algorithm negotiated. Previously, this library was lenient and did not consider it an error if the message in question was empty (zero bytes). But to correctly adhere to gRPC semantics, it must report this case as an `internal `error.
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
2 participants
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.
Previously, connect-go would accept a trailers-only gRPC response (where the trailers are in the HTTP headers, signaled by the presence of a "grpc-status" key in the headers) and assume it was valid w/out further verification.
However, it should reject trailers-only responses that also include response body data and/or HTTP trailers, after the HTTP headers as those are malformed responses.
This would have been caught by a conformance test, as exactly this sort of test is in the queue. But I haven't written those tests yet and just happened to notice when I was looking into something else in the reference client (working on somewhat-related checks of the wire-level details for gRPC-Web responses).