feature: Oauth authentication#41
Conversation
There was a problem hiding this comment.
via Claude:
Review
The architecture is solid: PKCE with server-side token exchange keeps the client secret off the frontend, the popup flow avoids full-page redirects in the console plugin context, and the component decomposition (PatModal, DisconnectConfirmModal) is a clear improvement. Good test coverage at 35 new tests.
Issues
1. Security: client_id exposed when OAuth is disabled
handleOAuthConfig returns client_id even when enabled is false. If a cluster has GITHUB_CLIENT_ID set but not GITHUB_CLIENT_SECRET, the client ID leaks to unauthenticated callers. Consider returning an empty client_id when enabled is false:
func handleOAuthConfig(w http.ResponseWriter, _ *http.Request) {
enabled := githubClientID != "" && githubClientSecret != ""
cid := ""
if enabled {
cid = githubClientID
}
// ...
}2. Security: GitHub token response forwarded verbatim to the client
handleOAuthCallback does w.Write(body), forwarding the entire GitHub response body. GitHub's token response may include fields beyond access_token (e.g., scope, token_type, and potentially future fields). It would be safer to extract only access_token and return that:
token, _ := ghResp["access_token"].(string)
if token == "" {
jsonError(w, "no access_token in GitHub response", http.StatusBadGateway)
return
}
json.NewEncoder(w).Encode(map[string]string{"access_token": token})3. Bug: PAT_KEY alias creates a migration trap
In types.ts:
export const TOKEN_KEY = 'func-console-token';
export const PAT_KEY = TOKEN_KEY;The storage key changed from 'func-console-pat' to 'func-console-token', but there is no migration. Any user who authenticated before this PR will have their session silently dropped (their func-console-pat entry is orphaned). If that is intentional since this is a PoC, the PAT_KEY alias is misleading and should just be removed. If it is not intentional, a migration read of the old key would be needed.
4. Bug: ForgeConnectionProvider not notified on disconnect
disconnect() in useUserAvatar clears sessionStorage and local state, but never calls connectToForge (or an equivalent disconnect callback) on ForgeConnectionContext. This means ForgeConnectionContext.isActive remains true and connectionId is stale after disconnect, so downstream consumers (e.g., data-fetching hooks) won't know the user logged out until a full page refresh.
5. Singleton OAuthService caches config forever
OAuthService.#configCache is set once and never invalidated. If a cluster admin enables OAuth after the plugin is loaded, the user must do a full page refresh. This might be acceptable for a PoC, but worth noting.
6. Missing redirect_uri in token exchange
The handleOAuthCallback handler sends client_id, client_secret, code, and code_verifier to GitHub's token endpoint, but omits redirect_uri. GitHub's OAuth docs state that redirect_uri is required if it was included in the authorization request (and the frontend does include it). This may cause token exchange failures in some configurations. Worth verifying empirically, since GitHub may not strictly enforce it in all cases.
7. Missing error status code check for GitHub response
In handleOAuthCallback, the code reads the body and checks for a JSON error field, but never checks resp.StatusCode. If GitHub returns a non-200 with a non-JSON body, the json.Unmarshal will fail with a generic "invalid response from GitHub" instead of surfacing the actual HTTP status.
Nits
- Inline style in PatModal:
style={{ marginBottom: '1rem' }}on the Alert. Should use a PF utility class likepf-v6-u-mb-mdfor consistency with the rest of the PR (which already uses utility classes likepf-v6-u-my-md). onOpenChangeparameter naming:onOpenChange={(setOpen) => !setOpen && closeDropdown()}-- the parameter namesetOpenis misleading since it is actuallyisOpen. Consider(open) => !open && closeDropdown().- Unused
AuthMethodtype:export type AuthMethod = 'pat' | 'oauth'is exported but never imported anywhere. The literal'pat' | 'oauth'is used inline infinishLogin. - No tests for
OAuthServiceorOAuthCallbackPage: The core PKCE/popup logic and the callback page rendering/postMessage behavior are untested.
Summary
The main items to address before merge are #3 (silent session drop / dead alias) and #4 (disconnect not propagated to context). Items #1, #2, and #6 are security/correctness fixes that are straightforward. The rest are minor.
|
Thanks for the thorough review! Pushed fixes in f86e356. Here's the rundown: Issues
Nits
|
| } | ||
| } | ||
|
|
||
| var ( |
There was a problem hiding this comment.
Put this new functionality into a new file e.g. oauth_handler.go
Summary
Screen.Recording.2026-06-15.at.13.32.53.mov
Changes
Backend (
backend/main.go)GET /api/oauth/config: returns{ client_id, enabled }from env varsPOST /api/oauth/callback: proxies authorization code + PKCE verifier to GitHub's token endpoint, returns access tokenFrontend
src/common/services/oauth/): popup-based PKCE flow with state/CSRF validation, popup-close detection, and sessionStorage cleanupsrc/pages/oauth-callback/): minimal popup callback page thatpostMessages the authorization code back to the opener windowsrc/common/components/PatModal.tsx): extracted from UserAvatar, OAuth button conditionally enabled based on backend config, "or" divider, PAT inputsrc/common/components/DisconnectConfirmModal.tsx): extracted confirmation dialog for disconnect actionsrc/common/components/UserAvatar.tsx): logged-in state shows dropdown with disconnect option, not-logged-in state shows connect button + PatModalTOKEN_KEYreplaces hardcoded'func-console-pat'across codebase,AUTH_METHOD_KEYtracks'pat' | 'oauth'Helm chart
values.yaml:plugin.oauth.enabled,githubClientId,githubSecretName,githubSecretKeydeployment.yaml: conditionally injectsGITHUB_CLIENT_IDandGITHUB_CLIENT_SECRET(from K8s Secret) as env varsTest plan
Relates to SRVOCF-856