fix(oauth): app group links via internal route (empty groups + access-gate bypass)#66
Merged
Merged
Conversation
…ps + access-gate bypass) The oauth service fetched application group links from the public /applications/:id/groups endpoint, which requires a bearer it doesn't carry — so the call 401'd and getAppGroupLinks always returned nil. That silently (a) left the groups claim empty for every third-party client and (b) disabled CheckAccessGate (nil links -> no required groups -> open), letting users bypass required-group gating. - core: add internal GET /core/applications/client/:clientID/groups (unauthenticated, service-to-service) resolving links by client_id - oauth: call the internal route instead of the auth-gated public one; fix applicationGroupLink to parse the group id from json "id" (the response is GroupWithRequired, which has "id", not "group_id")
Thread errors through the group/identity lookups instead of collapsing them to nil/empty, so a failed core call can't silently bypass the access gate or mint a token with missing claims: - getAppGroupLinks / getEntityGroups / FilteredGroups / BuildTokenClaims / BuildIDTokenClaims now return errors - CheckAccessGate propagates fetch errors; handlers map ErrAccessDenied to 403 access_denied and any other error to 502 (deny, never 'open') - token/refresh/login handlers abort issuance on claim-build errors - sentinel client: 5s timeout + 2 retries on idempotent GETs only (POSTs like token mint / login record are never retried)
Apply the same resty client hardening as oauth to the core and discord service-to-service clients: 5s timeout, 2 retries on idempotent GETs only (POSTs are never retried to avoid double-applying non-idempotent writes).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The oauth service resolved application→group links by calling the public
/applications/:id/groups, which requires a bearer it doesn't carry → the call 401'd andgetAppGroupLinksreturned nil for every client. That one failure caused two security-relevant symptoms (surfaced wiring ArgoCD SSO):groupsclaim for all third-party clients → ArgoCD users all landed inrole:readonly.len(required)==0→ app treated as open → required-group gating did nothing.Plus a latent field bug: oauth parsed
json:"group_id", but the endpoint returnsGroupWithRequiredwith the id asjson:"id".Fix (part 1): read links via an internal route
GET /core/applications/client/:clientID/groups(unauthenticated, service-to-service, like the other/core/*routes), resolving links byclient_id.json:"id".Fix (part 2): fail closed on core errors
Even with part 1, a transient core failure still failed open (nil links → gate "open"). Now errors propagate instead of collapsing to nil/empty:
getAppGroupLinks/getEntityGroups/FilteredGroups/BuildTokenClaims/BuildIDTokenClaimsreturn errors.CheckAccessGatepropagates fetch errors; handlers mapErrAccessDenied→ 403 access_denied and any other error → 502 (deny, never "open")./core/entity/logins) stays best-effort — a logging hiccup shouldn't block a valid login.sentinelclient: 5s timeout + 2 retries, GET-only (non-idempotent POSTs like token mint / login record are never retried).Impact / deploy
Affects every third-party OAuth client since the group features shipped. Needs a Sentinel release + infra image bump to take effect.
Verify after deploy
access_deniedat authorize./oauth/userinfoand the id_token.