feat(desktop): authorize client capabilities in managed sessions - #4143
Conversation
35facba to
b7dd148
Compare
|
|
||
| function navigationResult(url: string, requiresApproval: boolean): string { | ||
| return ( | ||
| `Navigated to ${sanitizedPageUrl(url)}.` + |
There was a problem hiding this comment.
[P1] Redact destination details beyond the Origin before approval
requiresApproval means this navigation crossed into an Origin for which the Session has no Grant, but sanitizedPageUrl() still exposes its pathname. A destination such as https://other.example/reset/<token>?source=...#account therefore reveals <token> to the model before other.example is approved.
Please format the two cases separately: when requiresApproval is true, parse only HTTP(S) URLs and return at most url.origin (or a generic unapproved-site marker), dropping userinfo, pathname, query, and fragment; when it is false, preserve the current origin + pathname result for same-Origin navigation. Invalid or non-HTTP(S) destinations should remain generic.
Please also cover sensitive path segments for redirect and click navigation, including an A→B→A or multi-hop case, so the first violated URL recorded by the monotonic lease cannot leak destination details.
2ba3ae1 to
f6d014a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving managed Client Capability admission into the Runtime Host and keeping the approval/grant commit atomic. One current-main integration boundary still needs a manual merge rather than choosing either side of the conflict. This is a suggestion from an outside review, so please do push back if the final owner-association contract differs from what is now on main.
AI-assisted review disclosure: Codex ran independent authority, security, recovery, and test analysis lanes; Astro-Han is the contributor of record for this review.
| readonly providerId: string; | ||
| readonly principalId: string; | ||
| readonly clientInstanceId: string; | ||
| readonly principalKind: ClientCapabilityConnectionIdentity['principalKind']; |
There was a problem hiding this comment.
[P1] (category ③ — Client/provider trust boundary)
Thanks for making the local Desktop provider trusted for this slice. During rebase, this identity state also needs to preserve the Client-owner binding that current main added in #4187. This head still falls back to the sole global provider when the initiating Client has no same-ID provider, and the state does not carry credentialBoundClientInstanceId / capabilityOwner; a remote owner can therefore inherit an unrelated trusted Desktop provider whenever it is the only candidate. Keeping only the PR side loses remote isolation, while keeping only main leaves the local local_owner untrusted and breaks the intended Auto Browser path. Could the conflict resolve to one authority that preserves #4187’s credential-bound owner association and remote fail-closed behavior while explicitly admitting the authenticated local owner, with local Auto / remote unrelated / remote associated regressions? Please feel free to push back if another final identity seam supersedes this coordinator.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the substantial recovery and ownership work in this update. One cancellation seam still appears able to keep the current Code Mode run alive indefinitely. This is an AI-assisted review; I independently traced the broker, interaction, and drain paths. These are suggestions from an outside perspective, so please do push back if the approval lifetime is intentionally governed by a different invariant.
| turnId: options.context.turnId, | ||
| runId: options.context.runId, | ||
| toolCallId: options.context.toolCallId, | ||
| providerSignal: prepared.providerSignal, |
There was a problem hiding this comment.
Thanks for adding Host approval here. In the category ② cancellation/recovery path, could the caller signal also close this approval? After the provider sends accepted, the invocation broker clears its timer. If Code Mode’s 30s deadline or the caller then aborts, the broker settles/removes the invocation and sends cancel/release, but it does not abort prepared.providerSignal; requestClientCapabilityApproval() listens only to that provider signal. prepareTool() therefore remains blocked on this decision, and executeCodeCell() waits for the host-operation drain before returning. The same pending approval promise can also be reused by later calls in the scope. That turns a bounded caller timeout into an unbounded turn/Code Mode hang, so I believe this is P1. Could approval observe the caller cancellation too, close the durable interaction, clear the pending entry, and add an accepted → approval pending → caller abort regression? Please push back if approvals are deliberately allowed to outlive callers and another owner guarantees the drain still terminates.
f32104d to
600ba0c
Compare
|
@Astro-Han Thanks for the detailed review. Both P1 findings have now been addressed on the current head. Client/provider trust boundaryResolved in The rebased coordinator now preserves the owner-association contract from #4187:
Regression coverage includes the local Auto path, a remote owner spoofing the built-in names, an unrelated remote provider, and an exactly associated remote provider. Caller cancellation while approval is pendingResolved in The caller signal is now forwarded into Client Capability approval and observed independently from the provider signal:
The end-to-end regression covers 中文感谢详细审查。这两个 P1 问题都已在当前 head 中修复。 Client/Provider 信任边界已在 rebase 后保留了 #4187 引入的 Owner 绑定:
测试覆盖了本地 Auto、远程名称伪造、无关远程 Provider,以及精确关联的远程 Provider。 等待审批期间的 Caller 取消已在 Caller 的取消信号现在会传入 Client Capability 审批流程,并与 Provider 断开信号独立处理:
端到端测试覆盖了 |
|
@me2seeks Thanks for catching this. This P1 has been resolved in The two result paths are now handled separately:
The monotonic Origin lease continues to retain the first violated URL, but that value is passed through the approval-required formatter before reaching the model. Regression coverage includes sensitive redirect and click paths, non-web destinations, snapshot and takeover A→B→A transitions, and the Provider second-check → first-page-await gap. 中文感谢指出这个问题。该 P1 已在当前 head 的 现在会分别处理两种返回路径:
单调 Origin lease 仍会记录首次违规 URL,但该值在返回模型前必须经过“需要审批”的脱敏格式化。回归测试覆盖了包含敏感路径的 redirect 和 click、非 Web 地址、snapshot 与 takeover 中的 A→B→A,以及 Provider 第二次检查到首次页面 await 之间的竞态窗口。 |
| clientCapabilityScopeIdentity(target.scope), | ||
| ].join('\0'); | ||
| const existing = this.#pendingApprovals.get(key); | ||
| if (existing) return existing; |
There was a problem hiding this comment.
[P1] Make every shared-scope waiter observe its own caller cancellation
When call A owns the pending approval and call B reaches the same key, this branch returns the Promise owned by A directly, so the callerSignal of B is never observed by the interaction coordinator. The invocation broker does react to the abort of B, but prepareTool() remains blocked on the approval of A and the cancelled call cannot drain.
I reproduced this on the current head with two real ToolRuntime.settleToolCall() calls: both reached provider accepted, they shared one pending interaction, then B was aborted. B remained unsettled until A was approved; only then did B continue and fail at the pre-T1 abort check. Adding a per-waiter abort race here made the same regression pass while leaving the approval of A intact.
Please make each joined waiter independently race its own caller signal, without closing the owner approval merely because a joiner cancels; cancel the prepared invocation of that waiter; and cover: two accepted calls → one shared approval → abort B → B settles before any decision → A can still be approved and publish the Session Grant.
7e4319a to
f07de3f
Compare
|
@me2seeks Thanks for catching this. Fixed in f07de3f. Each waiter that joins an existing shared-scope approval now races that approval against its own The integration regression uses real
I also rebased the PR onto current Focused verification: the final ToolRuntime and shared-approval regression suite passed 13/13; the broader Client Capability, Browser, Interaction, storage, and UI suites passed 200/200. 中文感谢指出这个问题,已在 f07de3f 修复。 现在每个加入共享 scope 审批的等待者,都会使用自己的 集成回归测试使用真实的
同时已 rebase 到最新 最终关键回归测试 13/13 通过;Client Capability、Browser、Interaction、storage 和 UI 的聚焦测试 200/200 通过。 |
f07de3f to
4a36d4c
Compare
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
4a36d4c to
45f92e3
Compare
Generated-by: OpenAI Codex
…che#4143) * refactor(runtime-host): separate capability acceptance from admission Generated-by: OpenAI Codex * feat(runtime-host): carry capability admission evidence Generated-by: OpenAI Codex * feat(storage): persist client capability session grants Generated-by: OpenAI Codex * refactor(core): centralize capability grant identity Generated-by: OpenAI Codex * feat(core): define client capability approval interactions Generated-by: OpenAI Codex * feat(storage): atomically commit capability approvals and grants Generated-by: OpenAI Codex * feat(runtime-host): host client capability approval interactions Generated-by: OpenAI Codex * feat(runtime): prepare client capabilities before durable dispatch Generated-by: OpenAI Codex * fix(storage): share capability grants by scope Generated-by: OpenAI Codex * feat(runtime-host): authorize managed client capabilities Generated-by: OpenAI Codex * feat(desktop): attest browser origins before admission Generated-by: OpenAI Codex * fix(desktop): limit cross-origin browser results Generated-by: OpenAI Codex * fix(runtime-host): close approvals when providers disconnect Generated-by: OpenAI Codex * feat(desktop): present client capability approvals Generated-by: OpenAI Codex * fix(runtime): classify client capability interaction events Generated-by: OpenAI Codex * fix(runtime-host): canonicalize client capability provider ids Generated-by: OpenAI Codex * fix(runtime): route client capability host admission Generated-by: OpenAI Codex * fix(desktop): enforce browser origin leases Generated-by: OpenAI Codex * chore(runtime-host): advance capability protocol epoch Generated-by: OpenAI Codex * chore(desktop): sync capability prompt inventory Generated-by: OpenAI Codex * test(desktop): complete side chat story port Generated-by: OpenAI Codex * test(runtime-host): honor capability admission handshake Generated-by: OpenAI Codex * test: trim duplicate capability coverage Generated-by: OpenAI Codex * test: simplify capability coverage Generated-by: OpenAI Codex * fix(desktop): redact unapproved browser destinations Generated-by: OpenAI Codex * fix(desktop): keep capability approval outside legacy shell Generated-by: OpenAI Codex * fix(runtime-host): preserve local capability trust Generated-by: OpenAI Codex * fix(runtime-host): close approvals with callers Generated-by: OpenAI Codex * fix(runtime-host): isolate shared approval waiters Generated-by: OpenAI Codex * docs: refresh Astryx surface inventory Generated-by: OpenAI Codex --------- Co-authored-by: YayoiNanoka <275864479+YayoiNanoka@users.noreply.github.com>
Summary
Allow trusted macOS Desktop Client Capabilities to cross the managed execution boundary without switching the Session to Full Access.
This is the first stacked slice for #4012. Computer Use and Desktop MCP are intentionally left to follow-up PRs.
Refs #4012
Verification
git diff --check main...HEADpassed.Draft follow-up
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex contributed implementation, tests, commit preparation, and this PR description. Each affected commit carries
Generated-by: OpenAI Codex.This draft was prepared and submitted by OpenAI Codex on behalf of the human contributor of record, YayoiNanoka.
Checklist
Does this PR entail a change in behavior?