Repository navigation
fix(output): clear inherited report ACLs before writing - #896
yashrajp22 wants to merge 78 commits into
Conversation
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com> (cherry picked from commit 58b7d0d)
rng1995
left a comment
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
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.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Decision: Approved at fc79a94d5e074a8a32bd49924f12a1146ee3fb30.
Value and readiness
The shared descriptor-based restriction removes inherited report access while the temporary file is empty. The current complete three-file change preserves the previously approved baseline behavior and report atomicity. Ready from code review; exact-head CI remains a merge gate.
Material findings
No required change found. restrict_temporary_access performs fchmod(0600), uses the already-open descriptor for macOS ACL removal, bounds the unsupported-filesystem fallback, and frees the allocated ACL on both success and failure. Report writes call it before stream.write; failures retain the old destination and unlink the temporary file through the anchored directory descriptor. Baseline restriction delegates to the same helper with its previous behavior. Regression assertions cover empty-file restriction, existing/new destinations, ACL failure cleanup and native macOS inherited grants. The production files are byte-identical to persisted reviewed head 8aba9646d9ca5e29edf7157e3f9ec52f9e668fde; the shared CLI test file incorporates intervening main work. Neither prior marked review had an unresolved code requirement.
PIC tradeoffs
The existing secure-output platform limitation and the ENOTSUP/EOPNOTSUPP fallback remain unchanged. No additional owner decision identified.
Verification and gaps
Read the complete current diff and exact-head file-output, baseline restriction/publication and relevant CLI test context, both prior own marked reviews, and the empty discussion/comment/thread records. Compared exact file contents with the persisted reviewed head rather than accepting a remapped review commit association as approval of this head. Fresh metadata still matches the inventory head, open and non-draft. No hosted checks attach to this head; author source/wheel and macOS evidence is not independently reproduced here. No contributor code, tests, imports, builds, setup, hooks, scanner or native ACL probes 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: