Fork-only CI: PR #1564 (provider-owned OAuth refresh grant) - #3
Open
GeiserX wants to merge 6 commits into
Open
Conversation
A provider that serves an indirection instead of a raw value can protect an
access token: that token's only use is to be sent to a bound host, and the
reply is not itself a credential. The refresh grant breaks that. The exchange
needs the real refresh token, and the authorization server's reply carries a
brand-new real access token, so serving an indirection here moves the exposure
one step later while appearing to remove it. Providers backed by a sealed store
have to refuse the refresh item outright today, which costs them refresh
entirely.
Add an optional `refreshGrant` to CredentialProvider so such a provider can own
the exchange instead: it spends the refresh token, seals the new access token
(and a rotated refresh token) under the same item ids, and returns only
`{ expiresAt, scope }`. The caller then reads the access token back through
`get`, the same hop every other credential already takes.
Absence is not a downgrade: when the method is missing the existing host-side
exchange runs unchanged. client_credentials is excluded deliberately - it has
no refresh token to spend. Secrets are named by item id, never passed as
values, since passing them would reintroduce the exposure this removes.
The test pins the custody property directly - that the host never resolves the
refresh token through the provider - rather than asserting the refresh
succeeded, because a provider that quietly served the token would also go
green.
Address review of the delegated refresh path: the fast path returned before the machinery that classifies an authorization-server refusal, so a provider that owned the grant lost re-auth entirely. - Report a refusal with a typed `RefreshGrantRejected` carrying the RFC 6749 §5.2 code. Both grant paths now share one classifier and one known-dead gate, so a delegated refresh surfaces `invalid_grant` to the caller and stops re-sending a doomed grant on every proactive cycle. - Resolve the client secret BELOW the delegated branch. It was read in plaintext and then never used, which both defeated the point of passing `clientSecretItemId` and meant a store that seals that item failed the refresh before `refreshGrant` was ever reached. - Read the new access token back before recording success, and fail when it cannot be resolved, instead of stamping a fresh expiry over a token nobody can read. - Enforce the host's OAuth endpoint URL policy on the delegated path too. - Report `expiresInSeconds` rather than an absolute instant, so the caller converts on the clock that later decides whether the token is due. - Carry `clientAuth` so a provider never has to guess the client authentication method. - Export `RefreshGrantInput`, `RefreshGrantResult` and `RefreshGrantRejected` from the package entry point; an external provider could not name them. - Add a changeset, and cover the refusal, read-back, scope, expiry and client_credentials paths.
A credential provider is an external plugin boundary, so nothing it authors may reach host error channels, telemetry, or persisted connection state — those values can carry token responses and other secret material. - Close the rejection classification to the standards-defined set (RFC 6749 §5.2 plus RFC 8707 `invalid_target`) and validate it at runtime, so an unrecognised value cannot reach `oauthErrorCode`, span attributes or persisted health. Drop `message`/`cause` from `RefreshGrantRejected` entirely; Executor now emits fixed host-facing text carrying only the validated code. - Contain provider storage failures, synchronous throws, Effect defects, and throwing or stateful property getters — including on the capability itself, on the success object's fields, and on the post-grant read-back. Cancellation is still propagated as cancellation; only the provider-authored reasons are dropped. - Rebuild the persisted scope from the host's own recorded grant set rather than the provider's string. `oauth_scope` is replayed to the authorization server on the next refresh, so accepting it verbatim was a persisted provider-controlled channel. A scope outside the granted set fails the refresh (RFC 6749 §6: a refresh may narrow scope, never widen it). - Bound the reported lifetime to finite, non-negative and at most ten years, instead of stamping NaN/Infinity/negative straight into `expires_at`. - Treat an unresolvable read-back as a retryable provider-invariant failure rather than demanding re-authentication: the authorization server ACCEPTED the grant, so re-auth is the one remedy that cannot be required, and a rotated refresh token may already be sealed. - Re-export the contract from the promise and shared surfaces too, and generalise the security note from `tokenUrl` to the whole caller-authored input tuple.
The scope validation compared the provider's reported scope against the connection's recorded grant set. When that set is empty the comparison had no subset to accept, so every reported scope failed the whole refresh — and the failure is a retryable StorageError, so the connection would retry a grant that could never succeed, indefinitely. An empty grant set is a legitimate state, not a corrupt one: RFC 6749 §5.1 lets an authorization server omit the scope it granted, and the refresh request then omits the scope parameter entirely. With nothing recorded there is also nothing to widen away from, so the safe action is to keep the refresh and record no scope. The provider's string is still never persisted, which is the property the validation exists to hold.
Fork-internal. Upstream's ci.yml targets blacksmith runners this fork does not have, so its jobs queue with no runner and UsefulSoftwareCo#1564 has never carried a check. This runs lint, format, typecheck and the full core/sdk suite - including the 909-line delegation test this PR adds - on ubuntu-latest, which is free and uncapped for public repositories. FORK-ONLY. Must never reach the upstream pull request.
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Fork-internal, never for upstream. Runs the repo's own gates against UsefulSoftwareCo#1564's content on free runners, because upstream's
ci.ymltargets blacksmith runners this fork does not have — which is why UsefulSoftwareCo#1564 has never carried a check.Content is
upstream-pr/oauth-refresh-grantplus the fork-only workflow. Every other workflow in this fork is disabled, so nothing can deploy or publish.