-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Verify that a trailers-only response is well-formed #685
Conversation
if connectErr, ok := asError(err); ok { | ||
return connectErr | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from some suggestions on the error text, this LGTM.
Co-authored-by: Akshay Shah <[email protected]>
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.
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).