🐛 Harden card proxy: CIDR fatal, context propagation, AbortController#4237
🐛 Harden card proxy: CIDR fatal, context propagation, AbortController#4237clubanderson merged 1 commit intomainfrom
Conversation
Follow-up to #4230 addressing code review and silent-failure findings: Backend (card_proxy.go): - CIDR parse failure now log.Fatalf instead of silent skip - Use http.NewRequestWithContext for client disconnect cancellation - Detect 3xx redirects and return helpful error instead of opaque status - Add logging on all error paths + audit log on success - Log response size on oversized body rejection Frontend (useCardFetch.ts): - Add AbortController to cancel in-flight fetches on URL change/unmount - Guard localStorage.getItem against SecurityError in sandboxed iframes - Set loading=false when concurrency limit is hit - Wrap res.json() with helpful error for non-JSON 200 responses Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
/lgtm |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@clubanderson: you cannot LGTM your own PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clubanderson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubestellar-prow
bot
added
the
approved
There was a problem hiding this comment.
Pull request overview
Hardens the Tier 2 card data-fetching pathway (frontend hook + backend proxy) to reduce silent failures, improve cancellation behavior, and make upstream errors more actionable.
Changes:
- Backend card proxy now fails fast on blocked-CIDR parse errors, ties upstream requests to client context, and provides explicit redirect errors with improved logging/audit output.
- Frontend
useCardFetchnow cancels in-flight requests viaAbortController, safely reads auth token fromlocalStorage, and emits clearer errors for non-JSON success responses. - Fixes a stuck-loading edge case when per-card concurrency limits are hit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| web/src/lib/dynamic-cards/useCardFetch.ts | Adds safe token retrieval, request cancellation, and improved JSON parsing/error handling for the sandbox fetch hook. |
| pkg/api/handlers/card_proxy.go | Hardens SSRF boundary initialization, propagates request context, detects redirects explicitly, and improves logging/auditability. |
| }) | ||
| .catch(err => { | ||
| // Ignore abort errors — expected when URL changes or card unmounts | ||
| if (err instanceof DOMException && err.name === 'AbortError') return |
There was a problem hiding this comment.
Abort handling relies on err instanceof DOMException, which can miss AbortError cases where the rejection is an Error (e.g., older Safari / polyfills). Elsewhere in the codebase the convention is to check (err as { name?: string })?.name === 'AbortError' to handle both Error and DOMException (see web/src/hooks/mcp/events.ts:176-178). Consider switching to a name-based check here as well so aborted fetches don’t surface as user-visible errors.
| if (err instanceof DOMException && err.name === 'AbortError') return | |
| const isAbortError = (err as { name?: string } | null)?.name === 'AbortError' | |
| if (isAbortError) return |
| return res.json().catch(() => { | ||
| throw new Error( | ||
| 'Response is not valid JSON. The external API may be returning HTML, XML, or plain text.', | ||
| ) | ||
| }) |
There was a problem hiding this comment.
On the success path, res.json() will also fail for legitimate empty responses (e.g., HTTP 204 No Content, or 200 with an empty body), and this currently gets turned into the generic “not valid JSON” error. Consider special-casing res.status === 204 and/or an empty body to return null (or {}) without error, so APIs that legitimately return no content don’t break.
| /** Safely read from localStorage — returns null if unavailable (sandboxed iframes, etc.) */ | ||
| function safeGetToken(): string | null { | ||
| try { | ||
| return localStorage.getItem(STORAGE_KEY_TOKEN) | ||
| } catch { | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
The new behaviors (safe localStorage access, AbortController cancellation, and non-JSON response messaging) don’t appear to have unit test coverage. Since this package already has Vitest coverage for other sandbox scopes (e.g., web/src/lib/dynamic-cards/tests/scope.test.ts), consider adding tests for: (1) aborts being ignored, (2) localStorage SecurityError returning null token, and (3) empty/204 or non-JSON 200 handling.
| }) | ||
| } | ||
|
|
||
| // Log successful proxy requests for audit trail |
There was a problem hiding this comment.
The comment says “Log successful proxy requests for audit trail”, but this log line will run for any non-redirect response including 4xx/5xx from the upstream. Either adjust the comment to reflect that it logs all proxied responses, or gate the log to 2xx if “successful” is meant literally.
| // Log successful proxy requests for audit trail | |
| // Log proxied responses for audit trail |
| // Detect redirects and return a helpful error instead of an opaque 3xx | ||
| if resp.StatusCode >= 300 && resp.StatusCode < 400 { | ||
| location := resp.Header.Get("Location") | ||
| log.Printf("[CardProxy] Redirect from %s (status %d, location=%s)", host, resp.StatusCode, location) |
There was a problem hiding this comment.
location comes from an upstream header and can be very long or contain unexpected characters; logging it unquoted with %s can produce noisy/misleading logs. Consider logging it with %q and optionally truncating to a reasonable length to keep logs safe and readable.
| log.Printf("[CardProxy] Redirect from %s (status %d, location=%s)", host, resp.StatusCode, location) | |
| safeLocation := location | |
| const maxLocationLogLen = 200 | |
| if len(safeLocation) > maxLocationLogLen { | |
| safeLocation = safeLocation[:maxLocationLogLen] + "...(truncated)" | |
| } | |
| log.Printf("[CardProxy] Redirect from %s (status %d, location=%q)", host, resp.StatusCode, safeLocation) |
| // Detect redirects and return a helpful error instead of an opaque 3xx | ||
| if resp.StatusCode >= 300 && resp.StatusCode < 400 { | ||
| location := resp.Header.Get("Location") | ||
| log.Printf("[CardProxy] Redirect from %s (status %d, location=%s)", host, resp.StatusCode, location) | ||
| return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{ | ||
| "error": fmt.Sprintf("External API returned a redirect (%d). Update the URL to the final destination.", resp.StatusCode), | ||
| }) | ||
| } |
There was a problem hiding this comment.
The redirect-detection behavior is new and security-relevant (ensures callers don’t silently accept 3xx). There’s currently only unit coverage for isBlockedIP; consider adding handler-level tests that assert 3xx responses return the expected JSON error and status, using the existing Fiber test patterns in pkg/api/handlers/*_test.go.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 3 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. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
All suggestions applied in commit Code suggestions:
General comments addressed:
|
Uh oh!
There was an error while loading. Please reload this page.