feat(mcp): OAuth for remote MCP servers - #2653
Conversation
GabrielDrapor
changed the title
b0d52fd to
7920585
Compare
|
CI triage for the two failing jobs — both pre-exist on
Everything that exercises this branch's own code — test, test_workspaces, test_runtime_host, storybook, package, audit — is green. Co-Authored-By: Claude noreply@anthropic.com |
7920585 to
f690f93
Compare
|
Rebased onto latest main ( Co-Authored-By: Claude noreply@anthropic.com |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for taking on this work. The OAuth direction is valuable, and much of the test suite exercises real HTTP and MCP SDK seams rather than testing mocks. However, I do not think the current PR is a safe or reviewable merge unit yet.
This is not primarily an editor/inspector polish PR. It combines four independently reviewable intents:
- command-line parsing and editor behavior;
- the
@maka/mcpOAuth engine; - Desktop credential storage, callback handling, secret-safe IPC, and activation;
- editor/inspector visual rework.
The OAuth and Desktop security changes account for most of the 4,572 added lines; the visual rework is only a small part of the total. Combining all four makes it difficult to establish which authority owns each invariant and prevents the smaller UI changes from receiving the quick review they otherwise could.
Please split this PR rather than continuing to patch all four slices in place. Given the current title and history, my recommendation is to narrow this PR to the @maka/mcp OAuth engine and its focused tests. Move Desktop activation, the command-line field, and visual polish to separate PRs. If the secret-safe configuration boundary is a prerequisite, land that as an even smaller preceding PR and retarget this PR onto it.
The findings below are not isolated edge cases. They converge on four missing ownership boundaries:
- IPC masking and runtime scrubbing do not share one secret inventory.
- A transport-security rule is being used as a network-authority rule, allowing remote metadata to inherit the loopback exception.
- The callback/controller contract truncates OAuth protocol state before the SDK can validate it.
- Serializing individual credential-store calls does not make a complete OAuth state transition atomic.
I recommend correcting those owners rather than adding another set of local guards:
- one shared secret-location/value plan for config views and runtime output scrubbing;
- provenance-aware URL policy that distinguishes a user-selected loopback endpoint from a remotely supplied destination;
- a typed callback payload that preserves SDK-required protocol fields such as
iss; - versioned atomic/CAS credential updates plus a per-server OAuth operation coordinator.
Please also make the configured Authorization header and OAuth bearer mutually exclusive authorities. With both present, the configured header can currently override the newly obtained OAuth token.
The current CI failures match the old base rather than this PR, so I am not treating them as PR findings. Current main is green, however, so the narrowed PR should be rebased and obtain a complete fresh CI result.
I am leaving this as a COMMENT review rather than Request Changes, but I recommend splitting and revising before merge because these include reproducible P1 credential, network, and protocol-boundary failures. I am happy to revisit severity or the proposed split if there is an architectural constraint I have missed—please feel free to push back with the intended authority model.
AI-assisted review disclosure
Codex performed the delegated adversarial analysis across OAuth standards, network security, credential concurrency, Desktop IPC, manager lifecycle, tests, UI behavior, and PR boundaries. Astro-Han reviewed the synthesized evidence and made the final decision to recommend a split and revision. The technical traces remain AI-assisted and should be independently verified by the contributor and the required human security/public-contract reviewer.
中文对照（默认折叠）
感谢推进这项工作。OAuth 的方向有价值，而且大量测试确实经过了真实 HTTP 和 MCP SDK 边界，并非单纯 mock 自证。但当前 PR 还不是一个安全、可审查的合并单元。
这不是一个单纯的编辑器或 Inspector 美化 PR。它同时包含 command-line 编辑、OAuth engine、Desktop 凭据与 IPC 安全边界、视觉重构四个可以独立审查的意图。OAuth 和 Desktop 安全改动占了 4,572 行新增中的绝大部分，视觉调整只占很小一部分。
建议必须拆分，而不是继续在当前 PR 中修补四个切片。结合当前标题与提交历史，建议当前 PR 只保留 @maka/mcp OAuth engine 及其 focused tests；Desktop activation、command-line field 和视觉重构分别拆出。如果 secret-safe config boundary 是前置条件，应先作为一个更小的前置 PR 合入。
下面的问题具有共同根因：secret inventory 没有单一权威；transport security 与网络访问授权混在一起；controller 截断了 SDK 需要验证的 OAuth 状态；单次存储调用串行化被误当成完整 OAuth 状态转换的原子性。
建议在职责 owner 处修复：统一 secret plan、按来源区分 URL 权限、传递完整 typed callback payload、使用 CAS/atomic update 和 per-server OAuth coordinator。同时应消除配置 Authorization header 与 OAuth bearer 的双重权威。
当前 CI 红灯与旧 base 一致，因此不作为 PR finding；但 current main 已经恢复绿色，拆分后的 PR 仍需 rebase 并取得新的完整 CI。
本次使用 Codex 进行多角度对抗性分析；Astro-Han 复核了综合证据，并决定建议拆分和修订。技术追踪仍属于 AI-assisted review，需要贡献者和仓库要求的独立人类安全/公开契约 Reviewer 再次验证。
本次使用普通 COMMENT review，不使用 Request Changes，但基于这些可复现的 P1 凭据、网络与协议边界问题，仍建议拆分并修复后再合并。欢迎基于明确的架构约束 push back，我们可以重新评估严重程度或具体拆分方式。
f690f93 to
ed202f1
Compare
GabrielDrapor
changed the title
|
Thank you for the review — the four ownership boundaries you identified were real, and fixing them at their owners (rather than patching locally) reshaped the code for the better. Done as recommended, in two parts: The split (linear chain, each PR reviewable by its last commit until the one below it merges):
The five corrections, each at its owner:
Each branch typechecks and tests green independently (mcp 92/92, storage, desktop 793/793 at the chain tip, biome clean). Fresh CI will run per PR; the chain is rebased onto current Happy to adjust slice boundaries if you'd prefer a different grouping. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head ed202f127d98a567482232ce3937ae0334f04d7e as the narrowed @maka/mcp OAuth-engine slice. The credential coordinator is the right authority for serialized transitions, epochs, and CAS, and the current checks are green. One production credential-integrity issue remains; see the inline finding.
This head is a coherent engine slice on top of the preceding secret-boundary work, so I do not recommend splitting it further. I did not identify a low-value test block to remove.
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.
ed202f1 to
bfd1d33
Compare
|
Good catch — fixed in the updated head. The harvesting storage wrapper now forwards the backend's New manager-level regression: a CAS-capable backend that reports |
bfd1d33 to
58d281f
Compare
|
CI triage for the current heads (whole stack rebased onto
|
58d281f to
91d1623
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thank you for tightening the OAuth boundary substantially. The URL binding, epoch/CAS ownership, redirect credential shedding, and secret scrubbing are strong improvements. I found two remaining fail-closed gaps that should be addressed before merge.
The simplest model is: credential cleanup is an authoritative state transition that must complete before connection ownership is removed, and OAuth is the sole owner of the Authorization header across open/start/finish. This removes special cases instead of adding another recovery path. The branch also currently conflicts with main, so these fixes should be applied after rebasing.
Reviewed with Codex using two independent review passes; I verified the cited control flow against this head and current main.
中文
这次对 OAuth 边界的收紧很扎实，尤其是 URL 绑定、epoch/CAS、重定向时剥离凭证和 secret scrub。仍有两个需要 fail-closed 的缺口。
最小方案是：删除凭证必须作为权威状态迁移，在移除连接所有权之前完成；同时 open/start/finish 全流程都只允许 OAuth 拥有 Authorization。当前分支也与 main 冲突，建议 rebase 后一起修复。
本次由 Codex 进行两轮独立审查，我核对了当前 head 与最新 main 的相关控制流。
91d1623 to
22fb972
Compare
📝 WalkthroughSummaryThis PR adds OAuth support for remote MCP servers in It also protects secrets across MCP boundaries. Shared scrubbing removes configured secrets from tool metadata, results, diagnostics, stderr, and errors. Desktop IPC redacts secrets before renderer responses and restores masked values only when configuration provenance remains valid. The PR adds transport security checks, issuer and redirect validation, OAuth and Source of truthThe PR extends the existing MCP configuration, connection manager, storage, redaction, and desktop IPC paths. It does not create a separate user-facing configuration system. OAuth state uses a dedicated storage abstraction because credentials need separate lifecycle and concurrency controls from server configuration. The desktop editor reuses the existing configuration model through extracted draft conversion helpers. Scope and complexityThe implementation is the smallest coherent solution supported by the current requirements and tests. OAuth requires explicit state, callback handling, credential persistence, cleanup, and concurrency protection. Secret handling requires shared secret-location discovery and provenance-aware restoration. Transport and issuer validation prevent unsafe redirects and credential disclosure. The added complexity is necessary for these guarantees. Test fixtures and helpers could be simplified later, but deletion would weaken coverage for credential races, redirects, issuer validation, secret reflection, cleanup failures, and abort handling. Validation
Complexity deltaThe PR adds:
The PR removes or centralizes:
Total maintenance complexity increases. The increase is justified by the required security, persistence, cleanup, and concurrency guarantees. The current evidence supports this conclusion through focused regression coverage and reported green product jobs. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThis change adds MCP OAuth support, credential coordination, transport validation, runtime secret scrubbing, desktop IPC secret protection, and shared editor draft utilities. Tests cover authorization flows, credential cleanup, redirects, secret handling, and configuration validation. ChangesMCP security and OAuth
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🟡 Moderate · up to OAuth credential handling still has bounded security risks: short credential fragments may appear in outbound diagnostics, and stored query credentials may be restored after URL userinfo changes. These issues could expose or broaden delivery of credential material, so merge should wait for fixes or explicit owner acceptance. Sequence Diagram(s)sequenceDiagram
participant Renderer
participant McpIpcMain
participant McpSecretGuard
participant ConfigStore
participant McpClientManager
Renderer->>McpIpcMain: send masked MCP configuration
McpIpcMain->>McpSecretGuard: restore stored secrets
McpIpcMain->>ConfigStore: persist restored configuration
McpIpcMain->>McpClientManager: synchronize configuration
McpClientManager-->>McpIpcMain: return scrubbed state
McpIpcMain-->>Renderer: return masked configuration
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Both fail-closed gaps are closed in the updated head, on your model: credential cleanup is an authoritative state transition that must complete before connection ownership moves, and OAuth is the sole owner of Authorization across open/start/finish. P1 — the connection stays tombstoned until the erase succeeds. Removal now erases FIRST and releases the entry only after success. On failure — removal or URL change alike — the entry is kept under its OLD config with P2 — Authorization exclusivity across all three flows. Also landed here at the engine (findings filed on #2920/#2921, implemented at their owner):
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: 2
🧹 Nitpick comments (1)
packages/mcp/src/__tests__/oauth.test.ts (1)
490-537: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe mid-write race window depends on wall-clock sleeps. The test proves the intended interleaving only if
saveTokensenters the 120 ms slow write inside the 40 ms wait. If the timing slips, the epoch check catches the write instead, and the assertion at line 536 still passes. The test then duplicates the coverage of "a logout during a token refresh is terminal" at lines 359-397 without adding the mid-write case.Consider gating on an observed event instead of a sleep. For example, resolve a promise from inside
slow.setbefore the delay, and startclearAuthorizationonly after that promise settles.Disposition: optional — the test does not fail spuriously today.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 741d07d3-3ed7-4d56-95ec-38fe66901456
⛔ 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 (21)
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/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; 2 remain after this review.
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| try { | ||
| await this.forgetAuthorization(serverId, entry?.credentialCleanupOwed ?? entry?.config); |
There was a problem hiding this comment.
1. Removal can orphan credentials 🐞 Bug ⛨ Security
Disposition: fix-now. sync() erases credentials only after the IPC layer has already removed the server from persistent config, so an erase failure followed by restart loses the blocked in-memory entry and leaves credentials that a same-ID/same-URL server can later inherit.
Agent Prompt
## Issue description
Server removal persists the config deletion before OAuth credential erasure. If erasure fails, a restart loses the manager's cleanup marker while the old credential record remains reusable.
## Issue Context
Reuse the PR's existing `forgetServerCredentials` seam rather than adding another cleanup authority. Both normal removal and install cancellation must complete credential erasure before calling the config store's remove operation.
## Fix Focus Areas
- apps/desktop/src/main/mcp-ipc-main.ts[77-93]
- packages/mcp/src/index.ts[1163-1178]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
22fb972 to
0ec82fe
Compare
|
CodeRabbit's two findings on this slice are fixed in the updated head:
Also hardened here from the sibling PRs' findings, since the owners live in this slice: the coordinator re-checks the flow guard AFTER the storage read (an abort or logout landing mid-read can no longer commit a stale verifier/token — deterministic regressions in the new |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The OAuth foundation is substantially improved: PKCE/state/issuer data reaches the SDK, credential records are endpoint-bound and fail closed when unbound, cross-origin configured headers are scoped, and logout has durable generation tombstones.\n\nThe existing concurrency thread still does not protect a complete refresh flow: a stale flow can re-read the newer record and delete another process’s freshly rotated token. The existing Qodo threads for concurrent PKCE rounds and cancelled background refreshes are also still actionable. I am not duplicating those inline findings.\n\nThree independent transport/capability issues remain below. The final-state principle is one lifecycle owner per connection generation: redirect scoping must shed every session credential, auth failure must revoke capabilities and retire the old transport, and reconnect must replace only after closing the prior generation.\n\nAll live checks are green, but merge state remains blocked by unresolved review findings.\n\nReviewed with Codex using three independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact head, existing discussion, OAuth storage flows, redirect handling, connection generations, capability publication, focused tests, and live CI.\n\n
中文
\n\nOAuth 基础已经明显完善：PKCE/state/issuer 数据会到达 SDK，credential record 与 endpoint 绑定且缺失绑定时 fail closed，跨 origin 的配置 header 被限制，logout 也有持久化 generation tombstone。\n\n但现有 concurrency 线程仍没有保护完整 refresh flow：过期 flow 可以重新读取较新的 record，再删除另一进程刚轮换出的 token。Qodo 关于并发 PKCE round 和取消后的后台 refresh 的线程也仍然有效；我不重复发布这些行内问题。\n\n下面还有三个独立的 transport/capability 问题。最终状态应让每个 connection generation 只有一个 lifecycle owner：redirect scoping 必须移除所有 session credential；auth failure 必须撤销 capability 并退休旧 transport；reconnect 只有在关闭上一代连接后才能替换。\n\n实时检查全绿，但 unresolved findings 仍使 merge state blocked。\n\n本次由 Codex 配合三个独立 reviewer agent，以及 OpenCode Go DeepSeek V4 Flash（high）审查；我核验了精确 head、现有讨论、OAuth storage flow、redirect handling、connection generation、capability publication、聚焦测试和实时 CI。\n\n
0ec82fe to
e69eed6
Compare
|
Everything raised on this slice is in the updated head: Concurrency body finding — a stale flow deleting another writer's rotation — the flow guard now carries a VERSION fence alongside the generation: captured on the flow's first read, advanced only by the flow's own writes, verified on every transition. A flow that read v1 while another process rotated to v2 has its delete/overwrite refused ( Concurrent PKCE rounds (Qodo) — Cancelled background refresh writes credentials (Qodo) — Removal orphans credentials (Qodo) — fixed at the ordering authority (stacked PR): Inline P1 — session credential on cross-origin redirects — Inline P2 — public refresh path 401 — Inline P2 — transport retirement — |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/desktop/src/main/mcp-secret-guard.ts (1)
352-362: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
sameRestomits URL userinfo; the query-secret restore is not fully pinned. Disposition: optional.
sameRestcomparesorigin,pathname,hash, key order, and unmasked values. It does not compareusernameorpassword. A renderer can sendhttps://injected@api.example.com/mcp?api_key=<marker>and the stored query secret is restored, becauseoriginexcludes userinfo.The impact is bounded. The host, port, protocol, path, and hash stay pinned, so the secret cannot be redirected to another endpoint. The credential is still forwarded under a launch basis the user did not configure, which is the exact condition the file header says must reject.
Note that
restoreRemoteline 269 does catch this for headers andoauth.clientSecret, becausenormalizeUrlincludes userinfo. Only the query path is looser.🔒 Proposed minimal pin
const sameRest = prior !== undefined && prior.origin === parsed.origin && + prior.username === parsed.username && + prior.password === parsed.password && prior.pathname === parsed.pathname && prior.hash === parsed.hash &&apps/desktop/src/main/__tests__/mcp-secret-guard.test.ts (1)
204-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a displaced-occurrence assertion for repeated query keys. Disposition: optional.
The repeated-key test asserts the happy path only. The moved-marker test at lines 228-245 covers env keys, not query occurrences. The per-occurrence binding in
restoreUrlQuerySecretsis therefore unprotected: if the occurrence index were dropped from the tag, this test would still pass while?token=a1a1&token=b2b2silently swapped or collapsed values.💚 Proposed addition
assert.deepEqual(new URL(restoredServer.url).searchParams.getAll('token'), ['a1a1', 'b2b2']); + + // Swapping the two occurrence-bound markers must reject, not swap the + // restored values. + const swapped = structuredClone(redacted); + const swappedServer = swapped.mcpServers.api; + assert.ok(swappedServer && 'url' in swappedServer); + swappedServer.url = + `https://api.example.com/mcp?token=${encodeURIComponent(mcpSecretMarker('query.token.1'))}` + + `®ion=eu&token=${encodeURIComponent(mcpSecretMarker('query.token.0'))}`; + assert.throws(() => restoreMcpConfigSecrets(swapped, previous), McpSecretRestoreError); });packages/mcp/src/index.ts (2)
838-841: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute these reads through the coordinator.
McpCredentialCoordinator.read()exists for exactly this case: "Lane-ordered read outside any flow (status displays, pending lookups)". Both call sites here readthis.oauthStoragedirectly, so they can observe a record mid-lane, between a flow's read and its commit.openClient()already usesthis.coordinator.read(serverId)for the same kind of lookup.Disposition: optional. The observable effect is a transiently wrong
authenticatedflag or a pending round reported during a write. Reusing the existing seam removes the second authority.♻️ Proposed change
const authenticated = - !isMcpStdioConfig(entry.config) && this.oauthStorage - ? Boolean((await this.oauthStorage.get(serverId))?.tokens) + !isMcpStdioConfig(entry.config) && this.coordinator + ? Boolean((await this.coordinator.read(serverId))?.tokens) : undefined;- if (!this.oauthStorage) return undefined; - const record = await this.oauthStorage.get(serverId); + if (!this.coordinator) return undefined; + const record = await this.coordinator.read(serverId);Also applies to: 1195-1216
2004-2076: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment at Line 2046 overstates what a bounce-back sheds.
credentialsShedremoves onlyCREDENTIAL_HEADERS. If a chain leavesserverUrl.originand later returns to it,initFor()re-adds every configured header (anX-API-Key, for example) becausetarget.origin === serverUrl.origin. The value goes back to the user-configured origin only, so this is not a leak, but the comment says credential headers "stay off for the remainder of the chain — even if it bounces back", which does not hold for configured headers.Disposition: optional. Correct the comment, or drop the configured headers as well once
credentialsShedis set.packages/mcp/src/__tests__/oauth.test.ts (1)
292-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
includesinstead of building a regex from a token value.
new RegExp(fixture.accessToken, 'u')treats the token as a pattern. The fixture tokens aretoken-<uuid>, so no metacharacter breaks it today, but the file already uses the direct form at Line 489 and Line 603 (assert.ok(!error.message.includes(verifier))). Static analysis flags all four sites.Disposition: optional. Consolidating on one form removes the warning and the latent pattern-injection risk if a fixture value ever changes.
♻️ Proposed change
- assert.doesNotMatch(status?.error ?? '', new RegExp(fixture.accessToken, 'u')); + assert.ok(!(status?.error ?? '').includes(fixture.accessToken));Also applies to: 298-299, 336-337, 450-453
Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b6ef8de-52a3-4c46-8def-c4d99b2f355f
📒 Files selected for processing (14)
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/__tests__/mcp-secrets.test.tspackages/core/src/mcp-secrets.tspackages/core/src/mcp.tspackages/mcp/src/__tests__/credential-coordinator.test.tspackages/mcp/src/__tests__/oauth.test.tspackages/mcp/src/credential-coordinator.tspackages/mcp/src/index.tspackages/mcp/src/oauth.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.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for centralizing OAuth credential transitions under a coordinator. The current head has the right lane/CAS/tombstone direction and fixes most earlier review items, but three owner-level gaps remain: two P1 revocation/binding races and one P2 cleanup gate bypass. They are called out inline and should be fixed here, after #2919 lands and this branch is rebased, rather than duplicated in dependent PRs.
The existing AI disclosure is sufficient: the commits identify Claude co-authorship and link the contributing session. This slice is runtime-only, so no screenshot is required. The branch is currently conflicting and needs an exact-head rereview after restacking.
Reviewed with Codex as an AI-assisted code review. I verified the exact-head diff, cross-process credential transitions, existing threads, CI, and provenance; no external model output was used.
中文说明
credential coordinator 的方向正确，但还有两个 P1 和一个 P2：update 可绕过 endpoint binding；flow 在首次 storage access 前没有固定 persisted generation；cleanup-owed 状态没有封住交互 OAuth。请等 #2919 合并后 rebase，并在本 owner PR 修复，不要在上层重复补丁。AI 说明完整，runtime-only 不需要截图。
e69eed6 to
c659d0d
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.
🧹 Nitpick comments (2)
packages/core/src/mcp-secrets.ts (1)
188-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider excluding the scheme token when splitting credential values.
A header value
Bearer <token>yields the partBearer, which is 6 characters, so line 191 pushes it intosubstitute. Every outbound message that contains the wordBearerthen becomes[redacted], including non-secret text such asWWW-Authenticate: Bearer realm="mcp". This loses diagnostic structure without protecting anything:Beareris a public scheme name, not credential material.A side effect appears in the tests:
assert.match(error.message, /\[redacted\]/u)inpackages/mcp/src/__tests__/oauth.test.tscan pass becauseBearerwas redacted, not because the token was.Disposition: optional. The smallest correction is to skip the leading part of a multi-part credential value, since only the tail carries the secret.
Proposed change
- for (const part of value.split(/\s+/u)) { - if (part === value || part.length === 0) continue; + const parts = value.split(/\s+/u); + for (const [partIndex, part] of parts.entries()) { + if (part === value || part.length === 0) continue; + // The leading token of "Bearer x" / "Basic x" is a public scheme + // name; substituting it redacts ordinary protocol prose. + if (partIndex === 0 && parts.length > 1) continue; if (part.length >= MCP_SECRET_MIN_SUBSTITUTION_LENGTH) { inventory.substitute.push(part); } else if (location.credential) {As per path instructions: "Flag concrete cases where code can be deleted or simplified."
Source: Path instructions
packages/mcp/src/__tests__/oauth.test.ts (1)
1090-1094: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe double cast to reach
beginFlowremoves type checking on an internal seam.
managerB as unknown as { beginFlow(...) }asserts a shape the compiler never verifies againstMcpClientManager. IfbeginFlowis renamed or its return type changes, this test does not fail to compile. It fails at runtime withflowB.set is not a function, orbeginFlowresolves toundefinedand the test throws before it reaches theassert.rejectsat line 1097.The invariant under test is real and worth keeping. Two options, in order of preference:
- Reuse the public seam. Drive the flow through
startAuthorizationwith a fixture that holds the probe, in the same wayholdRefreshholds the refresh. This keeps the test on observable behavior.- If the internal seam must stay, import the manager's own type for the cast so a rename breaks the build.
Disposition: optional. This is a test-maintenance cost, not a correctness defect.
As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88d7a544-22ed-443a-91fd-7ca2c952e63f
⛔ 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 (23)
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/__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 (20)
- packages/core/package.json
- packages/storage/src/tests/mcp-config-store.test.ts
- packages/core/src/tests/mcp-secrets.test.ts
- packages/core/src/redaction.ts
- packages/mcp/src/transport-security.ts
- apps/desktop/src/renderer/mcp-page.tsx
- apps/desktop/src/main/tests/mcp-secret-guard.test.ts
- packages/mcp/src/tests/credential-coordinator.test.ts
- apps/desktop/src/main/mcp-ipc-main.ts
- apps/desktop/src/renderer/mcp-editor-draft.ts
- packages/storage/src/mcp-config-store.ts
- packages/mcp/src/credential-coordinator.ts
- packages/mcp/src/tests/transport-security.test.ts
- packages/mcp/src/fixtures/stdio-server.ts
- packages/mcp/src/tests/manager.test.ts
- packages/core/src/mcp.ts
- packages/mcp/src/oauth.ts
- apps/desktop/src/main/tests/mcp-ipc-main.test.ts
- apps/desktop/src/main/mcp-secret-guard.ts
- packages/mcp/src/index.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
|
All three findings are fixed in the updated head, which is also restacked onto current P1 — endpoint binding before atomic updates — the provider now applies read()'s fail-closed binding to every mutation basis: P1 — pin the persisted generation at flow start — the coordinator gained P2 — cleanup-owed gates every OAuth path — the check moved into the shared |
c659d0d to
1835084
Compare
|
Restacked onto the dual-era negotiation rewrite of
mcp suite 164/164 green. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the runtime OAuth remediation. I re-reviewed exact head 18350848b9c2b53d20362c5187f47dae0cc9a836 and verified that the three prior blockers are fixed: endpoint-bound credentials are stripped before update, the coordinator generation is persisted before any remote await, and cleanup-owed state now fails closed across authorization flows. The focused MCP suite passes, and the Claude provenance is complete. I resolved 15 superseded threads.
I’m leaving COMMENT because this is stacked on #2919. Please merge #2919 first, then restack/rebase this PR onto current main and run the full checks for the resulting new SHA. The one remaining thread concerns Desktop credential-erasure ordering owned by the downstream #2920 activation slice; it is not a blocker in this runtime-only slice and should be deduplicated there.
AI-assisted review disclosure: OpenAI Codex performed the exact-head OAuth state, race, thread, provenance, and stack analysis; I verified the fixes, focused test results, ownership boundary, and live GitHub state before posting.
1835084 to
f0992b1
Compare
|
#2919 merged — restacked this PR onto current main ( |
f0992b1 to
b3f5224
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the SDK split is the right seam, and a lot of the hard parts here are done well. Reviewed exact head b3f5224d53ff793aadc2848a6f9fde8f6ba2d253; two independent passes, one on OAuth and transport security, one on integration and lifecycle.
What this solves: remote MCP servers requiring OAuth 2.1 are unusable today, so this adds a persistence and interaction layer around the SDK's auth protocol — background connects refresh silently and report needs-auth instead of error, while a user-initiated round hands out an authorization URL. Letting the SDK own the protocol and this own persistence and the interactive boundary is the correct division, and several non-obvious things are right: scopedFetch sheds credential headers for the rest of a redirect chain once any hop crosses an origin, and treats mcp-session-id and last-event-id as credentials; dropping the dynamically registered client and tokens when discovery moves to a different authorization server; PKCE is S256 with no plain downgrade path; every credential record carries serverUrl, so an offline mcp.json repoint cannot replay server A's token at server B; and the loopback pivot — a remote server aiming discovery at http://127.0.0.1:* — is closed and stays closed against every obfuscated literal we tried.
The finding I would act on first has nothing to do with OAuth. The new cleartext-URL rule in mcp-config-store.ts is the only part of this PR that is not inert at this head, and it fails the whole config file rather than the offending server. Details inline, reproduced by execution.
One P1, six P2s and three P3s inline. Not approving while P1/P2 findings are open.
On packaging: +3733 is not one revertable intent, and the seams are already separate files. The cleartext-URL policy (core/src/mcp.ts plus storage/src/mcp-config-store.ts) is the only part that changes behaviour for existing users, is the only part with a migration question, and needs no OAuth code — it should not ride into main inside a feature that is otherwise dormant. Transport hardening (transport-security.ts, scopedFetch, header shedding, error scrubbing) already has its own test file and its own non-OAuth effects. The OAuth engine is the third. If only one split is taken, take the first.
Review disclosure: this review was prepared with Claude Code, which ran two parallel adversarial passes over the diff at this head — one on OAuth spec conformance, SSRF and secret handling, one on lifecycle, ownership and deletable code — and I checked each surviving finding against the source before keeping it. Evidence grade is stated per finding; the P1 and one P2 were reproduced by execution, the rest are code reading or labelled inference. The human contributor reviewed this before posting.
| if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { | ||
| throw new Error(`${serverId}.url must use http or https`); | ||
| } | ||
| if (isNonLoopbackCleartextHttp(parsed)) { |
There was a problem hiding this comment.
[P1] Grandfather existing configs on read; refuse only on write. normalizeServer throwing here propagates through normalizeMcpConfig to readOrCreate, which recovers only ENOENT — so a single pre-existing http:// non-loopback server makes get(), upsert(), remove() and transform() all throw. It is all-or-nothing per file, not per server. Concretely: a user with {"internal": {"url": "http://mcp.internal.corp/mcp"}}, which every prior release accepted, upgrades and finds the MCP page empty — every other MCP server has disappeared too — and cannot delete the offending entry from the app, because remove takes the same path. Recovery means hand-editing mcp.json, and the CLI capability-provider command fails identically. Reproduced by execution: normalizeMcpConfig on main accepts that URL, and with one un-normalizable entry present, get, remove on a different server, and upsert of a new one all threw the same error. The comment in packages/core/src/mcp.ts says the editor mirrors this rule, but mcp-page-model.ts has no URL-scheme validation at this head, so that population exists in the field. Surface the offending server as error/disabled and keep the rest of the file loadable; refuse the scheme when a user tries to save it. Regression test: a config containing one non-loopback http server still returns the others from get() and can be repaired via remove().
| try { | ||
| result = await auth(provider, { | ||
| serverUrl: config.url, | ||
| scope: challenge?.scope || config.oauth?.scopes?.join(' ') || undefined, |
There was a problem hiding this comment.
[P2] Return the issuer and the resolved scope, not just a URL, so a consumer can show the user what they are consenting to. Both values are chosen by the untrusted server: scope comes verbatim from its WWW-Authenticate challenge, and the authorization server comes from the protected-resource metadata that same server points at via resourceMetadataUrl. McpAuthorizationStart carries only authorizationUrl, so the caller has nothing to render and the only place the issuer appears is inside a URL that is about to be handed to a browser. A malicious server can return scope="admin:all" with a PRM naming a real, well-known issuer whose resource field still passes checkResourceAllowed; the user sees a genuine consent screen from a name they trust, and the resulting token is then sent as Authorization: Bearer to the malicious server on every request. RFC 8707 resource is sent, but only helps against authorization servers that honour it, which most do not. Widen the result to { status, authorizationUrl, issuer, scopes } — both are available from provider.discoveryState() and the resolved scope — so the consumer must render them before opening a browser. Regression test: a fixture whose PRM names a third-party issuer, asserting the returned issuer is the third party rather than the server origin. Mechanism confirmed by reading code at this head; the end-to-end scenario is inference.
| if (url.protocol !== 'http:' && url.protocol !== 'https:') { | ||
| throw new Error(`refused non-HTTP request to ${url.protocol}//`); | ||
| } | ||
| if (url.protocol !== 'http:') return; |
There was a problem hiding this comment.
[P2] Extend the provenance rule to loopback and internal https, or state why it stops at cleartext. This early return means every https origin is permitted regardless of who supplied it, while this module's own header says a remotely supplied destination must not inherit the loopback exception — which is enforced for http: only. Since resourceMetadataUrl from WWW-Authenticate is passed straight into auth() and the SDK uses opts.metadataUrl verbatim with no origin check against the resource server, a malicious server can answer with resource_metadata="https://169.254.169.254/latest/meta-data/" and the client issues that GET; follow-on authorization_servers and token_endpoint values aim a form-encoded POST at any internal https host. It is blind SSRF — bodies do not return to the attacker — but it is a request-forgery primitive from inside the user's network. Reproduced by execution against the transcribed helpers: https://169.254.169.254/, https://192.168.1.1/, https://10.0.0.5/, https://127.0.0.1:8443/ and https://localhost:8443/ are all allowed under remote provenance. I am filing this as a contract decision rather than an oversight, because transport-security.test.ts asserts always allows https, regardless of provenance on purpose — so the ask is to make the decision explicitly: apply the same user-configured-endpoint test to loopback https, and either block private ranges or document that as accepted risk.
| // round's verifier overwrote the older one's. A caller that presents | ||
| // its round state must match the PERSISTED round, or its code would be | ||
| // exchanged against the wrong PKCE verifier. | ||
| if (callback.state !== undefined && record?.pendingState !== callback.state) { |
There was a problem hiding this comment.
[P2] Check the round binding whenever a pending state exists, not only when the callback supplies one. The condition keys off the caller-supplied value, so a callback that simply omits state skips the binding entirely — and that is reachable, because options.state is optional at startAuthorization and McpOAuthProvider defines state() only when one was passed, in which case the SDK omits the parameter. Anything that can reach the loopback redirect URI — any local process, or a page the user visits, since the port is guessable — can then submit ?code=<attacker code> with no state and have it redeemed against the pending verifier. PKCE makes the exchange itself fail, so the impact is round-mixing and denial of the in-flight login rather than token theft, which is why this is P2 and not P1; but the fix is one operator. Invert the condition to fire whenever record.pendingState is set, and make options.state required — or mint it inside startAuthorization with randomBytes, which this file already imports. The existing superseded-round test always passes an explicit state, so the omission path has no coverage.
| if (guard.generation !== undefined && basisGeneration !== guard.generation) { | ||
| throw new Error(`MCP credentials for "${serverId}" were revoked during the operation`); | ||
| } | ||
| if (guard.version !== undefined && (basis?.version ?? 0) !== guard.version) { |
There was a problem hiding this comment.
[P2] Serialize flows per server, not just per transition, or a routine background connect will kill a first login. Every flow pins guard.version at its first read, but nothing serializes two flows over one record — and the SDK persists discovery state on every authInternal pass, so a background connect against a 401-ing server is a writer, not just a reader. The sequence is ordinary: the server is needs-auth, the user clicks Login, and while startAuthorization's auth() is mid-discovery any sync() fires — the config watcher, an unrelated server edit, an install. The background connect's saveDiscoveryState bumps the version, and the login's own saveDiscoveryState/saveClientInformation/saveCodeVerifier then throws MCP credentials for "X" were rotated by another writer during the operation. The user's first login attempt fails with a message about rotation that describes nothing they did. The lease already exists — hold the lane for the flow rather than for each transition — or have startAuthorization cancel or await the in-flight connect through the existing entry.connectPromise/cancelConnect seam. Regression test: interleave a background connect that persists discovery into an interactive round and assert the round still completes. Inference from code at this head, with the SDK write path confirmed by reading the installed SDK.
| : remoteUrlChanged(current.config, serverConfig) | ||
| ? current.config | ||
| : undefined; | ||
| if (owed) { |
There was a problem hiding this comment.
[P2] Report a failed credential erase on the changed-endpoint path the way the removal path already does. The removal loop collects into removalFailures and rejects sync(); this branch catches, sets a per-server error status, and continues — so sync() resolves cleanly. The consequence is a silent divergence: the user edits an authenticated server's URL, the credential store is momentarily unwritable, mcp.json is already written to URL B while the manager keeps URL A blocked and unconnectable, and the caller that just wrote the config has no signal to roll back or warn. It persists until some later sync happens to succeed. Route the caught error into removalFailures so the config write path can react to it.
| // Absence still gets a tombstone: a cross-process flow that captured | ||
| // generation 0 on absence must not CAS its credentials back in after | ||
| // this revocation. | ||
| const tombstone: McpOAuthRecord = { |
There was a problem hiding this comment.
[P2] Do not write a tombstone for a server that never had a credential. commit() only ever calls compareAndSet/set and nothing in the coordinator calls storage.delete, so erase() writing a tombstone even when basis is undefined makes the credential file grow monotonically — and forgetAuthorization is deliberately not gated on server kind, while syncNow runs it for every removed connection. Install and remove ten stdio MCP servers and credentials.json holds ten {generation:1, version:1} records keyed by ids that never held a token, with no compaction path at all. The fencing that the tombstone buys — pinning a flow whose basis was absence — is real, but it is not needed where no flow can exist. Skip the erase when there is no record and the entry is stdio, or add compaction once no live flow can hold the prior generation. Regression test: removing a stdio server leaves the backing store untouched.
| * endpoints, and redirect hops. Storage validation, the runtime's fetch | ||
| * guard, the desktop OAuth controller and the editor's field validation | ||
| * all share it so the rule cannot drift. */ | ||
| export function isLoopbackHost(hostname: string): boolean { |
There was a problem hiding this comment.
[P3] Narrow isLoopbackHost so its guarantee does not rest on DNS. hostname.endsWith('.localhost') accepts any *.localhost name, and Node does not implement RFC 6761 section 6.3 — it hands the name to the system resolver. So http://mcp.localhost/mcp passes the new config-store rule and is then classified as a loopback trust root by urlProvenance, which reopens the cleartext exception for every remotely supplied destination. It needs resolver or hosts control to exploit, hence P3, but the docstring claims traffic never leaves the machine and that is not what the code checks. Narrow to hostname === 'localhost' plus the literal loopback addresses, or resolve and check the address. Worth noting the rest of this function is solid: 127.1, 2130706433, 0177.0.0.1 and [::ffff:127.0.0.1] are all correctly rejected, because WHATWG normalisation handles them. Reproduced by execution.
| // The status above is scrubbed; the rejection leaves the manager too | ||
| // (reconnect → IPC → renderer) and must not carry the raw message or | ||
| // the cause chain that holds it. | ||
| throw scrubbedError(exposedError, this.secretsFor(serverId, entry.config)); |
There was a problem hiding this comment.
[P3] Call out the cause-chain loss, or keep the sanitized cause here as the tool-call path does. This scrub drops cause from every remote connection failure, not only OAuth ones — visible as a contract change in manager-fallback.test.ts, where assert.ok(rejection.cause instanceof AggregateError) becomes assert.equal(rejection.cause, undefined). Losing the aggregate means a user whose remote server fails for an ordinary reason (DNS, TLS, connection refused across several candidate transports) now gets a message with no underlying cause to act on, which is a real diagnosability regression for servers that have nothing to do with OAuth. The tool-call path already solves this with an allowlisted sanitizedCause; apply the same treatment here, or say plainly in the PR body that connect errors lose their cause chain — right now the change is only visible as a flipped assertion.
| * stale record survive the id being freed for reuse. */ | ||
| private async forgetAuthorization( | ||
| serverId: string, | ||
| _config?: McpServerConfig, |
There was a problem hiding this comment.
[P3] Three small pieces of this surface are unreachable and can go. forgetAuthorization's _config parameter is unused, yet three call sites compute an argument for it, including the entry?.credentialCleanupOwed ?? entry?.config expression that exists only to feed it — drop the parameter and the computation. abandonAuthorization's if (storage.update) is always true because flowStorage always supplies update, so its implicit else is a silent no-op on a security-relevant clear that can never run; inline it, or type the coordinator's view with update required. And McpOAuthStorage.delete has no production caller — the only storage.delete calls run against the coordinator's flow view, whose delete is transition(→ {}), so the base backend's delete is reached only by the in-memory test storage, while every third-party backend must still implement it. Also one stale comment worth fixing while you are here: the background-flow comment claims the flow is fenced on the connection's abort, but connect() clears entry.connectController in its finally, so disconnect() aborts nothing once a connect has succeeded — only the epoch bump actually fences a live connection.
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
b3f5224 to
6c56aaf
Compare
|
Round addressed at head
Also, per #2920's review, the RFC 6749 scope-token validation now lives in this PR (it owns |
2 participants
Narrowed per the review: this PR now carries only the
@maka/mcpOAuth engine and its focused tests, rebased onto current greenmain. The other slices moved out:Stacked on #2919 (linear chain from the fork): until it merges, the diff also shows #2918/#2919's commits — review the
feat(mcp): OAuth for remote MCP serverscommit.All four architectural corrections from the review (plus the Authorization exclusivity) are implemented at their owners:
1. One shared secret plan
The runtime scrubber now derives its inventory from
@maka/core/mcp-secrets(#2919) — the same location plan the IPC guard masks by. Substitution vs withholding semantics live in the shared module; this PR consumes them for every outbound error, status string, stderr tail, tool payload and descriptor (object keys included).2. Provenance-aware URL policy
transport-security.tsseparates transport security from network authority: a remotely-supplied authorization URL may use loopback cleartext http only when the user-configured endpoint origin is itself loopback (or the same origin). A remote server can no longer steer the browser into the loopback exception. The scoped fetch re-checks the rule per redirect hop with the same provenance.3. Typed callback payload preserving
issfinishAuthorization(serverId, { code, iss? })carries the callback'sissthrough to the SDK's RFC 9207 issuer mix-up check. Tests cover both genuine and forged issuers against a real authorization-server fixture.4. Credential coordinator: versioned CAS + per-server lanes + epochs
credential-coordinator.tsis now the single owner of credential state transitions: every read/write/delete flows through a per-server lane; a complete read-apply-write transition is one lane operation pinned to a credential epoch (logout/removal bumps it — in-flight flows may finish reading but their writes are refused); every write stamps a monotonically increasing version validated against its read basis, and storage backends exposing compare-and-set (the desktop CredentialStore does) refuse external clobbers instead of silently overwriting.5. Authorization header / OAuth bearer exclusivity
Once OAuth owns a connection's authorization — configured, or evidenced by stored credentials — the configured
Authorizationheader is dropped from every request, so it can never override a freshly obtained token. The config store (#2919) rejects declaring both outright.Tests
92 tests in
@maka/mcp, exercising a real HTTP authorization-server fixture end to end: silent refresh, revoked-session recovery, replay refusal, logout-during-refresh finality in both interleavings, external-writer CAS refusal, forged/genuineiss, 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
https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj