🐛 Fix 3 security issues: scale auth, SSE cache namespace, JSON validation#4157
🐛 Fix 3 security issues: scale auth, SSE cache namespace, JSON validation#4157clubanderson merged 1 commit intomainfrom
Conversation
…tion (#4150, #4151, #4156) - #4150: Add validateToken check to /scale endpoint and reject GET mutations (POST-only) to prevent unauthenticated CSRF-style scaling attacks - #4151: Include namespace in SSE cache key to prevent cross-namespace data leakage when the same cluster is queried for different namespaces - #4156: Validate JSON decode errors on /auto-update/trigger and /predictions/analyze instead of silently accepting malformed bodies Signed-off-by: Andrew Anderson <andy@clubanderson.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole canceled.
|
There was a problem hiding this comment.
Pull request overview
Addresses three reported security vulnerabilities by tightening agent auth/method handling for scaling, preventing cross-namespace SSE cache collisions, and rejecting malformed JSON bodies that were previously silently accepted.
Changes:
- Require auth + POST-only semantics for the agent
/scalehandler; update auth test categorization. - Include
namespacein SSE cache keys to avoid cross-namespace stale/cross-leaked results. - Add explicit JSON decode error handling to
/auto-update/triggerand/predictions/analyze.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/api/handlers/sse.go | Adds namespace to SSE stream config and incorporates it into the cache key. |
| pkg/agent/server.go | Enforces auth + POST-only for scaling; rejects malformed JSON for auto-update trigger and predictions analyze. |
| pkg/agent/endpoint_auth_test.go | Moves /scale into the “sensitive endpoints” list and clears the “lacking auth” list. |
| if r.Body != nil && r.ContentLength != 0 { | ||
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| json.NewEncoder(w).Encode(map[string]string{"error": "invalid JSON body"}) | ||
| return |
There was a problem hiding this comment.
r.ContentLength != 0 will skip validation when ContentLength == -1 (unknown length, e.g. chunked transfer). That reintroduces the “malformed JSON silently accepted” behavior for some requests. Prefer attempting a decode whenever r.Body is present (or r.Body != http.NoBody) and treating io.EOF as “empty body” rather than gating on ContentLength.
| if r.Body != nil && r.ContentLength != 0 { | |
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { | |
| w.WriteHeader(http.StatusBadRequest) | |
| json.NewEncoder(w).Encode(map[string]string{"error": "invalid JSON body"}) | |
| return | |
| if r.Body != nil && r.Body != http.NoBody { | |
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { | |
| if err != io.EOF { | |
| w.WriteHeader(http.StatusBadRequest) | |
| json.NewEncoder(w).Encode(map[string]string{"error": "invalid JSON body"}) | |
| return | |
| } |
| if r.Body != nil && r.ContentLength != 0 { | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| http.Error(w, "invalid JSON body", http.StatusBadRequest) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue as above: the ContentLength != 0 guard will skip decoding when ContentLength == -1, allowing malformed JSON to bypass the new check. Decode when the body is present and treat io.EOF as an empty/absent body.
| json.NewDecoder(r.Body).Decode(&req) | ||
| if r.Body != nil && r.ContentLength != 0 { | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| http.Error(w, "invalid JSON body", http.StatusBadRequest) |
There was a problem hiding this comment.
http.Error returns a plain-text response (and sets Content-Type: text/plain), which can be inconsistent with the rest of this handler/endpoint family that appears to return JSON objects for errors. Consider returning a JSON error payload here (and setting status) for consistency with other agent endpoints.
| if r.Method != "POST" { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| json.NewEncoder(w).Encode(map[string]interface{}{ | ||
| "success": false, | ||
| "error": "POST required", | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
For 405 Method Not Allowed, it’s best practice to include an Allow header advertising the permitted method(s) (e.g., POST). This improves client behavior and aligns with HTTP semantics.
| // namespace is the optional namespace filter. When set it is included in | ||
| // the cache key so that requests for different namespaces on the same | ||
| // cluster do not return stale cross-namespace data (#4151). |
There was a problem hiding this comment.
The comment says “When set it is included in the cache key”, but the implementation always appends cfg.namespace (even when empty). Either adjust the comment to reflect that it’s always included, or conditionally include the namespace segment only when it’s non-empty.
| // namespace is the optional namespace filter. When set it is included in | |
| // the cache key so that requests for different namespaces on the same | |
| // cluster do not return stale cross-namespace data (#4151). | |
| // namespace is the optional namespace filter. Its value (including empty) | |
| // is included in the cache key so that requests for different namespaces on | |
| // the same cluster do not return stale cross-namespace data (#4151). |
- server.go: Remove ContentLength!=0 guard — use Body!=nil with io.EOF tolerance for empty bodies. JSON error response instead of plain text on /predictions/analyze (#4157) - sse.go: Fix comment to reflect namespace is always in cache key (#4157) - gateway.go: Use errors.Join for unwrappable error chains (#4159) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
- server.go: Remove ContentLength!=0 guard — use Body!=nil with io.EOF tolerance for empty bodies. JSON error response instead of plain text on /predictions/analyze (#4157) - sse.go: Fix comment to reflect namespace is always in cache key (#4157) - gateway.go: Use errors.Join for unwrappable error chains (#4159) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 2 code suggestion(s) and 3 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
Fixes three security vulnerabilities reported in #4150, #4151, and #4156:
Unauthenticated scale endpoint accepts GET mutations #4150 — Unauthenticated scale endpoint: Added
validateTokencheck to/scaleand removed GET-based mutations (POST-only now). Previously, the scale endpoint accepted unauthenticated requests and allowed mutations via GET query parameters, enabling CSRF-style attacks. Moved/scalefromendpointsLackingAuthtosensitiveEndpointsin the auth test suite.SSE cache key ignores namespace filter, causes cross-namespace data leakage #4151 — SSE cache key ignores namespace: Added
namespacefield tosseClusterStreamConfigand included it in the cache key (demoKey:cluster:namespace). Previously, cache keys were onlydemoKey:cluster, so requests for different namespaces on the same cluster could return stale data from a previous namespace query.Malformed JSON bodies are silently accepted on mutation/trigger paths #4156 — Malformed JSON silently accepted: Added JSON decode error checking on
/auto-update/triggerand/predictions/analyze. Previously, decode errors were discarded with_, causing malformed JSON payloads to produce zero-value structs and proceed with operations instead of returning 400.Test plan
TestEndpointAuth_*tests pass —/scalenow correctly rejects unauthenticated/bad-token requestsTestScaleWorkloadhandler test passesTestPredictionWorkerandTestPredictionMetricspasspkg/api/handlerstests pass (SSE cache key change is backward-compatible)go build ./pkg/...succeeds with no compilation errorsCloses #4150, closes #4151, closes #4156