Conversation
|
|
This was referenced Apr 27, 2026
Collaborator
Author
18 tasks
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughAdds a new configuration field Changes
Sequence DiagramsequenceDiagram
participant UI as User/UI
participant Handler as Config Handler
participant Store as Config Store
participant DB as Database
participant OAuth as OAuth/Metadata Handlers
participant Client as Downstream MCP Client
UI->>Handler: Submit updated mcp_external_base_url
Handler->>Store: UpdateClientConfig(updatedConfig)
Store->>DB: Persist MCPExternalBaseURL
DB-->>Store: Persisted
Store-->>Handler: Update confirmed
Handler-->>UI: Success
Client->>OAuth: Request OAuth callback / metadata
OAuth->>Store: GetMCPExternalBaseURL()
Store-->>OAuth: External base URL (resolved from env-var or literal)
OAuth->>OAuth: BuildBaseURL(ctx, externalBaseURL)
OAuth-->>Client: Redirect URI / metadata with resolved base URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 unit tests (beta)
Comment |
Contributor
Confidence Score: 5/5Safe to merge; no P0/P1 issues found All previously flagged issues (JSON tag mismatch, empty-host guard) are resolved in the current HEAD. The only remaining finding is a P2 UX concern about silent fallback on malformed override URLs. Core DB round-trip, env var resolution, and request-path logic are correct. No files require special attention Important Files Changed
Reviews (4): Last reviewed commit: "feat: add external base url support in p..." | 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/deployment-guides/config-json/client.mdx`:
- Line 83: The UI navigation path in the sentence that currently reads "Config →
Client Settings → Proxy Settings" is incorrect; update that text to point to the
actual location in the interface (the MCP settings view) so users are directed
correctly—for example replace the "Config → Client Settings → Proxy Settings"
fragment with the correct path referencing the MCP settings view (e.g., "Config
→ MCP Settings" or the exact menu label used in the app) within the line in
docs/deployment-guides/config-json/client.mdx.
In `@docs/openapi/schemas/management/config.yaml`:
- Around line 97-112: Update the Go struct tag for
ClientConfig.MCPExternalBaseURL so its JSON tag is "mcp_external_base_url"
(replace the incorrect "external_base_url") so serialized API output matches the
OpenAPI schema, and in the OpenAPI schema for mcp_external_base_url add
"additionalProperties: false" to the object branch to match config.schema.json
and prevent extra fields.
In `@framework/configstore/clientconfig.go`:
- Line 78: The struct field MCPExternalBaseURL has the wrong JSON tag key;
update its tag from json:"external_base_url,omitempty" to
json:"mcp_external_base_url,omitempty" so encoding/decoding uses the correct
config key (modify the tag on the MCPExternalBaseURL field in clientconfig.go).
- Around line 317-319: The hash currently uses only
c.MCPExternalBaseURL.GetValue(), which loses whether the value came from an env
var; change the hashing to include a source prefix so env-backed and literal
inputs differ (e.g., write "externalBaseURL:env:<value>" when the entry is from
an environment variable and "externalBaseURL:val:<value>" when it is a literal).
Concretely, update the block that references c.MCPExternalBaseURL (use its
existing helper such as IsSet()/IsFromEnv()/IsEnv() or equivalent) to choose the
"env:" vs "val:" prefix before hashing, then write the prefixed string to hash
instead of only GetValue().
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 653-661: BuildBaseURL currently accepts any non-empty
externalBaseURL (e.g., "/" or " /"), producing invalid base URLs; instead
validate the override: trim whitespace, reject values that are just "/" or lack
an absolute scheme/host, and only use the override when it is a well-formed
absolute URL (or at least starts with "http://" or "https://"); otherwise fall
back to the existing ctx-derived logic (using ctx.IsTLS and ctx.Host()). Update
BuildBaseURL to sanitize externalBaseURL, confirm its scheme/host (e.g., via
url.Parse or strings.HasPrefix checks), trim trailing slashes, and only return
the override when valid.
In `@ui/app/workspace/config/views/mcpView.tsx`:
- Around line 341-349: Add a stable test selector prop to the new EnvVarInput so
E2E tests can target it: update the EnvVarInput instance
(id="external-base-url", value bound to localConfig.mcp_external_base_url and
onChange handler) to include a data-testid prop with a clear name such as
"mcp-external-base-url-input"; ensure the prop is passed directly to the
EnvVarInput component so it appears on the rendered input element and respect
existing disabled logic (disabled={!hasSettingsUpdateAccess}).
🪄 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: 1fecddfb-a2af-4568-9335-a01a9bc6115b
📒 Files selected for processing (21)
docs/deployment-guides/config-json/client.mdxdocs/deployment-guides/config-json/schema-reference.mdxdocs/openapi/schemas/management/config.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/oauth2_metadata.gotransports/bifrost-http/handlers/oauth2_per_user.gotransports/bifrost-http/handlers/webrtc_realtime_test.gotransports/bifrost-http/handlers/wsresponses_test.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/config.schema.jsonui/app/workspace/config/views/mcpView.tsxui/lib/types/config.ts
Pratham-Mishra04
force-pushed
the
04-27-feat_add_external_base_url_support_in_per_user_oauth_flows
branch
from
4c5ed3a to
4c22bb3
Compare
Contributor
Merge activity
|
Pratham-Mishra04
force-pushed
the
04-27-feat_add_external_base_url_support_in_per_user_oauth_flows
branch
from
4c22bb3 to
6df9fea
Compare
Pratham-Mishra04
force-pushed
the
04-27-fix_list_models_log_sheet_ui_fixes
branch
from
e1baf67 to
930c72d
Compare
akshaydeo
changed the base branch from
04-27-fix_list_models_log_sheet_ui_fixes
to
graphite-base/3088
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
akshaydeo
force-pushed
the
04-27-feat_add_external_base_url_support_in_per_user_oauth_flows
branch
from
6df9fea to
f23102b
Compare
akshaydeo
deleted the
04-27-feat_add_external_base_url_support_in_per_user_oauth_flows
branch
18 tasks
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
3 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
When Bifrost runs behind a reverse proxy, OAuth callback URLs and well-known discovery endpoints (
/.well-known/oauth-authorization-server,/.well-known/oauth-protected-resource) were being built from the incomingHostheader, which reflects the proxy's internal address rather than its public-facing URL. This PR introducesmcp_external_base_urlto override that derived URL with the proxy's public base URL.Changes
mcp_external_base_urlfield toClientConfig(Go struct, DB table, JSON schema, OpenAPI schema, and config.schema.json) supporting both plain URL strings and env var references ("env.MY_VAR").lib.BuildBaseURL(ctx, externalBaseURL)as a single helper that centralizes base URL resolution — using the configured override when present, falling back to theX-Forwarded-Proto/Host-derived URL otherwise. Replaced all inline scheme+host construction acrossmcp.go,mcpserver.go,oauth2_metadata.go,oauth2_per_user.go, andctx.gowith calls to this helper.migrationAddMCPExternalBaseURLColumn) to add themcp_external_base_urlcolumn to the client config table.GetMCPExternalBaseURL()into theHandlerStoreinterface and implemented it onConfig; updated all test stubs accordingly.EnvVarInput, with change detection and RBAC gating.docs/deployment-guides/config-json/client.mdxandschema-reference.mdxwith usage examples for both plain URL and env var forms.Type of change
Affected areas
How to test
Manual validation:
Hostheader.mcp_external_base_urlinconfig.jsonor via the UI (Config → Client Settings → Proxy Settings):{ "client": { "mcp_external_base_url": "https://api.yourcompany.com" } }/.well-known/oauth-authorization-serverissuer both reflecthttps://api.yourcompany.comrather than the internal host.mcp_external_base_urlto"env.BIFROST_EXTERNAL_URL", export the env var, and confirm the resolved URL is used.Host-header-derived URL.New config field:
mcp_external_base_urlstring | EnvVar"env.MY_VAR"syntax.Breaking changes
Related issues
Security considerations
The
mcp_external_base_urlvalue is used to construct OAuth redirect URIs and discovery metadata URLs. Only trusted administrators with config update access can set this value (RBAC-gated in the UI; management API access required via API). Env var references are resolved server-side and never exposed to clients.Checklist
docs/contributing/README.mdand followed the guidelines