Skip to content

fix(windows): let Glob skip nested junctions - #3952

Open
HuYellow wants to merge 7 commits into
apache:mainfrom
HuYellow:codex/fix-3938-windows-glob-junction
Open

fix(windows): let Glob skip nested junctions#3952
HuYellow wants to merge 7 commits into
apache:mainfrom
HuYellow:codex/fix-3938-windows-glob-junction

Conversation

@HuYellow

@HuYellow HuYellow commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • bind one explicit nonFollowingReadRoot to the Windows broker manifest digest and expose it only for read-only recursive Glob operations
  • decompose that root into bounded grants for physical directories, excluding nested reparse entries and their targets while keeping raw recursive reads, writes, hard links, and a reparse-point root fail-closed
  • use a non-following Windows Glob walker that preserves Node matching, dotfile, ordering, and limit semantics, including omitting the junction entry itself from broad **/* results
  • align the English and Chinese Windows sandbox RFCs, focused tests, packaged evidence, and dependency notices with the specialized policy

Fixes #3938

The specialized-policy implementation incorporates the three functional commits from #3955 by @sunrioa and @Ling. Their commit authorship and Generated-by trailers are preserved in this branch; the unrelated dependency-only commit was not copied. The final lockfile instead keeps the newer audit fixes already merged to main.

Verification

  • npm run build:test
  • npm run lint
  • npm run format:check
  • npm run typecheck
  • npm run windows:inventory
  • npm audit --registry=https://registry.npmjs.org --audit-level=high (0 vulnerabilities)
  • npm Desktop/CLI third-party notice checks and Windows Cargo notice check
  • cargo fmt --manifest-path experiments/windows-sandbox/launcher/Cargo.toml -- --check
  • cargo test --locked --manifest-path experiments/windows-sandbox/launcher/Cargo.toml (62 passed)
  • release-mode Windows broker build
  • AppContainer smoke and packaged adversarial matrix
  • focused Runtime policy/manifest/walker tests (24 passed)
  • node --test packages/runtime/dist/__tests__/filesystem-worker-windows-smoke.test.js (5 passed)
  • full verify:windows-x64 on a locally generated package, including packaged dependency closure, broker/stdio/ACL recovery, 64-launch soak, adversarial matrix, real packaged Electron worker Glob, node-pty/ConPTY, renderer, installer and ZIP hashes

The local machine has no Visual Studio C++ Build Tools, so the verification package used the already installed patched node-pty binaries (npmRebuild=false) and the exact local Electron 43.4.1 distribution instead of rebuilding native dependencies. The normal package path is left to CI. check:release reached one unrelated standalone-launcher WSL path failure after all dependency-notice and release-closure checks passed; check:asf-source is locally blocked by unavailable file-symlink privileges and gzip.

Security review focus

  • nonFollowingReadRoot is validated as a declared recursive read root, rejects every writable launch, and is included in the launch digest.
  • Raw recursive roots retain their previous whole-tree reparse rejection, so the packaged adversarial matrix remains valid.
  • The broker grants only physical directories. Reparse entries and targets receive no ledger entry or ACE.
  • Planning fails closed above 4,096 grants, 100,000 inspected entries, or 256 directory levels.
  • The worker independently excludes Windows reparse Dirents before matching or traversal, including broad **/* patterns.

AI use

Select exactly one:

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

Tool(s) and scope: OpenAI Codex implemented and integrated the specialized Windows ACL policy, non-following Glob traversal, documentation, regression coverage, conflict resolution, and local verification. Affected commits retain the required Generated-by: OpenAI Codex trailers.

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

@Astro-Han

Copy link
Copy Markdown
Contributor

I reviewed this PR at exact head 949df79d846042ba0ea037cdc115baf9f47384cf (base 2d10b52, merge-base 38f0a275, 4 files +176−16).

