[JJLiebig]

Follow up #4777: elevated Windows servers require elevated clients by default. Ordinary servers remain accessible to ordinary and elevated clients from the same account.

Sharing can be enabled persistently with:

[server]
allow_unelevated_clients = true

Or enable it for one server process with herdr server --allow-unelevated-clients, including when config is false. Both options let ordinary same-account processes control administrator panes. Config is read at server startup, including automatic startup; reload does not change existing listener permissions. Remote Windows servers use their host's config. Account restrictions and rejection of direct remote named-pipe clients remain, and low-integrity clients stay excluded with either opt-in.

Validation: native Windows just check passed (3,330 Rust tests plus maintenance, integration, architecture, and docs checks). The real-server integration test probes both API and TUI endpoints with explicitly lowered client tokens under default, CLI opt-in, config opt-in, and restart without consent. Local medium-to-low cases and hosted elevated high-to-medium cases passed. Documentation contract checks passed. Deep local security review, Linux/macOS/Windows CI, Windows packaging and ConPTY smoke, CodeRabbit, and Greptile are clear on f1eca9f. The prior startup-test finding is addressed and resolved.

Existing servers retain their previous policy until restarted; stopping closes their panes. Config and recovery-file account permissions remain unchanged. This controls live server connections and does not isolate an elevated process from its account's writable configuration or executable files.

refs #4717

[kangal-bot]

@coderabbitai review
@greptileai

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 13e49857-c004-4282-8aa7-f901bbc023f0

📥 Commits

Reviewing files that changed from the base of the PR and between 331775c and f1eca9f.

📒 Files selected for processing (10)

• docs/next/website/src/content/docs/windows-beta.mdx
• docs/next/website/src/data/config-reference.json
• src/cli/server.rs
• src/cli/spec.rs
• src/config/model.rs
• src/main.rs
• src/platform/windows.rs
• src/server/headless/bootstrap.rs
• src/session.rs
• tests/windows_server_security.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

Windows server security descriptors now apply integrity labels. Elevated servers deny lower-integrity clients by default. A Windows-only configuration setting or --allow-unelevated-clients flag allows ordinary clients from the same account to control an elevated server. Configuration and command-line help, Windows documentation, and server security tests were updated. The session test now compares the full connection error.

Suggested reviewers: ogulcancelik

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to f1eca

This change makes elevated Windows servers require elevated clients by default, with an explicit opt-in for ordinary same-account clients. No merge-blocking issue was identified in the reviewed change.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f1eca

The default policy becomes more restrictive, and explicit sharing restores previously available same-account access rather than introducing broader privileges. Both client endpoints apply the same policy. Remaining uncertainty concerns direct remote-connection restrictions and uncommon failure or recovery paths.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — Opt-in delegates control of the elevated server and its administrator panes to ordinary local processes belonging to that account; it is not consent limited to one selected client. The account DACL remains, and the PR does not add a cross-account grant.

Security Findings and Attack Paths

• inferred — An ordinary same-account process controlling administrator panes is a supported consequence of explicit sharing, not an established new vulnerability: account-only access previously permitted that exposure without consent. No bypass of the new default was established. Direct remote-client reachability remains unresolved rather than verified safe or vulnerable.

Trust Boundaries and Controls

• observed — Windows account authorization and mandatory integrity enforcement jointly guard the listener boundary. Both endpoints are probed by a test that explicitly lowers client-token integrity, checks medium-integrity admission only with consent on elevated servers, and checks low-integrity rejection under every sharing mode.

Resilience and Maintainability Implications

• inferred — Descriptor failures fail closed, while process termination provides the demonstrated reset boundary for sharing authority. Listener ownership has shutdown cleanup paths, but concurrent initialization, injected partial-startup failures, and repeated in-process startup were not directly validated by the inspected tests.

Hardening Proposals

• proposed — Verify the pinned dependency's remote-client rejection behavior and preserve that transport boundary with a focused remote named-pipe regression test. This would close an evidence gap, not remedy an established PR-introduced vulnerability.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.65% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 8 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: cross-elevation Windows access now requires explicit opt-in.
Description check	✅ Passed	The description directly explains the Windows access policy, configuration and CLI opt-ins, behavior, testing, and operational constraints described by the changeset.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 17.65% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 8 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[JJLiebig]

@coderabbitai review
@greptileai

[greptile-apps]

Confidence Score: 5/5

[Critical risk] Changes Windows server access control and privilege elevation rules.

The PR appears safe to merge based on the reviewed changes and resolved prior finding.

Summary

The PR makes access to elevated Windows servers require an administrator client by default, with an explicit startup flag or configuration setting to allow ordinary same-account clients. The change since the previous review inlines a descriptor helper without changing its behavior. The previously reported startup-testing gap was addressed and its thread resolved.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Server startup] --> B{Elevated and sharing disabled?}
  B -- Yes --> C[High-integrity pipe label]
  B -- No --> D[Medium-integrity pipe label]
  C --> E[Same-account access check]
  D --> E

Loading

_{Reviews (4) · Last reviewed commit: "fix: require opt-in for cross-elevation ..."}

[coderabbitai]

⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[JJLiebig]

@coderabbitai review
@greptileai

[coderabbitai]

⚠️ Action not completed

Pull request base or head changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[JJLiebig]

@coderabbitai full review
@greptileai

[JJLiebig]

@coderabbitai full review
@greptileai

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

✅ Action performed

Full review finished.

[JJLiebig]

@ogulcancelik Greptile puts this in critical so tagged you for this.

tl;dr: #4777 allowed same-user attach from unelevated terminal to admin terminal herdr server.

This PR adds a config/flag for that to be allowed.

The behaviour mostly mirrors unix behaviour with how sudo works so figured we move it to opt-in but since it's same-user behaviour it's low-risk either way.