GH-11040: Hide exception in gRPC error message#11045
Conversation
cppwfs
force-pushed
the
SI-11040
branch
2 times, most recently
from
1472a49 to
9d942bd
Compare
cppwfs
changed the title
cppwfs
changed the title
Prevent internal exceptions from leaking in gRPC error responses. - Log the actual exception locally instead of sending it over the wire. - Return a generic internal server error for unhandled exceptions. - Update GrpcInboundGatewayTests to verify the new error handling. Fixes: spring-projects#11040
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
I think we also need to look into the docs and explain a bit the logic we expose for those server thrown errors.
Thanks
| return Status.fromThrowable(throwable) | ||
| private StatusRuntimeException toGrpcStatusException(Throwable throwable, @Nullable String description) { | ||
| logger.error(throwable, description == null ? "Internal Server Error" : description); | ||
| return Status.fromCode(Status.Code.INTERNAL) |
There was a problem hiding this comment.
I think this is not correct.
The application may through those gRPC-specific exceptions, so using always Status.Code.INTERNAL is not OK.
I think we need to fallback to the Internal Server Error message when Status is UNKNOWN after Status.fromThrowable(throwable).
This way we would give end-users opportunity to still emit the whole exception message when they throw it as a StatusException.
Like in our test. We deliberately made a choice for that specific status:
.transform(p -> {
throw Status.UNAVAILABLE.withDescription("intentional")
.asRuntimeException();
}))
| GrpcInboundGateway gateway = this.applicationContext.getBean(GrpcInboundGateway.class); | ||
| DirectFieldAccessor accessor = new DirectFieldAccessor(gateway); | ||
| LogAccessor originalLog = (LogAccessor) accessor.getPropertyValue("logger"); | ||
| LogAccessor spyLog = spy(originalLog); |
There was a problem hiding this comment.
I want this test back, and a new one when we throw arbitrary exception and the test verify for the UNKNOWN and Internal Server Error.
I don't want any spies on logging: doesn't prove anything.
Update error handling to only add "Internal Service Error" description with an `UNKNOWN` status. - This ensures that specific gRPC status codes are preserved. - Use Status.fromThrowable to retain the original status code. - Add a test to verify unknown errors are masked correctly.
| private StatusRuntimeException toGrpcStatusException(Throwable throwable, @Nullable String description) { | ||
| Status status = Status.fromThrowable(throwable); | ||
| String statusDescription = (description != null) ? description : status.getDescription(); | ||
| if (status.getCode().equals(Status.UNKNOWN.getCode()) || statusDescription == null) { |
There was a problem hiding this comment.
I think we can use Status.Code.UNKNOWN instead of extra method calls.
| [[grpc-inbound-error-handling]] | ||
| === Error Handling | ||
|
|
||
| If an error occurs during downstream processing, the `GrpcInboundGateway` attempts to map the resulting `Throwable` to a gRPC `StatusRuntimeException`. |
There was a problem hiding this comment.
or StatusException?
According to the Status.fromThrowable() that does not mean that root exception has to be such.
The logic in the app may decide to throw just plain StatusException and then wrap it or so.
| rpc ErrorOnHello(HelloRequest) returns (HelloReply) {} | ||
|
|
||
| // Fail with unknown error | ||
| rpc UnknownErrorOnHello(HelloRequest) returns (HelloReply) {} |
There was a problem hiding this comment.
I wonder if we really need a separate gRPC contract for this our new logic.
Perhaps we can model a logic of existing transform() to emit different errors according to the inbound payload?
Like simple if..else if switch does not yet.
- Use StatusException in documentation instead of StatusRuntimeException
| throw Status.UNAVAILABLE.withDescription("intentional") | ||
| .asRuntimeException(); | ||
| } | ||
| throw Status.UNKNOWN.withDescription("") |
There was a problem hiding this comment.
Well, I think what we want here is something like throw new RuntimeException("some service error");
And in the end we ensure that this some does not appear in the client stack trace.
2 participants
Prevent internal exceptions from leaking in gRPC error responses.
Fixes: #11040