Conversation
This commit adds a safety check to the `Dataset.save` method to prevent the recursive deletion of arbitrary directories when `overwrite=True`. Before proceeding with an overwrite, the method now verifies that the target directory is either empty or contains Zarr-specific metadata (`.zgroup` or `.zarray`). 🎯 What: Added a safety check before `zarr.group(..., overwrite=True)`.⚠️ Risk: Previously, `zarr.group(..., overwrite=True)` with a `DirectoryStore` would recursively delete the target directory regardless of its contents, which could lead to accidental or malicious data loss. 🛡️ Solution: Verify the directory is a Zarr store or empty before allowing deletion. Co-authored-by: cbyrohl <9221545+cbyrohl@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
…ests - Change bare Exception to ValueError for programmatic handling - Replace mock-heavy unittest tests with real filesystem integration tests using tmp_path (matching project conventions) - Add test verifying directory contents are preserved on rejection - 7 integration tests covering all safety check branches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
This PR fixes a security vulnerability in the
Dataset.savemethod where an arbitrary directory could be recursively deleted if theoverwriteparameter was set toTrue(which is the default).The Issue
The
Dataset.savemethod useszarr.DirectoryStore(fname)andzarr.group(store, overwrite=overwrite). Whenoverwrite=True, Zarr callsshutil.rmtree(fname)on the target path. Iffnameis controlled by a user or accidentally points to a non-Zarr directory, that directory and all its contents are deleted.The Fix
The fix adds a pre-check before calling
zarr.group. If the target path exists and is a non-empty directory, we ensure it contains either.zgroupor.zarraybefore allowing the overwrite to proceed. If it's a non-empty directory without these markers, anExceptionis raised.Verification
tests/test_save_safety.pythat covers:overwrite=False(allowed, no check performed).PR created automatically by Jules for task 6142097217887244602 started by @cbyrohl