fix: make content negotiation more resilient to bad input #645
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes made in this pull request focus on optimizing the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #645 +/- ##
==========================================
+ Coverage 92.70% 92.71% +0.01%
==========================================
Files 22 22
Lines 4879 4888 +9
==========================================
+ Hits 4523 4532 +9
Misses 308 308
Partials 48 48 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
negotiation/negotiation_test.go (1)
53-56: LGTM! The test cases effectively cover the reported panic scenario.The new test function properly validates the fix for issue #639 by testing the malformed input "a;," that previously caused a panic. The additional test case for ",a " is also valuable for ensuring robustness against other malformed inputs.
Consider adding more edge cases to strengthen the test coverage:
func TestMalformedFast(t *testing.T) { assert.Equal(t, "", SelectQValueFast("a;,", []string{"d", "e"})) assert.Equal(t, "a", SelectQValueFast(",a ", []string{"a", "b"})) + // Additional edge cases + assert.Equal(t, "", SelectQValueFast("a;;", []string{"a", "b"})) + assert.Equal(t, "", SelectQValueFast(";,", []string{"a", "b"})) + assert.Equal(t, "a", SelectQValueFast("a;q=invalid", []string{"a", "b"})) }negotiation/negotiation.go (4)
76-79: Consider adding a clarifying comment.While the logic is correct, the conditions for updating
endcould be more clearly documented to explain why we exclude comma and whitespace.if char != ',' && char != ' ' && char != '\t' { - // Update the end if it's not a comma or whitespace (i.e. end of string). + // Only update the end position for content characters, excluding separators + // and whitespace. This ensures clean boundaries for the parsed value and + // handles cases where the input ends with separators. end = pos }
86-89: Consider validating q-value range.While the parsing is more resilient, q-values should typically be between 0 and 1. Consider validating this range.
if len(header) > end+1 { - if parsed, _ := strconv.ParseFloat(header[start+2:end+1], 64); parsed > 0 { + if parsed, _ := strconv.ParseFloat(header[start+2:end+1], 64); parsed >= 0 && parsed <= 1 { q = parsed } }
118-122: Consider a minor performance optimization.The current whitespace handling is correct but does multiple position updates. Consider combining the conditions:
-if char != ' ' && char != '\t' { - // Only advance end if it's not whitespace. - end = pos - if header[start] == ' ' || header[start] == '\t' { - // Trim leading whitespace. - start = pos - } +if char != ' ' && char != '\t' { + end = pos + // Trim leading whitespace in one check + start = min(start, pos) }
Line range hint
61-122: Good architectural approach to error handling.The changes follow the robustness principle (Postel's Law) by being liberal in what is accepted while maintaining strict output requirements. The zero-allocation goal is preserved while making the code more resilient.
A few suggestions for future improvements:
- Consider adding logging for malformed inputs to help identify client issues
- Document the exact format requirements in package documentation
- Consider adding fuzzing tests to catch more edge cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
negotiation/negotiation.go(2 hunks)negotiation/negotiation_test.go(1 hunks)
🔇 Additional comments (5)
negotiation/negotiation.go (5)
62-62: Good improvement: Removed sentinel value.
Initializing start to 0 instead of -1 eliminates a potential source of index out of bounds errors while maintaining the same functionality.
69-70: Fixed: Proper handling of semicolon boundaries.
The updated positioning logic correctly handles malformed input like "a;," by ensuring proper start/end positions after encountering a semicolon.
92-93: Good: Proper position reset logic.
The position updates correctly handle the transition between segments, including empty ones.
105-105: Good: Proper state management.
The explicit name resets prevent state from leaking between iterations, making the parser more reliable.
Also applies to: 113-113
80-84: Consider adding input validation.
While the name parsing is more robust, consider validating the name content before using it. Empty or whitespace-only names could still be processed.
if name == "" {
// No name yet means we did not encounter a `;`. Either this is a `,`
// or the end of the string so whatever we have is the name.
// Example: "a, b, c"
- name = header[start : end+1]
+ name = strings.TrimSpace(header[start : end+1])
+ if name == "" {
+ continue // Skip empty segments
+ }
}
This PR fixes a bug that caused a panic in the content negotiation package due to bad input. The code has been modified to be more resilient to bad inputs and updated to not use the
-1sentinel value which could in some cases cause index out of bound errors. Existing tests all pass and the newly added bad inputs also work now, either returning no match or ignoring the bad part of the accept header.Performance is still good (about 304 -> 335 ns/op) and importantly fast matching remains zero allocation:
$ go test -bench=. ./negotiation goos: darwin goarch: arm64 pkg: github.com/danielgtaylor/huma/v2/negotiation cpu: Apple M3 Pro BenchmarkMatch-12 2725090 386.2 ns/op 320 B/op 8 allocs/op BenchmarkMatchFast-12 3572018 335.0 ns/op 0 B/op 0 allocs/op PASS ok github.com/danielgtaylor/huma/v2/negotiation 3.218sFixes #639.
Summary by CodeRabbit
New Features
Bug Fixes