BitoAgent commented on code in PR #13786:
URL: https://github.com/apache/dubbo/pull/13786#discussion_r1573855706
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcRequestHandlerMapping.java:
##########
@@ -42,9 +43,16 @@ protected boolean supportContentType(String contentType) {
@Override
protected void determineHttpMessageCodec(RequestHandler handler, URL url,
HttpRequest request) {
- HttpMessageCodec codec = CODEC_FACTORY.createCodec(url,
getFrameworkModel(), request.contentType());
- handler.setHttpMessageDecoder(codec);
- handler.setHttpMessageEncoder(codec);
+ GrpcCompositeCodec grpcCompositeCodec =
+ (GrpcCompositeCodec) CODEC_FACTORY.createCodec(url,
getFrameworkModel(), request.contentType());
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findMethodDescriptor(
+ handler.getServiceDescriptor(), handler.getMethodName(),
handler.isHasStub());
+ if (methodDescriptor != null) {
+ handler.setMethodDescriptor(methodDescriptor);
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
+ }
+ handler.setHttpMessageDecoder(grpcCompositeCodec);
+ handler.setHttpMessageEncoder(grpcCompositeCodec);
Review Comment:
**Optimization Issue**: The process of determining the HttpMessageCodec has
been refactored to directly use GrpcCompositeCodec and attempt to find and load
a MethodDescriptor. This approach assumes that the MethodDescriptor can always
be found and correctly loaded, which might not always be the case. This could
lead to scenarios where the codec is not correctly configured for the request,
impacting performance and correctness. <br> **Fix**: Add error handling and
checks around the MethodDescriptor finding and loading process. Ensure that
there is a fallback or error reporting mechanism in place if the
MethodDescriptor cannot be found or if the loading process fails, to maintain
the robustness and performance of the request handling. <br> **Code
Suggestion**:
```
+ try {
+ GrpcCompositeCodec grpcCompositeCodec =
+ (GrpcCompositeCodec) CODEC_FACTORY.createCodec(url,
getFrameworkModel(), request.contentType());
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findMethodDescriptor(
+ handler.getServiceDescriptor(),
handler.getMethodName(), handler.isHasStub());
+ if (methodDescriptor != null) {
+ handler.setMethodDescriptor(methodDescriptor);
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
+ } else {
+ throw new
MethodDescriptorNotFoundException("MethodDescriptor not found for methodName: "
+ handler.getMethodName());
+ }
+ } catch (MethodDescriptorNotFoundException e) {
+ logger.error(e.getMessage());
+ // Handle error or fallback
+ }
```
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcRequestHandlerMapping.java:
##########
@@ -42,9 +43,16 @@ protected boolean supportContentType(String contentType) {
@Override
protected void determineHttpMessageCodec(RequestHandler handler, URL url,
HttpRequest request) {
- HttpMessageCodec codec = CODEC_FACTORY.createCodec(url,
getFrameworkModel(), request.contentType());
- handler.setHttpMessageDecoder(codec);
- handler.setHttpMessageEncoder(codec);
+ GrpcCompositeCodec grpcCompositeCodec =
+ (GrpcCompositeCodec) CODEC_FACTORY.createCodec(url,
getFrameworkModel(), request.contentType());
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findMethodDescriptor(
+ handler.getServiceDescriptor(), handler.getMethodName(),
handler.isHasStub());
+ if (methodDescriptor != null) {
+ handler.setMethodDescriptor(methodDescriptor);
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
+ }
+ handler.setHttpMessageDecoder(grpcCompositeCodec);
+ handler.setHttpMessageEncoder(grpcCompositeCodec);
Review Comment:
**Security Issue**: The use of findMethodDescriptor without validating the
method's name, service descriptor, or the stub flag may lead to the execution
of unexpected methods if the input parameters can be influenced externally.
<br> **Fix**: Validate the method name, service descriptor, and stub flag
before using them to find and set the MethodDescriptor. <br> **Code
Suggestion**:
```
+ if (DescriptorUtils.validateMethodName(handler.getMethodName()) &&
+
DescriptorUtils.validateServiceDescriptor(handler.getServiceDescriptor()) &&
+ handler.isHasStub()) {
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findMethodDescriptor(
+ handler.getServiceDescriptor(), handler.getMethodName(),
handler.isHasStub());
+ if (methodDescriptor != null) {
+ handler.setMethodDescriptor(methodDescriptor);
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
+ }
+ }
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
