feat(gatekeeper-kit): oauth 2.0 token client - #555
Merged
Merged
Conversation
ndisidore
changed the title
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted one review with 2 actionable inline findings: a refresh race across Cloudflare reconnects and an OAuth Basic-auth encoding issue. |
Preview:
|
ndisidore
force-pushed
the
nathan/gatekeeper-kit-oauth-refresh
branch
from
September 23, 2026 14:20
609347f to
efd1f74
Compare
ndisidore
marked this pull request as ready for review
September 23, 2026 14:20
This comment was marked as outdated.
This comment was marked as outdated.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ndisidore
force-pushed
the
nathan/gatekeeper-kit-oauth-refresh
branch
from
September 23, 2026 14:32
efd1f74 to
13a6432
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ndisidore
force-pushed
the
nathan/gatekeeper-kit-oauth-refresh
branch
from
September 23, 2026 14:44
13a6432 to
2527251
Compare
|
No new actionable findings on the current head. The unresolved Cloudflare refresh-failure classification concern is already covered by an existing inline review comment. |
Maximo-Guk
approved these changes
Sep 23, 2026
ndisidore
force-pushed
the
nathan/gatekeeper-kit-oauth-refresh
branch
from
September 23, 2026 15:33
2527251 to
3bed2a2
Compare
|
No new actionable findings. The existing inline review comments cover the remaining concerns on this PR. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Every public gatekeeper hand-rolls its token exchange, refresh and revoke. None of them follows redirects manually, caps the response body or bounds the request with a timeout, and each classifies failures its own way. `./oauth-client` is the provider-independent protocol client from the internal Access hardening, ported without its session layer, which duplicates leaves the kit already ships. - `OAuthClient`: authorize URL, code exchange, refresh, RFC 7009 revoke, and an unreserved `request()` escape hatch. Every request uses `redirect: "manual"` (following a 307/308 would re-POST the secret and code), a timeout combined with the caller's signal, and `readTextCapped`. Client auth is `none`, `basic` or `post`. `basic` sends raw `base64(id:secret)` by default, as the existing gatekeepers and providers' docs do; `encoding: "form"` sends RFC 6749 §2.3.1's form-encoded pair to a server that decodes it, such as Ory Hydra. A raw id containing `:` is rejected (RFC 7617). - `OAuthResponseError` carries `httpStatus` and `oauthError`, and deliberately not `status` or `code`: `isNoAccessError` duck-types `status` and credential marks match `code`, so a token-endpoint 401 would otherwise read as "user lacks access" and a provider body could spoof `CredentialsExpiredError`. Neither its message nor its enumerable props carry provider text beyond a validated `error` code. A 2xx body carrying an `error` and no non-empty `access_token` is a rejection. - `isInvalidGrant` is the default proof of grant death: `invalid_grant` returned to a refresh, below HTTP 500, other than 429. `invalid_client` and the other codes are operator or provider faults under static client credentials, so they are rethrown unless a gatekeeper opts in. - `oauthRefresh` adapts a client onto `CredentialCoordinator` as a `RefreshCredentials<Creds>`, and `mergeOAuthTokens` is the canonical merge: it keeps an unrotated refresh token and unreported scopes, and replaces `expiresAt` rather than carrying a stale one forward. A refresh that requests scopes reports them when the response omits `scope` (RFC 6749 §5.1), so a narrowing is never recorded as the wider grant it gave up. - PKCE S256 (`createPkce`, `pkceChallenge`), plus `parseTokenResponse` for nested or renamed payloads. A workerd test drives it end to end through the connect handshake and the coordinator's fence, adjudication and revoke-on-discard paths. README, USAGE and the plan now state one grant-death doctrine keyed on the error code. Discovery and dynamic client registration stay deferred.
ndisidore
force-pushed
the
nathan/gatekeeper-kit-oauth-refresh
branch
from
September 23, 2026 15:42
3bed2a2 to
6573d18
Compare
|
LGTM! |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Behavior change: a refresh that fails for a reason that says nothing about the grant -- a 5xx, a 429, a 4xx without an OAuth error such as a WAF challenge, a network error, a timeout, a redirect or a malformed response -- no longer marks the account expired. Until now any failed refresh set the sticky `credentialsExpired` flag, which hid the account from binding pickers and asked for a reconnect that fixed nothing. Nor does `invalid_client`, which is the deployment's own client failing authentication: a bad `CLIENT_SECRET` expired every account that refreshed, and no reconnect could fix it. Every other OAuth error in a 4xx still expires the account, as before: how Cloudflare rejects a revoked grant is unverified, so `isGrantDeath` widens the kit's `isInvalidGrant` to those until a live test justifies narrowing it. A transient failure serves the cached token while it is unexpired, and otherwise throws the real error. `describe()` and `getUsableAccessToken()` still degrade to null; the other callers see the error rather than a synthetic expiry. `getAccessToken` now coalesces concurrent refreshes of a grant, so a rotating refresh token is never redeemed twice (the loser's `invalid_grant` would be a false death). A refresh that a reconnect or revoke overtook is discarded, even when the reconnect reuses the refresh token, and a read after the reconnect starts its own refresh rather than joining it, as the kit's `CredentialCoordinator` does. Both failure paths are logged, as `credentials.refresh.expired` and `credentials.refresh.failed`. The stored token shape is unchanged.
ndisidore
force-pushed
the
nathan/gatekeeper-kit-oauth-refresh
branch
from
September 23, 2026 15:46
6573d18 to
a33480c
Compare
|
LGTM! |
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
2 participants
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.
Adds
@gadgets/gatekeeper-kit/oauth-client, a provider-independent OAuth 2.0 token-endpoint client. It includes:OAuthResponseError, withisInvalidGrantas the default proof of grant death;oauthRefreshandmergeOAuthTokens, which adapt it ontoCredentialCoordinator.gatekeeper-cloudflarepilots it, and only its token code changes.The PR has two commits so that each can be reviewed alone:
plans/gatekeeper-kit.md;Nothing touches
workshop-backendorworkshop-shared. Discovery and DCR are deferred.