feat(desktop): MCP OAuth login flow - #2920
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head 421768d4605a14e33e1f014fe7d6e9522b917e8a, focusing this stacked PR on its Desktop OAuth activation slice: callback listener/controller, IPC/preload, credential adapter, boot composition, and the manager calls they drive. I found one P2 lifecycle issue in this slice; see the inline finding. Current checks are green.
The stack already separates the command editor (#2918), secret boundary (#2919), MCP OAuth engine (#2653), and this Desktop activation layer. I recommend preserving that merge order rather than splitting this activation slice further. Findings already reported on the natural owners are intentionally not duplicated here.
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.
421768d to
97297c8
Compare
|
Fixed in the updated head. One Two regressions added, both with endpoints that accept and never answer: hung discovery (browser never opens, guard provably released — the retry runs), and hung token exchange after a completed browser round (login rejects at the deadline and the callback port is provably closed). |
0524c10 to
e849999
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The security foundations are thoughtful: state/PKCE/iss, loopback callback provenance, hop-by-hop credential shedding, secret scrubbing, and credential epochs/CAS all have clear contracts and substantial tests. The previous indefinitely hung endpoint issue is also improved at the caller boundary.
Three production boundaries still prevent this activation slice from working safely. The new preload calls do not provide the Runtime Host scope required by their handlers; the timeout abandons callers but does not cancel or epoch-fence the underlying OAuth mutation; and a failed credential erase can leave an old token available to a reconfigured endpoint. From first principles, one OAuth round needs a cancellable/epoch-bound authority, and endpoint ownership must change only after credential retirement succeeds. The minimal solution is to use the existing scoped preload seam, propagate AbortSignal/round epoch through manager and SDK writes, and make credential cleanup a fail-closed prerequisite/tombstone. The stack is also conflicting with current main and needs rebase before its old-base green CI is meaningful.
Reviewed with Codex using two independent reviewer agents; I verified the latest head, scoped IPC registration/calls, timeout and credential-write paths, endpoint-change flow, current-main conflicts, prior review, and live CI.
中文
安全基础做得认真：state/PKCE/iss、loopback callback provenance、逐跳 credential shedding、secret scrubbing 和 credential epoch/CAS 都有清晰契约与较充分测试；此前 endpoint 永久挂起的问题也改善了调用方边界。
但仍有三个生产边界使这条 activation slice 无法安全工作：新增 preload 调用没有提供 handler 要求的 Runtime Host scope；timeout 只放弃调用方，没有取消或用 epoch 隔离底层 OAuth mutation；credential erase 失败后，旧 token 仍可能被新 endpoint 使用。按第一性原理，一轮 OAuth 应有可取消、受 epoch 约束的单一权威；endpoint 所有权只有在旧凭据退休成功后才能切换。最小方案是复用现有 scoped preload seam，把 AbortSignal/round epoch 贯穿 manager 与 SDK 写入，并把 credential cleanup 变成 fail-closed prerequisite/tombstone。该 stack 也与当前 main 冲突，需 rebase 后旧 base 的全绿 CI 才有意义。
本次由 Codex 配合两个独立 reviewer agent 审查；我核验了最新 head、scoped IPC 注册/调用、timeout 与 credential write 路径、endpoint change 流程、current-main 冲突、此前 review 和实时 CI。
e849999 to
b8f6b73
Compare
📝 WalkthroughProblem solvedAdds desktop MCP OAuth login for remote servers. The flow includes:
The PR also:
Source of truth and solution scopeThe PR extends the existing It does not create a parallel configuration or credential authority. OAuth storage adapts to The scope is necessary because OAuth affects connection state, persistence, transport security, configuration mutation, IPC, and user interaction. The shared deadline and Simplification opportunitiesThe renderer draft and parsing helpers already moved into No further deletion is evident from the supplied changes. The tests cover distinct OAuth, security, storage, IPC, ordering, cancellation, timeout, and cleanup behavior. Removing them would weaken regression coverage. Validation and concrete risksThe supplied validation reports:
The tests cover:
Concrete risks include OAuth lifecycle regressions, credential loss or stale writes, incorrect secret restoration, redirect or transport-security bypasses, IPC contract regressions, and configuration mutation races. Annotated screenshots for the login action and authenticated or error state remain a review request. Screenshots must not contain credentials or sensitive callback data. Complexity deltaThe PR adds:
The PR removes or consolidates:
The added complexity is necessary for the new OAuth capability and its security requirements. Total maintenance complexity increases, but the increase is justified by reuse of the existing configuration and credential authorities and by the reported regression coverage. Review-relevant risksThe diff changes user-visible MCP configuration, authentication, connection status, and IPC behavior. Material changes in these areas require independent human review under repository policy. The diff changes public package exports and TypeScript contracts. Material public-contract changes require independent human review under repository policy. The diff changes credential storage, secret redaction, callback validation, redirect handling, and transport security. Material security changes require independent human review under repository policy. The diff adds a development dependency and a package export. Material licensing and release-impact changes require independent human review under repository policy. Required-check status remains unverified except for the reported test results. The person performing the merge reviews the final diff, and a maintainer makes the final determination. WalkthroughMCP support now includes OAuth authorization, persisted credential coordination, transport validation, secret scrubbing, duplicate-safe configuration insertion, and desktop IPC and editor integration. ChangesMCP contracts, validation, and storage
OAuth engine and credential coordination
Secret protection and desktop integration
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The PR adds OAuth login, credential persistence, and server editing flows, but unresolved paths can expose credentials, orphan stored tokens, overwrite an existing server on duplicate IDs, or leave logout hanging. These are concrete security, data-integrity, and availability risks, so the PR is not safe to merge until the major issues are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Preload
participant RuntimeHost
participant McpIpcMain
participant McpClientManager
participant OAuthServer
Renderer->>Preload: mcp.login(serverId)
Preload->>RuntimeHost: invokeActiveRuntimeHost
RuntimeHost->>McpIpcMain: mcp:login
McpIpcMain->>McpClientManager: start and finish authorization
McpClientManager->>OAuthServer: discover, authorize, and exchange token
OAuthServer-->>McpClientManager: OAuth credentials
McpClientManager-->>McpIpcMain: authenticated status
McpIpcMain-->>Renderer: updated MCP status
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
All three boundaries are fixed in the updated head: P1 — scoped preload seam. P1 — the deadline cancels the round, not just the caller. P1 — failed credential erase fails closed. Implemented at the engine (#2653): the old entry stays blocked and non-connectable under its old config until the erase succeeds; the new endpoint never takes ownership while the old credentials survive. Delete-failure → reconnect-refusal → recovery is covered by regression there. Rebased onto current |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/mcp-page.tsx (1)
281-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSplit create and edit saves
saveDraftalways callswindow.maka.mcp.upsert, so a duplicate create overwrites the existing configuration. Useeditor.editingIdto callwindow.maka.mcp.addfor creates, handle{ status: 'exists' }on the ID field, and keepupsertfor edits.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/mcp-editor-draft.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the round-trip test this module was extracted to enable.
The header comment states the module is kept free of React so the Edit → Save contract is testable. This cohort adds no test for it. The unverified case is security-relevant:
draftFromConfigmust carryconfig.oauthandconfigFromDraftmust re-emit it, or an edit that touches only the URL deletes the OAuth block and the sentinel restore inmcp-secret-guard.tsnever runs.Do you want me to generate a test that asserts
configFromDraft(draftFromConfig(id, config), copy)preservesoauthfor a remote config and preservescwd/envfor a stdio config?Also applies to: 30-58
packages/mcp/src/index.ts (1)
1762-1806: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe alias layer can be deleted.
SecretInventory,EMPTY_INVENTORY,MIN_SUBSTITUTION_LENGTH,collectConfigSecrets,scrubKnownSecretsanddeepScrubonly rename imports from@maka/core/mcp-secrets. Two names now exist for each concept, which is the drift this module set out to prevent. Import the core names directly and delete the wrappers.Disposition: optional.
As per path instructions: "Flag concrete cases where code can be deleted or simplified."
Source: Path instructions
packages/mcp/src/__tests__/oauth.test.ts (2)
287-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert with
includesinstead of building a regex from a secret.The tokens are
token-${randomUUID()}today, so no metacharacter appears. The assertion still depends on that. Lines 484 and 579 already use!error.message.includes(...), so the file is inconsistent. Static analysis also flags the dynamic regex construction.♻️ Proposed change (pattern applies to all four sites)
- assert.doesNotMatch(status?.error ?? '', new RegExp(fixture.accessToken, 'u')); + assert.ok(!(status?.error ?? '').includes(fixture.accessToken));Disposition: optional.
Also applies to: 293-294, 331-332, 445-448
Source: Linters/SAST tools
505-537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test creates its race window with real sleeps.
The
slowstorage waits 120 ms per write, and the test waits 40 ms to land inside that write. Both numbers assume a lightly loaded machine. On a busy CI runner the 40 ms wait can overrun the 120 ms write,clearAuthorizationthen arrives after the write completes, and the test stops covering the mid-write case while still passing.The sibling test at Lines 359-397 uses explicit gates. Use the same approach: expose a promise the fake
setawaits, resolve it afterclearAuthorizationstarts.Disposition: follow-up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f782dd4-69da-490d-bb20-8884ee0ea13b
⛔ Files ignored due to path filters (3)
.maka-shots/after-dialog.pngis excluded by!**/*.png.maka-shots/before-dialog.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
apps/desktop/package.jsonapps/desktop/src/main/__tests__/mcp-editor-draft.test.tsapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-oauth-controller.test.tsapps/desktop/src/main/__tests__/mcp-preload-scope.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/mcp-ipc-main.tsapps/desktop/src/main/mcp-oauth-controller.tsapps/desktop/src/main/mcp-oauth-storage.tsapps/desktop/src/main/mcp-secret-guard.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.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/mcp/src/__fixtures__/stdio-server.tspackages/mcp/src/__tests__/manager.test.tspackages/mcp/src/__tests__/oauth.test.tspackages/mcp/src/__tests__/transport-security.test.tspackages/mcp/src/credential-coordinator.tspackages/mcp/src/index.tspackages/mcp/src/oauth.tspackages/mcp/src/transport-security.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; 0 remain after this review.
|
/agentic_review |
Code Review by Qodo
1. Stale resume blocks login
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
b8f6b73 to
2cb21bf
Compare
|
CodeRabbit findings addressed in the updated head:
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The controller gets the important security basics right: high-entropy state is checked before code/error, the callback preserves iss, loopback binding is explicit, and normal callback/timeout paths close the listener.
Two existing Qodo lifecycle findings remain actionable, so I am not duplicating them inline. First, denial/timeout/browser-launch failure leaves persisted verifier/redirect/state that boot resumes, repeatedly occupying the active guard and blocking an immediate retry. Second, openExternal() is awaited outside the shared deadline, so a hung shell launch can hold the listener and active round indefinitely. The smallest coherent fix is a narrow “abandon pending round” operation for terminal controller failures, plus putting browser launch under the same deadline; keep tokens/client registration intact.
The lower stacked OAuth PR #2653 also still has unresolved credential/PKCE/refresh findings, so this UI slice cannot be merge-ready independently. This head’s live checks are green, but merge state remains blocked.
Reviewed with Codex using three independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact head, existing threads, loopback/state/issuer flow, deadline/restart behavior, IPC boundaries, tests, and live CI.
中文
Controller 的关键安全基础是正确的：高熵 state 会在 code/error 之前校验，callback 保留 iss，loopback 绑定明确，正常 callback/timeout 路径会关闭 listener。
但两个已有 Qodo lifecycle finding 仍有效，我不重复发布行内问题。第一，拒绝/超时/browser launch failure 后仍保留 persisted verifier/redirect/state，boot 会不断 resume，占用 active guard 并阻塞立即重试。第二，openExternal() 没有进入共享 deadline，若系统 shell launch 永不结束，listener 与 active round 会被无限占用。最小一致修复是提供窄范围的 “abandon pending round” 操作处理 controller terminal failure，并让 browser launch 复用同一个 deadline；tokens/client registration 应保留。
下层 stacked OAuth PR #2653 也仍有 credential/PKCE/refresh finding，因此这个 UI slice 无法独立 merge-ready。当前 head 实时检查全绿，但 merge state 仍 blocked。
本次由 Codex 配合三个独立 reviewer agent，以及 OpenCode Go DeepSeek V4 Flash（high）审查；我核验了精确 head、已有线程、loopback/state/issuer flow、deadline/restart 行为、IPC boundary、测试和实时 CI。
|
This PR adds a user-visible Desktop OAuth login flow. Could you please add screenshots showing the MCP login action and the resulting authenticated or error state? One annotated composite is fine; please avoid including any credentials or sensitive callback data. Thanks! Posted by Codex on behalf of Astro-Han. |
2cb21bf to
78b90a5
Compare
|
Both Qodo lifecycle findings and the synthesis are addressed in the updated head: Stale resume blocks login (High) — exactly the narrow "abandon pending round" operation you suggested: the manager gained
Atomic setConfig restore (High) — same store-level Duplicate query secrets (High) — fixed in the guard slice (#2919): per-occurrence masking/restoring; see the regression there. Also, following Astro-Han's note on #2921 that renderer locks are not the authority: main now REJECTS config mutation for a server that has a login round in flight — the controller exposes |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for adding an owned Desktop OAuth round with bounded controller work and renderer locking. The current head fixes the existing timeout/abort and per-server concurrency threads, but one P2 remains inline: two IPC preflight awaits still occur before the controller deadline, so a stalled preflight can leave the visible login lock stuck indefinitely.
Please fix this after #2919 and #2653 land and the branch is rebased. This PR opens the system browser and renders success/failure callback pages, so it changes user-visible OAuth UX. The current .maka-shots artifacts do not show those surfaces; please add representative browser/callback screenshots, preferably covering both success and failure. The existing AI disclosure is sufficient because the commits identify Claude co-authorship and link the contributing session.
Reviewed with Codex as an AI-assisted code review. I verified the exact-head diff, IPC/controller deadline ownership, existing threads, CI, visual evidence, and provenance; no external model output was used.
中文说明
controller 内部的 timeout/abort 和 renderer lock 基本正确，但 mcp:login 在进入 deadline 前仍先等待 ensureReady 与 store.get；任一挂起都会让 UI lock 永不释放。请在前两层合并后 rebase，并把整个 preflight 纳入同一个 deadline。此 PR 会打开系统浏览器并展示 callback 页面，属于 UI/UX 改动；现有截图没有覆盖目标表面，请补成功/失败代表性截图。AI 说明完整。
78b90a5 to
34c89ac
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/desktop/src/main/mcp-secret-guard.ts (1)
309-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: mask per occurrence, not per key name.
queryKeyscollapses the scanner output to key names at Line 312. Line 323 then masks every occurrence of that key. For?q=hello&q=sk-ant-api03-..., the scanner marks only the second occurrence, but both are masked.The round trip stays symmetric, so no value is lost. The only effect is that a non-secret value is hidden from the editor, which contradicts the "free-form positions where full masking would destroy the editor" rationale at Lines 25-28. Disposition: optional.
Keep the occurrence index from the scanner order if you want the stated behavior.
Source: Path instructions
apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts (1)
236-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the env assertion; the env restore path runs here but is not checked.
Line 240 sends back
seenScratch, whoseenv.API_TOKENis a marker.restoreStdiorestores it from disk on the way to the store. Line 243 asserts onlyargs.The env restore is a distinct code path from the
arg-flag-valuepath. One more assertion covers it.💚 Proposed fix
assert.deepEqual(storedScratch.args, ['server', '--custom=sk-ant-api03-abcdef123456']); + assert.equal(storedScratch.env?.API_TOKEN, 'scratch-token');apps/desktop/src/renderer/mcp-editor-draft.ts (1)
30-58: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a guard against future MCP config drift. The draft currently covers every
McpServerConfigfield. BecauseconfigFromDraftcreates a new object, future fields can be silently dropped during edits. Add a type-level guard or observable round-trip test.Source: Path instructions
apps/desktop/src/main/mcp-ipc-main.ts (1)
84-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
McpConfigStore.insertAPI in a follow-up. No production caller exists in this repository. Keeping it duplicates the duplicate-ID rule and requires redundant test fixtures and storage tests. RetainMcpServerExistsErrorfor the IPC handler.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a80f3b01-f143-4cd6-a0f2-61f3a88fbf73
⛔ Files ignored due to path filters (3)
.maka-shots/after-dialog.pngis excluded by!**/*.png.maka-shots/before-dialog.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
apps/desktop/package.jsonapps/desktop/src/main/__tests__/mcp-editor-draft.test.tsapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-oauth-controller.test.tsapps/desktop/src/main/__tests__/mcp-preload-scope.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/mcp-ipc-main.tsapps/desktop/src/main/mcp-oauth-controller.tsapps/desktop/src/main/mcp-oauth-storage.tsapps/desktop/src/main/mcp-secret-guard.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/mcp-editor-draft.tsapps/desktop/src/renderer/mcp-page.tsxpackages/core/package.jsonpackages/core/src/__tests__/mcp-secrets.test.tspackages/core/src/mcp-secrets.tspackages/core/src/mcp.tspackages/core/src/redaction.tspackages/mcp/src/__fixtures__/stdio-server.tspackages/mcp/src/__tests__/credential-coordinator.test.tspackages/mcp/src/__tests__/manager.test.tspackages/mcp/src/__tests__/oauth.test.tspackages/mcp/src/__tests__/transport-security.test.tspackages/mcp/src/credential-coordinator.tspackages/mcp/src/index.tspackages/mcp/src/oauth.tspackages/mcp/src/transport-security.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/mcp/src/fixtures/stdio-server.ts
- apps/desktop/package.json
- packages/mcp/src/tests/transport-security.test.ts
- apps/desktop/src/main/runtime-host-boot.ts
- packages/core/src/redaction.ts
- apps/desktop/src/preload/bridge-contract.d.ts
- apps/desktop/src/main/mcp-oauth-storage.ts
- packages/mcp/src/tests/manager.test.ts
- apps/desktop/src/renderer/mcp-page.tsx
- packages/core/package.json
- apps/desktop/src/preload/preload.ts
- packages/core/src/mcp.ts
- packages/mcp/src/transport-security.ts
- apps/desktop/src/main/tests/mcp-secret-guard.test.ts
- packages/mcp/src/oauth.ts
- apps/desktop/src/main/mcp-oauth-controller.ts
- apps/desktop/src/main/tests/mcp-oauth-controller.test.ts
- packages/mcp/src/index.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.
| deps.ipcMain.handle('mcp:setConfig', async (_event, config: McpConfigFile) => { | ||
| const next = await deps.store.set(config); | ||
| // A bulk edit may change or remove a server mid-login. | ||
| const currentConfig = await deps.store.get(); | ||
| for (const [serverId, server] of Object.entries(currentConfig.mcpServers)) { | ||
| const incoming = Object.hasOwn(config.mcpServers, serverId) | ||
| ? config.mcpServers[serverId] | ||
| : undefined; | ||
| if (JSON.stringify(incoming) !== JSON.stringify(server)) assertNoActiveLogin(serverId); | ||
| } | ||
| // Credentials first here too: a bulk edit can REMOVE servers, and | ||
| // persisting the config deletion before the credential erase would — | ||
| // across a restart — orphan tokens a same-id re-add could inherit. | ||
| const removedIds = Object.keys(currentConfig.mcpServers).filter( | ||
| (serverId) => !Object.hasOwn(config.mcpServers, serverId), | ||
| ); | ||
| for (const serverId of removedIds) { | ||
| await deps.manager.forgetServerCredentials(serverId); | ||
| } | ||
| const next = await deps.store.transform((current) => | ||
| restoreMcpConfigSecrets(config, current), | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
removedIds comes from a snapshot taken outside the transform, so a concurrent add can be deleted without erasing its credentials.
Line 61 reads currentConfig in its own serialized slot. Lines 71-76 derive removedIds from that snapshot. Line 77 then replaces the whole config from a fresh current inside the transform.
If another handler (mcp:add, mcp:upsert, mcp:install) commits a server between Line 61 and the transform body, that server is not in config.mcpServers and not in removedIds. The transform deletes it, and forgetServerCredentials is never called for it. That is the exact orphaned-token outcome the comment at Lines 68-70 states this ordering prevents.
The restore itself is correct — it reads the transform's current. Only the removal set is stale.
An erase-before-write that is atomic with a wholesale replace would need a new store seam. The smallest correction that keeps the invariant is to detect the drift inside the transform and fail closed, so the renderer re-reads and retries.
Disposition: fix-now.
🔒 Proposed fix
const next = await deps.store.transform((current) => {
+ // The removal set and the credential erase were computed against the
+ // snapshot above. If the server set moved since, a server added in
+ // between would be deleted here with its credentials still on disk.
+ const snapshotIds = Object.keys(currentConfig.mcpServers).sort().join('\u0000');
+ const currentIds = Object.keys(current.mcpServers).sort().join('\u0000');
+ if (snapshotIds !== currentIds) {
+ throw new Error(
+ 'MCP configuration changed while this bulk edit was being applied — reload and retry',
+ );
+ }
+ return restoreMcpConfigSecrets(config, current);
+ });
- restoreMcpConfigSecrets(config, current),
- );Source: Path instructions
| deps.ipcMain.handle('mcp:logout', async (_event, serverId: string) => { | ||
| await deps.ensureReady(); | ||
| try { | ||
| return await deps.oauth.logout(serverId); | ||
| } finally { | ||
| changed(deps); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
mcp:logout keeps the preflight that mcp:login removed.
Line 202 awaits deps.ensureReady() outside any deadline. Line 189-191 states the rule for mcp:login: readiness runs inside the controller under the round deadline, so a stall cannot park the IPC promise and the renderer lock.
logout does not follow that rule. If ensureReady() hangs, the logout promise never settles and the renderer keeps its per-server lock. The blast radius is smaller than login, because logout only erases credentials, but the hazard class is the same one already resolved for login.
Move the readiness wait into oauth.logout under the controller deadline, or state why logout is exempt.
Disposition: follow-up.
Source: Path instructions
| if (value.scopes !== undefined) { | ||
| result.scopes = stringArray(value.scopes, `${serverId}.oauth.scopes`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed OAuth scope entries.
stringArray accepts oauth.scopes: [''] and oauth.scopes: ['read write']. Each array item must be one non-empty OAuth scope token. The current code persists malformed OAuth configuration.
Use an OAuth-specific scope-token validator here. Keep stringArray unchanged because other configuration fields can validly contain empty strings. Add regressions for empty and whitespace-containing scope entries.
Disposition: fix-now.
Proposed local correction
if (value.scopes !== undefined) {
result.scopes = stringArray(value.scopes, `${serverId}.oauth.scopes`);
+ if (result.scopes.some((scope) => !/^[!`#-`\[\]-~]+$/u.test(scope))) {
+ throw new Error(`${serverId}.oauth.scopes must contain valid OAuth scope tokens`);
+ }
}As per path instructions, this is a concrete, reproducible risk: “Report only concrete, reproducible risks.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (value.scopes !== undefined) { | |
| result.scopes = stringArray(value.scopes, `${serverId}.oauth.scopes`); | |
| } | |
| if (value.scopes !== undefined) { | |
| result.scopes = stringArray(value.scopes, `${serverId}.oauth.scopes`); | |
| if (result.scopes.some((scope) => !/^[!#-\[\]-~]+$/u.test(scope))) { | |
| throw new Error(`${serverId}.oauth.scopes must contain valid OAuth scope tokens`); | |
| } | |
| } |
Source: Path instructions
|
P2 — login preflight under the round deadline — done as prescribed, with the controller owning the preflight rather than a second timeout authority: Screenshots (real end-to-end rounds against a local OAuth+MCP fixture; no credentials shown — all tokens are throwaway fixture values): The login action and the resulting states, from the inspector:
The system-browser callback pages the controller renders (captured from the REAL loopback listener during live rounds — success, and a denied consent):
After a denied consent the app returns the server to needs-auth (the rejection toast carries only the sanitized RFC 6749 code): |
34c89ac to
eef4ed5
Compare
|
Restacked onto latest main (config version 2). Mechanical adaptation only: |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the OAuth controller remediation. I re-reviewed exact head eef4ed58b442174dabed5663e1973e7804a8ea68: the prior whole-round deadline, Runtime Host scope, timeout cancellation/fencing, resume cleanup, and direct remove/cancel ordering issues are fixed. The callback success/failure captures are valid current-slice UI evidence, and the Claude provenance is complete. I resolved 13 superseded or out-of-scope threads.
I cannot approve this head because the existing credential/config transaction thread remains a P1 blocker (PRRT_kwDOSpfFGs6aekEy). mcp:setConfig still computes removed IDs from a snapshot outside the later transform, so a concurrent add can be persisted as removed without having its credential pre-erased. URL-changing upsert/install/bulk edits also persist the new config before retiring credentials bound to the old endpoint; erase failure plus restart can then lose the in-memory tombstone while leaving credentials reusable by the old ID/URL.
The owner-level fix should be one serialized mutation transaction covering: read current snapshot → detect removals and URL changes → erase the affected credentials → conditionally persist the config, failing closed if the snapshot drifts. A second local removedIds patch would not close the authority gap.
Two non-blocking P3 threads remain open: logout preflight cancellation/timeout, and rejecting empty or whitespace-containing OAuth scope entries. The stack still needs #2919 and #2653 to land first, followed by a restack and full exact-head CI; the current workflows have not run.
AI-assisted review disclosure: OpenAI Codex performed the exact-head OAuth lifecycle, concurrency, credential-ordering, thread, screenshot, provenance, and stack analysis; I verified the reproduction, severity, deduplication, focused test evidence, and live GitHub state before posting.
中文说明
旧的 deadline、Runtime Host scope、timeout fencing、resume cleanup 和直接 remove/cancel 顺序问题都已修复，13 个过时或越界线程已关闭，callback 截图与 AI provenance 合规。
当前仍有一个 P1：配置更新与 credential 擦除没有由同一个串行事务拥有。bulk setConfig 在 transform 外读取旧 snapshot，并发新增可能被删却没先擦 credential；URL 变化也会先写新配置、后擦旧凭据，失败重启后会遗留可复用 credential。应由一个事务完成“读当前状态→识别删除/URL 变化→擦 credential→条件写配置”，snapshot 漂移就 fail closed。另有两个非阻塞 P3。上游合并后还需 restack 和完整 CI。
eef4ed5 to
d3f14cf
Compare
|
Restacked onto merged #2919 and fixed the P1 transaction blocker at the owner level (head One serialized config/credential transaction (
This closes both reported shapes: Regressions added: erase-failure-aborts-repoint (old URL survives), erase-before-write ordering, unchanged-URL upsert does not erase, drift fail-closed, and the updated install/cancel race sequence. Both P3s are also fixed: Full suites at this head: desktop 988, storage 840, mcp 164, core 560 — all green; biome clean. |
d3f14cf to
5a9e5d1
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this is a careful implementation and I want to be specific about that before the findings, because the parts that usually go wrong in an OAuth desktop flow are right here. Reviewed exact head 5a9e5d1e9c31b410dcc0ba492b3fd020a6980d0f; two independent passes, one on security, one on integration and boundaries.
A note on size first: GitHub reports 24 files and +5908, but this is a linear child of #2653 and this PR's own commit is 14 files, +2190/−151. The rest is the parent showing through.
It supplies exactly the two pieces #2653 assumed its caller would provide. The loopback listener binds 127.0.0.1 explicitly, takes an ephemeral port, matches /callback exactly on a parsed pathname, runs one round, and closes in a finally on success, failure and timeout with closeAllConnections(). The state gap I went looking for is closed twice over: the controller mints 128 bits with randomBytes, always passes it to both startAuthorization and finishAuthorization including on the resume path, and the listener verifies it before reading either code or error — so #2653's conditional binding check is never reached with an absent state from this caller, and a forged loopback access_denied cannot abort a live login. openExternal is behind a parsed-URL scheme allowlist. No token, verifier or authorization code crosses IPC; the error text that does is a strict RFC 6749 code allowlist rather than a shape check, and secretsFor harvests the PKCE verifier as well as the tokens. There is no console.* anywhere in the controller. Choosing the system browser and a loopback listener over a webview and a custom protocol is the right call, and the reason is worth keeping in the comment where it already is.
Some non-obvious things also check out. logout() deliberately not claiming the guard is correct, because coordinator.erase bumps the epoch before entering the lane, so an in-flight round's token write is refused rather than orphaned. resumeLogin claims before its first await, exactly as its comment says. The credential-first removal ordering is the fail-closed direction and is tested. And the CAS mapping in the new storage adapter has no TOCTOU — the second stage compares against the exact value read in the first, so a concurrent writer yields conflict rather than a clobber.
Four P2s and five P3s inline. Not approving while P2 findings are open.
One severity escalation to flag rather than re-file: isLoopbackHost's .localhost suffix clause is a #2653 line and I filed it there at P3, because at that head the OAuth path had no consumer and no token existed. In this PR's context it is worth more than P3 — this is where a bearer token starts being minted and stored, so a http://api.localhost/mcp entry that resolves off-box on a network whose DNS or search suffix an attacker controls now sends that token in cleartext, and the same name also passes this controller's isSecure check before openExternal. Reproduced: the predicate returns true for evil.localhost; off-box resolution is inference, since on darwin it resolved to ::1 and the exposure depends on glibc or Windows resolver behaviour. Fixing it in #2653 is right; I am noting the escalation so it does not get triaged at the parent's severity.
Two things about how this lands. Every IPC contract added here is dead at this head — no renderer calls mcp:add, mcp:login or mcp:logout until #2921 — which is legitimate for a declared stack but means neither a human nor any test above the IPC layer can exercise this end to end until that lands; worth saying in the body. And on splitting: +2190 is one revertable intent and I would not break it up further, with one exception at a seam that already exists — the transactional config-store rework (commitConfig, the exclusivity lane, credentialRetirements, mcp:add, the cancelInstall rollback change) is a distinct intent with its own revert semantics, it carries two of the four P2s below, and it is the part most needing to be read against the real store rather than a fake. assertNoActiveLogin is the only coupling and it already goes through an interface, so the seam is a one-line stub. If that split is declined, the regression test in the first finding becomes a merge condition rather than a nice-to-have.
Review disclosure: this review was prepared with Claude Code, which ran two parallel adversarial passes over this PR's own commit at this head, executed the loopback-host predicate and drove the real createMcpConfigStore to check the normalization claim, and ran reference searches for every claim of dead code. I checked each finding against the source before keeping it; evidence grade is stated per finding. The human contributor reviewed this before posting.
| // What THIS install committed, for the cancellation to compare | ||
| // against: a cancel must only roll back its own write, never a | ||
| // newer same-id configuration that landed after it. | ||
| operation.committed = JSON.stringify(installed); |
There was a problem hiding this comment.
[P2] Compare identity, not a serialized shape — this records the config before the store normalizes it, so cancellation silently no-ops whenever normalization changes anything. installed here is the restored config; commitConfig then persists through store.transform, and the real FileMcpConfigStore.transform runs normalizeMcpConfig, which rebuilds each server with a fixed key order and materializes enabled: true and transport: 'auto' and WHATWG-normalizes the URL. The later comparison against operation.committed therefore mismatches, cancelInstall returns current unchanged, the cancelled server stays in mcp.json, sync reconnects it, and the renderer shows it as installed. Reproduced by driving the real store: {"command":"npx","args":["-y","x"]} comes back as {"enabled":true,"command":...}; a remote entry gains "transport":"auto"; and key order alone is enough. Being fair about blast radius, I then ran all fourteen live mcp-catalog.ts entries through the same store and got zero mismatches today — the vercel entry (https://mcp.vercel.com → trailing slash) mismatches on the raw config and survives only because restoreUrlQuerySecrets happens to return parsed.toString(). So the guard is correct by coincidence, and the coincidence is one catalog entry away from breaking silently. Stamp the install with an opaque token held alongside installs and roll back iff it still matches, or normalize installed through normalizeMcpConfig before recording. The reason no test catches this is that every store in mcp-ipc-main.test.ts is a fake whose transform is config = apply(config); the regression test has to use a real createMcpConfigStore with a config omitting transport.
|
|
||
| const KIND = 'oauth_token' as const; | ||
|
|
||
| export function createCredentialMcpOAuthStorage(store: CredentialStore): McpOAuthStorage { |
There was a problem hiding this comment.
[P2] Move this adapter out of apps/desktop — it has no Electron dependency, and its location leaves the repo's other MCP manager credential-blind. createCredentialMcpOAuthStorage imports only @maka/mcp and @maka/storage, but living in Desktop main means packages/cli/src/runtime-host-capability-provider-command.ts, which constructs a second McpClientManager over the same mcp.json, cannot pass an oauthStorage. In that process pendingAuthorization short-circuits on if (!this.oauthStorage) return undefined and forgetAuthorization returns early on a missing coordinator. Concretely: a user authorizes a remote MCP server in Desktop; the CLI capability provider loads the same config, gets 401 on every connect, and is re-driven by the reconnect loop indefinitely — while a forgetServerCredentials call in that process reports success and erases nothing. Move it to packages/mcp or packages/storage and construct it from workspaceRoot at both sites, leaving Desktop only shell.openExternal and the listener. If the CLI path is deliberately out of scope for now, say so in the PR body — as written it reads as fully wired.
| copy?: { successTitle: string; successBody: string; failureTitle: string }; | ||
| } | ||
|
|
||
| export interface McpOAuthController { |
There was a problem hiding this comment.
[P2] Add a way out of a round, or tell the renderer one is in progress. The controller exposes login/logout/isActive/resumeLogin and no cancel, no mcp:cancelLogin channel is registered, and isActive is not projected to the renderer — while assertNoActiveLogin gates setConfig, add, upsert, install, remove and cancelInstall for that server. So the ordinary path is a trap: the user clicks Login, the browser opens, they recognise the server is wrong and close the tab, and for the next five minutes every edit answers MCP server "X" has a login in progress — including the remove that would evict it. It is worse unattended, because boot fires resumeLogin for every configured server, so a persisted round claims the guard for five minutes with no mcp:changed emitted at claim time and the user sees an unexplained veto on a server they never logged into. I checked #2921's own commit: it adds Login and Logout buttons and no cancel. Either add a cancel channel that aborts the round's deadline controller, or expose the active-round set in McpServerStatus so the renderer can show and explain the lock — and let remove pre-empt a round rather than be blocked by it. Regression test: start a login that never receives a callback, cancel it, assert the guard releases and a config mutation goes through immediately.
| transform(apply: (current: McpConfigFile) => McpConfigFile): Promise<McpConfigFile>; | ||
| /** Adds a new server; rejects with McpServerExistsError when the id is | ||
| * already taken, atomically with the write. */ | ||
| insert(serverId: string, config: McpServerConfig): Promise<McpConfigFile>; |
There was a problem hiding this comment.
[P2] Delete insert — it has no production caller, and every implementation and test fake now has to satisfy it. mcp:add reimplements the existence check inline inside commitConfig's transform, because it needs the same transaction to carry credentialRetirements and the active-login gate; a search at this head finds .insert( only in mcp-config-store.test.ts. #2921, the renderer consumer, adds no caller either — it adds ten more insert: stubs to test fakes, which is the cost compounding. Removing it drops roughly 40 production lines, 78 test lines, and a stub in every fake. Keep McpServerExistsError; the IPC layer does use that.
| * awaiting it unbounded would park the rejection — and the renderer's | ||
| * lock — forever. A late abandon completing afterwards is harmless: it is | ||
| * version-pinned against newer rounds. */ | ||
| const boundedAbandon = (serverId: string): Promise<void> => |
There was a problem hiding this comment.
[P3] Give the abandon its own short bound. boundedAbandon is bounded by the same timeoutMs as the round — five minutes by default — and is awaited inside the catch, so a round that times out at T+5min can hold the caller's promise for another five before rejecting. The comment's stated intent is not to park the caller on a wedged credential lane; as written it parks them for twice as long. A few seconds is enough for the thing it is guarding against.
| // rejects it — mark it handled so that path can't crash the process. | ||
| authorizationCode.catch(() => {}); | ||
|
|
||
| const server: Server = createServer((request, response) => { |
There was a problem hiding this comment.
[P3] Adopt the Host-pinning rule this repo's other loopback listener already established. apps/desktop/src/main/browser/cdp-bridge.ts is the in-repo precedent and it does three things: random loopback port, Host pinned to the loopback authority, and any request carrying a browser Origin rejected. This listener does the first and neither of the others, so it is open to DNS rebinding in principle — bounded in practice, because reaching any settle path still needs the 128-bit state, and an attacker without it can only fetch a static page. Worth closing anyway since the pattern is three lines away in the same package. Related, on the response itself: line 368 renders the authorization server's error_description verbatim into a page served from 127.0.0.1. It is HTML-escaped and both sinks are text nodes, so there is no XSS — the residue is that a hostile authorization server gets to put arbitrary instructional prose on a page the user reads as Maka's, which is a phishing surface even without markup. Show fixed local copy for the failure body and attribute the server's text clearly, or omit it; and add Cache-Control: no-store so the round-tripped code does not sit in browser history.
| // whole login (and the code coming back) to the network, so http is | ||
| // loopback-only — the same rule the config store applies to | ||
| // endpoint URLs. | ||
| const authorizationUrl = new URL(start.authorizationUrl); |
There was a problem hiding this comment.
[P3] This check can never refuse anything that reaches it, and its test proves only that. McpClientManager.startAuthorization already runs assertTransportSecurity(authorizationUrl, urlProvenance(new URL(config.url))) before returning {status:'redirect'}, and that predicate is strictly stronger — it additionally refuses a remotely-supplied loopback http destination unless the configured endpoint is itself loopback, where this one accepts any loopback http. So against the only real manager implementation this is unreachable as a refusal. The test at mcp-oauth-controller.test.ts:139 passes because it substitutes a stub manager that skips assertTransportSecurity, which makes it an assertion about this function's implementation rather than about any behaviour a user can reach. Either drop the check and the test, or keep it and say in the comment that it is deliberate defence-in-depth against a future non-McpClientManager implementer of McpOAuthLoginManager — that is a defensible reason, it just needs to be the stated one.
| } | ||
| // The shell launch rides the same deadline: a hung `openExternal` | ||
| // must not hold the listener and the active guard past it. | ||
| await deadline.race(deps.openExternal(authorizationUrl.toString())); |
There was a problem hiding this comment.
[P3] Record now that the issuer and scopes need to be shown before this opens. login() goes straight from the user's click to openExternal at a URL whose host, path, scope and resource are all chosen by the untrusted MCP server, with no in-app disclosure. The mitigation is real and deliberate — the system browser means the address bar and the authorization server's own consent screen do the disclosing, which is exactly why a webview would have been worse — and no renderer calls this at all yet, so nothing is user-reachable today. The reason to note it here rather than later is that the UI PR will inherit the omission silently if nobody writes it down: when #2921's Login button ships, the confirm step should name the resolved issuer origin and the requested scopes. That also depends on #2653 returning them, which I have raised there.
| // Same scoped channel as every other mcp:changed producer: the | ||
| // preload listener expects the runtime-host scope in the payload | ||
| // and drops a raw send. | ||
| if (status) sendActiveRuntimeHostEvent("mcp:changed", mcpManager.statuses()); |
There was a problem hiding this comment.
[P3] Three small ones, all deletions. This explicit mcp:changed emit duplicates the mcpManager.onChange handler registered earlier in this same file, and is strictly weaker — a successful resumeLogin ends in finishAuthorization → reconnect, which fires onChange, so this sends a second identical event and skips the capability refresh the handler does. Delete it and keep the .catch logging. Second: login()'s options.callbackPort parameter has no caller anywhere — production calls deps.oauth.login(serverId) and the tests exercise the deps.callbackPort resolver — so the options.callbackPort ?? branch is dead; deps.callbackPort is the single seam. Third, and a boundary rather than a deletion: the RFC 6749 §3.3 scope-token validation added at packages/storage/src/mcp-config-store.ts:293 belongs in #2653, which introduced normalizeOAuth — a revert of "Desktop login flow" should not silently relax config validation.
Remote servers that answer 401 now surface as 需要登录 instead of a connection error. Background connects run the SDK's OAuth client against stored tokens: silent refresh works (the provider always defines a redirectUrl — leaving it undefined routes the SDK into the non-interactive token path before it ever reads the refresh token), and a connect that would need the user refuses before dynamic registration and maps to the new needs-auth state. Interactive rounds live in startAuthorization / finishAuthorization: discovery reuses the state the background 401 round persisted (including a custom resource_metadata URL from WWW-Authenticate), a challenge probe — GET, then an initialize POST at the SDK's current protocol version, pressing on past parameterless challenges — carries the 401's scope into the authorization request, dynamic client registration (static clientId/clientSecret config as fallback), PKCE, and a persisted verifier + state so the exchange survives restarts. Credentials follow the endpoint they were issued for, and deletion is terminal. The stored record carries the server URL it was minted against and every read path drops a mismatched record (an offline mcp.json edit cannot replay a token against a new endpoint). All credential state transitions flow through one coordinator: a per-server operation lane carries every read, write and delete; removing a server, changing its URL, or logging out bumps a credential epoch in-flight flows are pinned to — so a stale flow can neither read old material past a queued delete, write a refresh result over a cleared record, nor delete what a newer flow just stored. Every write stamps a monotonically increasing version validated against the basis it read, and where the backing store exposes compare-and-set, an external edit trips the check instead of being silently overwritten. A failed delete holds the server in error rather than connecting anyway, and a 401 after connect (recognized through the scrub boundary, which preserves the transport's status code) marks the server needs-auth and bumps the tool-snapshot revision so stale capabilities drop out of the Runtime Host. Authorization stays bound to where it came from. The authorization URL a server supplies is checked against the provenance of the configured endpoint — transport security is not network authority, so remotely-supplied loopback cleartext http is refused unless the user themselves configured a loopback origin. The OAuth callback travels as a typed payload that preserves the `iss` parameter for the SDK's RFC 9207 issuer mix-up check. And a configured Authorization header and OAuth are mutually exclusive on the wire: once OAuth owns a connection's authorization — configured, or evidenced by stored credentials — the static header is dropped rather than raced against the bearer token. Secrets stay contained on every path out. Where they live is enumerated once, in @maka/core/mcp-secrets — the same location plan the desktop IPC guard masks by — so the scrubber and the boundary cannot drift. Requests: one scoped fetch carries every remote and OAuth request; configured resource headers ride only the endpoint's own origin, any redirect hop that crosses an origin sheds Authorization/Cookie for the rest of the chain, and no hop may downgrade to non-loopback cleartext http. Messages and payloads: errors, status strings, stderr tails, tool-call results, structured content and tool descriptors — object keys included, since a server can smuggle a credential through a property name — are all scrubbed of the config's credential values and of everything harvested from OAuth storage traffic (access, refresh and id tokens, registered client secrets, the PKCE verifier, and the in-flight authorization code during its exchange). A value long enough to be unambiguous is substituted in place; a message containing a credential too short to splice out is withheld wholesale, because the boundary allows no third option. Tested end to end against a real authorization-server fixture, including silent refresh, revoked-session recovery, replay refusal, logout-during-refresh finality in both interleavings, external-writer CAS refusal, forged- and genuine-issuer callbacks, Authorization exclusion under stored credentials, reflected-secret scrubbing across token endpoint / resource error / tool error / success payload / metadata / object-key / authorization-code paths, short-secret withholding, bearer stripping across cross-origin redirects, cleartext-downgrade refusal, and challenge scope propagation for GET, bare-GET and strict POST-only servers. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
login() binds an ephemeral 127.0.0.1 callback listener
(oauth.callbackPort pins it for statically registered clients) and
opens the system browser per RFC 8252 — refusing any authorization URL
that is not https or loopback http, since the URL comes from remote
OAuth metadata and a cleartext login off the machine would hand the
code exchange to the network. The listener verifies the OAuth state
round-trip before reading either the code or an error parameter, so a
forged access_denied on loopback cannot abort a real login — and it
settles with a typed payload that carries the code together with the
`iss` parameter, feeding the SDK's RFC 9207 issuer mix-up check
instead of dropping it on the floor. When a round does fail, only an
allowlisted RFC 6749 error code crosses toward the renderer; the
server-controlled error_description stays in the browser tab. A login
round interrupted by an app restart resumes at boot, from persisted
state alone: the resume claims its in-progress guard before any await,
rebinds the listener before and independent of the connect/publish
chain, treats a since-occupied port as nothing-to-resume rather than a
failure, and awaits readiness only for the final token exchange. One
deadline covers the complete round — discovery, the browser wait, and
the token exchange — so a remote endpoint that accepts a connection
and never answers cannot hold the in-progress guard or the loopback
listener; the listener closes on every exit. The deadline's abort
signal travels INTO the round — the manager aborts its requests and
fences its late storage writes on it — so a first round completing
after its timeout can neither exchange its code nor overwrite the
state of a newer round.
Tokens live in the shared CredentialStore (credentials.json, 0600) —
never in mcp.json — and the store exposes compare-and-set over the
stored record, so the runtime's credential coordinator refuses to
clobber a record something else edited underneath it.
The renderer reaches all of this through the same scoped Runtime Host
seam as every other MCP method: the handlers live on ScopedIpcMain,
whose first argument is the host ref, and a source-level contract test
pins every mcp: channel to invokeActiveRuntimeHost so a raw invoke
cannot sneak back in.
Removal is transactional in the safe direction: mcp:remove and
cancelled installs drop the server's stored credentials FIRST and
abort — config intact, retryable — if that fails, so a same-id re-add
can never inherit an orphaned token. The store gains an atomic insert
for the dialog's add path — a taken id answers as a typed
{ status: 'exists' } envelope (own-property checked) rather than as
prose fished out of a flattened IPC error string.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
5a9e5d1 to
088818d
Compare
|
Round addressed at head
Desktop suite 994+, cli 315, mcp 170 — green; biome clean. |
2 participants
Desktop activation slice of the #2653 split: the OAuth login flow, credential storage wiring, and the OAuth-aware IPC surface.
Stacked on #2653 (linear chain from the fork: #2918 → #2919 → #2653 → this): until those merge the diff shows their commits too — review the
feat(desktop): MCP OAuth login flowcommit.What
RFC 8252 loopback login.
login()binds an ephemeral 127.0.0.1 callback listener (oauth.callbackPortpins it for statically registered clients) and opens the system browser, refusing any authorization URL that is not https or loopback http. The listener verifies the OAuthstateround-trip before reading either the code or an error parameter (a forgedaccess_deniedon loopback cannot abort a real login), and settles with a typed payload carryingcode+issfor the engine's RFC 9207 issuer check. Failed rounds surface only an allowlisted RFC 6749 error code toward the renderer; the server-controllederror_descriptionstays in the browser tab.Restart-safe resume. A login interrupted by an app restart resumes at boot from persisted state alone: the resume claims its in-progress guard before any await, rebinds the listener independent of the connect/publish chain, and treats a since-occupied port as nothing-to-resume rather than a failure.
Credential storage. Tokens live in the shared CredentialStore (
credentials.json, 0600) — never inmcp.json— and the store implements compare-and-set over the stored record, activating the engine coordinator's external-clobber refusal.Transactional removal.
mcp:removeand cancelled installs drop stored credentials FIRST and abort (config intact, retryable) if that fails — a same-id re-add can never inherit an orphaned token. The config store gains an atomicinsert; a taken id answers the add dialog as a typed{ status: 'exists' }envelope (own-property checked).Tests
Controller tests run a real authorization server + loopback listener end to end (state verification, forged-error refusal, resume-at-boot in the claimed/occupied/clean-port interleavings, allowlisted error codes); IPC tests cover the add envelope and remove/cancel credential-first ordering; storage tests cover insert atomicity and CAS mapping.
Co-Authored-By: Claude noreply@anthropic.com
https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj