fix(windows): close the remaining ACL review findings
The desktop archive probe called the script with only -Path, which the new interface refuses, so it now passes -AllowRoot and -Out. The skill no longer suggests running the repair confined: the confined token cannot write a DACL, so that run would report the sandbox's own restriction as a missing right. The subtree walk bounds its first enumeration too, and a directory whose stat fails counts as unreadable instead of vanishing. The removal decision says inherited copies follow their repaired source, and the pre-flight comment names grant targets as well. Add the third behavior test the rewrite dropped: an inherit-only FullControl ACE survives a grant.
This commit is contained in:
Huanqi Cao committed
1 parent
d7defa7e51
commit
fd5e89feb7
5 files changed
+33
-9
No files matched your search
@@ -55,7 +55,7 @@ try {
|
||||
const directory = skill.resourceBase.path;
|
||||
assert.ok(!directory.includes('.asar'));
|
||||
const output = execFileSync('pwsh', ['-NoProfile', '-NonInteractive', '-File',
|
||||
join(directory, 'scripts/diagnose-windows-sandbox-acl.ps1'), '-Path', root],
|
||||
join(directory, 'scripts/diagnose-windows-sandbox-acl.ps1'), '-Path', root, '-AllowRoot', root, '-Out', join(root, 'out')],
|
||||
{ encoding: 'utf8', timeout: 30000, windowsHide: true });
|
||||
assert.ok(output.includes('READABLE=True'));
|
||||
await fiber.dispose();
|
||||
|
||||
@@ -26,7 +26,7 @@ Substitute full, quoted paths. Keep `-Out` persistent and user-owned, preferably
|
||||
|
||||
## Run it
|
||||
|
||||
Repeat the failing operation once confined, then the command above confined too. When sandbox setup itself fails — the error names the workspace root or `SetNamedSecurityInfoW` — every confined call fails before the command runs, so do not retry confined: request approval for that one command and run it unconfined. Unconfined does not elevate the Windows token.
|
||||
Repeat the failing operation once confined. The script writes permissions, which the confined token cannot do — run it there and it would report the sandbox's own restriction as a missing right — so request approval for that one command and run it unconfined. A confined run of the script is never useful. Unconfined does not elevate the Windows token.
|
||||
|
||||
Diagnose unexpected denials of workspace writes, listing, or plainly readable paths; explain expected ones instead: writes outside the workspace, any write in `read-only`, piped grandchild `spawn EPERM`, ConstrainedLanguage errors. If approval is refused or unavailable, report the path as undiagnosed and stop.
|
||||
|
||||
|
||||
+10
-6
@@ -281,8 +281,12 @@ function Get-SubtreePackageSources {
|
||||
$truncated = $false
|
||||
$queue = [System.Collections.Generic.Queue[string]]::new()
|
||||
# Unreadable directories are skipped, never guessed at: the caller reports the count.
|
||||
try { foreach ($child in [System.IO.Directory]::EnumerateDirectories($Root)) { $queue.Enqueue($child); $enqueued++ } }
|
||||
catch [System.UnauthorizedAccessException] { $failed++ }
|
||||
try {
|
||||
foreach ($child in [System.IO.Directory]::EnumerateDirectories($Root)) {
|
||||
if ($enqueued -ge $Limit) { $truncated = $true; break }
|
||||
$queue.Enqueue($child); $enqueued++
|
||||
}
|
||||
} catch [System.UnauthorizedAccessException] { $failed++ }
|
||||
catch [System.IO.IOException] { $failed++ }
|
||||
while ($queue.Count -gt 0) {
|
||||
if ($visited -ge $Limit) { $truncated = $true; break }
|
||||
@@ -290,7 +294,7 @@ function Get-SubtreePackageSources {
|
||||
$visited++
|
||||
if (Test-DangerousRoot -FullPath $current) { continue }
|
||||
$item = Get-Item -LiteralPath $current -Force -ErrorAction SilentlyContinue
|
||||
if ($null -eq $item) { continue }
|
||||
if ($null -eq $item) { $failed++; continue }
|
||||
if (($item.Attributes -band [System.IO.FileAttributes]::ReparsePoint) -ne 0) { continue }
|
||||
try {
|
||||
$acl = Get-Acl -LiteralPath $current
|
||||
@@ -611,8 +615,8 @@ try {
|
||||
if ($scan.truncated) { $script:scanTruncated = $true }
|
||||
}
|
||||
|
||||
# Nothing is changed while a package source is out of reach: a repair that cannot
|
||||
# finish would leave the tree half repaired and only then report the refusal.
|
||||
# Nothing is changed while a package source or a grant target is out of reach: a
|
||||
# repair that cannot finish would leave the tree half repaired before refusing.
|
||||
foreach ($packagePath in $packageTargets) {
|
||||
$refusal = Get-RepairRefusal -FullPath $packagePath -Root $AllowRoot -AllowEqual
|
||||
if ($refusal) { Write-Line ('REPAIR_REFUSED {0}' -f $refusal); Write-Decision $mode $packagePath refused $refusal; $refused++; break paths }
|
||||
@@ -687,7 +691,7 @@ try {
|
||||
}
|
||||
}
|
||||
if ($sources.Count -gt 0) {
|
||||
Write-Report decision remove_package_sources $full selected 'Remove explicit package allow ACEs from their sources, ancestor first. Inherited entries are verified after the source changes; inheritance stays enabled.' @{
|
||||
Write-Report decision remove_package_sources $full selected 'Remove explicit package allow ACEs from their sources, ancestor first, leaving inheritance enabled so inherited copies follow their repaired source.' @{
|
||||
sources = @($sources); affectedPaths = @($packageTargets)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -368,6 +368,26 @@ describe.skipIf(!isWin32 || !pwshAvailable())('diagnose-windows-sandbox-acl scri
|
||||
}
|
||||
}, timeout)
|
||||
|
||||
it('keeps an inherit-only ACE when it grants the missing right', () => {
|
||||
const scratch = newScratch()
|
||||
try {
|
||||
const target = makeDir(scratch, 'inherit-only')
|
||||
icacls(target, '/inheritance:r', '/grant:r', '*S-1-5-11:(M)')
|
||||
icacls(target, '/grant', '*S-1-5-32-545:(OI)(CI)(IO)(F)')
|
||||
const inheritOnlyBefore = aclLines(target).filter(line => line.includes('(IO)'))
|
||||
expect(inheritOnlyBefore).toHaveLength(1)
|
||||
|
||||
const run = runScript(['-Path', target, '-AllowRoot', scratch, '-Out', join(scratch, 'out')])
|
||||
expect(run.code, run.output).toBe(0)
|
||||
expect(run.output).toContain(`GRANTED ${target} SID=${meSid}`)
|
||||
expect(reports(run)).toContainEqual(containingObject({ kind: 'verification', operation: 'grant', path: target, status: 'verified' }))
|
||||
expect(aclLines(target).filter(line => line.includes('(IO)'))).toEqual(inheritOnlyBefore)
|
||||
expect(aclLines(target).join('\n')).toMatch(/\(F\)/u)
|
||||
} finally {
|
||||
dispose(scratch)
|
||||
}
|
||||
}, timeout)
|
||||
|
||||
it('preserves a deny that shares the removed package allow SID', () => {
|
||||
const scratch = newScratch()
|
||||
try {
|
||||
|
||||
@@ -15,7 +15,7 @@
|
||||
{"type":"session/title","data":{"title":"Load the diagnose-windows-sandbox-acl sk","messageSeqs":[8],"source":{"kind":"fallback"}}}
|
||||
{"type":"assistant/message","data":{"turn":1,"step":1,"message":{"role":"assistant","content":[{"type":"reasoning","text":"Load the requested skill."},{"type":"tool-call","id":"call_skill_load","name":"skill","arguments":"{\"name\":\"diagnose-windows-sandbox-acl\"}"}],"source":{"kind":"model","provider":"deepseek-official","model":"deepseek-v4-flash"},"id":"{{message:5}}"},"usage":{"inputTokens":100,"outputTokens":20,"cacheReadTokens":0,"reasoningTokens":5},"stream":[{"type":"chunk","time":1788699430688,"chunk":{"type":"block-start","index":0,"blockType":"reasoning"}},{"type":"reasoning-chunks","time0":1788699430688,"index":0,"dt":[],"texts":["Load the requested skill."]},{"type":"chunk","time":1788699430688,"chunk":{"type":"block-start","index":1,"blockType":"tool-call"}},{"type":"tool-call-chunks","time0":1788699430688,"index":1,"dt":[],"id":"call_skill_load","name":"skill","args":["{\"name\":\"diagnose-windows-sandbox-acl\"}"]},{"type":"chunk","time":1788699430688,"chunk":{"type":"block-end","index":0,"block":{"type":"reasoning","text":"Load the requested skill."}}},{"type":"chunk","time":1788699430688,"chunk":{"type":"block-end","index":1,"block":{"type":"tool-call","id":"call_skill_load","name":"skill","arguments":"{\"name\":\"diagnose-windows-sandbox-acl\"}"}}},{"type":"chunk","time":1788699430688,"chunk":{"type":"usage","usage":{"inputTokens":100,"outputTokens":20,"cacheReadTokens":0,"reasoningTokens":5}}},{"type":"chunk","time":1788699430688,"chunk":{"type":"finish","reason":{"kind":"tool-calls"}}}]},"surfaceOp":"append"}
|
||||
{"type":"tool/call","data":{"turn":1,"step":1,"callId":"call_skill_load","name":"skill","arguments":"{\"name\":\"diagnose-windows-sandbox-acl\"}"}}
|
||||
{"type":"tool/result","data":{"turn":1,"step":1,"message":{"role":"tool","source":{"kind":"tool","callId":"call_skill_load"},"toolCallId":"call_skill_load","content":[{"type":"text","text":"<skill_content name=\"diagnose-windows-sandbox-acl\">\n<skill_resources>\nBase directory for this skill: {{cwd}}/.dsh/skills/diagnose-windows-sandbox-acl\nResolve relative paths mentioned by this skill against the base directory before using them. Load referenced resources only as needed.\n</skill_resources>\n\n<skill_instructions>\n# Diagnose Windows sandbox ACL failures\n\n**Write every approval request in plain words, in the user's language.** The prompt is all the user reads before widening access, so it must stand alone: which folder the script touches, that it adds the signed-in user's full-control entry where a right is missing and removes foreign package entries, that file contents and owners are unchanged, and that the printed recovery command undoes each change. Keep error codes, `WRITE_DAC`/`WRITE_OWNER`, `S-1-15-2-*` SIDs, `icacls`, ACL, ACE, verdict names and switches out of the request: write Windows file permissions, not ACL or ACE. Ask once, for one command.\n\n## One command diagnoses and repairs\n\nThe script has no modes. `-Path`, `-AllowRoot` and `-Out` read the path and every ancestor and repair what the observations prove, in the same run:\n\n- directories on that chain that lack effective `WRITE_DAC` or `WRITE_OWNER` receive a full-control allow ACE for the signed-in user, because DSH cannot provision its workspace grant without them;\n- explicit AppContainer package allow ACEs (`S-1-15-2-*`, except the well-known groups ending in 1 or 2) are removed at their sources, ancestor first, which also removes those packages' access;\n- a directory the sandbox cannot provision — the state its provisioning error reports on the workspace root — or the authorized root itself also has its subtree searched for those ACEs in the same run, so a deeper entry needs no second request. That bounded walk reports `truncated` and unreadable directories; if truncated, pass the still-failing deeper path once more.\n\nEvery change is backed up, then verified by re-reading it. `-AllowRoot` bounds all of it: an object is changed only when it is that directory or strictly inside it, so a workspace root can repair itself. Never split this into a diagnostic call and a repair call, never ask twice for one repair, and never pass a mode switch that does not exist.\n\n```powershell\n& '<skill-directory>\\scripts\\diagnose-windows-sandbox-acl.ps1' -Path '<failing-path>' -AllowRoot '<authorized-directory>' -Out '<recovery-directory>'\nexit $LASTEXITCODE\n```\n\nSubstitute full, quoted paths. Keep `-Out` persistent and user-owned, preferably beside the failing workspace; never use the skill resource directory, which is deleted when the skill unloads. Pass one path per invocation.\n\n## Run it\n\nRepeat the failing operation once confined, then the command above confined too. When sandbox setup itself fails — the error names the workspace root or `SetNamedSecurityInfoW` — every confined call fails before the command runs, so do not retry confined: request approval for that one command and run it unconfined. Unconfined does not elevate the Windows token.\n\nDiagnose unexpected denials of workspace writes, listing, or plainly readable paths; explain expected ones instead: writes outside the workspace, any write in `read-only`, piped grandchild `spawn EPERM`, ConstrainedLanguage errors. If approval is refused or unavailable, report the path as undiagnosed and stop.\n\n## Read the output\n\nEvery record reaches stdout, the `acl-report-*.jsonl` file under `-Out`, and the final `RECAP` line, which carries the verdicts, changes, verifications, refusals and scans. Tool output keeps only its tail, so read the recap first, and read specific records from the report when it is not enough. Trust `verification` records, never `completed` actions. Decide from `details.nextAction`:\n\n| `nextAction` | Meaning |\n|---|---|\n| `verify_original_confined_operation` | Repairs verified; repeat the original operation confined. |\n| `stop` | Nothing repaired, or a refusal ended the run. Report and stop. |\n| `restore_pending_then_stop` | Rollback unverified. Run the printed recovery commands in order, then stop. |\n\nA deny ACE's presence alone does not establish causation, and the script never removes one; DSH's `S-1-4-*` grants and the Everyone `DeleteSubdirectoriesAndFiles` deny are expected, not conflicts. A deny that blocks the repair ends the run without a repair (`REPAIR_REFUSED`, or `GRANT_FAILED`) after restoring what it attempted. Each change leaves two files in `-Out` (`acl-backup-<id>.json` and its `.ps1`), and the run prints the matching `ROLLBACK` command.\n\n## After a repair, and when it stops\n\nRepeat the original failed operation confined. A verified repair is not undone because the original operation fails for a further reason: coLine truncated
|
||||
{"type":"tool/result","data":{"turn":1,"step":1,"message":{"role":"tool","source":{"kind":"tool","callId":"call_skill_load"},"toolCallId":"call_skill_load","content":[{"type":"text","text":"<skill_content name=\"diagnose-windows-sandbox-acl\">\n<skill_resources>\nBase directory for this skill: {{cwd}}/.dsh/skills/diagnose-windows-sandbox-acl\nResolve relative paths mentioned by this skill against the base directory before using them. Load referenced resources only as needed.\n</skill_resources>\n\n<skill_instructions>\n# Diagnose Windows sandbox ACL failures\n\n**Write every approval request in plain words, in the user's language.** The prompt is all the user reads before widening access, so it must stand alone: which folder the script touches, that it adds the signed-in user's full-control entry where a right is missing and removes foreign package entries, that file contents and owners are unchanged, and that the printed recovery command undoes each change. Keep error codes, `WRITE_DAC`/`WRITE_OWNER`, `S-1-15-2-*` SIDs, `icacls`, ACL, ACE, verdict names and switches out of the request: write Windows file permissions, not ACL or ACE. Ask once, for one command.\n\n## One command diagnoses and repairs\n\nThe script has no modes. `-Path`, `-AllowRoot` and `-Out` read the path and every ancestor and repair what the observations prove, in the same run:\n\n- directories on that chain that lack effective `WRITE_DAC` or `WRITE_OWNER` receive a full-control allow ACE for the signed-in user, because DSH cannot provision its workspace grant without them;\n- explicit AppContainer package allow ACEs (`S-1-15-2-*`, except the well-known groups ending in 1 or 2) are removed at their sources, ancestor first, which also removes those packages' access;\n- a directory the sandbox cannot provision — the state its provisioning error reports on the workspace root — or the authorized root itself also has its subtree searched for those ACEs in the same run, so a deeper entry needs no second request. That bounded walk reports `truncated` and unreadable directories; if truncated, pass the still-failing deeper path once more.\n\nEvery change is backed up, then verified by re-reading it. `-AllowRoot` bounds all of it: an object is changed only when it is that directory or strictly inside it, so a workspace root can repair itself. Never split this into a diagnostic call and a repair call, never ask twice for one repair, and never pass a mode switch that does not exist.\n\n```powershell\n& '<skill-directory>\\scripts\\diagnose-windows-sandbox-acl.ps1' -Path '<failing-path>' -AllowRoot '<authorized-directory>' -Out '<recovery-directory>'\nexit $LASTEXITCODE\n```\n\nSubstitute full, quoted paths. Keep `-Out` persistent and user-owned, preferably beside the failing workspace; never use the skill resource directory, which is deleted when the skill unloads. Pass one path per invocation.\n\n## Run it\n\nRepeat the failing operation once confined. The script writes permissions, which the confined token cannot do — run it there and it would report the sandbox's own restriction as a missing right — so request approval for that one command and run it unconfined. A confined run of the script is never useful. Unconfined does not elevate the Windows token.\n\nDiagnose unexpected denials of workspace writes, listing, or plainly readable paths; explain expected ones instead: writes outside the workspace, any write in `read-only`, piped grandchild `spawn EPERM`, ConstrainedLanguage errors. If approval is refused or unavailable, report the path as undiagnosed and stop.\n\n## Read the output\n\nEvery record reaches stdout, the `acl-report-*.jsonl` file under `-Out`, and the final `RECAP` line, which carries the verdicts, changes, verifications, refusals and scans. Tool output keeps only its tail, so read the recap first, and read specific records from the report when it is not enough. Trust `verification` records, never `completed` actions. Decide from `details.nextAction`:\n\n| `nextAction` | Meaning |\n|---|---|\n| `verify_original_confined_operation` | Repairs verified; repeat the original operation confined. |\n| `stop` | Nothing repaired, or a refusal ended the run. Report and stop. |\n| `restore_pending_then_stop` | Rollback unverified. Run the printed recovery commands in order, then stop. |\n\nA deny ACE's presence alone does not establish causation, and the script never removes one; DSH's `S-1-4-*` grants and the Everyone `DeleteSubdirectoriesAndFiles` deny are expected, not conflicts. A deny that blocks the repair ends the run without a repair (`REPAIR_REFUSED`, or `GRANT_FAILED`) after restoring what it attempted. Each change leaves two files in `-Out` (`acl-backup-<id>.json` and its `.ps1`), and the run prints the matching `ROLLBACK` command.\n\n## After a repair, and when it stops\n\nRepeat the original failed operation confined. A verified repair is not undone because the original operation fails for a further reason: continue fromLine truncated
|
||||
{"type":"step/end","data":{"turn":1,"step":1}}
|
||||
{"type":"step/start","data":{"turn":1,"step":2}}
|
||||
{"type":"assistant/message","data":{"turn":1,"step":2,"message":{"role":"assistant","content":[{"type":"reasoning","text":"The skill is loaded."},{"type":"text","text":"DONE"}],"source":{"kind":"model","provider":"deepseek-official","model":"deepseek-v4-flash"},"id":"{{message:7}}"},"usage":{"inputTokens":180,"outputTokens":10,"cacheReadTokens":0,"reasoningTokens":4},"stream":[{"type":"chunk","time":1788699430709,"chunk":{"type":"block-start","index":0,"blockType":"reasoning"}},{"type":"reasoning-chunks","time0":1788699430709,"index":0,"dt":[],"texts":["The skill is loaded."]},{"type":"chunk","time":1788699430709,"chunk":{"type":"block-start","index":1,"blockType":"text"}},{"type":"text-chunks","time0":1788699430709,"index":1,"dt":[],"texts":["DONE"]},{"type":"chunk","time":1788699430709,"chunk":{"type":"block-end","index":0,"block":{"type":"reasoning","text":"The skill is loaded."}}},{"type":"chunk","time":1788699430709,"chunk":{"type":"block-end","index":1,"block":{"type":"text","text":"DONE"}}},{"type":"chunk","time":1788699430709,"chunk":{"type":"usage","usage":{"inputTokens":180,"outputTokens":10,"cacheReadTokens":0,"reasoningTokens":4}}},{"type":"chunk","time":1788699430709,"chunk":{"type":"finish","reason":{"kind":"stop"}}}]},"surfaceOp":"append"}
|
||||
|
||||
Reference in new issue
Block a user