grpc-web interface not compatible with dart client

[bamnet]

Describe the bug

The grpc-web interface exposed by the connect-go server is not compatible with the grpc dart client when used in a web application. As an example, I'm working on a Flutter application which connects over Grpc on Android/iOS but Grpc-Web on Chrome. it works great on Android/iOS, but does not work on Chrome.

From what I can tell, the trailers set by connect-go are capitalized like Grpc-Status. The grpc dart client expects(?) these trailers to be all lowercase, like grpc-status. As a result of this mismatch, the Dart code can't parse the trailers and barfs returning a generic GRPC Status 2 error message even when the server has "correctly" returned a status 0 message.

To Reproduce

1. Start a standard connect-go server which exposes Grpc and Grpc-Web interfaces by default. (Server#1)
2. Also start a standard envoy proxy to expose an alternative Grpc-Web interface to the previously started server. (Proxy#1)
3. Point a dart grpc client at Proxy#1. (perhaps using a channel like GrpcWebClientChannel.xhr(Uri.parse('http://localhost:8080'));
4. Observe the client works fine when run in a browser.
5. Switch the grpc-web client to point to Server#1 powered by connect-go.
6. Observe the client no longer works.
7. Switch the client to connect to grpc NON WEB.
8. Observe the client works fine.

Along the way, you can use a packet sniffer to observe a difference in trailer capitalization between the quasi-official envoy proxy and the connect-go implementation.

To simulate a fix in, I lowercased the values of grpcHeaderMessage and grpcHeaderStatus in protocol_grpc.go. This fixes the client (but probably isn't the right way to fix this).

Environment (please complete the following information):

• connect-go version or commit: v1.5.1
• go version: go version go1.19 linux/amd64
• dart --version: Dart SDK version: 2.18.6 (stable) (Tue Dec 13 21:15:14 2022 +0000) on "linux_x64"
• dart grpc: v3.1.0

Additional context

I don't know what the Grpc-web "spec" says is correct here. I'm not sure if this is a bug in connect-go or the dart grpc client. I'm happy to chase down a bug on the dart side if this is more of a client issue than a server one but wanted to file here to start.

[bamnet]

Actually, https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md says "use lower-case header/trailer names",

This matches a fix made in improbable-eng/grpc-web#228

[jhump]

@bamnet, HTTP header names are not supposed to be case-sensitive.

With HTTP/2, they get normalized to all lower-case to aid with HPACK header compression. I wonder if that's why the protocol spec insists on a lower-case name. Many HTTP clients, however, normalize to camel-case when using HTTP 1.1 (including the internal representation of Go's http.Header: see here and here).

While a fix might be appropriate here in connect-go (since the web protocol document insists on all lower-case), it also seems appropriate to put fixes into both the protocol spec and the Dart gRPC runtime since the IETF specs for HTTP clearly state that header and trailer names are not case-sensitive.

[akshayjshah]

Echoing @jhump, this seems like a bug in grpc-dart. I've summarized the issue and tagged @cbatson on grpc/grpc-dart#594 - hopefully there's a way to fix the metadata implementation there to follow HTTP semantics. I've also opened grpc/grpc#32364 to propose bringing the gRPC-Web specification in line with the relevant HTTP RFCs.

I'd like to wait and see if we can come to some resolution on those issues before making any changes here.

[mraleph]

Responding here to @akshayjshah's comment

... because it violates RFCs 9110 (HTTP Semantics), 9112 (HTTP/1.1), and RFC 9113 (HTTP/2), all of which require that header and trailer names be compared case-insensitively.

This statement is not entirely correct. While it is true that HTTP & HTTP/1.1 field names are in fact case insensitive, HTTP/2 is pretty unambiguous:

Implementations MUST perform the following minimal validation of field names and values:

• A field name MUST NOT contain characters in the ranges 0x00-0x20, 0x41-0x5a, or 0x7f-0xff (all ranges inclusive). This specifically excludes all non-visible ASCII characters, ASCII SP (0x20), and uppercase characters ('A' to 'Z', ASCII 0x41 to 0x5a).

A request or response that contains a field that violates any of these conditions MUST be treated as malformed (Section 8.1.1).

gRPC-Web is derived from the native HTTP/2 protocol so naturally it simply follows the same requirement.

I would also like to highlight that gRPC-Web is a protocol on top of underlying HTTP/* protocol so it certainly can add additional requirements on top of what HTTP/1.1 requires. For example in this case it requires more normalized headers and trailers than what underlying protocol mandates.

Consequently as it stands now it is most certainly a bug in connect-go code, which is in clear violation of gRPC-Web specification. If gRPC-Web specification gets relaxed as proposed in your PR then it will become a bug in grpc-dart code, but not before that.

Note that canonical JavaScript implementation of gRPC-Web does also expect that field names are sent in lower case (e.g. see this code used for parsing trailers).

This also means that relaxing specification right now would effectively mean a breaking change for all the clients which rely on existing specification. I don't think that makes sense.

[akshayjshah]

@mraleph I agree that the current gRPC-Web specification insists on lower-cased header and trailer names on the wire. We can defer a discussion of whether that makes sense in HTTP/1.1 to grpc/grpc#32364; regardless of how that PR turns out, the current grpc-dart behavior seems like it may be incorrect to me.

HTTP/2 field names must indeed be lower-case on the wire, as the section of RFC 9113 you've quoted clearly says. However, the semantics of HTTP/2 header names are covered in RFC 9110. From section 1.2:

All three major versions of HTTP rely on the semantics defined by this document.

And later, in section 5.1:

Field names are case-insensitive and ought to be registered within the "Hypertext Transfer Protocol (HTTP) Field Name Registry"

RFC 7540, which was the authoritative HTTP/2 specification when the gRPC-Web spec was written, addressed the interplay between the wire format and the semantics more clearly:

Just as in HTTP/1.x, header field names are strings of ASCII
characters that are compared in a case-insensitive fashion. However,
header field names MUST be converted to lowercase prior to their
encoding in HTTP/2. A request or response containing uppercase
header field names MUST be treated as malformed (Section 8.1.2.6).

It seems to me that the intention here is to send lower-case names on the wire, but to treat headers.Get("Some-Name") and headers.Get("some-name") identically. I don't see anything in the gRPC-Web specification, even as it stands today, that suggests that these HTTP semantics don't apply - the current language exclusively addresses the wire representation. Even though they only support HTTP/2, other Google-authored gRPC implementations behave as I've suggested: see grpc-go's Metadata and grpc-java's Metadata.Key.

All that said, I don't intend to suggest any changes that would be backward-incompatible for Envoy or grpc-web - none of this is worth breaking the canonical server and client. Despite the code you've linked, I'm fairly confident that grpc-web works with connect-go as it is today: as part of connect-crosstest, we run the gRPC-Web interop tests nightly. I'll check with @timostamm or @doriable tomorrow and make sure, though - it's possible that I've lost track of the details of our compatibility test matrix.

More broadly, it seems to me that there are many more gRPC-Web implementations than the current pseudo-specification anticipated. It would be helpful to the whole ecosystem to have a formal specification rather than the current casual description: grpc/grpc#30565

[akshayjshah]

@mraleph Update on grpc-web: we're both half-right. When unary RPCs fail (or server streaming RPCs fail without sending any messages) and the server sends a trailers-only response, grpc-web treats field names case-insensitively. However, when grpc-web is hand-parsing length-prefixed trailers, it's case-sensitive. This seems fairly easy to fix in GrpcWebClientReadableStream.

[mraleph]

This seems fairly easy to fix in GrpcWebClientReadableStream.

GrpcWebClientReadableStream is perfectly aligned with the spec though. The server on the other hand is not and is sending a malformed response.

Currently two options are possible:

1. Specification gets changed and suddenly all clients which were previously perfectly fine are not and need fixing.
2. The server gets fixed.

1 is essentially a breaking change with wide implications. 2 is not - server code is wrong it gets fixed, everybody is happy. No non local impact. So I think the choice between 1 and 2 is fairly straightforward.

[akshayjshah]

Yes, the hand-written trailers parsing in grpc-web follows the spec as you describe. But most RPCs are unary, so most errors are sent as trailers-only responses, so most of grpc-web's grpc-status and grpc-message access is more permissive. As I understand it, the "spec" is intended to be a description of the canonical implementation(s) rather than a normative document. Furthermore, I assume that gRPC-Web intends to be as close to standard HTTP semantics as possible, so that it's easy to implement on top of commodity HTTP implementations (like fetch). Put differently, I assume that all portions of RFC 9110 not specifically overridden by the gRPC-Web specification still apply.

Changing grpc-web's hand-written trailer parsing doesn't seem like it'll break compatibility - both old and new versions will continue to work with Envoy, the reference server. All grpc-status and grpc-message values currently being read will continue to be read. I don't know much about Dart - I'm certainly not the compiler team's lead! - but it seems to me that normalizing field names to lower-case before exposing them to users wouldn't be onerous for the maintainers or a notable breaking change to users. I guess I just don't see clarifying the specification as the widespread correctness catastrophe you're describing.

I also see "2 is not - server code is wrong it gets fixed, everybody is happy" as inaccurate. I'd like gRPC-Web - and protobuf-based RPC in general - to be more attractive to web developers familiar with HTTP's usual semantics. Insisting on matching HTTP/1.1 headers case-sensitively is, IMO, an entirely avoidable wart in a protocol that's already struggling to gain significant traction.

If I'm off-base and the gRPC team intends user access to metadata to be case-sensitive, I don't mind changing connect-go. It sure seems like an odd break with standard HTTP semantics, though, so I'd like to at least ask.

[akshayjshah]

@bamnet connect-go is now correctly lower-casing the trailing metadata (the block of HTTP/1.1-formatted trailers at the end of the response body), so the Dart client should handle the grpc-status, grpc-message, and grpc-status-details-bin values as expected.

I'm waiting on grpc/grpc#32364 before also lower-casing HTTP headers.

[bamnet]

Thanks for the fix! My flutter app is now successfully working on Android, iOS, Web, and Desktop.

[akshayjshah]

As a final update here, grpc/grpc#32364 was accepted. This should clarify that connect-go is now correctly implementing the specification: HTTP header names may be upper- or mixed-case over HTTP/1.1, but the block of trailing metadata encoded in the body must use lower-case field names.