Repository navigation
fix(core): validate credential server before sending the key - #4
Open
Dextheking1 wants to merge 3 commits into
Open
Dextheking1 wants to merge 3 commits into
Dextheking1 wants to merge 3 commits into
Conversation
serverUrl returned credential.metadata.server verbatim and modelsFor sends the API key to it as a bearer token. Parse the value first: https only, http on loopback, anything else falls back to the default server.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR hardens credential server selection by parsing and restricting server URLs before model catalog requests, preventing imported or otherwise untrusted credentials from sending API keys to arbitrary hosts or insecure remote endpoints. Sequence diagram for validated credential server selectionsequenceDiagram
participant Catalog as CatalogLoader
participant Models as modelsFor
participant Server as serverUrl
participant API as CredentialServer
Catalog->>Models: modelsFor(http, value)
Models->>Server: serverUrl(value)
Server->>Server: Parse raw with URL
alt HTTPS or loopback HTTP
Server-->>Models: normalized origin and path
Models->>API: GET /v1/models with bearer API key
API-->>Models: ModelsResponse
else Invalid, insecure, or unparseable server
Server-->>Models: defaultServer
Models->>API: GET /v1/models with bearer API key
API-->>Models: ModelsResponse
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/core/src/plugin/provider/mars.ts" line_range="35" />
<code_context>
+ try {
+ const url = new URL(raw)
+ const loopback = url.hostname === "localhost" || url.hostname === "127.0.0.1" || url.hostname === "::1" || url.hostname === "[::1]"
+ if (url.protocol !== "https:" && !(url.protocol === "http:" && loopback)) return defaultServer
+ return `${url.origin}${url.pathname.replace(/\/+$/, "").replace(/\/v1$/i, "")}`
+ } catch {
</code_context>
<issue_to_address>
**HTTPS servers can steal API keys**
When an attacker-controlled legacy `auth.json` is imported with `metadata.server` set to an HTTPS host they control, `serverUrl` accepts the supplied HTTPS origin, and `modelsFor` attaches `value.key` as a bearer token; the catalog request therefore sends the key to the attacker-controlled host.
Restrict credential servers to trusted hosts, or require explicit user approval before sending the key to a new host.
</issue_to_address>
### Comment 2
<location path="packages/core/src/plugin/provider/mars.ts" line_range="35" />
<code_context>
+ try {
+ const url = new URL(raw)
+ const loopback = url.hostname === "localhost" || url.hostname === "127.0.0.1" || url.hostname === "::1" || url.hostname === "[::1]"
+ if (url.protocol !== "https:" && !(url.protocol === "http:" && loopback)) return defaultServer
+ return `${url.origin}${url.pathname.replace(/\/+$/, "").replace(/\/v1$/i, "")}`
+ } catch {
</code_context>
<issue_to_address>
**API keys travel over plaintext**
When `MARSAI_URL` is set to a remote `http://` endpoint and the credential's server is absent or rejected, `serverUrl` returns the unvalidated `defaultServer`, and `modelsFor` attaches `value.key` as a bearer token to its `/v1/models` request, exposing the key in transit.
Validate `MARSAI_URL` with the same HTTPS-or-loopback-HTTP policy before allowing it to serve as the fallback.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and a rejected or misparsed custom server silently falls back to the default endpoint, so requests may send the credential there instead of to the configured server. Reverting restores the old routing, but it cannot recall a key already transmitted.
Blocking findings: packages/core/src/plugin/provider/mars.ts:35, packages/core/src/plugin/provider/mars.ts:35
defaultServer took MARSAI_URL as-is, so a remote http:// value became the fallback and the key went to it in plaintext whenever the credential had no server of its own. Run it through the same normalizeServer check as credential servers; on failure use the hardcoded https default.
The import copied legacy auth.json metadata into the stored key credential verbatim. No first-party writer puts a server on a key credential (the opencode console flow writes it on OAuth credentials of its own integration), so the field only ever arrives as file-controlled text. Drop it during the import; imported keys use the configured default server. The serverUrl validation stays as a second layer.
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
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.
Issue for this PR
No linked issue.
Type of change
What does this PR do?
serverUrl()returnscredential.metadata.serververbatim, andmodelsFor()sends the API key as a bearer to{server}/v1/modelsevery time the catalog loads.Nothing validates the value. The legacy auth.json import copies credential metadata into the stored credential as-is, so a
servervalue from an imported file (or any future writer of that field) points the key at an arbitrary host, over plain http if it says so. Upstream validates its equivalent value before use (normalizeServerin opencode.ts).Fix: parse the value in
serverUrl(). Only https passes, plus http on loopback (localhost, 127.0.0.1, ::1). Anything else falls back to the default server. Trailing slash and/v1are stripped the same way the default server is normalized, and the result is rebuilt from origin + path, so userinfo and query in the value are dropped.How did you verify your code works?
Verified: ran the new function isolated, 18 cases (https ok, http off loopback rejected, loopback http ok, unparseable rejected,
/v1stripped, userinfo dropped). Patched mars.ts transpiles clean withbun build.Screenshots / recordings
Not a UI change.
Checklist
Summary by Sourcery
Prevent API keys from being sent to untrusted or insecure Mars servers by validating and normalizing configured endpoints.
Bug Fixes:
Enhancements: