feat: support EnvVar references for MCP OAuth client_id/client_secret with redacted GET responses and batch config fetching #3230
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughMCP OAuth client credentials were converted to EnvVar-backed types and propagated through schemas, DB table, configstore API/impl, handlers, OAuth core, redaction, and UI. RDB implementation added a bulk GetOauthConfigsByIDs method and stray numeric tokens were removed so the file compiles. ChangesEnvVar-backed OAuth credentials (single coherent change DAG)
Estimated code review effort Sequence Diagram(s)sequenceDiagram
participant UI as Browser/UI
participant API as Bifrost HTTP Handler
participant Store as ConfigStore/RDB
participant OAuthCore as OAuth2 Core
participant Provider as External OAuth Provider
UI->>API: GET /mcp/clients
API->>Store: Get clients + GetOauthConfigsByIDs(ids)
Store-->>API: client rows + oauth config map (EnvVar wrappers)
API->>UI: MCPClientConfig[] (OauthClientID/Secret redacted EnvVar)
UI->>API: POST /oauth/initiate { oauth_config.client_id: EnvVar, ... }
API->>OAuthCore: InitiateOAuthFlow (resolved client_id/client_secret)
OAuthCore->>Provider: /authorize?client_id=resolved
Provider-->>OAuthCore: auth code
OAuthCore->>Provider: /token (exchange with client_secret)
Provider-->>OAuthCore: tokens
OAuthCore-->>API: tokens/result
API-->>UI: response / redirect
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
changed the title
EnvVar references for MCP OAuth client_id/client_secret with redacted GET responses and batch config fetching
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
Confidence Score: 4/5Safe to merge with the noted P2 edge-case (unresolved env var silently sends empty client_id to the OAuth provider). The three P1 issues from prior review rounds all appear resolved by the per-field ShouldPreserveStored() guards and the FromEnv check in AfterFind. Only P2 gaps remain. transports/bifrost-http/handlers/oauth2.go — the Value()-based extraction path for unresolved env-var references deserves a resolved-value guard matching the one already in oauth2_per_user.go. Important Files Changed
Reviews (8): Last reviewed commit: "feat: envVar on oauth config in mcp sett..." | Re-trigger Greptile |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/handlers/mcp.go (2)
831-881:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve unchanged OAuth fields independently during credential rotation.
The edit sheet pre-populates both fields from the redacted GET response in
ui/app/workspace/mcp-registry/views/mcpClientSheet.tsxLines 161-163 and then resubmits both fields in Lines 210-214 when either one is dirty. Here you only short-circuit when both values are placeholders, so a secret-only or client-id-only rotation can serialize the untouched placeholder throughenvVarStorageValue(...)and pass that back intoInitiateOAuthFlowinstead of the stored credential.Suggested fix
if shouldRotateOAuthConfig { - // If both fields carry a redacted/empty placeholder (i.e. user didn't actually change - // them), treat this as a no-op so the existing credentials are preserved. - if req.OauthConfig.ClientID.ShouldPreserveStored() && req.OauthConfig.ClientSecret.ShouldPreserveStored() { + preserveClientID := req.OauthConfig.ClientID.ShouldPreserveStored() + preserveClientSecret := req.OauthConfig.ClientSecret.ShouldPreserveStored() + // If both fields carry a redacted/empty placeholder (i.e. user didn't actually change + // them), treat this as a no-op so the existing credentials are preserved. + if preserveClientID && preserveClientSecret { shouldRotateOAuthConfig = false } - } - if shouldRotateOAuthConfig { - oauthClientID = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientID)) - oauthClientSecret = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientSecret)) + if shouldRotateOAuthConfig { + if !preserveClientID { + oauthClientID = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientID)) + } + if !preserveClientSecret { + oauthClientSecret = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientSecret)) + } + } oauthAuthorizeURL = strings.TrimSpace(req.OauthConfig.AuthorizeURL) oauthTokenURL = strings.TrimSpace(req.OauthConfig.TokenURL) oauthRegistrationURL = strings.TrimSpace(req.OauthConfig.RegistrationURL) @@ if existingOauthConfig != nil { + if preserveClientID { + oauthClientID = strings.TrimSpace(existingOauthConfig.GetResolvedClientID()) + } + if preserveClientSecret { + oauthClientSecret = strings.TrimSpace(existingOauthConfig.GetResolvedClientSecret()) + } if oauthAuthorizeURL == "" { oauthAuthorizeURL = strings.TrimSpace(existingOauthConfig.AuthorizeURL) }🤖 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/mcp.go` around lines 831 - 881, The handler currently treats OAuth rotation as a no-op only when BOTH req.OauthConfig.ClientID.ShouldPreserveStored() and req.OauthConfig.ClientSecret.ShouldPreserveStored() are true, which lets a redacted placeholder for one field be passed through; change the logic to preserve each field independently: only call envVarStorageValue and assign oauthClientID when !req.OauthConfig.ClientID.ShouldPreserveStored(), and only assign oauthClientSecret when !req.OauthConfig.ClientSecret.ShouldPreserveStored(); when a field is preserved, after loading existingOauthConfig use existingOauthConfig.GetResolvedClientID() / GetResolvedClientSecret() (or existingOauthConfig values) to populate that specific oauthClientID/oauthClientSecret instead of the placeholder; adjust the validation that currently errors when oauthClientID=="" && oauthClientSecret=="" to require at least one of (resolved or provided) client_id or client_secret and fail with a clear message referencing the missing specific field(s).
280-327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep OAuth redaction consistent on both
/api/mcp/clientscode paths.This enrichment only runs in
getMCPClientsPaginated. The non-paginated branch in the same handler still returnsRedactMCPClientConfig(configClient)directly, sooauth_client_id/oauth_client_secretare present only when the caller passes pagination params. That makes the response shape query-dependent and breaks any non-paginated consumer that relies on the new redacted OAuth fields to prefill edit flows.🤖 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/mcp.go` around lines 280 - 327, The non-paginated code path in getMCPClientsPaginated is returning RedactMCPClientConfig(configClient) without applying the same OAuth enrichment done in the paginated branch, causing oauth_client_id/secret to be present only for paginated calls; fix by replicating the batch-fetch + merge logic used there: call h.store.ConfigStore.GetOauthConfigsByIDs to build oauthConfigsByID, populate clientConfig.OauthClientID and clientConfig.OauthClientSecret (using oauthCfg.ClientID and oauthCfg.GetClientSecretAsEnvVar()) for each returned DB row, then pass the enriched config through RedactMCPClientConfig before returning; ensure you reference the same helpers/variables (getMCPClientsPaginated, oauthConfigsByID, GetOauthConfigsByIDs, RedactMCPClientConfig) so both paginated and non-paginated branches produce the same redacted shape.
🤖 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/tables/oauth.go`:
- Around line 16-17: Tests failing because ClientID changed from string to
*schemas.EnvVar; update all test instantiations to wrap literals with
schemas.NewEnvVar(...) when setting the ClientID field (e.g., ClientID:
schemas.NewEnvVar("client-id-public")) and update assertions to compare the
underlying string via found.ClientID.GetValue() (and guard for nil where
appropriate). Locate occurrences by referencing the struct field ClientID in
tests under encryption_test.go and follow the existing AzureKeyConfig.ClientID
pattern (using schemas.NewEnvVar and GetValue()).
In `@transports/bifrost-http/handlers/oauth2_per_user.go`:
- Line 546: The code currently sets params.Set("client_id",
templateConfig.GetResolvedClientID()) without validating the resolved value;
ensure you validate templateConfig.GetResolvedClientID() before constructing the
redirect URL and fail fast with a clear HTTP 500/4xx error if it's empty or
blank. Locate the call that sets the query param (params.Set("client_id", ...))
and add a guard that checks the returned string from
templateConfig.GetResolvedClientID(); if empty, return a descriptive server
error response (including which tenant/template if available) instead of
continuing to build or redirect to the upstream OAuth endpoint.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 831-881: The handler currently treats OAuth rotation as a no-op
only when BOTH req.OauthConfig.ClientID.ShouldPreserveStored() and
req.OauthConfig.ClientSecret.ShouldPreserveStored() are true, which lets a
redacted placeholder for one field be passed through; change the logic to
preserve each field independently: only call envVarStorageValue and assign
oauthClientID when !req.OauthConfig.ClientID.ShouldPreserveStored(), and only
assign oauthClientSecret when
!req.OauthConfig.ClientSecret.ShouldPreserveStored(); when a field is preserved,
after loading existingOauthConfig use existingOauthConfig.GetResolvedClientID()
/ GetResolvedClientSecret() (or existingOauthConfig values) to populate that
specific oauthClientID/oauthClientSecret instead of the placeholder; adjust the
validation that currently errors when oauthClientID=="" && oauthClientSecret==""
to require at least one of (resolved or provided) client_id or client_secret and
fail with a clear message referencing the missing specific field(s).
- Around line 280-327: The non-paginated code path in getMCPClientsPaginated is
returning RedactMCPClientConfig(configClient) without applying the same OAuth
enrichment done in the paginated branch, causing oauth_client_id/secret to be
present only for paginated calls; fix by replicating the batch-fetch + merge
logic used there: call h.store.ConfigStore.GetOauthConfigsByIDs to build
oauthConfigsByID, populate clientConfig.OauthClientID and
clientConfig.OauthClientSecret (using oauthCfg.ClientID and
oauthCfg.GetClientSecretAsEnvVar()) for each returned DB row, then pass the
enriched config through RedactMCPClientConfig before returning; ensure you
reference the same helpers/variables (getMCPClientsPaginated, oauthConfigsByID,
GetOauthConfigsByIDs, RedactMCPClientConfig) so both paginated and non-paginated
branches produce the same redacted shape.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efef7f3a-93d6-4f64-9bd6-67502abf8070
📒 Files selected for processing (12)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
BearTS
force-pushed
the
05-05-feat_envvar_on_oauth_config_in_mcp_settings
branch
from
34c7deb to
c18544b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/configstore/tables/oauth.go (1)
50-67:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep env-backed client secrets out of the decrypt path.
Line 53 intentionally leaves
env.*client secrets unencrypted, but the row can still end up marked encrypted whenCodeVerifieris encrypted. On the next read,AfterFindwill try to decrypt the untouchedenv.*literal and fail, which breaks the new env-var OAuth flow in encrypted deployments. Mirror the same env-ref guard inAfterFind, and keepEncryptionStatusaligned so env-only rows are not treated as pending plaintext forever.🛠️ Minimal fix
func (c *TableOauthConfig) BeforeSave(tx *gorm.DB) error { // Ensure status is valid if c.Status == "" { c.Status = "pending" } // Encrypt sensitive fields (skip if value is an env var reference — the reference itself is not sensitive) if encrypt.IsEnabled() { - encrypted := false if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { if err := encryptString(&c.ClientSecret); err != nil { return fmt.Errorf("failed to encrypt oauth client secret: %w", err) } - encrypted = true } if c.CodeVerifier != "" { if err := encryptString(&c.CodeVerifier); err != nil { return fmt.Errorf("failed to encrypt oauth code verifier: %w", err) } - encrypted = true } - if encrypted { - c.EncryptionStatus = EncryptionStatusEncrypted - } + c.EncryptionStatus = EncryptionStatusEncrypted } return nil } // AfterFind hook to decrypt sensitive fields func (c *TableOauthConfig) AfterFind(tx *gorm.DB) error { if c.EncryptionStatus == EncryptionStatusEncrypted { - if err := decryptString(&c.ClientSecret); err != nil { - return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { + if err := decryptString(&c.ClientSecret); err != nil { + return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + } } - if err := decryptString(&c.CodeVerifier); err != nil { - return fmt.Errorf("failed to decrypt oauth code verifier: %w", err) + if c.CodeVerifier != "" { + if err := decryptString(&c.CodeVerifier); err != nil { + return fmt.Errorf("failed to decrypt oauth code verifier: %w", err) + } } } return nil }Based on learnings, in
framework/configstore/tables,EncryptionStatusshould be set to"encrypted"whenever encryption is enabled so the startup batch-encryption pass does not keep reprocessing the row.🤖 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/oauth.go` around lines 50 - 67, The AfterFind decryption path must mirror the encrypt-time guards: in the AfterFind method of the oauth row type, skip attempting to decrypt ClientSecret when it has the "env." prefix (i.e., check strings.HasPrefix(c.ClientSecret, "env.") before calling decrypt/encryptString) so you don't try to decrypt env-backed literals, and ensure EncryptionStatus is set consistently when encryption is enabled (set EncryptionStatus = EncryptionStatusEncrypted when encrypt.IsEnabled() even if only CodeVerifier was encrypted or the secret is env-backed) so rows with env.* secrets are not repeatedly reprocessed; update references to ClientSecret, CodeVerifier, AfterFind, encrypt.IsEnabled(), encryptString, and EncryptionStatusEncrypted accordingly.transports/bifrost-http/handlers/mcp.go (1)
831-845:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve unchanged OAuth fields independently during rotation.
Right now only the “both placeholders” case is treated as a no-op. If a user rotates only
client_idor onlyclient_secret, the other redacted field is reused as the placeholder/empty value instead of the stored credential, which can break the next OAuth authorization flow.🛠️ Proposed fix
- if req.OauthConfig.ClientID.ShouldPreserveStored() && req.OauthConfig.ClientSecret.ShouldPreserveStored() { + preserveClientID := req.OauthConfig.ClientID.ShouldPreserveStored() + preserveClientSecret := req.OauthConfig.ClientSecret.ShouldPreserveStored() + if preserveClientID && preserveClientSecret { shouldRotateOAuthConfig = false } } if shouldRotateOAuthConfig { - oauthClientID = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientID)) - oauthClientSecret = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientSecret)) + if !preserveClientID { + oauthClientID = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientID)) + } + if !preserveClientSecret { + oauthClientSecret = strings.TrimSpace(envVarStorageValue(req.OauthConfig.ClientSecret)) + } oauthAuthorizeURL = strings.TrimSpace(req.OauthConfig.AuthorizeURL) oauthTokenURL = strings.TrimSpace(req.OauthConfig.TokenURL) oauthRegistrationURL = strings.TrimSpace(req.OauthConfig.RegistrationURL) @@ if existingConfig.OauthConfigID != nil && *existingConfig.OauthConfigID != "" { existingOauthConfig, err = h.store.ConfigStore.GetOauthConfigByID(ctx, *existingConfig.OauthConfigID) @@ } + + if preserveClientID && existingOauthConfig != nil { + oauthClientID = strings.TrimSpace(envVarStorageValue(existingOauthConfig.ClientID)) + } + if preserveClientSecret && existingOauthConfig != nil { + oauthClientSecret = strings.TrimSpace(envVarStorageValue(existingOauthConfig.GetClientSecretAsEnvVar())) + } if oauthClientID == "" { if existingOauthConfig == nil || strings.TrimSpace(existingOauthConfig.GetResolvedClientID()) == "" {Also applies to: 848-881
🤖 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/mcp.go` around lines 831 - 845, The rotation logic treats both OAuth fields as a single no-op; instead, preserve each field independently so rotating one (client_id or client_secret) doesn't overwrite the other with the redacted placeholder. Update the block that sets oauthClientID/oauthClientSecret (and uses envVarStorageValue) to: if req.OauthConfig.ClientID.ShouldPreserveStored() then set oauthClientID to the stored/existing client id (trimmed) rather than envVarStorageValue(req.OauthConfig.ClientID), and similarly if req.OauthConfig.ClientSecret.ShouldPreserveStored() set oauthClientSecret to the stored/existing secret (trimmed); keep existing behavior for AuthorizeURL/TokenURL/RegistrationURL/Scopes and still validate that at least one of oauthClientID or oauthClientSecret is non-empty before proceeding (the SendError branch), using the independent-preserved values. Ensure you reference the same identifiers: req.OauthConfig.ClientID.ShouldPreserveStored(), req.OauthConfig.ClientSecret.ShouldPreserveStored(), envVarStorageValue, oauthClientID, oauthClientSecret, shouldRotateOAuthConfig and the existing/stored OAuth values used for preservation.
🤖 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/mcp.go`:
- Around line 434-445: The helper envVarStorageValue currently reimplements
EnvVar serialization; replace its manual logic with the canonical schema helper
by calling the EnvVar.Value() (or Value) method: update envVarStorageValue(e
*schemas.EnvVar) to return "" for nil and otherwise return e.Value() so it
follows the same pattern used by mcpExternalURLToString in
framework/configstore/rdb.go and stays correct if the EnvVar schema changes.
---
Outside diff comments:
In `@framework/configstore/tables/oauth.go`:
- Around line 50-67: The AfterFind decryption path must mirror the encrypt-time
guards: in the AfterFind method of the oauth row type, skip attempting to
decrypt ClientSecret when it has the "env." prefix (i.e., check
strings.HasPrefix(c.ClientSecret, "env.") before calling decrypt/encryptString)
so you don't try to decrypt env-backed literals, and ensure EncryptionStatus is
set consistently when encryption is enabled (set EncryptionStatus =
EncryptionStatusEncrypted when encrypt.IsEnabled() even if only CodeVerifier was
encrypted or the secret is env-backed) so rows with env.* secrets are not
repeatedly reprocessed; update references to ClientSecret, CodeVerifier,
AfterFind, encrypt.IsEnabled(), encryptString, and EncryptionStatusEncrypted
accordingly.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 831-845: The rotation logic treats both OAuth fields as a single
no-op; instead, preserve each field independently so rotating one (client_id or
client_secret) doesn't overwrite the other with the redacted placeholder. Update
the block that sets oauthClientID/oauthClientSecret (and uses
envVarStorageValue) to: if req.OauthConfig.ClientID.ShouldPreserveStored() then
set oauthClientID to the stored/existing client id (trimmed) rather than
envVarStorageValue(req.OauthConfig.ClientID), and similarly if
req.OauthConfig.ClientSecret.ShouldPreserveStored() set oauthClientSecret to the
stored/existing secret (trimmed); keep existing behavior for
AuthorizeURL/TokenURL/RegistrationURL/Scopes and still validate that at least
one of oauthClientID or oauthClientSecret is non-empty before proceeding (the
SendError branch), using the independent-preserved values. Ensure you reference
the same identifiers: req.OauthConfig.ClientID.ShouldPreserveStored(),
req.OauthConfig.ClientSecret.ShouldPreserveStored(), envVarStorageValue,
oauthClientID, oauthClientSecret, shouldRotateOAuthConfig and the
existing/stored OAuth values used for preservation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd7e853e-aad8-4b8e-b27a-8c1b6e19ad2c
📒 Files selected for processing (12)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (2)
- core/schemas/mcp.go
- ui/lib/types/mcp.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/handlers/oauth2_per_user.go
- framework/configstore/store.go
- ui/lib/types/schemas.ts
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/oauth2/main.go
BearTS
force-pushed
the
05-05-feat_envvar_on_oauth_config_in_mcp_settings
branch
from
c18544b to
9dd5520
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/tables/oauth.go (1)
50-67:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRow-level encryption state is now inconsistent for env-backed client secrets.
If
ClientSecretis stored asenv.MY_SECRETandCodeVerifieris non-empty, this path marks the row as encrypted even thoughClientSecretstayed plaintext.AfterFindthen unconditionally callsdecryptString(&c.ClientSecret)and fails on theenv.*reference, so encrypted deployments can no longer load that OAuth config. The opposite case also exists: when encryption is enabled but only env-backed/plain fields remain, the row staysplain_text, so the startup batch-encryption pass can keep revisiting it.💡 Minimal fix
func (c *TableOauthConfig) BeforeSave(tx *gorm.DB) error { // Ensure status is valid if c.Status == "" { c.Status = "pending" } // Encrypt sensitive fields (skip if value is an env var reference — the reference itself is not sensitive) if encrypt.IsEnabled() { - encrypted := false + c.EncryptionStatus = EncryptionStatusEncrypted if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { if err := encryptString(&c.ClientSecret); err != nil { return fmt.Errorf("failed to encrypt oauth client secret: %w", err) } - encrypted = true } if c.CodeVerifier != "" { if err := encryptString(&c.CodeVerifier); err != nil { return fmt.Errorf("failed to encrypt oauth code verifier: %w", err) } - encrypted = true - } - if encrypted { - c.EncryptionStatus = EncryptionStatusEncrypted } } return nil } func (c *TableOauthConfig) AfterFind(tx *gorm.DB) error { if c.EncryptionStatus == EncryptionStatusEncrypted { - if err := decryptString(&c.ClientSecret); err != nil { - return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { + if err := decryptString(&c.ClientSecret); err != nil { + return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + } } if err := decryptString(&c.CodeVerifier); err != nil { return fmt.Errorf("failed to decrypt oauth code verifier: %w", err) } } return nil }Based on learnings, in
framework/configstore/tablesencryption-enabled rows should still keepEncryptionStatusaligned with the encryption flow, andAfterFindhooks must guard decrypt calls to avoid decrypting values that were intentionally left unencrypted.🤖 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/oauth.go` around lines 50 - 67, The row-level encryption is inconsistent because AfterFind unconditionally calls decryptString on fields like ClientSecret while the encrypt path leaves env.* references unencrypted; fix by (1) in the AfterFind hook (method AfterFind on the oauth table struct) guard every decryptString(&c.ClientSecret) and decryptString(&c.CodeVerifier) call with strings.HasPrefix(field, "env.") checks so env.* values are skipped, and (2) ensure the encrypting block that calls encryptString (the code around encrypt.IsEnabled(), encryptString and the encrypted flag) only sets c.EncryptionStatus = EncryptionStatusEncrypted when actual encryption was performed (i.e., encrypted == true) so the persisted EncryptionStatus accurately reflects whether any non-env sensitive fields were encrypted.
🤖 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.
Outside diff comments:
In `@framework/configstore/tables/oauth.go`:
- Around line 50-67: The row-level encryption is inconsistent because AfterFind
unconditionally calls decryptString on fields like ClientSecret while the
encrypt path leaves env.* references unencrypted; fix by (1) in the AfterFind
hook (method AfterFind on the oauth table struct) guard every
decryptString(&c.ClientSecret) and decryptString(&c.CodeVerifier) call with
strings.HasPrefix(field, "env.") checks so env.* values are skipped, and (2)
ensure the encrypting block that calls encryptString (the code around
encrypt.IsEnabled(), encryptString and the encrypted flag) only sets
c.EncryptionStatus = EncryptionStatusEncrypted when actual encryption was
performed (i.e., encrypted == true) so the persisted EncryptionStatus accurately
reflects whether any non-env sensitive fields were encrypted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7c40cec-2694-4460-8dc2-61e07439784c
📒 Files selected for processing (13)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (3)
- ui/lib/types/schemas.ts
- transports/bifrost-http/handlers/oauth2_per_user.go
- core/schemas/mcp.go
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/lib/types/mcp.ts
- transports/bifrost-http/lib/config.go
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/oauth2/main.go
- framework/configstore/rdb.go
- transports/bifrost-http/handlers/mcp.go
BearTS
force-pushed
the
05-05-feat_envvar_on_oauth_config_in_mcp_settings
branch
from
9dd5520 to
aa9f119
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/tables/oauth.go (1)
50-67:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle env-var
ClientSecretconsistently across save/load encryption paths.
BeforeSaveskips encryption forClientSecretwhen it isenv.*, butAfterFindstill decryptsClientSecretwheneverEncryptionStatusis"encrypted". IfCodeVerifieris encrypted in the same row, reads can fail by attempting to decrypt a plaintext env reference.🔧 Proposed fix
func (c *TableOauthConfig) BeforeSave(tx *gorm.DB) error { @@ - if encrypt.IsEnabled() { - encrypted := false + if encrypt.IsEnabled() { + // Mark as encrypted to avoid repeated startup reprocessing of this row. + c.EncryptionStatus = EncryptionStatusEncrypted + encrypted := false if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { if err := encryptString(&c.ClientSecret); err != nil { return fmt.Errorf("failed to encrypt oauth client secret: %w", err) } encrypted = true } if c.CodeVerifier != "" { if err := encryptString(&c.CodeVerifier); err != nil { return fmt.Errorf("failed to encrypt oauth code verifier: %w", err) } encrypted = true } - if encrypted { - c.EncryptionStatus = EncryptionStatusEncrypted - } + _ = encrypted } return nil } @@ func (c *TableOauthConfig) AfterFind(tx *gorm.DB) error { if c.EncryptionStatus == EncryptionStatusEncrypted { - if err := decryptString(&c.ClientSecret); err != nil { - return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { + if err := decryptString(&c.ClientSecret); err != nil { + return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + } } - if err := decryptString(&c.CodeVerifier); err != nil { - return fmt.Errorf("failed to decrypt oauth code verifier: %w", err) + if c.CodeVerifier != "" { + if err := decryptString(&c.CodeVerifier); err != nil { + return fmt.Errorf("failed to decrypt oauth code verifier: %w", err) + } } } return nil }Based on learnings: in
framework/configstore/tables, rows should still be marked encrypted when encryption is enabled to avoid startup batch reprocessing loops.Also applies to: 73-80
🤖 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/oauth.go` around lines 50 - 67, BeforeSave currently skips encrypting ClientSecret when it’s an env.* reference but only sets EncryptionStatus = EncryptionStatusEncrypted if at least one field was actually encrypted; change it so that when encrypt.IsEnabled() is true you still set c.EncryptionStatus = EncryptionStatusEncrypted (even if ClientSecret was skipped) to avoid batch reprocessing loops; and in AfterFind, before calling decryptString on c.ClientSecret, guard with a check for strings.HasPrefix(c.ClientSecret, "env.") and skip decryption for env refs so you don’t attempt to decrypt a plaintext env reference (apply this logic to ClientSecret and mirror for any other env-ref sensitive fields like CodeVerifier if relevant).
🤖 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/mcp.go`:
- Around line 819-824: The no-op shortcut currently sets shouldRotateOAuthConfig
= false when only req.OauthConfig.ClientID.ShouldPreserveStored() &&
req.OauthConfig.ClientSecret.ShouldPreserveStored(), which allows updates to
authorize_url/token_url/registration_url/scopes to be dropped; change the logic
so rotation is skipped only if ALL relevant OAuth config fields are "preserve
stored" (ClientID, ClientSecret, AuthorizeURL, TokenURL, RegistrationURL,
Scopes). Update the condition that sets shouldRotateOAuthConfig (the block
referencing req.OauthConfig.ClientID, ClientSecret and shouldRotateOAuthConfig)
to check preservation for every OAuth field, and keep shouldRotateOAuthConfig =
true if any of those fields are being updated.
---
Outside diff comments:
In `@framework/configstore/tables/oauth.go`:
- Around line 50-67: BeforeSave currently skips encrypting ClientSecret when
it’s an env.* reference but only sets EncryptionStatus =
EncryptionStatusEncrypted if at least one field was actually encrypted; change
it so that when encrypt.IsEnabled() is true you still set c.EncryptionStatus =
EncryptionStatusEncrypted (even if ClientSecret was skipped) to avoid batch
reprocessing loops; and in AfterFind, before calling decryptString on
c.ClientSecret, guard with a check for strings.HasPrefix(c.ClientSecret, "env.")
and skip decryption for env refs so you don’t attempt to decrypt a plaintext env
reference (apply this logic to ClientSecret and mirror for any other env-ref
sensitive fields like CodeVerifier if relevant).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79e2312a-a4c0-484b-97f8-4775fc06c112
📒 Files selected for processing (13)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (5)
- transports/bifrost-http/handlers/oauth2_per_user.go
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- core/schemas/mcp.go
- ui/lib/types/mcp.ts
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/oauth2.go
- framework/configstore/store.go
- framework/oauth2/main.go
- ui/lib/types/schemas.ts
BearTS
force-pushed
the
05-05-feat_envvar_on_oauth_config_in_mcp_settings
branch
from
aa9f119 to
b408d70
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
1095-1115:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid returning 500 after earlier parts of the edit have already been committed.
By the time this branch runs, the handler has already updated the MCP client in DB/memory earlier in the request. If
InitiateOAuthFloworStorePendingMCPClientfails here, the caller gets an error even though part of the update is already live. Please either start/store the OAuth flow before mutating the client, or roll the prior changes back on failure.🤖 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/mcp.go` around lines 1095 - 1115, The handler currently updates the MCP client then calls h.oauthHandler.InitiateOAuthFlow and h.oauthHandler.StorePendingMCPClient, which can leave a partially-applied update visible on failure; either move the OAuth initiation/storage before mutating the live client or implement a rollback on failure. Specifically, call h.oauthHandler.InitiateOAuthFlow(...) and h.oauthHandler.StorePendingMCPClient(...) (using flowInitiation.OauthConfigID and the pendingConfig) before applying the earlier MCP client mutation (the code that writes schemasConfig to the live client), or, if you must keep the order, add a compensating revert that undoes the prior update (use the original schemasConfig snapshot) when InitiateOAuthFlow or StorePendingMCPClient returns an error and ensure errors are handled and logged before returning the response.
🤖 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/mcp.go`:
- Around line 280-294: The non-paginated GET handler that builds
schemas.MCPClientConfig never populates the OAuth fields, so ensure you
replicate the batch-fetch logic (oauthConfigsByID map populated via
ConfigStore.GetOauthConfigsByIDs) used by getMCPClientsPaginated and, when
building each runtime config from a db client, set the config's OauthClientID
and OauthClientSecret metadata from the corresponding oauthConfigsByID entry
(apply the same redaction/placeholder logic you use in getMCPClientsPaginated).
Update the code paths that construct schemas.MCPClientConfig for the plain
/api/mcp/clients response (the block that currently creates runtime configs from
dbClients) to use oauthConfigsByID and populate OauthClientID/OauthClientSecret
so both paginated and non-paginated endpoints return consistent redacted OAuth
metadata.
In `@transports/bifrost-http/handlers/oauth2.go`:
- Around line 213-224: The code currently converts EnvVar credentials to empty
strings by ignoring errors from req.ClientID.Value() and
req.ClientSecret.Value(); update the logic around req.ClientID.Value() and
req.ClientSecret.Value() so you check the returned error and do not silently
assign "" — if Value() returns an error or nil (unresolved), return or propagate
a clear configuration error (or validation error) from the handler indicating
missing/invalid client_id or client_secret instead of continuing with blank
credentials; locate the assignment of clientID and clientSecret in oauth2.go and
replace the silent ignores with explicit error handling that surfaces
configuration problems to the caller.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 1095-1115: The handler currently updates the MCP client then calls
h.oauthHandler.InitiateOAuthFlow and h.oauthHandler.StorePendingMCPClient, which
can leave a partially-applied update visible on failure; either move the OAuth
initiation/storage before mutating the live client or implement a rollback on
failure. Specifically, call h.oauthHandler.InitiateOAuthFlow(...) and
h.oauthHandler.StorePendingMCPClient(...) (using flowInitiation.OauthConfigID
and the pendingConfig) before applying the earlier MCP client mutation (the code
that writes schemasConfig to the live client), or, if you must keep the order,
add a compensating revert that undoes the prior update (use the original
schemasConfig snapshot) when InitiateOAuthFlow or StorePendingMCPClient returns
an error and ensure errors are handled and logged before returning the response.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b8846cf-47fb-48f2-8d90-a0e3593e508b
📒 Files selected for processing (13)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (3)
- framework/oauth2/main.go
- transports/bifrost-http/lib/config.go
- framework/configstore/tables/oauth.go
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/handlers/oauth2_per_user.go
- ui/lib/types/schemas.ts
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- ui/lib/types/mcp.ts
- core/schemas/mcp.go
BearTS
force-pushed
the
05-05-feat_envvar_on_oauth_config_in_mcp_settings
branch
from
b408d70 to
9f648fa
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/tables/oauth.go (1)
50-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
EncryptionStatushandling is inconsistent for env-backed secrets.When encryption is enabled, this path only sets
EncryptionStatusEncryptedif an actual encryption occurred. For env-backedClientSecret, that can leave rows asplain_text(or keep stale status), which risks repeated encryption sweeps and inconsistent decrypt behavior inAfterFind.Suggested patch
func (c *TableOauthConfig) BeforeSave(tx *gorm.DB) error { @@ // Encrypt sensitive fields (skip if value is an env var reference — the reference itself is not sensitive) if encrypt.IsEnabled() { - encrypted := false if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { if err := encryptString(&c.ClientSecret); err != nil { return fmt.Errorf("failed to encrypt oauth client secret: %w", err) } - encrypted = true } if c.CodeVerifier != "" { if err := encryptString(&c.CodeVerifier); err != nil { return fmt.Errorf("failed to encrypt oauth code verifier: %w", err) } - encrypted = true - } - if encrypted { - c.EncryptionStatus = EncryptionStatusEncrypted } + c.EncryptionStatus = EncryptionStatusEncrypted } return nil } @@ func (c *TableOauthConfig) AfterFind(tx *gorm.DB) error { if c.EncryptionStatus == EncryptionStatusEncrypted { - if err := decryptString(&c.ClientSecret); err != nil { - return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + if c.ClientSecret != "" && !strings.HasPrefix(c.ClientSecret, "env.") { + if err := decryptString(&c.ClientSecret); err != nil { + return fmt.Errorf("failed to decrypt oauth client secret: %w", err) + } } - if err := decryptString(&c.CodeVerifier); err != nil { - return fmt.Errorf("failed to decrypt oauth code verifier: %w", err) + if c.CodeVerifier != "" { + if err := decryptString(&c.CodeVerifier); err != nil { + return fmt.Errorf("failed to decrypt oauth code verifier: %w", err) + } } } return nil }Based on learnings: “ensure
EncryptionStatusis set toencryptedwheneverencrypt.IsEnabled()is true (even if no fields are actually encrypted) … and guard decrypt calls inAfterFind.”Also applies to: 73-81
🤖 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/oauth.go` around lines 50 - 67, When encryption is enabled (encrypt.IsEnabled()) ensure you always set c.EncryptionStatus = EncryptionStatusEncrypted even if no fields were actually encrypted (e.g., ClientSecret starting with "env.") by moving or adding the status assignment outside the encrypted flag check; additionally, update the AfterFind decryption logic to only attempt decryptString on ClientSecret/CodeVerifier when c.EncryptionStatus == EncryptionStatusEncrypted to avoid decrypting env-backed or plain-text rows. Use the existing symbols ClientSecret, CodeVerifier, EncryptionStatus, EncryptionStatusEncrypted, encrypt.IsEnabled(), encryptString, and the AfterFind method to locate and apply these changes.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/oauth2_per_user.go (1)
544-551:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the trimmed
client_idwhen setting query params.Line 551 still uses the untrimmed
resolvedClientID, so accidental leading/trailing spaces in env-backed values can still cause upstream auth failures.Suggested patch
- resolvedClientID := templateConfig.GetResolvedClientID() - if strings.TrimSpace(resolvedClientID) == "" { + resolvedClientID := strings.TrimSpace(templateConfig.GetResolvedClientID()) + if resolvedClientID == "" { SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("OAuth client_id for config %q could not be resolved — check that the referenced environment variable is set", templateConfig.ID)) return } @@ - params.Set("client_id", resolvedClientID) + params.Set("client_id", resolvedClientID)🤖 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_per_user.go` around lines 544 - 551, The code obtains resolvedClientID via templateConfig.GetResolvedClientID() but only checks the trimmed value; however params.Set("client_id", resolvedClientID) still uses the untrimmed string—trim the resolvedClientID once (e.g., assign trimmed := strings.TrimSpace(resolvedClientID)) and use that trimmed value when setting params ("client_id") and in the empty-check logic (replace uses of resolvedClientID in this block with the trimmed variable) so accidental whitespace is removed before building the URL query.
🤖 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.
Outside diff comments:
In `@framework/configstore/tables/oauth.go`:
- Around line 50-67: When encryption is enabled (encrypt.IsEnabled()) ensure you
always set c.EncryptionStatus = EncryptionStatusEncrypted even if no fields were
actually encrypted (e.g., ClientSecret starting with "env.") by moving or adding
the status assignment outside the encrypted flag check; additionally, update the
AfterFind decryption logic to only attempt decryptString on
ClientSecret/CodeVerifier when c.EncryptionStatus == EncryptionStatusEncrypted
to avoid decrypting env-backed or plain-text rows. Use the existing symbols
ClientSecret, CodeVerifier, EncryptionStatus, EncryptionStatusEncrypted,
encrypt.IsEnabled(), encryptString, and the AfterFind method to locate and apply
these changes.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/oauth2_per_user.go`:
- Around line 544-551: The code obtains resolvedClientID via
templateConfig.GetResolvedClientID() but only checks the trimmed value; however
params.Set("client_id", resolvedClientID) still uses the untrimmed string—trim
the resolvedClientID once (e.g., assign trimmed :=
strings.TrimSpace(resolvedClientID)) and use that trimmed value when setting
params ("client_id") and in the empty-check logic (replace uses of
resolvedClientID in this block with the trimmed variable) so accidental
whitespace is removed before building the URL query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73d14ae3-4f6e-4075-b6f2-8ece9b5533d3
📒 Files selected for processing (13)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (2)
- framework/configstore/store.go
- core/schemas/mcp.go
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/lib/types/mcp.ts
- transports/bifrost-http/lib/config.go
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- transports/bifrost-http/handlers/mcp.go
- framework/oauth2/main.go
- ui/lib/types/schemas.ts
BearTS
changed the title
EnvVar references for MCP OAuth client_id/client_secret with redacted GET responses and batch config fetchingEnvVar references for MCP OAuth client_id/client_secret with redacted GET responses and batch config fetching
BearTS
force-pushed
the
05-05-feat_envvar_on_oauth_config_in_mcp_settings
branch
from
9f648fa to
41b9996
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/mcp.go`:
- Around line 159-173: The batch OAuth lookup currently swallows errors and
returns 200 with missing OauthClientID/Secret; in both getMCPClients and
getMCPClientsPaginated replace the silent-fail behavior around
h.store.ConfigStore.GetOauthConfigsByIDs: check the (fetched, err) result and if
err != nil return an appropriate HTTP/internal error (or propagate the error up)
instead of continuing with an empty oauthConfigsByID map; keep the existing
oauthConfigsByID variable and only set it when fetched is non-nil, but ensure
the request fails fast (500 or wrapped error) so DB read failures don't silently
remove OAuth metadata from responses.
- Around line 847-848: The code unconditionally overwrites oauthClientSecret
with req.OauthConfig.ClientSecret which clears stored secrets when only the
client_id is rotated; update the assignment logic so you only replace existing
values when the request provides them: for oauthClientID and oauthClientSecret
check whether req.OauthConfig.ClientID / ClientSecret are non-empty (or non-nil)
before assigning (e.g. only set oauthClientSecret = req.OauthConfig.ClientSecret
if ClientSecret is provided), preserving previous oauthClientSecret when the
request omits it; apply the same conditional-assignment fix to the other similar
blocks referenced around the oauth handling (the blocks that use oauthClientID
and oauthClientSecret at the other occurrences).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecd65b0f-496c-4999-9648-73d75e302d23
📒 Files selected for processing (13)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (3)
- ui/lib/types/mcp.ts
- transports/bifrost-http/handlers/oauth2.go
- core/schemas/mcp.go
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/handlers/oauth2_per_user.go
- framework/configstore/tables/oauth.go
- transports/bifrost-http/lib/config.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/configstore/rdb.go
- framework/oauth2/main.go
BearTS
force-pushed
the
05-05-feat_envvar_on_oauth_config_in_mcp_settings
branch
from
41b9996 to
aabd301
Compare
BearTS
changed the title
EnvVar references for MCP OAuth client_id/client_secret with redacted GET responses and batch config fetching EnvVar references for MCP OAuth client_id/client_secret with redacted GET responses and batch config fetching
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
843-850:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the no-op OAuth check before rejecting disabled updates.
shouldRotateOAuthConfigis stilltruewhen thereq.Disabledguard runs, so a request that only echoes the redactedoauth_configfrom GET is rejected as a credential rotation. That blocks legitimate disable/edit calls unless clients know to stripoauth_configentirely.💡 Minimal fix
- if shouldRotateOAuthConfig && req.Disabled { - SendError(ctx, fasthttp.StatusBadRequest, "oauth credentials cannot be rotated while disabling a client; send these as two separate requests") - return - } if shouldRotateOAuthConfig { if req.OauthConfig.ClientID.ShouldPreserveStored() && req.OauthConfig.ClientSecret.ShouldPreserveStored() { shouldRotateOAuthConfig = false } } + if shouldRotateOAuthConfig && req.Disabled { + SendError(ctx, fasthttp.StatusBadRequest, "oauth credentials cannot be rotated while disabling a client; send these as two separate requests") + return + }🤖 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/mcp.go` around lines 843 - 850, The disabled-update guard is rejecting requests that only echo redacted oauth_config because shouldRotateOAuthConfig is still true; evaluate the no-op check first by inspecting req.OauthConfig.ClientID.ShouldPreserveStored() and req.OauthConfig.ClientSecret.ShouldPreserveStored() and set shouldRotateOAuthConfig = false when both indicate preservation before the if req.Disabled branch, so the subsequent SendError call only triggers for real rotations and not for redacted/no-op oauth_config updates.
🧹 Nitpick comments (1)
ui/app/workspace/mcp-registry/views/mcpClientForm.tsx (1)
33-33: ⚡ Quick winUse the existing
emptyEnvVarutility from@/lib/utils/envVarForm.This duplicates the
emptyEnvVar()factory function already exported fromui/lib/utils/envVarForm.ts. Import and use the utility to stay DRY and avoid potential mutation issues with a shared object reference.Suggested fix
import { parseArrayFromText } from "@/lib/utils/array"; +import { emptyEnvVar } from "@/lib/utils/envVarForm"; import { RbacOperation, RbacResource, useRbac } from "@enterprise/lib";Then update usages to call the factory:
-const emptyEnvVar: EnvVar = { value: "", env_var: "", from_env: false }; - const emptyOAuthConfig: OAuthConfig = { - client_id: emptyEnvVar, - client_secret: emptyEnvVar, + client_id: emptyEnvVar(), + client_secret: emptyEnvVar(), authorize_url: "",And similarly for
emptyForm.connection_stringand line 163.Based on learnings: "In the UI codebase, prefer alias imports using
@/... over relative imports" and the existence ofemptyEnvVarinui/lib/utils/envVarForm.ts.🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx` at line 33, Replace the duplicated constant emptyEnvVar in mcpClientForm.tsx with the factory import from '@/lib/utils/envVarForm' and call it where needed (use emptyEnvVar() instead of the shared object); specifically remove the local const emptyEnvVar and add an import of the factory, then update places that initialize or assign env var objects (including emptyForm.connection_string and the usage around line 163) to call emptyEnvVar() so each form gets a fresh object instance.
🤖 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.
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 843-850: The disabled-update guard is rejecting requests that only
echo redacted oauth_config because shouldRotateOAuthConfig is still true;
evaluate the no-op check first by inspecting
req.OauthConfig.ClientID.ShouldPreserveStored() and
req.OauthConfig.ClientSecret.ShouldPreserveStored() and set
shouldRotateOAuthConfig = false when both indicate preservation before the if
req.Disabled branch, so the subsequent SendError call only triggers for real
rotations and not for redacted/no-op oauth_config updates.
---
Nitpick comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Line 33: Replace the duplicated constant emptyEnvVar in mcpClientForm.tsx with
the factory import from '@/lib/utils/envVarForm' and call it where needed (use
emptyEnvVar() instead of the shared object); specifically remove the local const
emptyEnvVar and add an import of the factory, then update places that initialize
or assign env var objects (including emptyForm.connection_string and the usage
around line 163) to call emptyEnvVar() so each form gets a fresh object
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 767d0619-48eb-49e0-a894-2b07d0b77b61
📒 Files selected for processing (13)
core/schemas/mcp.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/oauth2.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (4)
- ui/lib/types/schemas.ts
- core/schemas/mcp.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/oauth2/main.go
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/handlers/oauth2_per_user.go
- transports/bifrost-http/handlers/oauth2.go
- framework/configstore/store.go
- framework/configstore/rdb.go
- framework/configstore/tables/oauth.go
- ui/lib/types/mcp.ts
akshaydeo
approved these changes
May 5, 2026
Contributor
Merge activity
|
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
OAuth
client_idandclient_secretfields now support environment variable references (e.g.env.MY_VAR) in addition to plain string values. Previously these fields were stored and handled as raw strings, which meant secrets had to be provided as literal values. This change allows credentials to be injected from environment variables at runtime, keeps references intact in the database, and surfaces redacted credential metadata back to callers on GET requests.Changes
TableOauthConfig.ClientIDchanged fromstringto*schemas.EnvVarso env var references are preserved in storage and resolved at runtime viaGetResolvedClientID()/GetResolvedClientSecret()helpers.OAuthConfigRequestandOAuthConfigUpdatein the HTTP transport now use*schemas.EnvVarinstead ofstringforclient_idandclient_secret.envVarStorageValue()helper added to extract the storage representation (env var reference or resolved value) before passing to the OAuth flow initiator.MCPClientConfiggains two new read-only fields —OauthClientIDandOauthClientSecret— populated on GET responses with redacted credential metadata fetched from the linked OAuth config.GetOauthConfigsByIDsbatch query was added to avoid N+1 database calls when listing MCP clients with OAuth configs.BeforeSavehook skips encryption forClientSecretvalues that are already env var references, since the reference string itself is not sensitive.client_idandclient_secretreplaced plainInputcomponents withEnvVarInput, enabling env var selection. The edit sheet pre-populates these fields from the redacted GET response and usesdirtyFieldsto determine whether rotation should be triggered.OAuthConfig.client_id,OAuthConfig.client_secret,OAuthConfigUpdate.client_id, andOAuthConfigUpdate.client_secretare allEnvVarinstead ofstring.Type of change
Affected areas
How to test
client_idas an env var reference (e.g.env.OAUTH_CLIENT_ID) andclient_secretasenv.OAUTH_CLIENT_SECRET.oauth_client_idandoauth_client_secretare returned as redactedEnvVarobjects.EnvVarInputcomponents appear for both fields and correctly reflect the stored env var reference.Breaking changes
TableOauthConfig.ClientIDchanged fromstringto*schemas.EnvVar. Any code that directly accessed.ClientIDas a string must be updated to use.GetResolvedClientID()or dereference theEnvVar. TheOAuthConfigRequestandOAuthConfigUpdateAPI request bodies now expectclient_idandclient_secretasEnvVarobjects rather than plain strings; API clients sending raw strings will need to wrap values in theEnvVarstructure.Related issues
N/A
Security considerations
BeforeSave, since the reference itself (e.g.env.MY_VAR) is not sensitive.Redacted()path, so actual secret values are never exposed in API responses.Checklist
docs/contributing/README.mdand followed the guidelines