Skip to content

chore: upgrade dependencies#10090

Closed
ChrisGe4 wants to merge 1 commit into
GoogleContainerTools:mainfrom
ChrisGe4:dependency-upgrade
Closed

chore: upgrade dependencies#10090
ChrisGe4 wants to merge 1 commit into
GoogleContainerTools:mainfrom
ChrisGe4:dependency-upgrade

Conversation

@ChrisGe4
Copy link
Copy Markdown
Contributor

Upgrades dependencies after Go upgrade.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +82 to 87
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)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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)
		}
	}

Comment on lines +255 to +260
if r.Body != nil && maxBody >= 0 {
if maxBody == 0 {
maxBody = DefaultMaxUploadBodySize
}
r.Body = http.MaxBytesReader(nil, r.Body, maxBody)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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).

Suggested change
panic("internal error: no router is configured")
return nil, false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant