Skip to content

Fix unauthorized error handling, panic recovery, and concurrent JWT cache#1067

Draft
oliverbaehler with Copilot wants to merge 3 commits into
mainfrom
copilot/fix-comments-in-review-thread
Draft

Fix unauthorized error handling, panic recovery, and concurrent JWT cache#1067
oliverbaehler with Copilot wants to merge 3 commits into
mainfrom
copilot/fix-comments-in-review-thread

Conversation

Copilot AI commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Addresses review feedback on PR #1066: several error paths returned 500 for auth failures, the JWT token cache had data races, and the panic recovery middleware swallowed http.ErrAbortHandler and returned plain-text instead of JSON.

Error handling: distinguish unauthorized from internal errors

ResolveUserAndGroups returns *req.ErrUnauthorized for missing/invalid auth — previously all failures were routed through HandleError (500). Now all call sites check error type and call HandleUnauthorized (403) or HandleError (500) accordingly, matching the pattern already used in impersonateHandler.

Affected: authorizationMiddleware, registerModules (webserver.go), CheckUserInIgnoredGroupMiddleware, CheckUserInCapsuleGroupMiddleware (user_in_group.go).

JWT middleware: fix concurrent map writes

invalidatedToken is a sets.Set[string] (backed by a map) captured in a closure and accessed across concurrent requests. Added sync.RWMutex to protect all reads and writes.

var mu sync.RWMutex
invalidatedToken := sets.New[string]()

// read path
mu.RLock()
tokenInvalidated := invalidatedToken.Has(token)
mu.RUnlock()

// write path
mu.Lock()
invalidatedToken.Insert(token)
mu.Unlock()

Panic recovery: correct http.ErrAbortHandler semantics + JSON response

The custom recoveryMiddleware (replacing gorilla/handlers.RecoveryHandler) now:

  • Re-panics http.ErrAbortHandler so net/http can handle connection abort correctly
  • Returns a Kubernetes-style JSON metav1.Status via server.HandleError instead of plain-text http.Error

errors/panic.go: remove panics, add proper status codes

HandleError and HandleUnauthorized previously ended with panic(), relying on a recovery handler to terminate the request. They now write the response (with correct HTTP status codes and headers) and return normally. Call sites were updated to add explicit return statements.

Other

  • Removed gorilla/handlers dependency (no longer needed)
  • Added tokenreviews RBAC rule to the FlowSchema chart template (required by JWT middleware)

Copilot AI changed the title [WIP] Fix code based on review comments Fix unauthorized error handling, panic recovery, and concurrent JWT cache Jul 2, 2026
Copilot AI requested a review from oliverbaehler July 2, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants