fix: prevent body DoS and Prometheus label injection in telemetry handler#4
Conversation
- Limit incoming POST body to 10 MB via http.MaxBytesReader to prevent memory exhaustion from arbitrarily large telemetry payloads - Validate X-Cluster-ID against [a-zA-Z0-9._-] (max 253 chars) before use as a Prometheus label value; reject with 400 otherwise. Without this check a value containing '"' or newline corrupts the text-format output forwarded to VictoriaMetrics Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> -e Signed-off-by: tym83 <6355522@gmail.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 10MB limit on telemetry request bodies and adds validation for the X-Cluster-ID header to prevent injection. The review feedback identifies a bug where exceeding the body size limit results in redundant error responses and suggests handling *http.MaxBytesError specifically. Additionally, it recommends using a regular expression for header validation to improve code idiomaticity and maintainability.
| return | ||
| } | ||
|
|
||
| r.Body = http.MaxBytesReader(w, r.Body, maxTelemetryBodySize) |
There was a problem hiding this comment.
Using http.MaxBytesReader is correct for limiting the request body size. However, the error handling for io.ReadAll on the subsequent lines is now incomplete. When the size limit is exceeded, io.ReadAll returns a *http.MaxBytesError. The http.Server automatically sends a 413 response in this case. The current code at lines 144-147 will then attempt to write another error response, which will cause a superfluous response.WriteHeader error log on the server.
You should specifically check for *http.MaxBytesError and return without writing another response.
body, err := io.ReadAll(r.Body)
if err != nil {
var maxBytesErr *http.MaxBytesError
if errors.As(err, &maxBytesErr) {
// http.Server automatically sends a 413 response.
// We don't need to write another error, just log and return.
log.Printf("Request rejected: body exceeded %d bytes limit", maxBytesErr.Limit)
return
}
log.Printf("Error reading request body: %v", err)
http.Error(w, fmt.Sprintf("Error reading request: %v", err), http.StatusBadRequest)
return
}You will also need to import the errors package.
| func isValidClusterID(s string) bool { | ||
| if s == "" || len(s) > 253 { | ||
| return false | ||
| } | ||
| for _, c := range s { | ||
| if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || | ||
| (c >= '0' && c <= '9') || c == '-' || c == '_' || c == '.') { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
For validating string formats like this, using a compiled regular expression is a common and idiomatic approach in Go. It's more declarative and can be easier to read and maintain than manual character-by-character iteration.
Consider replacing this function with a regex-based implementation for better readability and maintainability:
// At package level, to compile once:
// var isValidClusterIDRegexp = regexp.MustCompile(`^[a-zA-Z0-9._-]{1,253}$`)
func isValidClusterID(s string) bool {
// Assumes isValidClusterIDRegexp is defined at package level
// and 'regexp' package is imported.
return isValidClusterIDRegexp.MatchString(s)
}This would make the validation logic more concise and align with common Go practices for this type of task. You would need to add an import for the regexp package.
When io.ReadAll returns *http.MaxBytesError, http.Server has already sent the 413 response. Writing another http.Error produces a "superfluous response.WriteHeader" log. Detect the specific error via errors.As and return without writing again. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
2 participants
Summary
Two security issues found during review of #3, both in the existing telemetry ingestion handler:
io.ReadAll(r.Body)had no size limit — any client could POST an arbitrarily large body and exhaust pod memory. Fixed withhttp.MaxBytesReadercapped at 10 MB.X-Cluster-IDheader value was used directly as a Prometheus label value without validation. A value containing",\n, or}would corrupt the text-format output forwarded to VictoriaMetrics (e.g.X-Cluster-ID: foo",injected="barwould produce broken metrics). Fixed by validating the header against[a-zA-Z0-9._-](max 253 chars, matching Kubernetes naming rules) and rejecting with 400 otherwise.Test plan
X-Cluster-ID: foo"barreturns 400X-Cluster-ID: valid-cluster.name_01is accepted as before🤖 Generated with Claude Code
Summary by CodeRabbit