Skip to content

fix: apply Windows private ACLs to scan output directories - #202

Open
batmnnn wants to merge 4 commits into
openai:mainfrom
batmnnn:fix/windows-scan-output-acl
Open

fix: apply Windows private ACLs to scan output directories#202
batmnnn wants to merge 4 commits into
openai:mainfrom
batmnnn:fix/windows-scan-output-acl

Conversation

@batmnnn

@batmnnn batmnnn commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes Windows scan output directories are not ACL-hardened like credential homes #201: Windows scan output previously skipped all privacy checks (requirePrivateOutputDirectory / requireSecureOutputAncestry no-op on win32) while credential homes already applied a current-user-only ACL.
  • Introduce requirePrivateScanOutput and a shared secureWindowsPrivateDirectory helper; apply + verify ACL on prepare and on every contract scan-root check.
  • Enforce the same private-output gate for multiscan / bulk-scan campaign roots after mkdir.
  • Add injectable Windows ACL unit coverage and a real-Windows ACL verification test (skipped on POSIX).

Test plan

  • bun test --timeout 60000 ./tests-ts/runtime.test.ts ./tests-ts/contract.test.ts ./tests-ts/multiscan.test.ts ./tests-ts/bulk-scan-discovery.test.ts
  • Injectable secureWindowsOutput is required for Windows scan-output privacy
  • validatePreparedOutputDir invokes the Windows ACL hook
  • On a Windows host: scan output ACL is protected + current-user-only (test included, skipped on POSIX CI)

Mirror credential-home ACL hardening for scan output prepare and resolve
paths, including multiscan campaign roots, so shared Windows directories
cannot inherit Allow access for other local principals.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI review requested due to automatic review settings August 1, 2026 16:30
@github-actions github-actions Bot added the bug Something isn't working label Aug 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR closes a security gap on Windows by ensuring scan output directories are protected with current-user-only ACLs (mirroring existing credential-home hardening), and by enforcing the same privacy gate across contract validation and bulk/multiscan flows.

Changes:

  • Add requirePrivateScanOutput and refactor Windows ACL enforcement into a shared secureWindowsPrivateDirectory helper.
  • Apply Windows scan-output ACL hardening during output preparation and during contract scan-root validation checks.
  • Extend privacy enforcement to multiscan and bulk-scan output roots; add unit/integration test coverage (including a Windows-only ACL verification test).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
sdk/typescript/src/runtime.ts Introduces requirePrivateScanOutput and shared Windows private-ACL helper; wires scan-output validation to enforce privacy on Windows and POSIX.
sdk/typescript/src/contract.ts Re-validates scan roots using requirePrivateScanOutput to ensure Windows scan directories are ACL-hardened during contract load/verification.
sdk/typescript/src/multiscan.ts Ensures multiscan output directories are created and then privacy-validated (including Windows ACL enforcement via requirePrivateScanOutput).
sdk/typescript/src/bulk-scan-discovery.ts Applies the same privacy gate to bulk-scan wizard output roots after creation.
sdk/typescript/tests-ts/runtime.test.ts Adds injectable Windows-ACL hook coverage plus a Windows-only integration test verifying ACL protection/current-user-only access.
sdk/typescript/tests-ts/multiscan.test.ts Adjusts test directory setup to satisfy stricter privacy requirements for multiscan output roots.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sdk/typescript/src/multiscan.ts Outdated
Comment thread sdk/typescript/src/multiscan.ts Outdated
Comment thread sdk/typescript/src/bulk-scan-discovery.ts Outdated
batmnnn and others added 2 commits August 3, 2026 12:26
Base chmod on the directory mode instead of umask, refresh metadata
after correcting permissions, and clarify non-directory/symlink errors.

Co-authored-by: Cursor <cursoragent@cursor.com>
@batmnnn

batmnnn commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Copilot review notes:

  • chmod now keys off the directory’s actual mode (mode & 0o077), not umask
  • re-lstat after chmod so requirePrivateScanOutput sees current metadata
  • clearer error when the path exists but is not a non-symlink directory

@mldangelo-oai

Copy link
Copy Markdown
Collaborator

@codex review Please review exact contributor-updated head 55b9aac, focusing on verified Windows output ACLs, actual POSIX directory modes, revalidated metadata, symlink/non-directory paths, and the failing Prettier checks in src/runtime.ts, src/multiscan.ts, and src/bulk-scan-discovery.ts.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55b9aac385

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/runtime.ts Outdated
Comment thread sdk/typescript/src/multiscan.ts Outdated
Comment thread sdk/typescript/src/runtime.ts Outdated
@chatgpt-codex-connector

Copy link
Copy Markdown

Security review completed. No security issues were found in this pull request.

Reviewed commit: 55b9aac385

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Address Codex review: format sources for Prettier, chmod to exact 0700
when permission bits drift, and revalidate directory identity after ACL
hardening before returning the canonical scan output path.

Co-authored-by: Cursor <cursoragent@cursor.com>
@batmnnn

batmnnn commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Codex review on 55b9aac:

  • Ran Prettier on runtime.ts, multiscan.ts, and bulk-scan-discovery.ts
  • POSIX mode normalization now requires exact 0700 (mode & 0o777), not only clearing group/world bits
  • After Windows ACL apply/verify, re-lstat and bind the canonical path to the original dev/ino (rejects replacement mid-hardening)
  • Added a regression for directory replacement during the Windows ACL hook

Focused suite: runtime, contract, multiscan, bulk-scan-discovery — all passing.

@batmnnn

batmnnn commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@mldangelo-oai can you take a look over this again please, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows scan output directories are not ACL-hardened like credential homes

3 participants