Spec: GO — no P0–P3

  • Root reparse points are still unconditionally rejected; recursive write still rejects the whole tree; recursive read now terminates at a nested reparse boundary (acl_ledger.rs:443-477).
  • The Windows ACL path correctly uses icacls /L /T so it does not follow the target; the real junction ACL test proves the target and its descendants do not receive the synthetic SID and verifies cleanup.
  • The filesystem-worker Windows smoke covers both ordinary main.go enumeration and the junction-descendant pattern returning empty.
  • No implemented-but-wrong / missing / scope-creep finding within bug(windows): Glob fails when an approved tree contains a nested junction #3938; the existing hard-link scan→grant race is not introduced by this diff.

Standards: NO-GO — 1 hard P1 + 1 judgment-only P3

  • P1 — governing security contract and release evidence are not in sync with the new strategy. acl_ledger.rs:443-477 now accepts a nested reparse boundary for recursive read, but docs/architecture/windows-sandbox-rfc-v1.md still requires “recursive reparse-point rejection before ACL mutation” in several places and defines the packaged recursive-junction admission refusal as W1/release evidence. adversarial-matrix-smoke.ps1:208-216 also still requires the same nested-junction read request to fail closed as before. As a result the exact-head package run 32993195237 fails directly: launcher exits 0, matrix reports Junction alias admission did not fail closed. Fix by either formally revising the RFC and the machine-readable matrix to a read traversal-boundary contract (keep root/write rejection, prove target denial with a real child) or restoring the recursive rejection.
  • Judgment-only P3: skip_nested_reparse_points: bool with bare false/true encodes a security policy as a boolean; a named enum would reduce inversion risk.

Other checks: git diff --check passes. windows_recovery and windows_sandbox_w0_protocol are green; package is red for the reason above (directly related), test is red due to an unrelated Desktop prompt-rail E2E (66 pass / 1 fail / 1 skip). OPEN / MERGEABLE / REVIEW_REQUIRED, no reviews/comments, head did not drift.

What I did not check: full local test suite beyond the focused checks noted.

Gate: exact head has no P0–P2, but the P1 standards finding and the failing required package/test checks must be closed (update RFC + adversarial-matrix-smoke.ps1 to match the new read-boundary contract, or restore rejection) before merge.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 27, 2026
…s-glob-junction

# Conflicts:
#	package-lock.json
Generated-by: OpenAI Codex
@HuYellow HuYellow changed the title fix(windows): skip nested junctions for read ACL grants fix(windows): let Glob skip nested junctions Aug 27, 2026
@HuYellow

Copy link
Copy Markdown
Contributor Author

Addressed at head 82c8766b7.

  • P1: replaced the global recursive-read relaxation with a digest-bound nonFollowingReadRoot available only to read-only recursive Glob. Raw recursive reads and writes still reject nested reparse points, so the original packaged adversarial admission check remains valid.
  • Synchronized the English/Chinese RFCs, Windows sandbox README, Runtime smoke coverage, and packaged verifier. The verifier now proves broad **/* omits the junction entry and descendants while ordinary files remain visible.
  • P3: removed the skip_nested_reparse_points: bool entirely. The policy is represented by an optional canonical root and validated at the client, profile, manifest protocol, digest, and broker admission boundaries.
  • Added fail-closed planner limits for grants, inspected entries, and depth, plus hard-link/root/policy-tamper coverage.

Local results include Rust 62/62, Windows worker smoke 5/5, AppContainer smoke, raw packaged adversarial matrix, and full verify:windows-x64 including the packaged Electron worker, 64-launch soak, node-pty/ConPTY, renderer, installer, and ZIP checks.

The specialized implementation incorporates the three functional commits from #3955 by @sunrioa and @Ling with authorship preserved. The branch is also merged with current main; the final lockfile keeps brace-expansion 5.0.9, and both generated notices are synchronized.

@HuYellow

Copy link
Copy Markdown
Contributor Author

Follow-up: all current checks are green at 82c8766. The CI test job passed in 16m45s, and the Windows release package job passed in 17m28s, including Package the Windows installer and ZIP, Verify the Windows release, packaged adversarial evidence, upgrade/autoupdate, and rollback checks.

@github-actions github-actions Bot added effort/XL Over 1000 readable lines and removed effort/M Under 500 readable lines labels Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Over 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(windows): Glob fails when an approved tree contains a nested junction

3 participants