Skip to content

fix(output): clear inherited report ACLs before writing - #896

Open
yashrajp22 wants to merge 36 commits into
mainfrom
yashraj/fix-report-inherited-acls
Open

yashrajp22 wants to merge 36 commits into
mainfrom
yashraj/fix-report-inherited-acls

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

On macOS, a report saved in a folder with inheritable ACL permissions could remain readable by other users even with mode 0600.

Clear inherited access from the already-open temporary file before writing report data, using the same protection as baseline files. If access cannot be restricted, preserve the old report and remove the temporary file. Baseline ownership, ACL preservation, and atomic writes keep their existing behavior.

Validation:

  • Fresh source and installed-wheel runs: 594 report/baseline tests passed in each mode; eight macOS-specific tests skipped on Linux.
  • All 20 report-format checks and 48 sample-skill scans passed, with matching findings and behavior after excluding generated IDs and scan metadata.
  • Native macOS checks reproduced the old exposure and confirmed its removal using the release candidate's source and wheel. The report and baseline helpers are byte-for-byte identical to this PR; this is shared-helper evidence, not a full macOS run of this main-branch wheel.
  • Lint, formatting, wheel packaging, and both independent review tools passed.

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
(cherry picked from commit 58b7d0d)

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Value and readiness

The shared temporary-file restriction now protects report bytes as well as baselines. Descriptor-based ACL removal happens while the temporary report is empty, before any sensitive report content is written. I found no required code change at 972639c15b6597891d07a4dd2bb445bf4dc8f2e5.

Material findings

No required change found. The extracted helper retains the existing baseline implementation, fchmod(0600), descriptor-based macOS ACL removal, bounded unsupported-filesystem fallback and cleanup/error behavior. Tests cover new/existing targets, restriction-before-write, preservation on ACL-removal failure, and native inherited macOS grants.

PIC tradeoffs

The existing platform limitation for secure file output remains relevant; the change reuses its current descriptor and atomic-replace design.

Verification and gaps

Read the complete file-output/suppression changes and report tests, pinned source, description, and empty prior discussion. The author reports source/wheel parity and native macOS confirmation; I did not rerun those checks. No hosted checks are attached to the saved head, so CI remains a merge gate. No contributor code was executed.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Value and readiness

Approval holds after main synchronization. The report/baseline ACL implementation is unchanged from the complete code review.

Material findings

No new required changes found. The intervening commits are merged main work plus automatic synchronization. Neither file_output.py nor suppression.py changes; only the shared CLI test file gains already reviewed main coverage. Descriptor restriction still precedes writes, and error cleanup retains the reviewed behavior.

PIC tradeoffs

The prior platform-specific secure-output limitations remain unchanged.

Verification and gaps

Reviewed exact head 5402ced5f56d12e6fc76a53e82d3c045b8bffdb1 against previously assessed 972639c15b6597891d07a4dd2bb445bf4dc8f2e5. Combined the earlier complete assessment with the full since-review comparison and current pinned source. Checked the inherited CLI test deltas against this run's merged output/baseline changes; no production overlap appears. No hosted checks are attached to the current head, which must receive CI before merge. No contributor code, tests, builds, imports or scanner invocations were executed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants