Skip to content

fix(runtime-host): restrict Windows Local IPC ACL - #3182

Merged
M4n5ter merged 1 commit into
mainfrom
fix/windows-local-ipc-acl
Aug 18, 2026
Merged

fix(runtime-host): restrict Windows Local IPC ACL#3182
M4n5ter merged 1 commit into
mainfrom
fix/windows-local-ipc-acl

Conversation

@M4n5ter

@M4n5ter M4n5ter commented Aug 18, 2026

Copy link
Copy Markdown
Member

Summary

English

Windows Local IPC now applies a protected DACL for the current user and SYSTEM before granting Local Owner authority. After forcing replacement pipe instances, the Windows trust check covers read-only, write-only, and duplex access from a separate standard user.

The test harness keeps temporary-account deletion independent from fixture startup and cleanup failures, and reports a deletion failure instead of silently leaving the account behind.

简体中文

Windows Local IPC 现在会在授予 Local Owner authority 前,为当前用户和 SYSTEM 设置受保护的 DACL。Windows 信任检查会先迫使 listener 创建 replacement pipe instances,再使用独立标准用户验证只读、只写和双向访问。

测试工具会独立执行临时账户删除,不受 fixture 启动或清理失败影响;账户删除失败也会被明确报告,不再静默遗留账户。

Follow-up to #3179

Verification

English
  • npm run build:test
  • npm --workspace @maka/runtime-host run typecheck
  • npm --workspace @maka/runtime-host run test:dist — 962 passed
  • Biome check and git diff --check
  • Real Windows cross-user verification — passed
简体中文
  • npm run build:test
  • npm --workspace @maka/runtime-host run typecheck
  • npm --workspace @maka/runtime-host run test:dist — 962 passed
  • Biome check 与 git diff --check
  • 真实 Windows 跨用户验证 — passed

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex implemented the Windows endpoint ACL and expanded trust harness under maintainer direction.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b63e2e57-94af-47b7-94bc-2e260b11dc07

📥 Commits

Reviewing files that changed from the base of the PR and between fb16d81 and f98c04f.

📒 Files selected for processing (2)
  • packages/runtime-host/src/control/endpoint.ts
  • scripts/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.


📝 Walkthrough

Problem solved

This PR restricts Windows Local IPC access to the current user and SYSTEM before the runtime host grants Local Owner authority. The trust probe now verifies read-only, write-only, and duplex access from a separate standard user.

The test harness also removes temporary Windows accounts when fixture startup or cleanup fails.

Source of truth

The PR extends the existing runtime-host endpoint setup and Windows IPC trust probe. It does not create a separate production path. The PowerShell script provides the Windows-specific ACL operation required by the existing endpoint lifecycle.

Solution size and complexity

The solution is coherent and focused:

  • prepareAfterListen applies a protected DACL before authority is granted.
  • PowerShell execution uses SystemRoot, a hidden window, and a 10-second timeout.
  • Execution failures map to RuntimeHostEndpointError with code insecure_endpoint_directory.
  • The trust probe covers all supported pipe directions.
  • Fixture cleanup protects both process disposal and temporary-account removal.

The added PowerShell invocation and cleanup nesting are necessary for ACL enforcement and reliable failure handling. No deletion or simplification is apparent without weakening security coverage or fixture cleanup guarantees.

Validation and risks

Validation includes:

  • Test build.
  • Runtime-host type checking.
  • Biome checks.
  • git diff --check.
  • Windows cross-user verification is pending in CI.

Required-check status remains unverified without direct CI evidence.

Review-relevant risks

  • The protected DACL changes Windows IPC access behavior. Material security or user-visible behavior changes require independent human review under repository policy.
  • The exported MakaWindowsPipeTrustProbe.TryOpenAsUser signature changes. Material public-contract changes require independent human review under repository policy.
  • The PR adds PowerShell execution and maps failures to a runtime-host error. Material release or operational-behavior changes require independent human review under repository policy.
  • The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Windows runtime-host endpoints now apply explicit named-pipe ACLs through PowerShell. The IPC trust probe tests foreign-user access for In, Out, and InOut directions and improves fixture failure cleanup.

Changes

Windows named-pipe security

Layer / File(s) Summary
Apply named-pipe ACLs
packages/runtime-host/src/control/endpoint.ts
prepareAfterListen invokes PowerShell to disable inherited named-pipe permissions and grant access to the current user and SYSTEM. PowerShell lookup, execution timeout, hidden-window handling, and failures map to insecure_endpoint_directory.
Validate access directions and cleanup
scripts/windows-runtime-host-local-ipc-trust.ps1
The trust probe accepts a PipeDirection and requires foreign-user attempts for In, Out, and InOut to return access_denied. Fixture startup and cleanup now dispose processes and remove temporary users through nested cleanup paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to f98c0

