feat: use *SecretVar for OAuth2Config credentials to preserve env./vault.

[Pratham-Mishra04]

Summary

OAuth2Config.ClientID and ClientSecret were plain string fields, which meant env-variable and vault references (e.g. env.MY_VAR) were silently flattened to literal strings before being stored or compared. This PR changes both fields to *SecretVar, so reference metadata is preserved end-to-end: from config parsing through storage, change detection, OAuth flow initiation, and redaction.

Changes

• OAuth2Config.ClientID and ClientSecret changed from string to *SecretVar, enabling env./vault. references to be carried intact rather than resolved prematurely.
• InitiateOAuthFlow no longer wraps credentials in a new SecretVar at the point of use; the *SecretVar from the config is passed through directly, and dynamically registered credentials are wrapped once at registration time.
• pendingOAuthConfigToRequest no longer manually unpacks SecretVar values into plain strings and re-wraps them — credentials now pass through as-is.
• mcpOauthBlockChanged compares credentials by resolved value (GetValue()), so an env. reference that resolves to the same credential as the stored value is not treated as a config drift.
• RedactMCPClientConfig now calls Redacted() directly on the *SecretVar fields instead of routing through an intermediate SecretVar construction, and no longer mutates the live stash.
• Added TestOAuth2ConfigSecretVarCredentials covering bare-string JSON backward compatibility, marshal round-trip reference preservation, and redaction isolation.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

go test ./core/schemas/... ./framework/oauth2/... ./transports/bifrost-http/...

Verify that:

• An OAuth2Config with client_id: "env.MY_VAR" resolves to the environment variable value at use time, not at parse time.
• Redacting an MCPClientConfig with a PendingOAuthConfig does not modify the original struct's credential values.
• Config change detection does not flag an env. reference as drift when it resolves to the same value as the stored credential.

Breaking changes

• Yes
• No

OAuth2Config.ClientID and ClientSecret are now *SecretVar instead of string. Any code that directly reads or assigns these fields as plain strings must be updated to use GetValue() for reading and NewSecretVar(...) or nil for assignment. Existing JSON blobs with bare string credentials remain backward-compatible via SecretVar's JSON unmarshalling.

Security considerations

Credentials stored as env./vault. references are no longer resolved to their plaintext values before being persisted or compared. Resolved values are only materialized at the point of use (e.g. when building the authorize URL), reducing the surface area where plaintext secrets appear in memory or logs. Redaction now operates directly on SecretVar, ensuring the live config stash is never mutated by a redaction call.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Enhancements
  • OAuth client IDs and secrets now support environment and vault references.
  • Secure credential references remain intact during registration, authorization, rotation, and authentication.
  • Credentials are resolved only when required, reducing unnecessary exposure of secret values.
  • OAuth credentials can be updated without replacing configured references.
  • Redacting pending OAuth settings no longer changes active configuration.
  • OAuth authentication flows now handle referenced credentials consistently.

Walkthrough

OAuth client credentials now use SecretVar values across configuration, registration, handlers, rotation, redaction, and tests. The code preserves environment and vault reference metadata and resolves values only when required.

Changes

OAuth SecretVar propagation

Layer / File(s)	Summary
OAuth contract and registration core/schemas/oauth.go, framework/oauth2/main.go	OAuth2Config.ClientID and ClientSecret use *SecretVar. Registration retains wrappers and uses IsSet(). Dynamic registration stores literal SecretVar values. Authorization URL construction resolves ClientID with GetValue().
Handler credential propagation transports/bifrost-http/handlers/mcpoauth2.go, transports/bifrost-http/handlers/mcp.go	Handlers forward existing SecretVar values directly. Pending credentials are forwarded when IsSet() is true.
Rotation, redaction, and validation transports/bifrost-http/lib/config.go, transports/bifrost-http/lib/config_test.go	Rotation preserves SecretVar metadata. Pending credential redaction avoids mutating live configuration. Tests use SecretVar fixtures and resolve client IDs through GetValue().

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OAuthHandler
  participant OAuthConfig
  participant DynamicRegistration
  participant AuthorizationURL
  OAuthHandler->>OAuthConfig: pass ClientID and ClientSecret as SecretVar
  OAuthConfig->>DynamicRegistration: retain credential wrappers
  DynamicRegistration->>AuthorizationURL: resolve ClientID with GetValue()

