feat(sccm): add private capture destination primitive - #456
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds Unix private SCCM capture publication. It validates limits and namespaces, uses handle-relative create-new operations, synchronizes durable updates, tracks identities for rollback, and adds Unix tests for failure, replacement, durability, and FIFO cases. ChangesPrivate capture publication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PrivateBundleTransaction
participant HandleBoundCaptureRoot
participant UnixFilesystemHelpers
PrivateBundleTransaction->>HandleBoundCaptureRoot: validate destination and limits
HandleBoundCaptureRoot->>UnixFilesystemHelpers: openat and mkdirat relative to trusted handles
UnixFilesystemHelpers-->>PrivateBundleTransaction: return identities and created-entry metadata
PrivateBundleTransaction->>UnixFilesystemHelpers: publish files and synchronize directories
UnixFilesystemHelpers-->>PrivateBundleTransaction: commit or identity-aware rollback result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
github-actions
Bot
added
enhancement
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Pull request overview
Adds a private, handle-bound SCCM “capture destination” filesystem primitive intended to safely support future bundle publication work (without yet introducing any public API surface or source enumeration/copy seam).
Changes:
- Introduces a Unix-only destination-root + create-new transactional writer with strict file/byte ceilings and rollback-on-drop semantics.
- Adds no-follow, handle-relative directory/file creation plus namespace/identity revalidation checks for commit durability.
- Wires the new
private_fsmodule intosccmas intentionally dead-code (no production caller yet).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src-tauri/src/sccm/private_fs.rs |
Adds the private capture destination root, transactional create-new writer, namespace/identity validation, and extensive tests (Unix; macOS-specific namespace checks). |
src-tauri/src/sccm/mod.rs |
Includes the private module with an explicit “no production caller yet” note and #[allow(dead_code)]. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/sccm/private_fs.rs (1)
1706-1719: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rejection reason in the symlink test.
The test asserts only that
open_private_capture_rootreturns an error. The call can fail for unrelated reasons, for example a non-private ancestor of the temporary directory. Assert the error kind and message so the test proves that no-follow traversal caused the rejection.♻️ Proposed assertion
- open_private_capture_root(&linked_parent.join("private-root")) - .expect_err("capture-root traversal never follows a parent symlink"); + let error = open_private_capture_root(&linked_parent.join("private-root")) + .expect_err("capture-root traversal never follows a parent symlink"); + assert_eq!(error.raw_os_error(), Some(libc::ELOOP));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/sccm/private_fs.rs` around lines 1706 - 1719, Update private_capture_root_rejects_a_symlinked_parent_component to inspect the error returned by open_private_capture_root and assert both its expected error kind and exact no-follow traversal message, rather than only asserting failure. Preserve the existing symlink setup and ensure the assertions specifically prove rejection caused by the parent symlink.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 763-775: Update the ownership check in the filesystem validation
flow after fstatfs to handle u32::try_from(libc::MNT_IGNORE_OWNERSHIP)
conversion failure explicitly, returning an appropriate error instead of
defaulting the mask to zero. Preserve the existing PermissionDenied behavior
when the ownership-ignore flag is present and continue to reject extended ACLs
via reject_macos_extended_acl.
---
Nitpick comments:
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 1706-1719: Update
private_capture_root_rejects_a_symlinked_parent_component to inspect the error
returned by open_private_capture_root and assert both its expected error kind
and exact no-follow traversal message, rather than only asserting failure.
Preserve the existing symlink setup and ensure the assertions specifically prove
rejection caused by the parent symlink.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0f8d15e-81da-4fd6-8b7e-578585f31911
📒 Files selected for processing (2)
src-tauri/src/sccm/mod.rssrc-tauri/src/sccm/private_fs.rs
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 4 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src-tauri/src/sccm/private_fs.rs:293
- On non-Unix platforms,
begin_private_bundle_transactionvalidateslimitsandbundle_namebefore hitting theUnsupportedbranch, so callers can observeInvalidInputerrors even though destination publication is explicitly unsupported on those platforms. If the intended contract is “always Unsupported on non-Unix”, gate these validations behind#[cfg(unix)](and ensurelimitsis still referenced on non-Unix to avoid unused warnings).
if !limits.is_valid() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"SCCM private publication limits are invalid",
));
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src-tauri/src/sccm/private_fs.rs (2)
508-546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
create_file_atleavesO_NONBLOCKon the returned descriptor.The rest of this module clears
O_NONBLOCKafter a no-follow open, and the existing tests assert that the final descriptor is blocking (safe_open_returns_only_regular_blocking_files,handle_relative_open_returns_a_blocking_final_descriptor).create_file_atreturns the descriptor with the flag still set.
O_CREAT | O_EXCL | O_NOFOLLOWguarantees a newly created regular file, sowrite_allbehavior is unaffected today. The inconsistency becomes a hazard if this descriptor is later returned to a caller or reused for a non-regular target. Clear the flag after the open, or add a comment that records why the flag stays set here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/sccm/private_fs.rs` around lines 508 - 546, Update the create_file_at flow to clear O_NONBLOCK on the descriptor returned by create_file_at, matching the blocking-descriptor behavior established by safe_open_returns_only_regular_blocking_files and handle_relative_open_returns_a_blocking_final_descriptor; only retain the flag if you document the specific reason in this path.
446-467: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider indexing created directories instead of a linear scan.
create_new_relative_filescanscreated_entriesfor every ancestor component of every published file.created_entriesholds both file and directory entries, so the scan grows with the number of published files. At the ceiling limits (MAX_PRIVATE_CAPTURE_FILES= 4096, 32 path components) this becomes a quadraticPathBufcomparison cost on the publication path.A
HashMap<PathBuf, FileIdentity>for directory entries, maintained next tocreated_entries, keeps the lookup constant time while the rollback order stays driven by the existingVec.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/sccm/private_fs.rs` around lines 446 - 467, Replace the linear directory lookup in create_new_relative_file with a HashMap<PathBuf, FileIdentity> maintained alongside created_entries, containing only created directories. Use the map to retrieve and validate each ancestor identity while preserving created_entries as the Vec controlling rollback order and keeping file entries out of the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 1719-1736: Canonicalize the tempfile root before constructing
paths in private_capture_root_rejects_a_symlinked_parent_component, then create
actual-parent and linked-parent beneath that canonical root. In the FIFO test at
src-tauri/src/sccm/private_fs.rs lines 1944-1955, canonicalize the temporary
root before building the FIFO path and tighten the assertion to require the
specific intended rejection reason rather than accepting any error.
---
Nitpick comments:
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 508-546: Update the create_file_at flow to clear O_NONBLOCK on the
descriptor returned by create_file_at, matching the blocking-descriptor behavior
established by safe_open_returns_only_regular_blocking_files and
handle_relative_open_returns_a_blocking_final_descriptor; only retain the flag
if you document the specific reason in this path.
- Around line 446-467: Replace the linear directory lookup in
create_new_relative_file with a HashMap<PathBuf, FileIdentity> maintained
alongside created_entries, containing only created directories. Use the map to
retrieve and validate each ancestor identity while preserving created_entries as
the Vec controlling rollback order and keeping file entries out of the index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dd8d1dc-4362-42d3-bc92-2efcd732db93
📒 Files selected for processing (2)
src-tauri/src/sccm/mod.rssrc-tauri/src/sccm/private_fs.rs
|
Exact-head correction disposition at
Independent exact-delta review: GO, no P0-P3. Local CodeRabbit exact delta: zero findings. Production behavior is unchanged. Fresh exact-head hosted reviews and CI are requested below. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 4 minutes. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/sccm/private_fs.rs (1)
603-617: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning the validated component instead of validating in place.
validate_single_componentvalidatesPath::new(value).components(), but the caller passes the rawvaluetomkdiratandopenat.Path::componentsnormalizes forms such as"a/"and"a/."to a singleNormalcomponent, so the syscall receives a string that differs from the validated component. The current syscalls fail closed for these forms, so there is no traversal escape today. Returning the singleOsStringcomponent and using it for every handle-relative operation would remove the divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/sccm/private_fs.rs` around lines 603 - 617, Update validate_single_component to return the validated single component as an OsString instead of only io::Result<()>, then use that returned component for all subsequent mkdirat and openat handle-relative operations. Ensure every syscall consumes the normalized component produced by validation rather than the original raw value, while preserving existing rejection behavior for unsafe names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 603-617: Update validate_single_component to return the validated
single component as an OsString instead of only io::Result<()>, then use that
returned component for all subsequent mkdirat and openat handle-relative
operations. Ensure every syscall consumes the normalized component produced by
validation rather than the original raw value, while preserving existing
rejection behavior for unsafe names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3e2e896-3fae-44f1-82dc-0a7f3c2e8e3f
📒 Files selected for processing (2)
src-tauri/src/sccm/mod.rssrc-tauri/src/sccm/private_fs.rs
|
Resolved the final CodeRabbit normalized-component concern at 636b612. Independent reproduction showed that mkdirat accepts a trailing-slash spelling on macOS, so this was corrected rather than dismissed: the red test bundle_name_requires_the_exact_validated_component failed at 160e5b3, then passed after validation began returning the exact OsString component and every handle-relative syscall consumed it. Final local gates: private_fs 17/17; SCCM-enabled native lib 450/450; full parser suite green; strict native Clippy green; wasm32 check green; TypeScript check green; scoped rustfmt and git diff --check green. The local CodeRabbit CLI reached reviewing but emitted no terminal attestation, so hosted re-review remains the merge gate. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/sccm/private_fs.rs (1)
1944-1975: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for
validate_relative_path.The new tests cover limits, rollback, replacement, durability, and FIFO handling. No test drives
write_new_filewith a rejected relative path.validate_relative_pathguards theunreachable!branches increate_new_relative_file, so a regression there converts a rejected input into a panic. Add cases for.., an absolute path, a.component, more than 32 components, and a path longer than 1024 bytes. AssertInvalidInputand assert that no entry appears under the bundle root.💚 Suggested test
#[test] fn relative_path_validation_rejects_unsafe_spellings() { let (_temporary, path, root) = private_capture_root(); let mut transaction = begin_private_bundle_transaction(&root, "capture-001", test_limits()) .expect("new transaction"); for unsafe_relative in [ Path::new("../escape.log"), Path::new("/absolute.log"), Path::new("./relative.log"), ] { let error = transaction .write_new_file(unsafe_relative, b"x") .expect_err("unsafe relative path is rejected"); assert_eq!(error.kind(), io::ErrorKind::InvalidInput); } let deep = "a/".repeat(33) + "deep.log"; let error = transaction .write_new_file(Path::new(&deep), b"x") .expect_err("component ceiling is enforced"); assert_eq!(error.kind(), io::ErrorKind::InvalidInput); assert!(!path.join("capture-001/a").exists()); assert!(!path.join("capture-001/relative.log").exists()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/sccm/private_fs.rs` around lines 1944 - 1975, Add a negative test near the existing transaction tests, using write_new_file to verify validate_relative_path rejects parent traversal, absolute paths, "." components, paths exceeding 32 components, and paths exceeding 1024 bytes. Assert each call returns io::ErrorKind::InvalidInput and verify no rejected entry or escape path is created under the bundle root.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 1944-1975: Add a negative test near the existing transaction
tests, using write_new_file to verify validate_relative_path rejects parent
traversal, absolute paths, "." components, paths exceeding 32 components, and
paths exceeding 1024 bytes. Assert each call returns io::ErrorKind::InvalidInput
and verify no rejected entry or escape path is created under the bundle root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 342fe000-d5e6-4ecb-bf42-b98481da58b5
📒 Files selected for processing (2)
src-tauri/src/sccm/mod.rssrc-tauri/src/sccm/private_fs.rs
|
Hosted Windows MSRV correctly rejected 636b612 because OsString was still imported under cfg(unix). The exact job log showed E0412 at private_fs.rs:603. Commit cb0a3d9 moves only OsString into the platform-neutral import scope. Local Rust 1.88 workspace all-features locked check, focused regression, scoped rustfmt, and diff checks pass. A local Windows cross-target attempt reached ring but is environment-blocked by absent MSVC headers, so fresh hosted Windows remains the acceptance gate. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 19 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src-tauri/src/sccm/private_fs.rs:152
- The failure mapping here collapses both ownership and permission validation failures into an error message that only mentions privacy.
verify_private_directorycan fail because the directory is not owned by the capture user (uid check) as well as because it is not mode-private, so the message can be misleading when debugging admission failures.
let metadata = directory.metadata()?;
verify_private_directory(path, &metadata).map_err(|_| {
io::Error::new(
io::ErrorKind::PermissionDenied,
"SCCM capture destination root is not private",
)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/sccm/private_fs.rs (1)
455-457: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider indexing created directories to avoid a linear scan.
create_new_relative_filescanscreated_entriesfor every intermediate component of every file. With the production ceilings (MAX_PRIVATE_CAPTURE_FILES= 4096, depth limit 32) the worst case reaches roughly 4096 × 32 × 4096PathBufcomparisons. AHashMap<PathBuf, FileIdentity>for directory entries keeps the same identity guarantee at constant lookup cost. The PR notes state this index was deferred, so this is a follow-up suggestion only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/sccm/private_fs.rs` around lines 455 - 457, Replace the linear created_entries lookup in create_new_relative_file with a HashMap<PathBuf, FileIdentity> index for created directory entries. Update the index whenever a directory entry is created, and use it to preserve the existing identity reuse behavior for accumulated paths while avoiding repeated scans.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 455-457: Replace the linear created_entries lookup in
create_new_relative_file with a HashMap<PathBuf, FileIdentity> index for created
directory entries. Update the index whenever a directory entry is created, and
use it to preserve the existing identity reuse behavior for accumulated paths
while avoiding repeated scans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e0a69fbb-6a2e-40f2-811a-1d03cf89265e
📒 Files selected for processing (2)
src-tauri/src/sccm/mod.rssrc-tauri/src/sccm/private_fs.rs
|
Independent adjudication of the exact-head CodeRabbit directory-index follow-up: the premise is real and the actual bound is larger than 4,096. Up to 4,096 zero-byte files with 31 unique intermediate directories can retain about 131,072 entries, making the current linear lookup a very large bounded O(n^2) path. This does not block the present slice because private_fs is destination-only dead code with no production/native caller, and the current correction does not expand reachability. It is now an explicit required optimization/cap gate before any capture writer or public/native caller is wired; it will not be silently forgotten or represented as impossible. |
Add the reviewed private, handle-relative SCCM destination transaction primitive. The module remains unreachable from production callers; native capture/publication, Windows support, and live acceptance remain future gated work.
Part of #319.
This isolated native slice adds a destination-only private filesystem primitive for eventual SCCM bundle publication.
Scope:
Review disposition at cb0a3d9:
Verification:
Repository-wide format still has inherited unrelated drift; the exact owned file and range are clean.
Merge remains blocked on exact-head hosted CI, CodeRabbit, Copilot, zero unresolved threads, and a repeated live base/head/tree guard.
Summary by CodeRabbit