fix: address Copilot review on PR #6031#6080
Conversation
Two cleanups flagged on the merged security bundle PR. 1. exec.go race window — close the gap where /ws/exec could outlive logout. Previously the exec context was created and registered in the per-user registry only AFTER reading the init message, looking up the k8s client + REST config, and building the SPDY executor. If the user logged out during that ~hundreds-of-ms window, CancelUserExecSessions had nothing to cancel — the about-to-start exec session leaked past logout. Move execCtx + registerExecSession() to immediately after JWT validation, before any of the long-running setup. Registration now happens essentially synchronously with the first byte the handler receives, so a follow-up Logout always finds and cancels the in-flight session. 2. gitops_test.go — add /api/gitops/argocd/sync to gitopsMutationCases so TriggerArgoSync is exercised by the editor/admin/viewer RBAC matrix the same way the helm-* and sync handlers are. Adding the test surfaced a pre-existing ordering bug in TriggerArgoSync: the k8sClient nil check ran BEFORE body parsing and required-field validation, so a request with valid auth and a valid body shape would 503 instead of returning the 400 the matrix expects. Reordered to body-parse → required-fields → validateK8sName → k8sClient check, matching Sync / UpgradeHelmRelease conventions. Fixes #6075 Signed-off-by: Andrew Anderson <[email protected]>
|
[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 |
kubestellar-prow
Bot
added
the
dco-signoff: yes
✅ Deploy Preview for kubestellarconsole canceled.
|
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
Bot
added
the
size/M
There was a problem hiding this comment.
Pull request overview
Follow-up hardening for the previously-merged security bundle (#6031), addressing two Copilot-raised gaps: an exec-session cancellation race on logout and missing RBAC matrix coverage for ArgoCD sync.
Changes:
- Register
/ws/execsessions immediately after JWT validation so logout can reliably cancel in-flight exec setup. - Add RBAC test coverage for
POST /api/gitops/argocd/sync. - Reorder
TriggerArgoSyncvalidation to return consistent 400s for invalid bodies before failing on missing Kubernetes client config.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/api/handlers/exec.go | Moves exec context creation + session registry registration earlier to eliminate the logout race window. |
| pkg/api/handlers/gitops.go | Reorders TriggerArgoSync to parse/validate request fields before checking h.k8sClient availability. |
| pkg/api/handlers/gitops_test.go | Extends mutation RBAC test matrix with an argocd-sync case. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Summary
Two cleanups flagged by Copilot on the merged security bundle PR #6031.
1. exec.go race window
Where: `pkg/api/handlers/exec.go` (around the original line 393)
Problem: the exec context + registry registration happened only AFTER reading the init message, looking up the k8s client + REST config, and building the SPDY executor. If the user logged out during that ~hundreds-of-ms window, `CancelUserExecSessions` had nothing to cancel — the about-to-start exec session leaked past logout.
Fix: move `execCtx, execCancel := context.WithCancel(...)` and the `registerExecSession()` call to immediately after JWT validation, before any of the long-running setup. Registration now happens essentially synchronously with the first byte the handler receives, so a follow-up `Logout` always finds and cancels the in-flight session.
2. gitops_test.go missing argocd-sync coverage
Where: `pkg/api/handlers/gitops_test.go:148`
Problem: `gitopsMutationCases` covered `sync`, `helm-upgrade`, `helm-uninstall`, `helm-rollback` but NOT `/api/gitops/argocd/sync`, even though `TriggerArgoSync` was gated by `requireEditorOrAdmin` in PR #6031 (#6022).
Fix: added an `argocd-sync` entry. Adding the case surfaced a pre-existing ordering bug in `TriggerArgoSync`: the k8s client nil check ran BEFORE body parsing and required-field validation, so a request with valid auth + valid body shape would 503 instead of returning the 400 the matrix expects. Reordered `TriggerArgoSync` to: body-parse → required-fields → `validateK8sName` → `k8sClient` check — matching the `Sync` / `UpgradeHelmRelease` conventions.
Verification
```
go build ./...
go vet ./...
go test ./pkg/... -count=1 # all pass including the new argocd-sync RBAC matrix
```
Fixes #6075