path parameters are overwritten by query parameters with the same name

[ljmsc]

🐛 Bug Report

if you have path parameter in your proto endpoint definition, you can overwrite the value by setting a query parameter with the same name. This is a security issue in my opinion since a gateway or routing rule maybe only checks the path but not the query parameters.

To Reproduce

The following service definition given:

service MyService {
  rpc GetResource(GetResourceRequest) returns (GetResourceResponse) {
    option (google.api.http) = {
      get: "/admin/{resource_id}"
    };
  }
}

One can call the API with GET <host>/admin/<some_resource_id> and gets the resource back. But it is also possible to call the endpoint like this: GET <host>/admin/<my_resource_id>?resourceId=<some_other_resource_id>

In the GRPC Handler the value of resource_id is set to <some_other_resource_id> instead of <my_resource_id>

Expected behavior

It should not be possible to overwrite path parameters with query parameters.

Actual Behavior

It is possible to overwrite path parameters with query parameters.

[johanbrandhorst]

Hi, thanks for your issue report. Could you share an example of this? I did some quick code spelunking and it seems to me that values that are specified in the path are added to the query parameter filter appropriately:

1. See definition of Custom rpc here:

   grpc-gateway/examples/internal/proto/examplepb/a_bit_of_everything.proto

   Lines 575 to 579 in 1e64a41

   	rpc Custom(ABitOfEverything) returns (ABitOfEverything) {
   	option (google.api.http) = {
   	post: "/v1/example/a_bit_of_everything/{uuid}:custom"
   	};
   	}

   . It defines uuid to be part of the path.
2. This is the generated code:

   grpc-gateway/examples/internal/proto/examplepb/a_bit_of_everything.pb.gw.go

   Line 760 in 1e64a41

   filter_ABitOfEverythingService_Custom_0 = &utilities.DoubleArray{Encoding: map[string]int{"uuid": 0}, Base: []int{1, 1, 0}, Check: []int{0, 1, 2}}

   . uuid is added to the filter.
3. This filter is used in PopulateQueryParameters to skip parsing of this value:

   grpc-gateway/runtime/query.go

   Line 56 in 1e64a41

   if filter.HasCommonPrefix(fieldPath) {

   .

If this bug report is accurate I'd suspect a problem with the filter. It also strikes me as possible that you could construct a query parameter using the json name instead of the proto name and bypass the filter. Could you confirm whether resourceId also works?

[ljmsc]

hey @johanbrandhorst you are actually right. You need to define resourceId as query param to overwrite the resource_id. That was wrong in my example above. That is also the reason why the HasCommonPrefix() function does not detect the field because the json name differs from the actual field name.

I fixed the example in my original post above.

[johanbrandhorst]

OK, then I both understand the issue and the cause. The query parameter parser is intentionally lenient in the parsing of query parameters, but clearly that is causing this problem. Removing the parsing of json name query parameters would be a breaking change, and although I'm happy to do that for what could arguably be considered a security issue, I think we should just make the filter look for both the protobuf and json name format. We can modify the gateway generator to output both the proto name and json name into the double array, which should fix the issue. Would you be willing to help contribute this fix?

[ljmsc]

hey @johanbrandhorst, I would like to help providing a fix for that. 👍 I‘ll give it a try.

[johanbrandhorst]

I think we need to add the jsonName to this line:

grpc-gateway/protoc-gen-grpc-gateway/internal/gengateway/template.go

Line 142 in 1e64a41

return fmt.Sprintf("&utilities.DoubleArray{Encoding: map[string]int{%s}, Base: %#v, Check: %#v}", e, f.Base, f.Check)

.

[ljmsc]

there we go #3072