fix(credentials): restore Windows ACL inheritance to fix EPERM on read - #301
Open
aythin wants to merge 2 commits into
Open
fix(credentials): restore Windows ACL inheritance to fix EPERM on read#301aythin wants to merge 2 commits into
aythin wants to merge 2 commits into
Conversation
The previous icacls call used /inheritance:r which strips all inherited ACEs and relies solely on the USERNAME env grant. On Windows, USERNAME may not resolve to the same SID that owns the file (e.g. Microsoft Account or domain account mismatches), leaving the file unreadable by anyone including the file owner. Fix: call icacls /reset first to re-enable inherited permissions from the parent directory, then add an explicit /grant:r entry as belt-and-suspenders full-control grant. Verified on Windows 11 Pro: testsprite doctor passes after fix. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the PR, @aythin! A quick note on our workflow: for features and behavior changes we require contributors to open an issue first, claim it by commenting |
github-actions
Bot
added
the
needs-issue
WalkthroughChangesWindows ACL hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/credentials.ts`:
- Around line 242-246: The ACL update flow around run('icacls', [path,
'/reset']) must validate the reset before issuing /grant:r. Capture the /reset
result, handle both an execution error and a non-zero status by reporting the
failure and stopping the ACL transition, and add tests covering each failure
form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6de32c2f-8823-409f-9734-67e69f459548
📒 Files selected for processing (2)
src/lib/credentials.test.tssrc/lib/credentials.ts
Comment on lines
+242
to
+246
| run('icacls', [path, '/reset'], { | ||
| shell: false, | ||
| stdio: 'ignore', | ||
| windowsHide: true, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)credentials\.ts$|DOCUMENTATION\.md$|samples\.ts$|test|spec' || true
echo "== credentials outline =="
ast-grep outline src/lib/credentials.ts --view expanded || true
echo "== relevant lines 200-270 =="
sed -n '200,270p' src/lib/credentials.ts | nl -ba -v200
echo "== icacls usages =="
rg -n "icacls|/reset|grant:r|windowsHide" src/lib/credentials.ts src test tests . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: TestSprite/testsprite-cli
Length of output: 5182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== credentials.ts lines 200-270 =="
sed -n '200,270p' src/lib/credentials.ts > /tmp/cred_slice.txt || true
awk '{printf "%6d\t%s\n",$1,$0}' /tmp/cred_slice.txt
echo "== credentials tests around windows acl =="
rg -n "ensureRestrictiveMode|Windows|icacls|reset|grant:r|restrictiveMode|acl" src/lib/credentials.test.ts src/lib/credentials.ts test --glob '!node_modules' || true
echo "== credentials tests surrounding ensureWindowsRestrictiveAcl/reset/grant =="
rg -n -C 8 "ensureRestrictiveMode|Windows|icacls|reset|grant:r|restrictiveMode|acl" src/lib/credentials.test.ts || trueRepository: TestSprite/testsprite-cli
Length of output: 11992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mock parent ACL references =="
git ls-files | rg '(^|/)parent(-acl|-mode|-permission).*|acl.*parent|ensureWindowsRestrictiveAcl|samples\.ts' || true
echo "== read-only behavior probe for current code path =="
python3 - <<'PY'
import ast, pathlib
text = pathlib.Path('src/lib/credentials.ts').read_text()
tree = ast.parse(text)
# Minimal read-only AST scan: identify icacls run calls and whether first call result is inspected.
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == 'ensureWindowsRestrictiveAcl':
calls = [n for n in ast.walk(node) if isinstance(n, ast.Call) and isinstance(n.func, ast.Name) and n.func.id == 'run']
first = calls[0] if calls else None
print(f"ensureWindowsRestrictiveAcl called {len(calls)} times")
for i, call in enumerate(calls, 1):
print(f" call {i} args:", [str(a) for a in call.args])
print(" assigns result:", [str(n.targets[0].value) for n in ast.walk(node) if isinstance(n, ast.Assign) and isinstance(n.value, ast.Call) and isinstance(n.value.func, ast.Name) and n.value.func.id == 'run'])
print(" result inspected before grant?",
[str(n.test) for n in ast.walk(node) if isinstance(n, ast.If) and isinstance(n.test, ast.Attribute) and n.test.value.id == 'result'])
# Node semantics probe: spawnSync-like result fields are available on returned object;
# omitting assignment means /reset error/status are never read by this function.
class Result:
error = RuntimeError("icacls not found")
status = 1606
result = Result()
print("Node-like result fields:", result.error, result.status)
PYRepository: TestSprite/testsprite-cli
Length of output: 536
Handle /reset failures before issuing /grant:r.
/reset may leave the credentials file with inherited ACLs, including broader directory permissions. If /reset fails and /grant:r succeeds, the current code only checks the grant result, so it does not report the failed reset or avoid issuing the wrong ACL transition. Capture the /reset result, handle error and non-zero status, and add tests for both failure forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/credentials.ts` around lines 242 - 246, The ACL update flow around
run('icacls', [path, '/reset']) must validate the reset before issuing /grant:r.
Capture the /reset result, handle both an execution error and a non-zero status
by reporting the failure and stopping the ACL transition, and add tests covering
each failure form.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
ensureWindowsRestrictiveAclpreviously called:/inheritance:rstrips all inherited ACEs and leaves the file protected solely by theUSERNAMEenv-var grant. On Windows with Microsoft Accounts or domain-joined machines,USERNAMEmay not resolve to the same SID that owns the file — the resulting ACL locks out the file owner, causing every subsequent credential read to fail withEPERM: operation not permitted.Fix: call
icacls /resetfirst (re-enables inheritance from the parent directory), then/grant:r USERNAME:Fas an explicit belt-and-suspenders Full Control entry.Verified on Windows 11 Pro:
testsprite setup --from-env --yes+testsprite doctorpreviously failed immediately with EPERMtestsprite doctorreports all checks passed ✅Related issue
None — small bug fix, no issue required per CONTRIBUTING.md.
Type of change
Checklist
mainbranch.fix(credentials): ...).npm run lintandnpm run format:checkpass.npm run typecheckpasses.npm testpasses and coverage stays at or above the 80% gate.credentials.test.tsto expect twoicaclscalls).README.md/DOCUMENTATION.mdwhere relevant. (N/A — internal Windows ACL fix)Notes for reviewers
Only two files changed:
src/lib/credentials.ts(the fix) andsrc/lib/credentials.test.ts(updated test assertions to match the new two-call sequence:/resetthen/grant:r).Summary by CodeRabbit
Bug Fixes
Tests