The Windows Local IPC ACL and test-account cleanup changes are localized, and no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant RuntimeHostEndpoint
  participant PowerShell
  participant WindowsNamedPipe
  RuntimeHostEndpoint->>PowerShell: Execute ACL script with pipe path
  PowerShell->>WindowsNamedPipe: Restrict access to current user and SYSTEM
  PowerShell-->>RuntimeHostEndpoint: Return execution result
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Ai Use Disclosure ⚠️ Warning The PR discloses substantive OpenAI Codex use, but its sole introduced commit embeds \\n\\nGenerated-by: Codex in prose; Git parses no valid trailer. Add a standalone Generated-by: OpenAI Codex trailer to the material-AI commit. Review CONTRIBUTING.md’s “Human ownership and AI attribution” section and preserve it through squash or amend.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main change: restricting Windows Local IPC ACLs.
Description check ✅ Passed The description includes the required summary, verification, AI use, checklist, behavior change, and issue reference.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/windows-local-ipc-acl

Comment @coderabbitai help to get the list of available commands.

@M4n5ter
M4n5ter marked this pull request as ready for review August 18, 2026 02:42
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Restrict Windows Local IPC named pipe ACLs

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Protects Windows named pipes with current-user and SYSTEM-only ACLs before IPC admission.
• Verifies foreign users cannot open read-only, write-only, or duplex connections.
• Guarantees temporary test-user cleanup when fixture startup or shutdown fails.
Diagram

sequenceDiagram
  actor Host as Runtime Host
  participant Endpoint as Endpoint Setup
  participant PS as PowerShell ACL
  participant Pipe as Named Pipe
  actor Owner as Current User
  actor Foreign as Foreign User
  Host->>Endpoint: Start listening
  Endpoint->>PS: Apply protected DACL
  PS->>Pipe: Allow owner and SYSTEM
  Owner->>Pipe: Open duplex connection
  Pipe-->>Owner: Grant Local Owner
  Foreign->>Pipe: Open in, out, duplex
  Pipe-->>Foreign: Deny access
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Creation-time native security descriptor
  • ➕ Eliminates the interval between pipe creation and post-listen ACL application.
  • ➕ Avoids launching PowerShell during endpoint initialization.
  • ➖ Node's named-pipe server API does not expose Windows security descriptors directly.
  • ➖ Requires native bindings, platform-specific maintenance, and additional build complexity.
2. Compiled .NET security helper
  • ➕ Provides a typed interface to Windows ACL APIs.
  • ➕ Could produce richer diagnostics than an inline PowerShell command.
  • ➖ Introduces another compiled artifact and runtime dependency.
  • ➖ Adds packaging and architecture compatibility concerns for a narrowly scoped operation.

Recommendation: The protected-DACL PowerShell approach is pragmatic because Windows PowerShell is system-provided and Node lacks a direct named-pipe ACL API. It also fails closed when PowerShell is unavailable or ACL application fails. A creation-time native descriptor would be stronger if the post-listen interval becomes unacceptable, but its native build and maintenance costs are not justified by this focused fix.

Files changed (2) +101 / -33

Bug fix (1) +52 / -2
endpoint.tsApply a protected ACL to Windows Local IPC pipes +52/-2

Apply a protected ACL to Windows Local IPC pipes

• Runs system Windows PowerShell after the named-pipe listener starts to replace inherited permissions with a protected DACL granting full control only to the current user and SYSTEM. Endpoint preparation fails with a security-specific error if PowerShell cannot be located or ACL application fails.

packages/runtime-host/src/control/endpoint.ts

Tests (1) +49 / -31
windows-runtime-host-local-ipc-trust.ps1Expand cross-user pipe checks and harden fixture cleanup +49/-31

Expand cross-user pipe checks and harden fixture cleanup

• Tests foreign-user access using input, output, and duplex pipe directions while retaining the current-user Local Owner assertion. Restructures process startup and nested cleanup so fixture objects are disposed and temporary Windows accounts are removed even when startup or teardown fails.

scripts/windows-runtime-host-local-ipc-trust.ps1

@qodo-code-review

qodo-code-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Windows endpoint test now fails ✓ Resolved 🐞 Bug ≡ Correctness
Description
The Windows endpoint test calls prepareAfterListen() without first creating the named pipe, but
the changed hook now runs Get-Item against that nonexistent path and rejects. Running the
runtime-host test suite on Windows therefore fails deterministically instead of validating the new
lifecycle.
Code

packages/runtime-host/src/control/endpoint.ts[R54-55]

      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.
+        await secureWindowsNamedPipe(path);
Relevance

●●● Strong

