feat(desktop): keep MCP config secrets on the main-process side of IPC - #2919
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
The shared secret-location plan and basis-bound sentinel restore are the right ownership direction, and the focused checks for this head are green. I found two concrete boundary issues that should be addressed before this slice is treated as complete; details are inline.
This PR is already presented as the secret-boundary slice stacked after #2918, which is a useful separation. Please keep the merge order explicit: the command-line editor can land first, this config boundary next, and the runtime scrubber/OAuth engine after it. The status/error channels still depend on that later runtime scrubber, so this PR should not by itself be described as closing every main-to-renderer credential path.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
3dae326 to
ee51986
Compare
|
Both findings fixed in the updated head: P1 — remove/cancel paths now redact. P2 — the OAuth block survives an edit. The draft round-trip moved out of the page component into a pure module, Desktop suite is green at the new head (786 tests). The stacked PRs are rebased on top. |
a2e66a9 to
7596046
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The ownership direction is good: mcp-secrets.ts is the shared location authority, main remains the only persistence owner, basis-bound restoration prevents masked drafts from becoming a confused-deputy channel, and the previously reported remove/cancel and OAuth round-trip gaps are fixed.
One free-form argument shape still crosses IPC in cleartext. From first principles, pattern recognition must apply to the value regardless of whether it happens to follow an arbitrary --flag= prefix; flag sensitivity is an additional signal, not an exclusion. The smallest fix is to check flag.value with isPatternSecret for every parsed flag and emit the same arg-flag-value location. The branch must also be rebased to drop the already-merged #2918 parent commit; the security-only commit applies cleanly to current main, while the stacked form conflicts in mcp-page.tsx.
Reviewed with Codex using two independent reviewer agents; I reproduced the arbitrary-flag leak and verified the latest head, shared secret plan, prior fixes, stacked-commit conflict, and CI state.
中文
职责方向正确：mcp-secrets.ts 是共享 location authority，main 仍是唯一持久化 owner，basis-bound restoration 防止 masked draft 变成 confused-deputy channel；此前的 remove/cancel 和 OAuth round-trip 问题也已修复。
但仍有一种 free-form argument 会明文穿过 IPC。按第一性原理，无论 token 前面是否恰好有任意 --flag= 前缀，都应对 value 做模式识别；flag 是否敏感只能作为额外信号，不能成为排除条件。最小修复是对所有 parsed flag 的 flag.value 调用 isPatternSecret，命中后生成相同的 arg-flag-value location。分支还必须 rebase，去掉已合并的 #2918 父提交；security-only commit 可干净应用到当前 main，而当前 stacked 形式会在 mcp-page.tsx 冲突。
本次由 Codex 配合两个独立 reviewer agent 审查；我复现了 arbitrary-flag 泄漏，并核验了最新 head、共享 secret plan、此前修复、stacked commit 冲突和 CI 状态。
7596046 to
18a86db
Compare
|
Warning Review limit reached
Next review available in: 59 minutes Limit details: You’ve used all 3 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (14)
📝 WalkthroughProblem solvedThe PR prevents MCP secrets from crossing desktop IPC to the renderer in cleartext. It masks remote headers, OAuth client secrets, stdio environment values, sensitive command arguments, and sensitive URL query parameters. The main process restores masked values only when the incoming configuration matches the stored configuration basis. Restore failures now raise The PR also adds remote OAuth configuration with validation. It rejects configurations that contain both OAuth credentials and a static Source of truthThe PR extends the existing MCP configuration and storage paths.
Scope and complexityThe design is the smallest coherent solution shown by the diff:
The restoration basis checks are necessary to prevent secrets from being restored into changed endpoints, commands, arguments, environment keys, or OAuth clients. The editor extraction removes duplicated parsing and conversion logic from No further deletion is apparent without weakening behavior or regression coverage. ValidationTests cover:
The author reports that the desktop suite is green and that other jobs pass. The reported Complexity deltaThe PR adds:
The PR removes duplicated draft conversion and parsing logic from Total maintenance complexity increases in the short term. The increase is justified by the security boundary, OAuth validation requirements, and centralized secret discovery. The diff provides no evidence that the added complexity exceeds those requirements. Review-relevant risks
Required-check status remains unverified from direct evidence. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe change adds MCP OAuth configuration support, shared secret discovery and scrubbing, identity-bound secret restoration in desktop IPC, and centralized renderer draft conversion. Tests cover normalization, redaction, restoration, and IPC response handling. ChangesMCP secret protection
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds a secret-masking and restoration boundary, but valid configurations can currently fail to restore when URL query encoding differs, and an argument edge case can alter how a command is launched. These bounded correctness issues make the change not merge-ready until fixed; the missing restore guidance is a lower-impact follow-up. Sequence Diagram(s)sequenceDiagram
participant Renderer
participant MCP IPC
participant Config Store
participant MCP Secret Guard
participant MCP Manager
Renderer->>MCP IPC: Submit configuration with secret markers
MCP IPC->>Config Store: Run serialized transform
Config Store->>MCP Secret Guard: Restore matching stored secrets
MCP IPC->>MCP Manager: Synchronize restored configuration
MCP IPC-->>Renderer: Return redacted configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Both asks are in the updated head: P1 — pattern recognition applies to every flag value. Rebase. The stack is rebased onto current Desktop suite green at the head (952 incl. the stacked slices; this slice's own jobs pass standalone). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b37e028-1b0d-44c8-a9a9-34b2da47bb52
⛔ Files ignored due to path filters (2)
.maka-shots/after-dialog.pngis excluded by!**/*.png.maka-shots/before-dialog.pngis excluded by!**/*.png
📒 Files selected for processing (13)
apps/desktop/src/main/__tests__/mcp-editor-draft.test.tsapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/mcp-ipc-main.tsapps/desktop/src/main/mcp-secret-guard.tsapps/desktop/src/renderer/mcp-editor-draft.tsapps/desktop/src/renderer/mcp-page.tsxpackages/core/package.jsonpackages/core/src/mcp-secrets.tspackages/core/src/mcp.tspackages/core/src/redaction.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.
|
/agentic_review |
Code Review by Qodo
1. Stale snapshot restores old secrets
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
CI triage: the only red here is |
18a86db to
d53f29a
Compare
|
CodeRabbit's four findings are addressed in the updated head:
Also landed in this slice's shared plan (flagged on the sibling PRs): substitution now replaces longer secrets before their prefixes (dedup + length-desc sort — |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/__tests__/mcp-secret-guard.test.ts (1)
223-232: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the rejection message excludes the secret value.
mcp-secret-guard.tslines 28-29 state the invariant: "The rejection message names the position, never the value." No test protects it. A future edit toMcpSecretRestoreErrorcould interpolate the stored value into the message, and every current test would still pass.assert.throwswith only the error class does not inspect the message.Add one assertion to an existing rejection test. This reuses existing coverage instead of adding a new test case.
Disposition: optional.
Proposed assertion
notionIn.url = 'https://attacker.example/mcp'; - assert.throws(() => restoreMcpConfigSecrets(incoming, previous), McpSecretRestoreError); + assert.throws(() => restoreMcpConfigSecrets(incoming, previous), (error: unknown) => { + assert.ok(error instanceof McpSecretRestoreError); + // The message names the position, never the value. + assert.doesNotMatch(error.message, /real-secret/u); + return true; + });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ea1a2d4-2206-4dd0-95dc-7f8a1c41a41a
📒 Files selected for processing (6)
apps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/mcp-secret-guard.tspackages/core/src/__tests__/mcp-secrets.test.tspackages/core/src/mcp-secrets.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Moving secret redaction/restoration to the main-process IPC boundary is the right authority decision, and the current head covers the normal get/set/upsert/install/remove/cancel round trips without exposing raw values.
This is not approval-ready because the existing Qodo threads are reproducible and remain open: stale get → restore → write snapshots can roll a rotated secret back; the fixed sentinel collides with valid config values; repeated sensitive query keys cannot round-trip; and sensitive-flag parsing can consume the next flag as a value. I am not duplicating those inline findings. From first principles, the smallest coherent correction is one store-level atomic transform for restore+mutation, plus an occurrence-bound, non-confusable redaction representation. That removes the race and the marker ambiguity without adding a second secret authority.
All live checks are green, but merge state remains blocked by unresolved review findings.
Reviewed with Codex using three independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact head, current discussion, IPC/store serialization, redaction locations, round-trip failures, and live CI.
中文
把 secret redaction/restore 放到 main-process IPC 边界是正确的权威设计，当前 head 也覆盖了正常的 get/set/upsert/install/remove/cancel round-trip，不会把原始值返回 renderer。
但现在还不能批准，因为已有 Qodo 线程都能复现且仍未解决：过期的 get → restore → write snapshot 会把已轮换 secret 回滚；固定 sentinel 会与合法配置值冲突；重复敏感 query key 无法 round-trip；敏感 flag 解析还会把下一个 flag 误当作 value。我不重复发布这些行内问题。按第一性原理，最小且一致的修复是提供一个 store-level atomic transform，把 restore+mutation 放在同一串行边界内，并使用 occurrence-bound、不可混淆的 redaction representation。这样能同时消除竞态和 marker 歧义，也不新增第二套 secret 权威。
实时检查全绿，但 unresolved findings 仍使 merge state blocked。
本次由 Codex 配合三个独立 reviewer agent，以及 OpenCode Go DeepSeek V4 Flash（high）审查；我核验了精确 head、现有讨论、IPC/store serialization、redaction locations、round-trip failures 和实时 CI。
|
This PR includes before and after dialog image files in the branch, but they are not currently displayed or linked in the PR description. Since the change affects how MCP configuration values are presented and round-tripped in the editor, could you please embed those screenshots in the PR description or add them in a comment so the UI evidence is directly reviewable? Please ensure all secret values are sanitized. Thanks! Posted by Codex on behalf of Astro-Han. |
d53f29a to
6d0c7af
Compare
|
All four Qodo threads plus the synthesis are addressed in the updated head, along the prescribed shape — one store-level atomic transform, plus an occurrence-bound, non-confusable marker: Stale snapshot restores (High) — the store gained Sentinel collides with valid values (High) — the fixed public sentinel is replaced by a marker carrying a per-process random nonce (a legitimate value cannot collide with it, stale markers from a previous run are rejected) AND a position tag: Repeated query values lost (High) — query masking/restoring is per occurrence, in order, preserving duplicates; the URL identity check is positional. Regression: Flags become secret values (High) — the scanner no longer classifies a flag following a sensitive flag as its value: Short secrets bypass withholding (High, filed on the shared plan) — The reject-not-drop restore semantics from the previous round carry over unchanged (a moved/stale/unrestorable marker throws |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts (1)
263-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the restored env value, and add the rejection path.
Two gaps in an otherwise strong round trip.
First,
seenScratchcarries a maskedAPI_TOKEN, but no assertion checks that it returns asscratch-token. Env values use the absolute masking rule, so that position deserves the same assertion the arg gets at line 270.Second, no IPC test covers a rejected restore. The guard tests prove
McpSecretRestoreErroris thrown; this file should prove the handler rejects and the store stays unwritten.🧪 Proposed additions
assert.deepEqual(storedScratch.args, ['server', '--custom=sk-ant-api03-abcdef123456']); + assert.deepEqual(storedScratch.env, { API_TOKEN: 'scratch-token' }); + + // A changed launch basis with a kept sentinel must reject, and must not + // commit a partially-restored config. + await assert.rejects( + upsertScratch({}, 'scratch', { ...seenScratch, command: 'curl' }), + /cannot be restored/, + ); + const unchanged = config.mcpServers.scratch; + assert.ok(unchanged && 'command' in unchanged); + assert.equal(unchanged.command, 'npx');The rejection case needs
seenScratchre-fetched throughmcp:getConfigif the earlier upsert already replaced the stored values; adjust ordering as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1de3e0e2-1a6c-4419-ab89-943e328a83e3
📒 Files selected for processing (7)
apps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/mcp-ipc-main.tsapps/desktop/src/main/mcp-secret-guard.tspackages/core/src/mcp-secrets.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the extensive secret-boundary hardening. On the current head, the nonce markers, occurrence binding, fail-closed restore behavior, and most existing review items are addressed. Two nonduplicate P2 boundary gaps remain inline: inconsistent URL canonicalization can reject an unchanged endpoint and lose restoration, and a short credential fragment can still escape when reflected without its longer header value.
Please fix these owner-level issues here before the dependent OAuth stack is restacked. Several old unresolved threads appear already fixed; those should be resolved rather than patched again. The existing AI disclosure is sufficient: the commits identify Claude co-authorship and link the contributing session. No new visual design is introduced by this slice, so I am not adding a screenshot gate.
Reviewed with Codex as an AI-assisted code review. I verified the exact-head diff, secret inventory/restore paths, existing threads, CI, and provenance; no external model output was used.
中文说明
当前 head 已修好大部分 secret boundary 问题，但仍有两个新的 P2：URL restore 两侧 canonical form 不一致，会把未改变的 endpoint 判成改变；短 credential fragment 单独被上游回显时仍可能泄漏。请先在 #2919 的 owner 层修复，再让后续 OAuth PR restack。已有 AI 说明完整，本层没有新增可见 UI，不额外要求截图。
6d0c7af to
ed960ad
Compare
|
Both P2s are fixed in the updated head (rebased onto current Inconsistent URL canonicalization — Short credential fragments reflected alone — the shared inventory gained a third class, Housekeeping from this round's comments: the stray |
The renderer is semi-trusted (SECURITY.md §3), so credential material in mcp.json never crosses the bridge in cleartext, wherever it sits: the static OAuth clientSecret, every remote header value, every stdio env value (key-name heuristics miss real secrets — PGPASSWORD, DATABASE_URL — and the boundary is absolute), URL query parameters under a sensitive name or with a pattern-recognized value, and command-line arguments a sensitive flag introduces. WHERE those secrets live is enumerated once, in @maka/core/mcp-secrets: a shared location plan that this IPC guard masks by and that the MCP runtime's outbound scrubber will consume for error/status/payload scrubbing — one authority, so the two halves of the boundary cannot drift. The plan also fixes the scrubbing semantics: a value long enough to be unambiguous is substituted in place; a credential too short to splice out of prose forces the whole containing message to be withheld, because the boundary allows no third option. Every config leaves main with secrets replaced by sentinels; configs coming back have sentinels restored from what is on disk, and every restore is bound to the full basis the value was configured for — the URL for remote credentials (plus the clientId for a clientSecret); command, arguments, cwd and the rest of the env for stdio, since any of those can redirect which executable runs or where it connects. A renderer cannot keep a sentinel, repoint where it flows, and have main forward a secret it cannot read. The config schema gains the remote `oauth` block (clientId, clientSecret, scopes, callbackPort — consumed by the upcoming OAuth engine) with storage normalization and bounds, and one invariant enforced at the store: a config may declare a static Authorization header or oauth, never both — one authority per header, rejected at write time rather than arbitrated at request time. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
ed960ad to
d46c4e4
Compare
|
Restacked onto latest main (074f719, config version 2 / dual-era negotiation). Notable adaptation: this PR's |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved exact head d46c4e4c2da223864fcde21ab427bfdc0b0d7b87.
I verified that the prior URL-canonicalization and short-fragment redaction P2s are fixed, the focused Desktop/Core/Storage tests pass, the Claude disclosure/provenance is complete, and this slice introduces no new UI/UX behavior. I resolved 11 superseded review threads.
One existing non-blocking P3 remains: the renderer replaces the actionable “re-enter the secret” restore error with generic copy. That should be improved, but it does not block this approval.
I approved the exact-head fork workflows; this is not merge-ready until they finish green.
AI-assisted review disclosure: OpenAI Codex performed the exact-head secret-handling, thread, provenance, and CI analysis; I verified the fixes, local focused results, severity, and live GitHub state before approving.
中文说明
旧的 URL 规范化与短 secret 片段泄漏两个 P2 已修复，11 个过时线程已关闭，AI provenance 合规，也没有新增 UI 变化。仍保留一个非阻塞 P3：恢复 secret 失败时可操作提示被通用文案替换。当前 workflows 已授权，全部绿色后才算 merge-ready。
2 participants
Extracted from #2653 per the review recommendation: the secret-safe configuration boundary lands as the small preceding PR, and the narrowed OAuth-engine PR retargets onto it.
This PR also implements the review's first architectural correction: one shared secret-location/value plan owning both halves of the boundary.
What
@maka/core/mcp-secrets— the single secret-location authority.listMcpSecretLocations(config)enumerates every credential-bearing position in an MCP server config (remote header values, the static OAuthclientSecret, stdio env values, sensitive-flag command-line arguments, sensitive URL query parameters). Two consumers derive from this one plan so they cannot drift:mcp-secret-guard.ts, this PR) masks exactly these locations before a config crosses toward the semi-trusted renderer (SECURITY.md §3);@maka/mcpruntime scrubber (feat(mcp): OAuth for remote MCP servers #2653) builds its outbound scrub inventory from the same plan.The plan also fixes the scrubbing semantics both consumers share: values long enough to be unambiguous are substituted in place; a credential too short to splice out of prose forces the whole containing message to be withheld — the boundary allows no third option.
Sentinel masking with basis-bound restores. Configs leave main with secrets replaced by sentinels; configs coming back have sentinels restored from disk, and each restore is bound to the full basis the value was configured for (URL for remote credentials — plus clientId for a clientSecret; command/args/cwd/env-set for stdio, since any of those redirect where the secret flows). A renderer cannot keep a sentinel, repoint it, and have main forward a secret it cannot read. All env values mask at the boundary — key-name heuristics miss PGPASSWORD and DATABASE_URL.
Config schema: the remote
oauthblock (clientId, clientSecret, scopes, callbackPort) with storage normalization/bounds, plus one invariant the review asked for enforced at the store: a config may declare a staticAuthorizationheader oroauth, never both — one authority per header, rejected at write time. The engine PR enforces the same exclusivity on the wire.Tests
Guard tests cover masking per location kind and restore-basis binding (repointed sentinels refuse to restore); IPC tests prove a
clientSecretround-trips renderer-side as a sentinel while the store and manager see the real value; storage tests cover oauth-block normalization bounds and the Authorization/oauth conflict.Stacked on #2918 (linear branch chain from the fork): the diff shows its small commit too until it merges — review the
feat(desktop): keep MCP config secrets…commit.Co-Authored-By: Claude noreply@anthropic.com
https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj