Skip to content

fix(credentials): restore Windows ACL inheritance to fix EPERM on read #301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aythin wants to merge 2 commits into
base: main
Choose a base branch
from
+33 −3
Open

fix(credentials): restore Windows ACL inheritance to fix EPERM on read #301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0488f22
fix(credentials): restore Windows ACL inheritance to fix EPERM on read
Aug 3, 2026
4010631
test(credentials): update Windows ACL test to expect /reset + /grant:…
Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/lib/credentials.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,15 +216,31 @@ describe('ensureRestrictiveMode', () => {
spawnSync: spawn,
});

expect(spawn).toHaveBeenCalledWith(
// First call: /reset re-enables inheritance from the parent directory so the
// owner can always access the file (fixes EPERM on Microsoft Account / domain
// account machines where USERNAME does not resolve to the file-owner SID).
expect(spawn).toHaveBeenNthCalledWith(
1,
'icacls',
[credentialsPath, '/inheritance:r', '/grant:r', 'alice:F'],
[credentialsPath, '/reset'],
{
shell: false,
stdio: 'ignore',
windowsHide: true,
},
);
// Second call: /grant:r adds an explicit Full Control entry as belt-and-suspenders.
expect(spawn).toHaveBeenNthCalledWith(
2,
'icacls',
[credentialsPath, '/grant:r', 'alice:F'],
{
shell: false,
stdio: 'ignore',
windowsHide: true,
},
);
expect(spawn).toHaveBeenCalledTimes(2);
});

it('warns on Windows when credentials ACL tightening cannot run', () => {
Expand Down
16 changes: 15 additions & 1 deletion src/lib/credentials.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,21 @@ function ensureWindowsRestrictiveAcl(path: string, options: RestrictiveModeOptio
}

const run = options.spawnSync ?? spawnSync;
const result = run('icacls', [path, '/inheritance:r', '/grant:r', `${username}:F`], {

// Reset to re-enable inheritance from the parent directory first.
// Using /inheritance:r (the previous approach) strips all inherited ACEs and
// relies solely on the USERNAME-based grant — on Windows the env USERNAME may
// not resolve to the same SID that owns the file (e.g. Microsoft Account /
// domain account mismatches), which leaves the file unreadable by anyone.
// /reset restores inherited ACEs so the owner can always access the file, then
// the explicit /grant:r adds a belt-and-suspenders Full-Control entry.
run('icacls', [path, '/reset'], {
shell: false,
stdio: 'ignore',
windowsHide: true,
});
Comment on lines +242 to +246

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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' || true

Repository: 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 || true

Repository: 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)
PY

Repository: 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


const result = run('icacls', [path, '/grant:r', `${username}:F`], {
shell: false,
stdio: 'ignore',
windowsHide: true,
Expand Down
Loading