feat: add OAuth2 issuance endpoints (DCR, authorize, token) with PKCE and refresh token rotation#4506
Open
Pratham-Mishra04 wants to merge 1 commit into
Conversation
|
|
Collaborator
Author
This was referenced Jun 17, 2026
Open
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds complete OAuth2 issuance support to Bifrost: three new GORM-backed database tables ( ChangesOAuth2 Issuance Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as OAuth2IssuanceHandler
participant ConfigStore
participant TempToken as TempTokenService
rect rgba(173, 216, 230, 0.5)
note over Client,TempToken: Authorization Request
Client->>Handler: GET /oauth2/authorize?client_id&code_challenge&resource
Handler->>ConfigStore: GetOAuth2ClientByClientID
Handler->>Handler: matchRedirectURI (loopback port-agnostic)
Handler->>ConfigStore: CreateOAuth2AuthorizeRequest
Handler->>TempToken: MintTempToken (oauth2_consent, 15-min TTL)
Handler-->>Client: 302 Found → consent page
end
rect rgba(144, 238, 144, 0.5)
note over Client,ConfigStore: Token Exchange (authorization_code)
Client->>Handler: POST /oauth2/token grant_type=authorization_code&code&code_verifier&client_id
Handler->>Handler: hashSHA256Hex(code)
Handler->>ConfigStore: GetOAuth2AuthorizeRequestByCodeHash
Handler->>Handler: verifyPKCES256(code_verifier vs. stored challenge)
Handler->>ConfigStore: GetOAuth2SigningKey → RS256 JWT
Handler->>ConfigStore: ConsumeOAuth2AuthorizeRequest (atomic: status→code_issued + insert refresh token)
Handler-->>Client: access_token, refresh_token, scope, expires_in
end
rect rgba(255, 218, 185, 0.5)
note over Client,ConfigStore: Token Rotation (refresh_token)
Client->>Handler: POST /oauth2/token grant_type=refresh_token&refresh_token&client_id
Handler->>Handler: hashSHA256Hex(refresh_token)
Handler->>ConfigStore: GetOAuth2RefreshTokenByHash
Handler->>ConfigStore: GetOAuth2SigningKey → RS256 JWT
Handler->>ConfigStore: RotateOAuth2RefreshToken (atomic: revoke old + insert new)
Handler-->>Client: new access_token, rotated refresh_token, expires_in
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
framework/configstore/tables/oauth2_issuance.go (1)
13-25: ⚡ Quick winConsider adding UpdatedAt for audit trail.
While OAuth2 clients are relatively static after registration, tracking the last update time can be useful for audit, troubleshooting, and compliance purposes (e.g., when
client_nameorredirect_urisare modified).📝 Suggested addition
Scope string `gorm:"type:varchar(255)" json:"scope"` CreatedAt time.Time `gorm:"index;not null" json:"created_at"` + UpdatedAt time.Time `gorm:"not null" json:"updated_at"` // Virtual fieldsGORM will automatically populate
UpdatedAton every save.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/tables/oauth2_issuance.go` around lines 13 - 25, The TableOAuth2Client struct is missing an UpdatedAt field for audit trail tracking. Add an UpdatedAt field of type time.Time to the struct with GORM tags that include indexing (similar to the existing CreatedAt field). GORM will automatically populate this field on every save operation, providing audit history for when OAuth2 client records are modified.transports/bifrost-http/handlers/oauth2_issuance.go (1)
427-476: 💤 Low valueConsider supporting IPv6 loopback in redirect URI matching.
RFC 8252 §7.3 notes that "localhost" should resolve to loopback addresses for both IPv4 and IPv6. The current implementation handles
localhostand127.0.0.1but not[::1](IPv6 loopback). While most OAuth clients use IPv4, some environments may prefer IPv6.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/oauth2_issuance.go` around lines 427 - 476, The matchRedirectURI function currently only recognizes localhost and 127.0.0.1 as loopback addresses but does not handle IPv6 loopback ([::1]). Update the loopback detection logic to also check if the parsed hostname equals [::1] in addition to the existing localhost and 127.0.0.1 checks, ensuring that IPv6 loopback addresses are properly handled alongside IPv4 loopback addresses when matching redirect URIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 10932-10963: The migrationAddOAuth2IssuanceTables function creates
three OAuth2 tables (TableOAuth2Client, TableOAuth2AuthorizeRequest, and
TableOAuth2RefreshToken) but provides no rollback mechanism in the
migrator.Migration struct. Add a Rollback function to the migration that drops
these three tables in reverse order (TableOAuth2RefreshToken first, then
TableOAuth2AuthorizeRequest, then TableOAuth2Client) to properly handle schema
rollbacks, or if rollback is not supported, explicitly mark the migration as
non-rollbackable using the appropriate migrator option.
In `@framework/configstore/rdb.go`:
- Around line 6843-6852: The GetOAuth2RefreshTokenByHash method in
RDBConfigStore filters out revoked tokens with the WHERE clause condition
"revoked_at IS NULL", which prevents the token endpoint from detecting token
reuse and loading the FamilyID for family-wide revocation. Remove the "AND
revoked_at IS NULL" condition from the database query so that the method returns
the token row regardless of revocation status, and allow the caller to check and
handle the revoked status separately.
- Around line 6860-6869: The code updates the OAuth2AuthorizeRequest table
status but does not verify that the update actually modified any rows. If the
request is already consumed, expired, or missing, the update will affect zero
rows yet the code continues to create the refresh token, violating single-use
authorization code semantics. After the Updates call on the
OAuth2AuthorizeRequest table, check the RowsAffected value from the result. If
RowsAffected is zero, return an error before proceeding to the tx.Create(rt)
call that mints the refresh token. This ensures that the refresh token is only
created when the authorization request is successfully transitioned to the
CodeIssued status.
- Around line 6881-6888: The refresh token rotation in the transaction block is
missing a check to ensure only active tokens are revoked. Modify the WHERE
clause in the Update call that sets revoked_at to also include the condition
revoked_at IS NULL to prevent rotating already-revoked tokens. Additionally,
after the Update operation completes, check the RowsAffected property of the
result to verify that exactly one active token was revoked before proceeding to
create the newRT. If RowsAffected is zero, return an appropriate error to
prevent creating a new token when no old token was actually revoked.
In `@framework/configstore/tables/oauth2_issuance.go`:
- Around line 29-45: The BeforeSave method in TableOAuth2Client does not
validate that the required OAuth2 fields are present before marshaling them. Add
validation logic at the beginning of the BeforeSave method to check if either
RedirectURIs or GrantTypes is nil, and return a formatted error message if
either field is missing (these are required per RFC 7591). Import the "fmt"
package at the top of the file to support error message formatting.
In `@transports/bifrost-http/handlers/oauth2_issuance.go`:
- Around line 378-387: The code calls GetOAuth2SigningKey and checks for errors,
but does not verify that the returned signingKey is not nil before accessing its
PrivateKeyPEM field on line 383. Add a nil check immediately after the error
check for GetOAuth2SigningKey to ensure signingKey is not nil before calling
parseRSAPrivateKeyPEM with signingKey.PrivateKeyPEM. If signingKey is nil, send
an appropriate OAuth error response using sendOAuthError with a suitable error
code and description.
- Around line 335-337: The current code incorrectly falls back to using rt.Scope
when the resource parameter is empty, violating RFC 8707 where resource and
scope are orthogonal concepts. Add a Resource field to the
TableOAuth2RefreshToken struct (similar to how Scope is already stored) to
preserve the original resource indicator across token rotations. Then update the
refresh token handling logic to populate and retrieve the Resource field from
the stored token instead of falling back to Scope when the resource parameter is
empty. This ensures the resource/audience binding remains intact across token
rotations.
---
Nitpick comments:
In `@framework/configstore/tables/oauth2_issuance.go`:
- Around line 13-25: The TableOAuth2Client struct is missing an UpdatedAt field
for audit trail tracking. Add an UpdatedAt field of type time.Time to the struct
with GORM tags that include indexing (similar to the existing CreatedAt field).
GORM will automatically populate this field on every save operation, providing
audit history for when OAuth2 client records are modified.
In `@transports/bifrost-http/handlers/oauth2_issuance.go`:
- Around line 427-476: The matchRedirectURI function currently only recognizes
localhost and 127.0.0.1 as loopback addresses but does not handle IPv6 loopback
([::1]). Update the loopback detection logic to also check if the parsed
hostname equals [::1] in addition to the existing localhost and 127.0.0.1
checks, ensuring that IPv6 loopback addresses are properly handled alongside
IPv4 loopback addresses when matching redirect URIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 67cf22e7-348f-45b9-82b1-4f1ac1a97df4
📒 Files selected for processing (8)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth2_issuance.gotransports/bifrost-http/handlers/oauth2_issuance.gotransports/bifrost-http/handlers/temp_token_scopes.gotransports/bifrost-http/server/server.gotransports/go.mod
Pratham-Mishra04
force-pushed
the
06-17-feat_adds_mcp_oauth2_authorization_server
branch
from
a3e6af6 to
8bebc42
Compare
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
2 times, most recently
from
226ccda to
bd05880
Compare
Pratham-Mishra04
force-pushed
the
06-17-feat_adds_mcp_oauth2_authorization_server
branch
from
8bebc42 to
0c48164
Compare
Pratham-Mishra04
force-pushed
the
06-17-feat_adds_mcp_oauth2_authorization_server
branch
from
0c48164 to
4eb43f0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/handlers/oauth2_issuance.go`:
- Around line 488-492: The loopback redirect URI validation is missing a
hostname comparison check despite the comment indicating it should match scheme
plus host plus path. In the conditional statement within the loopback validation
block (where scheme and path are already compared), add a hostname comparison
between parsed.Hostname() and rParsed.Hostname() to ensure that localhost and
127.0.0.1 are treated as distinct hosts, complying with RFC 8252 requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85ec5950-56c1-48bc-b72f-4ebf0cd2ab81
📒 Files selected for processing (9)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth2_issuance.gotransports/bifrost-http/handlers/oauth2_issuance.gotransports/bifrost-http/handlers/temp_token_scopes.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/go.mod
🚧 Files skipped from review as they are similar to previous changes (8)
- transports/bifrost-http/handlers/temp_token_scopes.go
- transports/go.mod
- framework/configstore/tables/oauth2_issuance.go
- framework/configstore/store.go
- framework/configstore/migrations.go
- transports/bifrost-http/lib/config_test.go
- transports/bifrost-http/server/server.go
- framework/configstore/rdb.go
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
from
bd05880 to
18d1803
Compare
Pratham-Mishra04
force-pushed
the
06-17-feat_adds_mcp_oauth2_authorization_server
branch
from
4eb43f0 to
02df1ae
Compare
Pratham-Mishra04
force-pushed
the
06-17-feat_adds_mcp_oauth2_authorization_server
branch
from
02df1ae to
d37e5e4
Compare
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
from
18d1803 to
67e3649
Compare
Pratham-Mishra04
force-pushed
the
06-16-feat_adds_mcp_server_oauth_tables
branch
from
67e3649 to
46280b1
Compare
Pratham-Mishra04
force-pushed
the
06-17-feat_adds_mcp_oauth2_authorization_server
branch
from
d37e5e4 to
5117e2b
Compare
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.
Summary
This PR implements the downstream OAuth2 token issuance flow, enabling Bifrost to act as a full OAuth2 authorization server. It adds the three core RFC-compliant endpoints (Dynamic Client Registration, Authorization, and Token), the backing database tables and store methods, and the consent temp-token scope needed to bind the consent UI to a specific authorization request.
Changes
oauth2_clients,oauth2_authorize_requests,oauth2_refresh_tokens) added via a newadd_oauth2_issuance_tablesmigration step, with GORM model definitions intables/oauth2_issuance.go.TableOAuth2Clientserializesredirect_urisandgrant_typesas JSON columns withBeforeSave/AfterFindhooks.ConfigStoreinterface extended with methods for creating/fetching OAuth2 clients, managing authorize requests (including code-hash lookup and expiry sweeping), and refresh token operations (GetOAuth2RefreshTokenByHash,ConsumeOAuth2AuthorizeRequest,RotateOAuth2RefreshToken).RDBConfigStoreimplements all new interface methods.ConsumeOAuth2AuthorizeRequestandRotateOAuth2RefreshTokenare wrapped in transactions so failures leave the grant in a retryable state.OAuth2IssuanceHandler(handlers/oauth2_issuance.go) wires three public routes:POST /oauth2/register— RFC 7591 DCR; only public clients (token_endpoint_auth_method=none) are accepted.GET /oauth2/authorize— PKCE-S256 (RFC 7636) + resource indicator (RFC 8707); creates a pending authorize request and redirects to the consent UI with an optional scoped temp token.POST /oauth2/token— handlesauthorization_codeandrefresh_tokengrants; issues RS256 JWTs signed with the persisted signing key and rotates refresh tokens on every use.FamilyIDon refresh tokens: all tokens descended from the same authorization grant share a family ID, enabling full family revocation when a revoked token is re-presented (RFC 9700 §2.2.2).localhost/127.0.0.1).oauth2ConsentScopetemp-token scope registered at startup, binding consent-page API calls to a single authorize request ID via path substitution.github.com/golang-jwt/jwt/v5promoted from indirect to a direct dependency.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./transports/bifrost-http/...GET /oauth2/authorizewithresponse_type=code,code_challenge(S256),resource, and the returnedclient_id.POST /oauth2/tokenwithgrant_type=authorization_codeand the PKCE verifier.grant_type=refresh_tokenand verify the old refresh token is revoked and a new one is issued.Breaking changes
Security considerations
ConsumeOAuth2AuthorizeRequestatomically transitions the request tocode_issuedand creates the refresh token in one transaction.Checklist
docs/contributing/README.mdand followed the guidelines