feat(desktop): show effective provider endpoints - #3639
Conversation
ARE404
left a comment
ARE404
left a comment
There was a problem hiding this comment.
NO-GO at head 0ba01eef0f79c0c4a18687003abefa2b51747430.
Gate: test ❌ — Build step fails (run 32651465871, on this exact head): apps/desktop/src/main/__tests__/goal-services-adapter.test.ts:64/70/76 error TS2353: Object literal may only specify known properties, and 'type' does not exist in type 'SessionChangedEvent'. This file is not touched by this PR — it's a stale-branch build break: SessionChangedEvent (packages/core/src/session.ts:687) has no type field, yet the branch's copy of that test passes type: 'sessions_changed'. The PR needs a rebase onto current main so the build goes green. audit not triggered (no pkg.json/lockfile change).
Credential review — the focus you flagged: SOUND, no P0/P1 leak.
providerEndpointPresentationresolveseffectiveBaseUrl,endpointForDisplaystrips URL userinfo (https://user:pass@host→https://<redacted>@host) and runs the value through the shared display redactorredactSecrets.- I independently verified masking against a battery of realistic credentials: OpenAI
?key=sk-…/sk-ant-…(Anthropic), Google?key=AIza…(URL-query rule or provider-key rule), GitHubghp_…in userinfo, Slackxox…, plus?token=,?api_key=,?access_token=— all masked. The PR also ships a dedicated test asserting userinfo+query masking. - The only sink is a React text node (
<code>{endpoint.value}</code>in provider-connection-detail.tsx) — escaped by React, not logged, not re-persisted, not in any attribute. No leak surface found.
Findings: 1 × [P3] hardening note (inline on provider-endpoint-presentation.ts:86) re: the display redactor's word-boundary edge; 0 × P0/P1/P2 in the feature code. The [P1-level blocker is the build gate, not this feature's logic].
No APPROVE (I only hold read scope); flagging to orchestrator to arrange a MEMBER approve once the branch is rebased and test builds green. Not merging.
| // Persistence already validates provider base URLs. A legacy malformed | ||
| // value still gets best-effort masking rather than disappearing. | ||
| } | ||
| return redactSecrets(value); |
There was a problem hiding this comment.
[P3] Note for the redaction hardening (not blocking): the shared redactSecrets 'long opaque token' rule requires a word boundary before the 40+ char run (\b(?=[A-Fa-f0-9_-]*[A-Fa-f0-9])[A-Fa-f0-9_-]{40,}\b). So a bare 40+ hex/base64 secret embedded directly after a word character in the URL path — e.g. .../v1/key40999e1fa66d139b2baa1b6b9a0c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0c — is NOT caught by that rule (no \b between key and the hex digits). I verified userinfo + all the realistic query vectors and provider-key shapes (OpenAI sk-, Anthropic sk-ant-, Google AIza, GitHub ghp_, Slack xox) are masked by this path, so this is an edge case; but if upstream ever allows a path-embedded token this is the gap to close (e.g. also strip a leading user/-less token segment). Realistic provider base URLs don't hit it today.
There was a problem hiding this comment.
Good catch — confirmed on my side too: for key<40-hex> there is no word boundary between key and the digits, so the long-opaque-token rule cannot anchor and the run passes through unmasked.
Since you verified that userinfo, the realistic query vectors, and all current provider key shapes are covered, and no real provider base URL embeds a token in the path this way today, I will keep this PR scope as-is. If upstream ever allows path-embedded tokens, the right close looks like URL-scoped segment masking in endpointForDisplay (mask any path segment containing a 40+ hex/base64 run after URL parsing) rather than relaxing the shared rule's \b — that boundary is deliberately conservative to avoid eating benign long runs in the other display surfaces (quiet panel, TUI) that share redactSecrets.
Recording this as a follow-up. Thanks for the thorough audit!
Generated-by: OpenAI Codex
0ba01ee to
d44881c
Compare
| }, | ||
| ): ProviderEndpointPresentation { | ||
| const defaults = PROVIDER_DEFAULTS[connection.providerType]; | ||
| const effective = effectiveBaseUrl(connection).trim(); |
There was a problem hiding this comment.
[P2] This derives the displayed endpoint only from effectiveBaseUrl(connection), but the Runtime applies model-level provider overrides before constructing a request. For example, ZenMux displays https://zenmux.ai/api/v1 while resolveModelRuntime({ providerType: "zenmux" }, "anthropic/claude-opus-4.6") sends through https://zenmux.ai/api/anthropic/v1; the Cohere north-mini-code-1-0 override has the same shape. The new "effective" value is therefore a stable lie for normal enabled models. Please expose model-specific effective endpoints (or explicitly label this as the connection base and explain routing), with coverage for override models.
There was a problem hiding this comment.
Fixed in b52cfbc — the presentation now carries a modelOverrides flag computed exactly the way resolveModelRuntime resolves: it only applies when the connection has no configured baseUrl (a configured baseUrl wins over every model-level override, so then the row is the exact truth and carries no caveat). When it applies, a note renders under the endpoint value ("Some models send requests through a different endpoint" / 部分模型会通过其他服务端点发送请求). The sweep uses modelMetadataIdsForProvider + lookupModelProviderOverride over the generated table; today only zenmux and cohere trip it, and both are covered by new tests along with a test that a configured baseUrl suppresses the caveat.
| // Persistence already validates provider base URLs. A legacy malformed | ||
| // value still gets best-effort masking rather than disappearing. | ||
| } | ||
| return redactSecrets(value); |
There was a problem hiding this comment.
[P2] The shared redactor does not mask arbitrary credential query keys: providerEndpointPresentation({ providerType: "openai-compatible", baseUrl: "https://relay.example/v1?key=secret-value" }).value remains unchanged, and client_secret=secret-value is also left intact. Since the base URL validator accepts query parameters and issue #3636 explicitly requires that secret-bearing query parameters not be exposed, this normal custom-relay value reaches the endpoint text node. Please parse and mask credential-bearing query names (including plain key and client_secret) before rendering, and add these cases to the focused tests.
There was a problem hiding this comment.
Fixed in b52cfbc — endpointForDisplay no longer relies on the shared redactor's named-key rule for query strings: it parses the URL and masks every query value while keeping the key names (?api-version=<redacted>&api_key=<redacted>), which closes arbitrary key names such as key and client_secret. The userinfo <redacted>@ marker is preserved, and redactSecrets still sweeps whatever remains (path-embedded runs included). New tests cover the arbitrary-key cases.
| </> | ||
| )} | ||
| {showsEndpoint && ( | ||
| {endpoint.editable ? ( |
There was a problem hiding this comment.
[P2] The editable branch still passes the raw persisted baseUrl into TextInput. A custom relay such as https://user:password@relay.example/v1?key=secret-value is masked in the collapsed endpointValue, but clicking Edit exposes both the URL userinfo and query credential in a normal text input. That remains a settings-page credential sink under issue #3636’s explicit no-exposure requirement. Please make editing reveal-gated/redacted (or reject credential-bearing base URLs) and add an interaction test for the expanded row.
There was a problem hiding this comment.
Fixed in b52cfbc — the endpoint editor is now reveal-gated: when the persisted endpoint embeds userinfo or any query parameter (endpointCarriesCredentials), the row renders the shared PasswordInput — masked by default with an explicit Eye toggle and an explanatory description — instead of a plain TextInput, so credentials are never prefilled into visible text. I chose reveal-gating over rejecting credential-bearing URLs so existing relay configurations remain editable; happy to switch to validation-level rejection if preferred. Unit tests cover the gating predicate.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
NO-GO at exact head d44881c42ac4434ea21c8fc49ed775153f139c04.
Gate: no checks are reported for this exact head, so there is no approval gate to satisfy.
Cross-file conclusion: the new row is correctly wired across the provider detail UI, locales, and Storybook coverage, and the focused endpoint presentation test passes 6/6. However, the feature has three P2 issues in the inline comments: model-level runtime endpoint overrides are not reflected by the connection-level display, credential-bearing query keys such as key/client_secret are not fully masked, and the editable custom-endpoint path still exposes raw URL credentials. git diff --check passes. The desktop workspace build remains blocked by pre-existing shared workspace export/type drift in @maka/storage, not by the changed files.
- Mask every query value in displayed endpoints, not just known secret key names: a relay may carry credentials under arbitrary keys. - Gate the endpoint editor behind the masked-by-default PasswordInput when the saved URL embeds userinfo or query credentials. - Note model-level endpoint overrides under connection-level defaults, and only then: a configured baseUrl wins over every override.
|
All three P2 findings are addressed in b52cfbc (replies in each thread):
Local verification: full |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
#3639 b52cfbc — full review (bind exact head) — 6 files +483/-31
Gate: CI test success (run 32691039006) on this head, BLOCKED (likely review required). No check-freshness issue.
Scope: Full 6-file review. 4 prior threads assessed against this head first, then full diff.
Prior threads on this head:
provider-endpoint-presentation.ts:65 [P2] effectiveBaseUrl vs model overrides— still valid: display still derives from connection base only, model-level overrides (zenmux/cohere) not reflected -> stable lie for override models.provider-connection-detail.tsx:465 [P2] raw baseUrl in TextInput on Edit— still valid: edit reveals userinfo/query credentials.- Two
isOutdated=truethreads ([P2] query key=key/client_secretnot masked, [P3] \b token rule gap) — code shows query masking not yet added; gap remains but marked outdated due to line shift — still present.
Verdict: COMMENT — 2×[P2] still open (effective endpoint lie + credential exposure on edit), gate BLOCKED so no APPROVE.
中文
两条 P2 仍未修，待作者处理。
|
@Astro-Han Head binding check: the fork head is b52cfbc (fix commit on top of d44881c), and CI run 32691039006 ran green on exactly that head. But the three "still valid" findings describe the pre-fix tree — all three fixes are present at b52cfbc, each pinned by a test that passed in that run: 1. "[P2] model-level overrides not reflected" — 2. "[P2] raw baseUrl in TextInput on Edit" — 3. "query masking not yet added" — Suggestion: the pass may have diffed a cached/stale checkout — could you re-bind to the live head (or |
3 participants
Summary
Provider connection details now always show the effective base endpoint. Built-in and derived endpoints are read-only; custom relays and local runtimes keep their existing editor. Long URLs wrap as machine text, and embedded URL credentials or secret query values are masked before rendering.
Fixes #3636
Verification
npm run lintnpm run format:checknpm --workspace @maka/desktop run build:renderersettings-modelsfixtureFull Desktop typecheck is currently blocked by current
main(42cc5108c): the newly addedgoal-services-adapter.test.tspasses atypefield thatSessionChangedEventdoes not declare. The renderer and Storybook projects affected by this PR pass.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex implemented the endpoint presentation, added coverage and the Alibaba visual fixture, ran verification, and captured the real Electron evidence. The commit carries a
Generated-by: OpenAI Codextrailer.Checklist
Does this PR entail a change in behavior?