Modifying errors that occur before interceptors

[anzboi]

Is your feature request related to a problem? Please describe.

Our security team flagged a low risk issue that is probably worth addressing anyway. Errors that occur before a request reaches interceptors/endpoint are not masked with a generic error response.

Describe the solution you'd like

In general it is good practice to catch any unhandled errors and provide some kind of generic response to clients. Connect currently has no generic error handler for errors that occur before request handling reaches the interceptors or endpoint. In particular, we would like the following

1. Errors that occur before a request reaches the interceptors/endpoint should not be returned, instead replaced with some generic error.
2. The ability to customise handling for errors like this.

Describe alternatives you've considered

Since this occurs before control is handed to anything we have control over, the only other option is to implement a custom http.ResponseWriter that somehow detects when the error response is created before handling reaches interceptors/endpoints. This is not a trivial task.

Additional context

Example test case. This produces a response showing some internal service details is being returned to a client (a client might guess the service is written in go). It is also worth noting there is no way to log this error on the server side AFAICT.

• We use the enum field to trigger an error in protojson unmarshal. While we technically have control over that and could mask/log errors, it is annoying and obscure to implement the above feature request using this method.

service TestService {
    rpc Method(Request) returns (Response) {}
}
message Request {
    MyEnum field1 = 1 [(validate.rules).enum = {defined_only: true}];
}
message Response {}
enum MyEnum {
    MyEnum_UNSPECIFIED = 0;
    MyEnum_VALUE = 1;
}

func TestInvokeProtojsonError(t *testing.T) {
	mux := http.NewServeMux()
	mux.Handle(myapiconnect.NewTestServiceHandler(myapiconnect.UnimplementedTestServiceHandler{}))
	svr := &http.Server{Handler: h2c.NewHandler(mux, &http2.Server{})}
	lis, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		t.Fatal(err)
	}
	go func() { _ = svr.Serve(lis) }()
	t.Cleanup(func() { _ = svr.Close() })

	// Set the enum field to a value outside int32 range
	body := bytes.NewReader([]byte(`{"field1": 1111111111111}`))
	url := "http://" + lis.Addr().String() + myapiconnect.TestServiceMethodProcedure
	contentType := "application/json"

	response, err := http.Post(url, contentType, body)
	if err != nil {
		t.Fatal(err)
	}
	responseBody, err := io.ReadAll(response.Body)
	if err != nil {
		t.Fatal(err)
	}

	t.Log(response.Status)
	t.Log(string(responseBody))
	t.Fail() // fail so we can see test output
}

Resulting output

Status: 400 Bad Request
Body: {"code":"invalid_argument","message":"unmarshal into *myapi.Request: proto: (line 1:12): invalid value for enum type: 1111111111111"}

[jhump]

@anzboi, we are looking into this and exploring options in the connect-go runtime.

[jhump]

I've audited all of the kinds of errors that can happen before the interceptor or handler is called.

There are three errors that do not result in any RPC error written to the output, because they are protocol errors -- the server can't even determine what protocol the client is using, so it doesn't know what format to use for errors. So it just writes an HTTP code, sets some HTTP response headers, and leaves the response body empty:

1. 505 HTTP Version Not Supported: When a bidi stream is invoked using HTTP 1.
2. 405 Method Not Allowed: When the request uses an unsupported HTTP method. Also sets Allow header.
3. 415 Unsupported Media Type: When the request uses an unsupported content-type. Also sets Accept-Post header.
4. 404 Not Found: When the request uses an unrecognized URL path, to a resource/service/method that is not known to the server.

Then there are other errors that can prevent dispatch of the RPC. So the protocol is known, but something else about the request cannot be handled by the server. The table below enumerates them all.

Issue	Code	Message
Unsupported compression encoding	Unimplemented	unknown compression %q: supported encodings are %v
http.ResponseWriter does not implement http.Flusher	Internal	%T does not implement http.Flusher
Missing connect query param *	Invalid Argument	missing required query parameter: set %s to %q
connect query param has unexpected value *	Invalid Argument	%s must be %q: got %q
Missing encoding or message query param *	Invalid Argument	missing %s parameter
Unsupported codec/sub-format *	Invalid Argument	invalid message encoding: %q
Missing Connect-Protocol-Version header **	Invalid Argument	missing required header: set %s to %q
Connect-Protocol-Version header has unexpected value **	Invalid Argument	%s must be %q: got %q
Request for unary RPC is too large	Resource Exhausted	%s: exceeded %d byte http.MaxBytesReader limit
		message size %d is larger than configured max %d
		message is larger than configured max %d - unable to determine message size: %w
I/O error reading request for unary RPC	Unknown	read message: %w
Error decompressing request for unary RPC	Invalid Argument	decompress: %w
Error deserializing request for unary RPC	Invalid Argument	unmarshal into %T: %w

* Only applies to Connect unary protocol when using GET HTTP method.
** Only applies to Connect unary protocol when using POST HTTP method.

@anzboi, so I think the only case that potentially leaks information is the one you describe -- the last entry in the table, where the request message for a unary RPC cannot be deserialized. In this case, most of the error comes from the Codec implementation, but not all. (The second entry in the table also includes a %T format, but that would not be an intermittent error, but a configuration error that would likely cause all requests to fail, so highly unlikely to ever be observed outside of a failing test case.)

So a very targeted solution to this one class of errors is to fix the framework so that all of the error message comes from the Codec. Then you can use custom Codec implementations to mask the errors. These implementations can delegate to the normal serialization/deserialization methods (in the google.golang.org/protobuf/proto and google.golang.org/protobuf/encoding/protojson packages), but return masked errors in the face of failures.

How does that sound as a very pinpointed work-around?

[anzboi]

@jhump
Yeah that sounds like it will work. It is good to know that implementing a custom Codec will cover all current cases.

I do worry about the robustness of this work-around in the long term. While this table is great to see, it can change over time, very easy to accidentally introduce a new error that occurs in some cases.

It also feels like an odd way to handle an issue like this.

• Problem: you are returning internal error details such as an unmarshal error
• Solution: Implement a custom Codec that logs errors and returns generic
• Explanation: The Codec is the only place that could potentially throw an error like this.

[akshayjshah]

@anzboi What error message would you expect here? We designed under the assumption that the schema for the API is available to all clients of the API (as is typical for Protobuf), so to my eye the error message isn't leaking anything that the client doesn't already have access to.

[anzboi]

What error message would you expect here

Something generic. This particular error case was flagged as low risk internally so keep in mind this is not really a big deal.
Some of the concern was around unknown errors that could be more significant, It was the fact that there appeared to be no generic error message that raised the concern. You might also be able to guess some things that you probably shouldn't, like that fact that the server is written in go, and uses the connect framework, again, not the end of the world.

Knowing the full set of possible errors alleviates this somewhat so personally I'm happy without a fix (or simply implementing a custom Codec). Maybe our security partner @danh-ng can provide more details.

[jhump]

FWIW, a proper solution could still be implemented in the future. But at the moment, a good solution is not clear, and any solution would almost certainly increase exported API surface area that we must remain compatible with until a v2 (or forever). So we're being cautious in what we change and add, so that we don't get stuck supporting a "smelly band-aid" that could make better solutions harder to implement in the future.

Actually winding these calls through an interceptor, particularly a unary interceptor, is problematic because the signature of the interceptor functions doesn't provide anyway for the framework to indicate that the RPC has already failed. The unary interceptor is the most problematic because it receives a deserialized request message, which cannot actually be provided under the above error scenarios. One possible thought is to provide a nil message in these cases, but that would be error-prone and not actually backwards-compatible as it would likely cause existing interceptor implementations to start panic'ing due to dereferencing the nil pointer.

So implementing a custom Codec isn't meant to be a solution so much as a work-around.

[akshayjshah]

The error in question is now fully controlled by the Codec implementation. To customize it, users can now replace the protobuf and/or protojson codec.

This will go out with the next release.