chore: upgrade dependencies#10090
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates dependencies and introduces several enhancements to the go-openapi/runtime package, most notably connection-level tracing, TLS diagnostics, and a new BindForm utility with security caps. It also addresses security vulnerabilities related to header injection and TUF metadata validation. Feedback from the review points out a bug in the validation middleware where certain error types are incorrectly ignored, an efficiency issue involving redundant wrapping of the request body during form parsing, and a suggestion to avoid using panic for configuration errors in the router.
| for _, e := range result.Errors { | ||
| var validationErr *errors.Validation | ||
| if stderrors.As(e, &validationErr) { | ||
| v.result = append(v.result, validationErr) | ||
| } | ||
| v.result = append(v.result, result) | ||
| } |
There was a problem hiding this comment.
This loop filters out any error that is not of type *errors.Validation. However, the binder can return other critical error types, such as *errors.ParseError when a parameter is malformed (e.g., invalid integer format in a query string). By ignoring these errors, the validation middleware may incorrectly treat a request with invalid parameters as successful. All non-nil errors from the binder should be accumulated in v.result.
for _, e := range result.Errors {
if e != nil {
v.result = append(v.result, e)
}
}| if r.Body != nil && maxBody >= 0 { | ||
| if maxBody == 0 { | ||
| maxBody = DefaultMaxUploadBodySize | ||
| } | ||
| r.Body = http.MaxBytesReader(nil, r.Body, maxBody) | ||
| } |
There was a problem hiding this comment.
The request body is wrapped with http.MaxBytesReader every time parseFormBody is called. In the untyped binder path (e.g., in middleware/parameter.go), this function is invoked for every form parameter, leading to multiple redundant layers of wrapping. This is inefficient and can lead to deep call stacks during reading. Consider checking if the form has already been parsed before proceeding with the wrapping logic.
if r.PostForm != nil || r.MultipartForm != nil {
return nil
}
if r.Body != nil && maxBody >= 0 {
if maxBody == 0 {
maxBody = DefaultMaxUploadBodySize
}
r.Body = http.MaxBytesReader(nil, r.Body, maxBody)
}| if Debug { | ||
| d.debugLogf("there are no known routers") | ||
| } | ||
| panic("internal error: no router is configured") |
There was a problem hiding this comment.
Using panic in a library for a configuration issue is generally discouraged as it can lead to ungraceful application crashes in production. It would be safer to return an error or handle the missing router state through the existing return values of the Lookup method (e.g., returning nil, false).
| panic("internal error: no router is configured") | |
| return nil, false |
Upgrades dependencies after Go upgrade.