feat(provider): support custom Base URL for LiteLLM/built-in providers - #729
feat(provider): support custom Base URL for LiteLLM/built-in providers#729xp880906 wants to merge 3 commits into
Conversation
|
|
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
[bug · medium] 📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if isPreset && strings.TrimSpace(result.url) != "" && result.url != preset.BaseURL { | ||
| entry.URL = result.url |
There was a problem hiding this comment.
[bug]
Inconsistent URL trimming in validation and storage logic. The code trims result.url to check if it's empty, but uses the untrimmed version when comparing to preset.BaseURL and when storing in entry.URL. This can cause:
- URLs with trailing/leading whitespace to be incorrectly treated as different from the preset default
- Whitespace-polluted URLs being persisted to the config file
For example, if result.url = "https://api.example.com " and preset.BaseURL = "https://api.example.com", the condition would evaluate as true (non-empty after trim, but different when untrimmed), and the whitespace-containing URL would be saved.
Should trim the URL once and use the trimmed version consistently:
Suggestion:
| if isPreset && strings.TrimSpace(result.url) != "" && result.url != preset.BaseURL { | |
| entry.URL = result.url | |
| trimmedURL := strings.TrimSpace(result.url) | |
| if isPreset && trimmedURL != "" && trimmedURL != preset.BaseURL { | |
| entry.URL = trimmedURL |
| if isPreset && strings.TrimSpace(result.url) != "" && result.url != preset.BaseURL { | ||
| entry.URL = result.url | ||
| } else { | ||
| entry.URL = "" | ||
| } |
There was a problem hiding this comment.
[security]
Security Vulnerability: SSRF and Command Injection via Unvalidated URL Input
The result.url from the TUI is persisted directly to the config file without any validation. This creates multiple security risks:
-
SSRF (Server-Sent Request Forgery): User can enter internal URLs like
http://169.254.169.254/latest/meta-data/(AWS metadata endpoint),http://localhost:8080/admin, or private network addresses. These URLs will be used by the LLM client for API calls. -
Command/Shell Injection: URLs can contain shell metacharacters (e.g.,
;,|,&,$()) that might be exploited if the URL is ever passed to shell commands or curl. -
Credential Exfiltration: URLs can be crafted to forward requests to attacker-controlled servers, potentially leaking API keys sent in headers.
-
No Scheme Validation: Accepts
file://,javascript:, or other dangerous schemes.
Required Fixes:
- Validate URL scheme (only
http://orhttps://) - Reject private IP ranges (RFC 1918: 10.x.x.x, 172.16-31.x.x, 192.168.x.x)
- Reject localhost and loopback addresses (127.0.0.1, ::1)
- Reject link-local addresses (169.254.x.x)
- Block known cloud metadata endpoints (169.254.169.254)
- Validate URL format with
url.Parse()and reject malformed URLs - Consider allowlisting for production deployments
| // Pre-fill the official Base URL input with the effective URL: a | ||
| // configured override (entry.URL) if present, else the preset default. | ||
| // Editing this value later overrides preset.BaseURL in the resolver. | ||
| selected := providers[m.officialIdx] | ||
| effectiveURL := selected.BaseURL | ||
| if entry, ok := cfg.Providers[cfg.Provider]; ok && entry.URL != "" { | ||
| effectiveURL = entry.URL | ||
| } | ||
| m.officialURLInput.SetValue(effectiveURL) |
There was a problem hiding this comment.
[maintainability · low]
Dead code: this pre-fill of officialURLInput is always overwritten before the user ever sees it. When the user reaches the Base URL step (via handleEnter at stepModel), loadOfficialURL() is called first, which re-sets the value using the same logic but via currentProvider() and m.existingCfg. If the user presses Esc from the API key step back to Base URL, the existing value in the input is preserved (not re-loaded from here). Removing this block avoids confusion and keeps the pre-fill logic in a single place (loadOfficialURL).
| func (m providerTUIModel) effectiveBaseURL() string { | ||
| if m.activeTab == tabOfficial { | ||
| p := m.currentProvider() | ||
| // A configured override (entry.URL) takes precedence over the preset | ||
| // default so the model-selection step reflects the gateway in use. | ||
| if m.existingCfg != nil { | ||
| if entry, ok := m.existingCfg.Providers[p.Name]; ok && entry.URL != "" { | ||
| return entry.URL | ||
| } | ||
| } | ||
| if p.BaseURL != "" { | ||
| return p.BaseURL | ||
| } | ||
| } |
There was a problem hiding this comment.
[bug · medium]
When the user edits the Base URL in the stepBaseURL step and then presses Esc to go back to the model selection step, the displayed "Base URL" in the model view is read from effectiveBaseURL() which checks m.existingCfg.Providers[p.Name] — the on-disk config — rather than the in-progress value in m.officialURLInput. This means the user will see the old URL on the model step, not the value they just typed. Consider having effectiveBaseURL() also check m.officialURLInput.Value() (when non-empty and on the official tab) so the model step reflects the pending edit.
…sc display Address 4 of 5 code review findings on PR alibaba#729: 1. URL trim consistency (provider_cmd.go): trim the Base URL once and use the trimmed value for both comparison and persistence, preventing whitespace-polluted URLs from being written to config. 2. URL format validation (provider_cmd.go): validate that the Base URL has an http/https scheme and non-empty host before persisting, giving immediate feedback instead of a runtime failure. Rejects malformed values like bare hosts or ftp:// schemes. 3. Dead code removal (provider_tui.go): remove the init-time pre-fill of officialURLInput that is always overwritten by loadOfficialURL() when the user enters the Base URL step. Pre-fill logic now lives in a single place. 4. effectiveBaseURL reflects pending edit (provider_tui.go): when the user edits the Base URL and presses Esc back to model selection, effectiveBaseURL() now returns the in-progress value from officialURLInput instead of the stale on-disk config. The SSRF/private-IP finding (alibaba#2 in review) is not addressed — it is a false positive for a local CLI tool where localhost and private network endpoints are the primary use case (the litellm preset default is http://localhost:4000/v1).
|
@xp880906 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. brucexu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
…sc display Address 4 of 5 code review findings on PR alibaba#729: 1. URL trim consistency (provider_cmd.go): trim the Base URL once and use the trimmed value for both comparison and persistence, preventing whitespace-polluted URLs from being written to config. 2. URL format validation (provider_cmd.go): validate that the Base URL has an http/https scheme and non-empty host before persisting, giving immediate feedback instead of a runtime failure. Rejects malformed values like bare hosts or ftp:// schemes. 3. Dead code removal (provider_tui.go): remove the init-time pre-fill of officialURLInput that is always overwritten by loadOfficialURL() when the user enters the Base URL step. Pre-fill logic now lives in a single place. 4. effectiveBaseURL reflects pending edit (provider_tui.go): when the user edits the Base URL and presses Esc back to model selection, effectiveBaseURL() now returns the in-progress value from officialURLInput instead of the stale on-disk config. The SSRF/private-IP finding (alibaba#2 in review) is not addressed — it is a false positive for a local CLI tool where localhost and private network endpoints are the primary use case (the litellm preset default is http://localhost:4000/v1).
1e3ab30 to
5e2930e
Compare
The official-provider tab in `ocr config provider` only captured API key and model, with no way to override a preset provider's Base URL. The resolver already honored `entry.URL` over `preset.BaseURL`, but the TUI never exposed it — litellm (a self-hosted gateway rarely at http://localhost:4000/v1) was the canonical pain point. Add a Base URL step to the official-tab flow (stepModel -> stepBaseURL -> stepAPIKey), pre-filled with the effective URL (configured override or preset default). Persist `providers.<name>.url` only when the entered value differs from the preset default, so the preset remains the fallback and configs without an explicit url are unchanged. Custom/manual tabs are unaffected. Add resolver regression tests (litellm override + default fallback) and TUI tests (pre-fill with preset/override, Esc navigation, persistence of override vs. clearing on preset default). Update the four official-tab tests that assumed stepModel -> stepAPIKey to traverse the new step.
…t it With the wizard now able to set a Base URL override for built-in providers, make the override visible and discoverable. - `ocr config model` shows the effective Base URL for a preset provider (the configured `providers.<name>.url` override, or the preset default when none is set) so users can confirm their gateway is in use. - The provider-wizard model-selection step shows the same effective URL via a tab-aware `effectiveBaseURL()` helper (official override/preset, or custom provider URL). - Document `providers.<name>.url` as a built-in provider override in the configuration docs, with a litellm example and the preset-as-default semantics; note the wizard's editable Base URL step. Add tests covering the model-selector display (override vs preset default) and the wizard's effectiveBaseURL resolution.
…sc display Address 4 of 5 code review findings on PR alibaba#729: 1. URL trim consistency (provider_cmd.go): trim the Base URL once and use the trimmed value for both comparison and persistence, preventing whitespace-polluted URLs from being written to config. 2. URL format validation (provider_cmd.go): validate that the Base URL has an http/https scheme and non-empty host before persisting, giving immediate feedback instead of a runtime failure. Rejects malformed values like bare hosts or ftp:// schemes. 3. Dead code removal (provider_tui.go): remove the init-time pre-fill of officialURLInput that is always overwritten by loadOfficialURL() when the user enters the Base URL step. Pre-fill logic now lives in a single place. 4. effectiveBaseURL reflects pending edit (provider_tui.go): when the user edits the Base URL and presses Esc back to model selection, effectiveBaseURL() now returns the in-progress value from officialURLInput instead of the stale on-disk config. The SSRF/private-IP finding (alibaba#2 in review) is not addressed — it is a false positive for a local CLI tool where localhost and private network endpoints are the primary use case (the litellm preset default is http://localhost:4000/v1).
5e2930e to
2f85c19
Compare
|
Thanks! I've updated the commit author email to the GitHub noreply address ( |
Review FeedbackThanks for the work on this PR! Code quality and test coverage are solid, but I have a UX design suggestion: Suggestion: Don't add a Base URL step to the wizardThe current approach inserts a Base URL step into the wizard flow for all official providers ( Suggested approach: Keep the wizard flow unchanged; guide users to Rationale:
Suggested to keep:
Suggested to remove:
This would significantly reduce the PR's scope while preserving the core value (documentation + display), without affecting the wizard experience for the majority of users. |
|
Minor addition: the documentation changes to |
3 participants
Summary
The built-in LiteLLM provider's Base URL was hardcoded to
http://localhost:4000/v1. This makes it configurable while keeping the current value as the default fallback — usable for any built-in provider whose endpoint differs from the preset (self-hosted LiteLLM gateways are the canonical case).The resolver already honored
entry.URLoverpreset.BaseURL, andocr config set providers.<name>.urlalready worked, but theocr config providerwizard never exposed it. These two slices close that gap end-to-end.Slice 1 — Editable Base URL in the official provider wizard
stepModel→stepBaseURL→stepAPIKey), pre-filled with the effective URL (configured override or preset default). Custom/manual tabs are unchanged.providers.<name>.urlonly when the entered value differs from the preset default; otherwise clear it so the preset remains the fallback.stepModel→stepAPIKeyto traverse the new step.Slice 2 — Surface the override URL and document it
ocr config modelshows the effective Base URL for a preset provider (override when set, preset default otherwise).effectiveBaseURL()helper.providers.<name>.urlas a built-in provider override inpages/.../en/configuration.md, with a litellm example and preset-as-default semantics.effectiveBaseURLresolution.Behavior
urlfield resolves topreset.BaseURLexactly as before.providers.<name>.url(via wizard orocr config set) routes requests to that URL.urlfield.Test plan
go build ./...go test ./cmd/opencodereview/(full suite)go test ./internal/llm/gofmt -lclean on changed Go filesocr config providerandocr config set;ocr config modelshows the effective URL;ocr llm testreached the gateway (returned a business-level response, confirming the URL override works).