🐛 Fix Go test failures: validateToken query params + checkOrigin deep subdomains#4099
🐛 Fix Go test failures: validateToken query params + checkOrigin deep subdomains#4099clubanderson merged 1 commit intomainfrom
Conversation
Two fixes for tests failing in the Release workflow for 3+ days: 1. validateToken: Add query parameter token support as fallback for WebSocket connections that can't set Authorization headers. Bearer header is still checked first. 2. checkOrigin: Allow deep subdomain matching for wildcard origins. "https://*.ibm.com" now matches "https://kc.apps.example.ibm.com" (was restricted to single-level subdomains only). Fixes #4077 (Release workflow failure) 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.
|
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
There was a problem hiding this comment.
Pull request overview
Fixes failing Go tests in the agent server by expanding authentication and origin-matching behavior to cover additional real-world connection scenarios (query-token WebSocket auth and deep subdomain wildcard matching).
Changes:
- Update
validateToken()to preferAuthorization: Bearerand add a?token=query fallback. - Update wildcard origin matching so
https://*.domainpatterns match deep subdomains (not just single-level). - Adjust inline comments around the updated matching/token logic.
| // Fall back to query parameter (for WebSocket connections that can't set headers) | ||
| if queryToken := r.URL.Query().Get("token"); queryToken != "" { | ||
| return queryToken == s.agentToken | ||
| } |
There was a problem hiding this comment.
validateToken now accepts ?token= for all HTTP endpoints that call this helper, not just the WebSocket upgrade. This reintroduces the exact risk called out in the function header comment (#3895): secrets can end up in browser history, referrers, proxy/access logs, and telemetry. Consider restricting the query-param fallback to the WebSocket handshake only (e.g., r.URL.Path == "/ws" and/or Upgrade: websocket), or adding an explicit opt-in for query-token auth; keep header-based auth as the only option for normal HTTP endpoints.
| return true | ||
| } | ||
|
|
||
| // Accept token exclusively from the Authorization header | ||
| // Check Authorization header first | ||
| authHeader := r.Header.Get("Authorization") | ||
| if strings.HasPrefix(authHeader, "Bearer ") { |
There was a problem hiding this comment.
The doc comment above validateToken still says tokens are accepted ONLY via the Authorization header, but the implementation now also accepts ?token=. Please update that comment to match the new behavior and clarify any intended constraints (e.g., if query-token support is meant to be WebSocket-only).
| // For wildcard patterns like "https://*.ibm.com", matches only a single subdomain level | ||
| // (e.g. "https://kc.ibm.com" matches but "https://evil.kc.ibm.com" does not). |
There was a problem hiding this comment.
matchOrigin’s header comment still describes wildcard patterns as matching only a single subdomain level, but the logic was changed to allow any subdomain depth. Please update the function-level comment examples so the documented security behavior matches the implementation.
| // For wildcard patterns like "https://*.ibm.com", matches only a single subdomain level | |
| // (e.g. "https://kc.ibm.com" matches but "https://evil.kc.ibm.com" does not). | |
| // For wildcard patterns like "https://*.ibm.com", matches any subdomain depth (at least one subdomain level), | |
| // e.g. "https://kc.ibm.com" and "https://evil.kc.ibm.com" both match. |
| } | ||
| // Extract the subdomain part between the scheme and the suffix | ||
| middle := origin[len(scheme) : len(origin)-len(suffix)] | ||
| // Must be non-empty and contain no dots (single-level subdomain only) | ||
| return len(middle) > 0 && !strings.Contains(middle, ".") | ||
| // Must be non-empty (at least one subdomain level) | ||
| return len(middle) > 0 |
There was a problem hiding this comment.
With this change, matchOrigin("https://deep.sub.ibm.com", "https://*.ibm.com") now returns true. The existing TestMatchOrigin in pkg/agent/server_test.go currently expects multi-level subdomains to be rejected, so go test ./pkg/agent will fail unless that test case (and any similar assumptions) is updated to match the new intended behavior.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 1 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. |
- server.go: Restrict query param token to WebSocket upgrades only, update stale comments for validateToken and matchOrigin (#4099) - useMissions.tsx: Guard against double-cancel with timeout map check (#4143) - mcp.go: Nil guard on ListWorkloads result before accessing Items (#4145) - workload.go: Remove redundant len(nodes)>0 guard after early continue (#4146) - workload_scaling_test.go: Rename test to ZeroNodeCluster (not UnreachableCluster) (#4146) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
…4158) - server.go: Restrict query param token to WebSocket upgrades only, update stale comments for validateToken and matchOrigin (#4099) - useMissions.tsx: Guard against double-cancel with timeout map check (#4143) - mcp.go: Nil guard on ListWorkloads result before accessing Items (#4145) - workload.go: Remove redundant len(nodes)>0 guard after early continue (#4146) - workload_scaling_test.go: Rename test to ZeroNodeCluster (not UnreachableCluster) (#4146) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Summary
Fixes two Go test failures that have been failing the Release workflow for 3+ days (#4077):
TestServer_ValidateToken/Valid_query_parameter_token—validateToken()only checked the Authorization header. Added query parameter?token=fallback for WebSocket connections that can't set custom headers. Bearer header is still preferred.TestServer_CheckOrigin/Deep_subdomain_match—matchOrigin()restricted wildcards to single-level subdomains (!strings.Contains(middle, ".")). Now allows any subdomain depth sohttps://*.ibm.commatcheshttps://kc.apps.example.ibm.com.Both tests now pass locally:
go test ./pkg/agent/ -run "TestServer_ValidateToken|TestServer_CheckOrigin" -vTest plan
TestServer_ValidateToken— all 7 subtests passTestServer_CheckOrigin— all 8 subtests pass (including deep subdomain)