Skip to content

fix(server): enforce client.AllowedConnectors in handleTokenExchange (GHSA-7qjx-gp9h-65qj)#4784

Merged
nabokihms merged 1 commit into
dexidp:masterfrom
matte1782:fix/tokenexchange-allowedconnectors-gate
May 11, 2026
Merged

fix(server): enforce client.AllowedConnectors in handleTokenExchange (GHSA-7qjx-gp9h-65qj)#4784
nabokihms merged 1 commit into
dexidp:masterfrom
matte1782:fix/tokenexchange-allowedconnectors-gate

Conversation

@matte1782
Copy link
Copy Markdown
Contributor

Overview

Adds enforcement of client.AllowedConnectors in handleTokenExchange. Two sibling code paths already enforce this client policy; the token exchange path was missing it.

Addresses GHSA-7qjx-gp9h-65qj. Only master is affected.

What this PR does / why we need it

The asymmetry before this PR:

  • server/handlers.go::handleConnectorLogin line 377 calls isConnectorAllowed(client.AllowedConnectors, connID) before getConnector and returns 403 on deny.
  • server/oauth2.go::parseAuthorizationRequest line 535 calls the same helper and returns a redirected errInvalidRequest on deny.
  • server/handlers.go::handleTokenExchange did not call it. A confidential client with AllowedConnectors: ["okta"] could exchange a subject token via connector_id=ldap provided that LDAP connector existed in the dex configuration and supported the token-exchange grant type.

The change inserts one call to isConnectorAllowed in handleTokenExchange between the existing subjectToken empty check and getConnector. Placement mirrors handleConnectorLogin:377 and oauth2.go:535 (check runs before the connector fetch) so a disallowed request cannot probe connector existence. Error shape mirrors sibling token-endpoint errors in the same function: tokenErrHelper(errInvalidRequest, "Connector not allowed for this client.", http.StatusBadRequest). Log message and field shape mirror handleConnectorLogin:378-379 exactly.

Tests added under TestHandleTokenExchangeAllowedConnectors cover four cases: allowed (single entry), allowed (non-first entry in a multi-entry list), denied, and empty list (existing permit-any semantic). Structure mirrors TestHandleTokenExchangeConnectorGrantTypeRestriction. go test ./server/... passes for the diff. One unrelated pre-existing flake in TestOAuth2DeviceFlow/verify_id_token_and_oauth2_token_expiry#01 reproduces on stock master at the same commit (token-expiry timing assertion drifting under containerized scheduling) and is not introduced by this PR.

Special notes for your reviewer

The token-exchange grant did not check client.AllowedConnectors before
invoking the connector, while handleConnectorLogin (handlers.go:377) and
parseAuthorizationRequest (oauth2.go:535) both call isConnectorAllowed.
This left a confidential client able to exchange a subject token via a
connector not in its AllowedConnectors list, provided the connector
existed in the dex configuration and supported the token-exchange grant.

Insert isConnectorAllowed before getConnector in handleTokenExchange so
a disallowed request cannot probe connector existence. Mirrors the
placement and error shape of the two existing call sites.

Tests: TestHandleTokenExchangeAllowedConnectors covers allowed,
allowed-non-first-entry, denied, and empty (permit-any) cases.

Addresses GHSA-7qjx-gp9h-65qj.

Signed-off-by: Matteo Panzeri <matteo1782@gmail.com>
@nabokihms nabokihms added the release-note/bug-fix Release note: Bug Fixes label May 11, 2026
@nabokihms nabokihms self-requested a review May 11, 2026 05:43
Copy link
Copy Markdown
Contributor

@cardoe cardoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. +1

Copy link
Copy Markdown
Member

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch!

@nabokihms nabokihms merged commit f0cd676 into dexidp:master May 11, 2026
10 checks passed
kanywst added a commit to kanywst/dex that referenced this pull request May 13, 2026
Resolve a conflict in server/handlers.go between dexidp#4784, which enforces
client.AllowedConnectors in handleTokenExchange, and the ID-JAG token
exchange branch. The AllowedConnectors check now runs before the
ID-JAG dispatch so the per-client connector allowlist applies to both
standard and ID-JAG token exchanges.

Signed-off-by: kanywst <niwatakuma@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/bug-fix Release note: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants