grpc / grpc-dart

The Dart language implementation of gRPC.
https://pub.dev/packages/grpc
Apache License 2.0
861 stars 271 forks source link

Surprising behavior possible due to lower-case metadata keys #594

Open cbatson opened 1 year ago

cbatson commented 1 year ago

Observation 1: As per the spec, "header field names MUST be converted to lowercase prior to their encoding in HTTP/2." This is enforced silently client-side in the _sanitizeMetadata method.

Observation 2: On the server side, in ServiceCall the clientMetadata property is of type Map<String, String>.

Consequence: If

  1. A user is not aware of the lower-case metadata key requirement; and
  2. is not aware of the Dart gRPC client's lower-casing of metadata keys; and
  3. uses metadata key names with upper case letters on the client/server

then those metadata key names with upper case letters will not be retrieved on the server side, because there will be no keys with upper case letters in the clientMetadata map.

Thus, it is humbly suggested to implement one of the following enhancements:

  1. Require that key names on the client side be provided by the user already lower case. In other words, rather than silently adjust them to lower case, throw i.e. an ArgumentError. (Obviously a potentially breaking change.)
  2. On the server side, use something other than vanilla Map for clientMetadata. This could be a map that treats keys as case-insensitive, or one that lower-cases incoming keys (i.e. in operator[]).

P.S. Not just academic, I am the aforementioned unaware and surprised user. 😅

akshayjshah commented 1 year ago

👋🏽 I've come here via https://github.com/bufbuild/connect-go/issues/453, where @bamnet encountered the same problem.

This seems like a particularly pernicious issue because it violates RFCs 9110 (HTTP Semantics), 9112 (HTTP/1.1), and 9113 (HTTP/2), all of which require that header and trailer names be compared case-insensitively. I've opened https://github.com/grpc/grpc/pull/32364 to suggest clarifying this portion of the gRPC-Web protocol document (though the same semantics apply to the HTTP/2 gRPC protocol).

I know very little about Dart, but these semantics seem to be handled correctly in the non-gRPC HTTP ecosystem: the HttpHeaders class from dart:io always treats names case-insensitively. Among the Google-authored gRPC implementations, at least grpc-java and grpc-go also treat field names case-insensitively.

Is there any way to pursue option (2) from @cbatson's proposal above?

stonymahony commented 9 months ago

I am unfortunately still suffering from this problem. I have been following the discussions in grpc-dart, grpc-web and connectrpc and the consensus seems to be that the http headers should be handled case insensitive by the client side (see e.g. https://github.com/connectrpc/connect-go/issues/453, https://github.com/grpc/grpc/pull/32364)

Would a PR that implements this be welcome?

mosuem commented 9 months ago

Hi @stonymahony, I believe adapting the clientMetadata to be case-insensitive seems like a good idea, feel free to open a PR!