Loading

Possibly related PRs

• maximhq/bifrost#4860: Both PRs update OAuth client_id and client_secret support for SecretVar references.
• maximhq/bifrost#5715: Both PRs modify OAuth SecretVar handling and rotation logic.
• maximhq/bifrost#5724: Both PRs modify OAuth2 ClientID and ClientSecret handling in the schema and OAuth framework.

Suggested reviewers: akshaydeo, bearts, danpiths

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the primary change: using SecretVar for OAuth2 credentials to preserve environment and vault references.
Description check	✅ Passed	The description covers the purpose, implementation, breaking-change impact, affected areas, testing steps, security considerations, and tests.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 07-02-feat_add_secretvar_support_to_oauth_client_id_and_secret

---

_{Comment @coderabbitai help to get the list of available commands.}

[Pratham-Mishra04]

• feat: add stack-absorb Claude skill for manual Graphite stack change distribution #5786 : 2 dependent PRs (#5787 , #5812 )
• docs+feat: expand MCP connection states into reference table and add tooltip to State column header #5785
• feat: add token-exchange demo MCP server for testing against real identity providers #5730
• docs: add token exchange (on-behalf-of) MCP auth documentation #5729
• docs: document make-before-break reconnection and shared-client retry behavior #5728
• feat: make-before-break reconnect for HTTP/SSE MCP clients with generation-guarded stale write protection #5727
• feat: retry shared-connection MCP auth failures after bounded reconnect wait with concurrent dedup #5726
• feat: add ui for token_exchange (On-Behalf-Of) auth type MCP clients #5725
• feat: add token_exchange MCP auth type with delegated identity-provider token exchange #5724
• docs: add needs_reauth state, reauthorize endpoint, per-client tool sync/timeout, allowed_extra_headers, and OAuth config rotation to MCP OpenAPI spec #5723
• docs: add OAuth reauthorization, admin discovery credentials, auth failure recovery, and credential rotation docs #5722
• docs: add env./vault. reference support for OAuth client_id/client_secret and update OpenAPI EnvVar schema #4860
• docs: add pending_verification state and initiate-verification / verify-headers MCP client endpoints to OpenAPI spec #4858
• docs: add config.json bootstrap support for auth-aware MCP clients (oauth, per_user_oauth, per_user_headers) #3876
• feat: use *SecretVar for OAuth2Config credentials to preserve env./vault. #4859 👈 (View in Graphite)
• feat: add LRU credential cache to MCP headers provider with scoped eviction #5721
• feat: add in-memory LRU cache for per-user MCP OAuth access tokens with targeted eviction #5720
• feat: add generation-guarded LRU cache with single-flight fills and secondary index #5719
• feat: add per-user OAuth/headers admin discovery credential repair flow with registry needs_reauth projection #5718
• feat: retain admin bootstrap credential for periodic per-user MCP tool discovery refresh #5717
• feat: add MCPAuthModeAdmin + reauthorize endpoint for shared OAuth MCP clients #5716
• feat: replace OAuth config pin-and-warn with in-place rotation and needs_reauth cascade #5715
• feat: add needs_reauth state and replace per-user connection badge with sessions link #5714
• feat: add auth-failure retry with forced credential refresh for MCP tool calls #5713
• feat: add NeedsReauth MCP connection state for dead OAuth2 credentials #5712
• refactor: make GetExpiringOauthTokens auth modes caller-configurable via AuthModes field #5711
• refactor: unify shared and per-user OAuth refresh under a single RefreshAccessToken(tokenID) path, dropping oauth_configs.token_id FK shortcut #5710
• refactor: migrate OAuth flow state/PKCE fields from oauth_configs to new mcp_oauth_flows table #5709
• refactor: merge oauth_tokens/oauth_user_tokens into unified mcp_oauth_tokens table #5708
• feat: add per_user_headers bootstrap verification flow via submitHandler abstraction #3875
• feat: add verify-headers endpoint and pending_verification state for per-user-headers MCP clients #3874
• fix: extend pending_verification bootstrap path to cover per_user_oauth clients #3873
• feat: add OAuth authorization flow for config.json-bootstrapped pending_verification MCP clients #3872
• feat: add pending_verification state and initiate-verification endpoint for config.json shared-OAuth MCP clients #3871
• docs: document flow mode access rules, temp token toggle, and per-server tool manager overrides #3870 : 1 other dependent PR (#3882 )
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[greptile-apps]

