fix(windows): resolve the secret ACL principal from the effective token SID (#1149) - #1216
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea8f5d042c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| `DOMAIN\User` 라는 **형태**는 그 계정이 현재 토큰의 주체라는 **증거가 아니다**. | ||
| 두 환경변수 모두 우리를 띄운 프로세스가 쓸 수 있다. 그리고 optional 경로도 | ||
| `required` 와 똑같은 파괴적 시퀀스를 돈다: |
There was a problem hiding this comment.
Because this added note is under devlog/_plan, these lines record the still-unshipped #1149 security reasoning, including the environment-spoofing condition and the destructive ACL sequence. The repository instructions require this kind of unreleased vulnerability reasoning to stay in scratch until the fix has shipped; if this PR is pushed as-is, the note becomes public history before the fix lands. Please move the planning note to .tmp/mktemp or wait until after release and keep only the published outcome in the tracked tree.
AGENTS.md reference: AGENTS.md:L79-L83
Useful? React with 👍 / 👎.
ea8f5d0 to
89e32ae
Compare
36936da to
86b31f2
Compare
Resolve the current token SID instead of trusting USERDOMAIN and USERNAME on workgroup hosts. Keep identity lookup failures separate from icacls timeouts.\n\nRefs #1149
…dable Builds on the contributor fix by luvs01 (#1180), which replaced the USERDOMAIN\USERNAME ACL principal with the effective token SID. Three things that fix left open: The synthetic principal POSIX CI needs lived in windows-secret-acl.ts and was chosen before the injected runner. That ordering made a lookup FAILURE unreachable outside Windows, so the two cases that defend the fail-closed boundary and the timedOutPaths isolation were guarded with `if (process.platform !== "win32") return;` and never ran on Linux or macOS. A test that silently returns on two of three CI platforms is not coverage of a security boundary. The synthetic value moves to the resolver as its own seam, runner selection becomes explicit > synthetic > default, and both guards are gone. sanitizedAclError re-attaches only allow-listed codes, and EACLIDENTITY was not among them. A required-mode harden therefore threw with the cause in the message but `error.code === undefined`, so no caller could branch on "the SID could not be resolved" versus "icacls stalled". The existing test matched the message and hid this. The absence of a name-shaped fallback is now stated as the fix rather than left as an omission. `DOMAIN\User` has a valid shape, but shape is not evidence of the token's subject, and both variables are writable by whatever launched us. runIcacls grants the principal Full Control and then removes inheritance, so a wrong principal either leaves another account holding the secret or strands the file with no usable ACE. An independent audit rejected an earlier draft of this change that restored that fallback for the optional read path. Coverage now runs the sync and async paths across required and optional on every platform, and asserts zero icacls invocations when the environment names a plausible-looking account. Ablation: reverting the runner ordering makes identityCalls 0 and the required harden succeed (2 red); dropping EACLIDENTITY from the allow-list makes both toMatchObject assertions fail (2 red). Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Closes #1149
89e32ae to
fca2cd7
Compare
86b31f2 to
355640d
Compare
2 participants
Summary
Replaces the
USERDOMAIN\USERNAMEACL principal with the effective Windows token SID, and closes three gaps the contributor fix left open.Adopts #1180 by @luvs01 — the first commit here is theirs, cherry-picked unchanged, with
Co-authored-bypreserved. Their design was right: extract a low-level SID resolver rather than reuse the coordinator identity module, reuseresolveTrustedWindowsPowerShellExe()instead of writing a third System32 resolver, keep the identity failure in its own error class so it cannot poison the icacls timeout memo, and cache only successful lookups. This PR keeps all of that and layers three fixes on top.The fail-closed tests never ran outside Windows. The synthetic principal that POSIX CI needs lived in
windows-secret-acl.tsand was selected before the injected runner, which made a lookup failure unreachable on Linux and macOS. The two cases defending the fail-closed boundary and thetimedOutPathsisolation were therefore guarded withif (process.platform !== "win32") return;and counted as passing while executing nothing. A test that silently returns on two of three CI platforms is not coverage of a security boundary. The synthetic value moves to the resolver as its own seam, runner selection becomesexplicit > synthetic > default, and both guards are gone.EACLIDENTITYwas dropped at the required boundary.sanitizedAclErrorre-attaches only allow-listed codes and this one was missing, so a required-mode harden threw with the cause in the message buterror.code === undefined. No caller could tell "the SID could not be resolved" apart from "icacls stalled". The existing test matched the message and hid it.The absence of a name fallback is now the stated fix.
DOMAIN\Userhas a valid shape, but shape is not evidence of the token's subject, and both variables are writable by whatever launched the process.runIcaclsgrants the principal Full Control and then removes inheritance, so a wrong principal either leaves another account holding the secret or strands the file with no usable ACE. An independent audit rejected an earlier draft of this change that restored that fallback for the optional read path; the reasoning is recorded indevlog/_plan/260807_untouched_bug_stack/081_windows_acl_userdomain_adopt_1180.md.Coverage now exercises sync and async across required and optional on every platform, and asserts zero icacls invocations when the environment names a plausible-looking account.
Verification
bun run typecheck— clean.bun run test— 9720 pass, 8 skip, 0 fail across 605 files.bun run privacy:scan— passed.identityCalls0 and lets the required harden succeed — 2 tests red.EACLIDENTITYfrom the allow-list makes bothtoMatchObject({ code })assertions fail — 2 tests red.This is a backend-only change. The frontend directory has zero changed paths.
Checklist
Stacked on #1211. Replaces #1180.
Closes #1149