refactor: migrate OAuth flow state/PKCE fields from oauth_configs to new mcp_oauth_flows table - #5709
Conversation
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughOAuth configuration rows now store reusable credentials. Transient state and PKCE data move to ChangesOAuth flow persistence
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant OAuthHandler
participant ConfigStore
participant mcp_oauth_flows
participant OAuthProvider
OAuthHandler->>ConfigStore: create config and admin flow transactionally
ConfigStore->>mcp_oauth_flows: persist state and PKCE data
OAuthProvider-->>OAuthHandler: return callback state and code
OAuthHandler->>ConfigStore: claim flow by state
OAuthHandler->>OAuthProvider: exchange code with flow verifier
OAuthHandler->>ConfigStore: replace shared token transactionally
ConfigStore->>mcp_oauth_flows: delete terminal flow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
404348f to
83be929
Compare
5025fd1 to
7949fe6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
7949fe6 to
fb096cd
Compare
83be929 to
f2c9723
Compare
Merge activity
|
The base branch was changed.
…aim atomic against concurrent reauth
f2c9723 to
e43d331
Compare

Summary
Introduces
mcp_oauth_flowsas a dedicated table for in-flight OAuth authorize attempts, replacing the dual-purposeoauth_user_sessionstable and removing CSRF/PKCE columns (state,code_verifier,code_challenge,expires_at) fromoauth_configs. The newTableMCPOauthFlowcovers all flow kinds — per-identity (user,vk,session) and admin-mode (admin) alike — whileTableOauthConfigbecomes a pure, durable credential template.Changes
TableMCPOauthFlowstruct andmcp_oauth_flowstable: Carriesstate,code_verifier,redirect_uri,expires_at, andflow_modefor a single authorize attempt.CodeVerifieris encrypted viaBeforeSave/AfterFindhooks, matching the behavior previously onTableOauthConfig.create_mcp_oauth_flows_table: Createsmcp_oauth_flowsand backfills fromoauth_user_sessionsviaINSERT...SELECT. The unique index onstateis created after the backfill to avoid ordering issues.drop_oauth_config_pkce_columns: Dropsstate,code_verifier,code_challenge, andexpires_atfromoauth_configs. These columns wereNOT NULLat the DB level and would have broken future INSERTs once the Go struct stopped setting them.TableOauthConfigsimplified: PKCE/CSRF fields removed from the struct,BeforeSave/AfterFindhooks updated to only encrypt/decryptclient_secret. TheEncryptionStatusis now set only whenclient_secretis present.InitiateOAuthFlowupdated: Creates theoauth_configsrow and aflow_mode='admin'flow row atomically in a single transaction to prevent orphaned config rows on partial failure.CompleteOAuthFlowupdated: Claims the admin-mode flow row via the newClaimOauthFlowByState(scoped toflow_mode='admin'), readsCodeVerifierandRedirectURIfrom the flow row, and deletes the flow row on completion.ClaimOauthFlowByStatemethod: Admin-mode counterpart toClaimOauthUserSessionByState. The two methods partitionmcp_oauth_flowsbyflow_modeso a given state token can only be claimed by one of them.GetOauthUserSessionByStatemethod: Non-mutating lookup by state for anyflow_modeor status, used by callback error handling to classify and mark a flow failed without consuming it.GetOauthConfigByStateremoved: No longer needed;oauth_configsno longer has astatecolumn.GetPendingMCPClientByStateremoved: Callers now useGetPendingMCPClientdirectly after resolving the config ID from the flow row.handleCallbackErrorupdated: Classifies admin vs. per-user flows viaflow.FlowModeon a singlemcp_oauth_flowslookup instead of the old two-stepoauth_configs-then-assume-per-user inference.ListPendingOauthUserSessionsupdated: Now queriesmcp_oauth_flowsand explicitly excludesflow_mode='admin'rows, mirroringListOauthUserTokens'auth_mode='shared'exclusion.DeleteExpiredOauthUserSessionsupdated: Targetsmcp_oauth_flows; deliberately unfiltered byflow_modesince expiry-based sweeping is safe for all flow kinds.DeleteMCPClientConfigandDeleteVirtualKeynow delete frommcp_oauth_flowsinstead ofoauth_user_sessions.reconcileVKDirectTokensDBandreadVKsHoldingOauthCredsForMCPupdated: Referencemcp_oauth_flowswith aflow_mode='vk'defense-in-depth filter.migrationWidenEncryptedVarcharColumnsguarded: TheALTER COLUMN code_verifierstatement is now conditional on the column existing, since fresh installs afterdrop_oauth_config_pkce_columnsships will never have it onoauth_configs.TableOauthUserSessionmarked deprecated: Retained only so the backfill migration can reference its table via GORM; flagged for removal in the next major version.TestMigrationCreateMCPOauthFlowsTablePerfseeds ~1,000,000 rows intooauth_user_sessionsand measuresmigrationCreateMCPOauthFlowsTableagainst both SQLite and Postgres, gated behindBIFROST_RUN_MIGRATION_PERF_TESTS=1.TestTableOauthConfig_EncryptDecryptand related tests dropcode_verifier/state/expires_atassertions; newTestTableMCPOauthFlow_EncryptDecryptandTestTableMCPOauthFlow_EncryptionDisabled_StoresPlaintextcover the equivalent round-trips on the new table.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./framework/oauth2/... ./transports/bifrost-http/...To run the migration performance test against a live Postgres instance:
BIFROST_RUN_MIGRATION_PERF_TESTS=1 go test ./framework/configstore/... -run TestMigrationCreateMCPOauthFlowsTablePerf -vFor an existing deployment, apply the two new migrations (
create_mcp_oauth_flows_table,drop_oauth_config_pkce_columns) and verify:mcp_oauth_flowsexists and contains all rows previously inoauth_user_sessions.oauth_configsno longer hasstate,code_verifier,code_challenge, orexpires_atcolumns.DeleteExpiredOauthUserSessions) removes rows frommcp_oauth_flows.Breaking changes
GetOauthConfigByStateandGetPendingMCPClientByStateare removed from theConfigStoreinterface. Any external implementation ofConfigStoremust addGetOauthUserSessionByStateandClaimOauthFlowByState, and remove the deleted methods. Theoauth_configstable loses four columns; any direct SQL queries against those columns will fail after the migration runs.Security considerations
mcp_oauth_flows, which has a unique index onstateand a 15-minuteexpires_at. The atomic claim-by-state pattern (pending → claiming) is preserved and extended to admin-mode flows viaClaimOauthFlowByState, preventing duplicate callback processing.InitiateOAuthFlow, eliminating the window where a committed config row with no corresponding flow row could be exploited or leak state.CodeVerifierencryption is unchanged in behavior; it has moved fromTableOauthConfigtoTableMCPOauthFlow.Checklist
docs/contributing/README.mdand followed the guidelines