Deterministic Windows test failure from invoking ACL setup before pipe creation is an obvious
lifecycle bug.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test obtains only an endpoint description and immediately invokes the hook, whereas production
first calls server.listen(endpoint.path). The added hook now launches a PowerShell script whose
Get-Item requires that pipe path to exist.

packages/runtime-host/src/tests/control-endpoint.test.ts[24-31]
packages/runtime-host/src/server/local-ipc-listener.ts[39-40]
packages/runtime-host/src/control/endpoint.ts[21-22]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Update the Windows endpoint test so the post-listen hook is invoked only after a named pipe exists.

## Issue Context
The hook's contract now requires a listening endpoint. Reuse the production listener seam where practical rather than duplicating its listen-and-prepare ordering in the test.

## Fix Focus Areas
- packages/runtime-host/src/__tests__/control-endpoint.test.ts[24-31]
- packages/runtime-host/src/server/local-ipc-listener.ts[13-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Replacement pipes lose restricted ACL ✗ Dismissed 🐞 Bug ⛨ Security
Description
SetAccessControl changes only an existing named-pipe instance, while libuv creates other and
replacement instances with NULL security attributes, restoring the default DACL that grants Everyone
read access. A foreign standard user can therefore open a read-only instance after the secured
instance is consumed, violating this PR’s stated trust boundary and enabling connection exhaustion
or unintended reads.
Code

packages/runtime-host/src/control/endpoint.ts[R21-22]

+$pipe = Get-Item -LiteralPath $env:${WINDOWS_PIPE_PATH_ENV}
+$pipe.SetAccessControl($security)
Relevance

●● Moderate

Security concern is technically specific, but history lacks a close Windows named-pipe ACL precedent
for decisive team behavior.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new script opens the current pipe path and updates that object's ACL after the server has
already listened. libuv's Windows implementation creates each accept instance with
CreateNamedPipeW(..., NULL), while Microsoft documents that a NULL security descriptor applies the
process token's default DACL, including read access for Everyone; the repository's trust probe
consumes one instance as the current user before attempting foreign read-only access.

packages/runtime-host/src/control/endpoint.ts[11-22]
packages/runtime-host/src/server/local-ipc-listener.ts[39-45]
scripts/windows-runtime-host-local-ipc-trust.ps1[146-179]
🌐 libuv's Windows accept path creates named-pipe instances with CreateNamedPipeW and passes NULL security attributes.
🌐 Microsoft documents that a NULL pipe security descriptor uses the default DACL, which grants Everyone and anonymous users read access.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PowerShell command changes one live named-pipe instance, but Node/libuv creates multiple and replacement instances with the default security descriptor. Apply the protected current-user/SYSTEM DACL atomically to every server instance when it is created.

## Issue Context
Deleting the post-listen mutation alone would leave the original vulnerability, and the existing `prepareAfterListen` seam cannot control future libuv `CreateNamedPipeW` calls. Node's public `net.Server` pipe API does not expose creation security attributes, so a Windows-specific native listener or equivalent creation seam is necessary; this introduces a platform-specific listener implementation and corresponding trust-test maintenance.

## Fix Focus Areas
- packages/runtime-host/src/control/endpoint.ts[11-22]
- packages/runtime-host/src/server/local-ipc-listener.ts[39-45]
- scripts/windows-runtime-host-local-ipc-trust.ps1[146-179]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
✅ Web pages:
  +14 more
Review mode: ⚖️ Balanced: This is a security-sensitive Windows IPC ACL and trust-boundary change with runtime behavior and cleanup/test-harness logic; it warrants a complete single-pass review, though the scope is not broad or defect-dense enough for extended.

Grey Divider

Tip of the day
💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread packages/runtime-host/src/control/endpoint.ts
Comment thread packages/runtime-host/src/control/endpoint.ts
@M4n5ter
M4n5ter force-pushed the fix/windows-local-ipc-acl branch 4 times, most recently from 623faff to 4283042 Compare August 18, 2026 02:57
Apply a protected current-user DACL before Local IPC admission so foreign connections cannot enter the listener. Extend the Windows probe across every pipe direction and make temporary-account cleanup independent from process startup and teardown failures.

Generated-by: Codex
@M4n5ter
M4n5ter force-pushed the fix/windows-local-ipc-acl branch from 4283042 to 86727ac Compare August 18, 2026 02:58
@M4n5ter

M4n5ter commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

I have manually reviewed this PR and confirmed that its impact scope is relatively small. I take full responsibility for this, so I am going to merge it.

@M4n5ter
M4n5ter merged commit 38bd18f into main Aug 18, 2026
12 checks passed
@M4n5ter
M4n5ter deleted the fix/windows-local-ipc-acl branch August 18, 2026 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant