Repository navigation
fix(output): clear inherited report ACLs before writing - #896
yashrajp22 wants to merge 36 commits into
Conversation
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com> (cherry picked from commit 58b7d0d)
rng1995
left a comment
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
[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.
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: