security: bind Brew cleanup audit records to private storage - #187
security: bind Brew cleanup audit records to private storage#187seonghobae wants to merge 20 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review Please review the unchanged exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Git identity and delta =='
git rev-parse HEAD
git rev-parse 8e676ea5e89dd473a7b0c6ca763ddb7f71599ea5
git merge-base be418bac9bfbdc8aa41b1a698713833e3dbccfdc 8e676ea5e89dd473a7b0c6ca763ddb7f71599ea5
git diff --name-status be418bac9bfbdc8aa41b1a698713833e3dbccfdc 8e676ea5e89dd473a7b0c6ca763ddb7f71599ea5
echo '== Rust structure =='
ast-grep outline src-tauri/src/brew_cleanup.rs --items all --type function,impl,struct,mod --view expanded
ast-grep outline src-tauri/src/brew_cleanup_audit_authority_tests.rs --items all --type function,mod --view expanded
ast-grep outline src-tauri/src/lib.rs --items all --type mod --view expanded
echo '== Focused audit implementation references =='
rg -n -C 5 --type rust \
'write_audit_record|brew-cleanup-audit-(parent|directory)-writable-by-others|brew-cleanup-records|DirBuilderExt|OpenOptionsExt|set_permissions|sync_all|remove_file' \
src-tauri/src/brew_cleanup.rs src-tauri/src/brew_cleanup_audit_authority_tests.rs src-tauri/src/lib.rs
echo '== PR and check state =='
gh pr view 187 --repo ContextualWisdomLab/disksage \
--json number,headRefOid,baseRefOid,mergeStateStatus,statusCheckRollup,files \
--jq '{number, headRefOid, baseRefOid, mergeStateStatus, checks: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}], files: [.files[].path]}'Length of output: 1981 🏁 Script executed: #!/bin/bash
set -u
echo '== Supported structural maps =='
ast-grep outline src-tauri/src/brew_cleanup.rs --items all --type function,struct,mod --view expanded || true
ast-grep outline src-tauri/src/brew_cleanup_audit_authority_tests.rs --items all --type function,struct,mod --view expanded || true
ast-grep outline src-tauri/src/lib.rs --items all --type function,struct,mod --view expanded || true
echo '== Exact source diff =='
git diff --no-ext-diff --unified=80 \
be418bac9bfbdc8aa41b1a698713833e3dbccfdc \
8e676ea5e89dd473a7b0c6ca763ddb7f71599ea5 \
-- src-tauri/src/brew_cleanup.rs src-tauri/src/brew_cleanup_audit_authority_tests.rs src-tauri/src/lib.rs
echo '== Focused implementation and regression source =='
rg -n -C 12 --type rust \
'fn write_audit_record|fn .*writable|brew-cleanup-audit-(parent|directory)-writable-by-others|brew-cleanup-records|DirBuilderExt|OpenOptionsExt|set_permissions|sync_all|remove_file|audit_authority_tests' \
src-tauri/src/brew_cleanup.rs src-tauri/src/brew_cleanup_audit_authority_tests.rs src-tauri/src/lib.rsLength of output: 37684 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 187 --repo ContextualWisdomLab/disksage \
--json number,headRefOid,baseRefOid,mergeStateStatus,statusCheckRollup,files \
--jq '{number, headRefOid, baseRefOid, mergeStateStatus, checks: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}], files: [.files[].path]}'Length of output: 5512 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Existing audit-record tests =='
sed -n '620,760p' src-tauri/src/brew_cleanup.rs
echo '== Test configuration candidates =='
fd -a -t f 'Cargo.toml|Cargo.lock' . \
-E target -E node_modules \
| while IFS= read -r file; do
echo "--- $file"
rg -n -C 3 'tempfile|^\[dev-dependencies\]|^\[workspace' "$file" || true
done
echo '== Direct audit writer references and test module registration =='
rg -n -C 5 --type rust \
'\bwrite_audit_record\s*\(|mod\s+brew_cleanup_audit_authority_tests\b|audit_records_are_create_new_and_private' \
src-tauri/srcLength of output: 9131
The implementation rejects group and other write authority on the app-data directory and on an existing audit directory. It creates the audit directory with mode The existing audit test verifies create-once behavior and final Unix mode The Linux, macOS, and Windows Release builds succeeded. Test, Semgrep, and Security Scan checks remain queued. The PR remains
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headb633e3efc90fcb053db3b6ca4f3695304206cf2c. -
Head SHA:
b633e3efc90fcb053db3b6ca4f3695304206cf2c -
Workflow run: 31796494843
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
|
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headb633e3efc90fcb053db3b6ca4f3695304206cf2c. -
Head SHA:
b633e3efc90fcb053db3b6ca4f3695304206cf2c -
Workflow run: 31799045915
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Dismissing: this REQUEST_CHANGES was posted against commit b633e3e where coverage-evidence failed. The PR's current head has coverage-evidence passing (verified via gh pr checks). No unresolved review threads remain. Re-request review to re-validate.
opencode-agent
Bot
added
area: security
* test: prove private evidence parent replacement race * fix: bind private evidence writes to opened parent * test: prove private evidence parent permission drift * fix: revalidate private evidence parent permissions * test: preserve replaced private evidence on failed cleanup * security: preserve identity-mismatched evidence replacements * security: invalidate failed evidence through opened descriptor * test: prove private evidence mode survives restrictive umask * fix: normalize private evidence mode after open * test: isolate restrictive umask to record creation * test: cover post-create parent replacement * refactor: centralize private parent validation * fix(stack): preserve current audit root in private evidence owner * test: expose pre-open private evidence parent race * fix: bind private evidence parent before canonicalization * refactor: expose object-bound evidence publication primitive * test: expose shared object-bound publication seam * fix: use C-compatible private evidence mode argument
# Conflicts: # src-tauri/Cargo.toml
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| if let Err(error) = publication { | ||
| let invalidation = file | ||
| .set_len(0) | ||
| .and_then(|_| file.sync_all()) | ||
| .and_then(|_| directory.sync_all()); | ||
| if invalidation.is_err() { | ||
| return Err(ObjectBoundPublicationError::InvalidationFailed); | ||
| } |
There was a problem hiding this comment.
🟡 Failed private exports block retries
When any post-creation step fails, set_len(0) leaves the requested output name occupied. Retrying the private export at that path then fails.
Prompt for agents
Restore retryable failure semantics for write_object_bound_bytes_create_new_with_hooks in src-tauri/src/private_evidence.rs without reintroducing pathname-replacement deletion. After a post-create failure, remove the entry only through the opened parent descriptor and only after verifying that the descriptor-relative name still identifies the opened file. Preserve any foreign replacement. Add tests for both an ordinary post-create failure followed by a successful retry at the same path and the existing replacement-record case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut builder = std::fs::DirBuilder::new(); | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::DirBuilderExt; | ||
| builder.mode(0o700); | ||
| } | ||
| match builder.create(&directory) { | ||
| Ok(()) => {} | ||
| Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} |
There was a problem hiding this comment.
🟡 Restrictive umasks suppress cleanup audits
With an owner-write-masking umask, builder.mode(0o700) creates a read-only audit directory. Every cleanup then completes without an audit record.
Prompt for agents
Make audit_directory normalize a newly created brew-cleanup-records directory to exact private, owner-writable permissions through an object-bound directory descriptor before record publication. DirBuilder mode is filtered by umask, so creation under umask 0200 currently yields mode 0500. Preserve the existing symlink, identity, and shared-writable checks, and add an isolated child-process restrictive-umask regression like private_evidence_umask.rs.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| if let Err(error) = publication { | ||
| let invalidation = file | ||
| .set_len(0) | ||
| .and_then(|_| file.sync_all()) | ||
| .and_then(|_| directory.sync_all()); | ||
| if invalidation.is_err() { | ||
| return Err(ObjectBoundPublicationError::InvalidationFailed); | ||
| } |
There was a problem hiding this comment.
📝 Info: Foreign replacements survive cleanup
Descriptor-bound truncation preserves a different file installed under the output name. The dedicated replacement test exercises this identity-drift case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if let Some(forbidden_root) = forbidden_root { | ||
| let canonical_forbidden = std::fs::canonicalize(forbidden_root) | ||
| .map_err(|_| ObjectBoundPublicationError::ForbiddenRootUnavailable)?; | ||
| if canonical_parent.starts_with(canonical_forbidden) { | ||
| return Err(ObjectBoundPublicationError::InsideForbiddenRoot); | ||
| } |
There was a problem hiding this comment.
📝 Info: Sibling output roots remain valid
Canonical Path::starts_with compares components. A sibling such as /repo-backup remains outside /repo and is not rejected.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let record_fd = unsafe { | ||
| libc::openat( | ||
| directory_file.as_raw_fd(), | ||
| record_name.as_ptr(), | ||
| libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL | libc::O_CLOEXEC | libc::O_NOFOLLOW, | ||
| 0o400, | ||
| ) | ||
| }; | ||
| if record_fd < 0 { | ||
| return Err("brew-cleanup-audit-create-failed".into()); | ||
| } | ||
| let mut file = unsafe { std::fs::File::from_raw_fd(record_fd) }; | ||
|
|
||
| let cleanup = || { | ||
| unsafe { | ||
| libc::unlinkat(directory_file.as_raw_fd(), record_name.as_ptr(), 0); | ||
| } | ||
| let _ = directory_file.sync_all(); | ||
| }; |
There was a problem hiding this comment.
📝 Info: Directory replacement cannot redirect records
Both openat and unlinkat use the admitted directory descriptor. Final identity checks reject a renamed directory without touching its replacement.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Scheduled review-feedback autofix for this PR head.
|
|
Scheduled review-feedback autofix for this PR head.
|
|
Scheduled review-feedback autofix for this PR head.
|
|
Scheduled review-feedback autofix for this PR head.
|
seonghobae
added
the
status: needs-review
ChatGPT Codex Connector
seonghobae
removed
the
status: needs-review
seonghobae
added
the
status: draft
ChatGPT Codex Connector
1 participant
Purpose
Bind immutable Homebrew cleanup audit publication to the exact private directory object that was authorized, closing shared-writable storage and same-user pathname-replacement authority gaps without changing Homebrew command, model, human-approval, or evidence semantics.
Exact current state — 2026-09-07
fa1fead83d77d01a1ec00dddcb6c0fd317e145e0;fea9144a15234035fbdf37e9fa914e5fd51efdec;ahead 20 / behind 0) with merge base exactly fix: verify Windows release artifact namespace #264 head; the remaining semantic diff is the five Brew/private-evidence security paths;fb274c2d1de12459c24da5a093a21301e7d5830efailed Release run33261164075, job99125782036, after all three platform builds and artifact downloads succeeded. The verifier expectedrelease-disksage-windows-latest-1while the matrix producedrelease-disksage-windows-2022-1;fa1fead83d77d01a1ec00dddcb6c0fd317e145e0integrates it without copying or reimplementing the fix;npm testis GREEN: 40 Vitest files / 164 tests plus 2 Node concurrency contracts. Fresh hosted exact-head evidence remains required; predecessor results do not transfer.Test-first security repair
The implementation proves the object-bound publication contract by opening the audit directory with no-follow directory semantics, validating opened object and pathname device/inode identity plus private mode, creating immutable records descriptor-relative with
openat(O_CREAT | O_EXCL | O_NOFOLLOW), writing/fsyncing and hardening through the exact opened record, fsyncing the exact directory, cleaning failures descriptor-relative, and revalidating directory/record identities before returning. A same-user rename-and-replacement race fails closed withbrew-cleanup-audit-directory-identity-driftand does not redirect the record into the replacement directory.Unsupported non-Unix publication remains explicit/fail-closed; Homebrew execution authority remains macOS-only.
Stack / ownership
This PR remains the security-delta owner for issue #194's descriptor/handle-relative publication primitive, stacked on canonical release owner #264. Downstream #205 has now been advanced onto this exact head while retaining only its Intel Homebrew compatibility delta. Closed predecessor #228 is preserved by verified complete successor carryover into this exact head: its two semantic files (
src-tauri/src/private_evidence.rsandsrc-tauri/tests/private_evidence_umask.rs) differ only by rustfmt-equivalent trailing-comma/line layout. The evidence and retirement rationale are recorded on #228. #206 remains downstream of #205.#174 owns generic cleanup, #156 repository-wide exact coverage/release evidence, #150 Podman desktop evidence, and #149 canonical acquisition documentation.
Required before merge
Keep Draft until the unchanged exact head satisfies every applicable live required workflow, repository-wide exact 100% owned-production coverage without threshold or production-scope weakening, zero valid current-head findings, fresh live-base ancestry, and actual live governance. Pending, queued, skipped-required, failed, stale, predecessor, model-only, status-only, diagnostic-only, synthetic, or author-only evidence is non-passing.