Confidence Score: 4/5

The change is mechanically correct and well-tested; the two findings are both non-blocking polish items.

The core credential-threading logic is sound: SecretVar's nil-safe methods handle every nil/empty/reference case, dynamic registration still triggers correctly via IsSet(), and redaction no longer mutates the live stash. The two gaps are the missing schema description update for the env./vault. prefix syntax and the non-constant-time credential comparison in the drift-detection path — both real but low-impact.

transports/config.schema.json (oauth_config.client_id / client_secret descriptions) and transports/bifrost-http/lib/config.go (mcpOauthBlockChanged credential comparison).

Important Files Changed

Filename	Overview
core/schemas/oauth.go	Cleanly changes ClientID/ClientSecret from string to *SecretVar with nil-safe method support; comments updated appropriately.
framework/oauth2/main.go	Dynamic registration wraps plain response strings in NewSecretVar; the IsSet() guard correctly preserves DCR logic; resolvedClientID uses GetValue() only at the point of building the authorize URL.
transports/bifrost-http/handlers/mcp.go	pendingOAuthConfigToRequest correctly passes *SecretVar through without unwrapping; IsSet() replaces the empty-string guard correctly.
transports/bifrost-http/handlers/mcpoauth2.go	Credential unwrap/re-wrap boilerplate removed; *SecretVar assigned directly from request to config, simplifying the flow.
transports/bifrost-http/lib/config.go	mcpOauthBlockChanged compares by resolved value (GetValue()) which is correct; redaction path correctly copies the struct and avoids live-stash mutation. Non-constant-time credential comparison is a minor policy deviation.
transports/bifrost-http/lib/config_test.go	New TestOAuth2ConfigSecretVarCredentials covers bare-string backward compat, marshal round-trip reference preservation, and redaction isolation; existing tests updated to use NewSecretVar.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant CF as config.json
    participant LC as Config Loader
    participant OA as OAuthHandler
    participant IV as InitiateOAuthFlow
    participant DB as oauth_configs table
    participant OP as OAuth Provider

    CF->>LC: client_id: "env.MY_VAR" (bare string)
    LC->>LC: "UnmarshalJSON → *SecretVar{ref:"env.MY_VAR", Val:resolved}"
    LC->>OA: "OAuth2Config.ClientID = *SecretVar"
    OA->>IV: "config.ClientID (*SecretVar, ref preserved)"
    alt "clientID.IsSet() == false"
        IV->>OP: Dynamic Client Registration (RFC 7591)
        OP-->>IV: regResp.ClientID (plain string)
        IV->>IV: NewSecretVar(regResp.ClientID)
    end
    IV->>DB: "Store *SecretVar (ref string stored, not resolved value)"
    IV->>IV: "resolvedClientID = clientID.GetValue()"
    IV->>OP: "authorize_url?client_id=resolved_value"

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant CF as config.json
    participant LC as Config Loader
    participant OA as OAuthHandler
    participant IV as InitiateOAuthFlow
    participant DB as oauth_configs table
    participant OP as OAuth Provider

    CF->>LC: client_id: "env.MY_VAR" (bare string)
    LC->>LC: "UnmarshalJSON → *SecretVar{ref:"env.MY_VAR", Val:resolved}"
    LC->>OA: "OAuth2Config.ClientID = *SecretVar"
    OA->>IV: "config.ClientID (*SecretVar, ref preserved)"
    alt "clientID.IsSet() == false"
        IV->>OP: Dynamic Client Registration (RFC 7591)
        OP-->>IV: regResp.ClientID (plain string)
        IV->>IV: NewSecretVar(regResp.ClientID)
    end
    IV->>DB: "Store *SecretVar (ref string stored, not resolved value)"
    IV->>IV: "resolvedClientID = clientID.GetValue()"
    IV->>OP: "authorize_url?client_id=resolved_value"

Loading

_{Reviews (1): Last reviewed commit: "feat: add secretvar support to oauth cli..." | Re-trigger Greptile}

[coderabbitai]

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.

[Pratham-Mishra04]

Merge activity

• Aug 8, 8:47 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Aug 8, 9:42 AM UTC: Graphite rebased this pull request as part of a merge.
• Aug 8, 9:43 AM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

The base branch was changed.