Copilot commented on code in PR #3298:
URL: https://github.com/apache/dubbo-go/pull/3298#discussion_r3106167355
##########
protocol/jsonrpc/server.go:
##########
@@ -153,9 +153,10 @@ func (s *Server) handlePkg(conn net.Conn) {
if contentType != "application/json" && contentType !=
"application/json-rpc" {
setTimeout(conn, httpTimeout)
Review Comment:
`Content-Type` is compared using exact string equality. Requests like
`application/json; charset=utf-8` (a very common form) will be treated as
unsupported and return an error. Consider parsing/canonicalizing the media type
(e.g., via `mime.ParseMediaType` and comparing the base type) before this check.
##########
protocol/jsonrpc/server.go:
##########
@@ -153,9 +153,10 @@ func (s *Server) handlePkg(conn net.Conn) {
if contentType != "application/json" && contentType !=
"application/json-rpc" {
setTimeout(conn, httpTimeout)
r.Header.Set("Content-Type", "text/plain")
- if errRsp := sendErrorResp(r.Header,
[]byte(perrors.WithStack(err).Error())); errRsp != nil {
+ errMsg := "unsupported content type: " + contentType
+ if errRsp := sendErrorResp(r.Header, []byte(errMsg));
errRsp != nil {
logger.Warnf("sendErrorResp(header:%#v,
error:%v) = error:%s",
- r.Header, perrors.WithStack(err),
errRsp)
+ r.Header, errMsg, errRsp)
}
return
Review Comment:
There are existing jsonrpc tests in this package, but this fix is currently
untested. Please add a test that sends a non-JSON `Content-Type` request to the
jsonrpc server and asserts it returns an error response without panicking (and
ideally covers `application/json; charset=utf-8` if you adjust the parsing).
##########
protocol/jsonrpc/server.go:
##########
@@ -153,9 +153,10 @@ func (s *Server) handlePkg(conn net.Conn) {
if contentType != "application/json" && contentType !=
"application/json-rpc" {
setTimeout(conn, httpTimeout)
r.Header.Set("Content-Type", "text/plain")
- if errRsp := sendErrorResp(r.Header,
[]byte(perrors.WithStack(err).Error())); errRsp != nil {
+ errMsg := "unsupported content type: " + contentType
+ if errRsp := sendErrorResp(r.Header, []byte(errMsg));
errRsp != nil {
logger.Warnf("sendErrorResp(header:%#v,
error:%v) = error:%s",
Review Comment:
This branch reports an unsupported request media type but `sendErrorResp`
always uses HTTP 500. Consider returning `415 Unsupported Media Type` (or at
least a 4xx) to avoid misclassifying a client error as a server error; this
likely requires letting `sendErrorResp` accept a status code.
##########
protocol/jsonrpc/server.go:
##########
@@ -153,9 +153,10 @@ func (s *Server) handlePkg(conn net.Conn) {
if contentType != "application/json" && contentType !=
"application/json-rpc" {
setTimeout(conn, httpTimeout)
r.Header.Set("Content-Type", "text/plain")
Review Comment:
`r.Header.Set("Content-Type", "text/plain")` has no effect here because
`sendErrorResp` unconditionally deletes `Content-Type` (and `Content-Length`)
from the header map it reuses. If the intent is to return a `text/plain` error
body, set the response `Content-Type` inside `sendErrorResp` (after the `Del`
calls) or stop deleting it for this path.
```suggestion
```
--
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]
