fix(runtime-host): gate Windows Local Owner authority - #3179
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughSummary
Design assessmentThis change extends the existing Runtime Host listener and endpoint verification flow. It does not create a parallel authentication path. The solution is the smallest coherent approach shown by the diff. The Windows script adds complexity because it must create a foreign user, impersonate that user, and test the same named-pipe endpoint. This validates the OS boundary without a native helper or additional authentication state. No changed code or test can be removed without weakening the startup race protection or cross-user regression coverage. The endpoint change is documentation-only apart from the existing hook behavior. Risks and validationThe main risk is incorrect connection handling during the interval before endpoint verification. The listener now retains and aborts those connections instead of accepting them. The Windows verification also depends on temporary-user creation, C# impersonation, process startup, timeout handling, and cleanup. The PR objectives report successful build, typecheck, 962 distribution tests, Biome, Review-relevant risks
The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe Runtime Host now defers Windows Local IPC admission until endpoint verification. A Windows fixture and PowerShell probe verify same-user acceptance and foreign-user denial. Windows CI runs the verification after building test artifacts. ChangesWindows Local IPC trust
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change gates Windows Local Owner authority until endpoint trust is established and adds cross-user validation; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
2713659 to
8da31fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/runtime-host/src/__tests__/fixtures/windows-local-ipc-trust-host.ts (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelease stdin so the fixture exits by itself.
process.stdin.resume()keeps a referenced handle on the event loop. AftercloseAdmission()andcleanup(), that handle stays active, so the process does not exit. The PowerShell script then waits 5000 ms inWaitForExit, callsKill($true), and logs a non-zero exit code as a warning. Pause or unref stdin after the await to keep the exit clean.♻️ Proposed fix
process.stdin.resume(); await once(process.stdin, 'data'); +process.stdin.pause(); +process.stdin.unref(); await listener.closeAdmission(); await listener.cleanup();scripts/windows-runtime-host-local-ipc-trust.ps1 (1)
103-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence the PSScriptAnalyzer error for the ephemeral CI password.
PSScriptAnalyzer reports
PSAvoidUsingConvertToSecureStringWithPlainTextas an error on line 105. The password is generated per run for a throwaway local account, so the finding is acceptable, but an unsuppressed error can fail a lint lane. Add a scoped suppression with the reason.♻️ Proposed suppression
+[Diagnostics.CodeAnalysis.SuppressMessageAttribute( + 'PSAvoidUsingConvertToSecureStringWithPlainText', + '', + Justification = 'Ephemeral per-run local account password for cross-user trust verification.' +)] +param() + $ErrorActionPreference = 'Stop'Source: Linters/SAST tools
.github/workflows/ci.yml (1)
153-157: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope the job token permissions.
zizmor reports that this job inherits default permissions. The lane only checks out code and runs a build plus a script. Set
permissions: {}on the job to drop the token grant.🔒️ Proposed fix
windows_local_ipc_trust: needs: changes if: needs.changes.outputs.runtime_host == 'true' runs-on: windows-2025 + permissions: {} steps:Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0803846d-8d83-442a-9fdc-e452eed7308d
📒 Files selected for processing (7)
.github/workflows/ci.ymldocs/architecture/runtime-host-architecture.mddocs/architecture/runtime-host-architecture.zh-CN.mdpackages/runtime-host/src/__tests__/fixtures/windows-local-ipc-trust-host.tspackages/runtime-host/src/control/endpoint.tspackages/runtime-host/src/server/local-ipc-listener.tsscripts/windows-runtime-host-local-ipc-trust.ps1
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
Local IPC previously accepted connections before endpoint verification, while the Windows named-pipe boundary had no blocking cross-user evidence. Gate startup connections until the endpoint is trusted and require a real Windows foreign-user admission check for Runtime Host changes. Generated-by: Codex
8da31fd to
3636f76
Compare
PR Summary by QodoGate Windows Local Owner authority behind IPC endpoint trust
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Foreign read connections remain allowed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| async prepareAfterListen() { | ||
| // Node creates the pipe with the process token's default DACL. The | ||
| // blocking cross-user CI pins that a foreign user cannot open it duplex. | ||
| }, |
There was a problem hiding this comment.
1. Foreign read connections remain allowed 🐞 Bug ⛨ Security
Disposition: Fix now. The Windows branch performs no endpoint hardening and tests only duplex denial, although the default named-pipe DACL permits Everyone read access; a foreign user can therefore establish read-only connections that receive Local Owner authority at the listener and occupy the unbounded handshake set until timeout, violating the documented same-user boundary and enabling connection-flood denial of service.
Agent Prompt
## Issue description
The Windows Local IPC endpoint relies on the default named-pipe DACL and verifies only that foreign users cannot open it duplex. Windows' default descriptor still grants read access to Everyone, so foreign read-only connections can reach the listener and consume Runtime Host handshake resources.
## Issue Context
Reuse the existing `prepareAfterListen()` trust-boundary seam to enforce and verify a current-user-only pipe ACL. Deleting the Windows path cannot satisfy the required Local IPC behavior, and protocol authority cannot be consolidated safely while foreign connections still reach the endpoint; explicit endpoint hardening is therefore required. Extend the Windows probe to test every relevant access direction, including read-only access, rather than introducing a separate authority mechanism.
## Fix Focus Areas
- packages/runtime-host/src/control/endpoint.ts[39-42]
- packages/runtime-host/src/server/local-ipc-listener.ts[22-36]
- scripts/windows-runtime-host-local-ipc-trust.ps1[51-79]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
English
Local IPC no longer grants Local Owner authority to connections opened before the endpoint trust boundary is established. Runtime Host changes now also run a blocking Windows check that admits the current user and attempts the same duplex named-pipe connection under a separate standard-user token.
The real Windows check confirms that the platform boundary denies a foreign standard user, so no native helper or additional authentication state is required.
简体中文
Local IPC 不再向 endpoint 信任边界建立前打开的连接授予 Local Owner authority。Runtime Host 变更现在还会运行 blocking Windows 检查：确认当前用户可以连接，并使用独立标准用户 token 尝试相同的双向 named-pipe 连接。
真实 Windows 检查已确认平台边界会拒绝其他标准用户，因此不需要 native helper 或额外认证状态。
Refs #2522
Verification
English
npm run build:testnpm --workspace @maka/runtime-host run typechecknpm --workspace @maka/runtime-host run test:dist— 962 passedgit diff --check简体中文
npm run build:testnpm --workspace @maka/runtime-host run typechecknpm --workspace @maka/runtime-host run test:dist— 962 passedgit diff --checkAI use
Tool(s) and scope: OpenAI Codex implemented the listener gate, Windows trust harness, CI wiring, and concise architecture updates under maintainer direction.
Checklist
Does this PR entail a change in behavior?