release: v0.2.0 — security fixes, bug fixes, documentation#7
Merged
Conversation
… atomic replace - Add path-traversal and home-directory guards to the else branch of restore_backup() to match the protections already present for the single-file branch - Preserve original file permissions on atomic replace in merge_pulled_claude_json(), save_manifest(), and save_config() - Fix test_restore_backup_all_files to mock Path.home() (required by the new home-directory guard) - Add test_restore_all_rejects_dest_outside_home (backup.py) - Add test_merge_preserves_file_permissions (sanitize.py) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…error handler, JSON shape validation - Rebuild local manifest from post-sync file state after a successful pull, so the stored manifest reflects what actually landed on disk - Add _local_to_remote_path() helper and use it in _build_manifests() to translate local absolute paths to remote equivalents before querying remote file hashes (fixes hash lookup always missing) - Handle FileNotFoundError in get_remote_file_hashes() when SSH binary is absent, wrapping it as SyncError - Validate that JSON root is a dict in sanitize_claude_json(), merge_pulled_claude_json() (both remote and local sides), and load_manifest() — raises ValueError for non-object JSON - Add tests: test_get_remote_file_hashes_raises_on_missing_ssh, test_sanitize_rejects_non_dict_json, test_merge_rejects_non_dict_remote, test_merge_rejects_non_dict_local, test_load_manifest_rejects_non_dict_json, test_pull_rebuilds_manifest_after_sync Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ced, test improvements - Count .claude.json file transfers in SyncSummary.files_transferred - Validate sync.strategy config field at load time against allowed values - Make SyncedFileEntry.last_synced optional (TypedDict inheritance) for legacy manifest compatibility - Add test_detect_conflicts_last_sync_none_uses_fallback regression test - Fix hardcoded /tmp paths in test_cli.py; use tmp_path fixture instead - Add assert result.exit_code checks to all test_cli.py runner.invoke calls - Remove unused tmp_path parameter from mock_config() fixture Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion - Remove redundant inline comment from FileConflict.winner field — the type WinnerSide | None already documents the possible values - Make dry_run parameter keyword-only on _rsync_global() and _rsync_project() to prevent future accidental positional misuse - Tighten test_rsync_project_aggregates_all_failures assertion from >= 2 to == 3 (3 items all fail, so exactly 3 'error' substrings) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Bump version to 0.2.0 in pyproject.toml and __init__.py - Add CHANGELOG.md following Keep a Changelog format, documenting all changes in v0.2.0 and v0.1.0 - Add comprehensive README.md covering installation, quick start, CLI reference, architecture, configuration, security considerations, and development instructions - Update SECURITY.md: replace GitHub template placeholder content with actual supported versions table (0.2.x supported, 0.1.x EOL) and vulnerability reporting instructions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review Summary by QodoRelease v0.2.0 — security fixes, bug fixes, and documentation
WalkthroughsDescription• Security hardening: path-traversal guards in bulk restore, file permission preservation on atomic writes • Bug fixes: manifest rebuild after pull, remote path translation in hash lookup, JSON shape validation, missing SSH error handling • Minor improvements: .claude.json transfer counting, config strategy validation, optional last_synced field • Release v0.2.0 with comprehensive documentation (README, CHANGELOG, SECURITY.md) Diagramflowchart LR
A["Security Hardening"] --> B["Path Traversal Guards"]
A --> C["File Permission Preservation"]
D["Bug Fixes"] --> E["Manifest Rebuild After Pull"]
D --> F["Remote Path Translation"]
D --> G["JSON Validation"]
D --> H["SSH Error Handling"]
I["Enhancements"] --> J["Transfer Counting"]
I --> K["Config Validation"]
I --> L["Optional Fields"]
M["Documentation"] --> N["README.md"]
M --> O["CHANGELOG.md"]
M --> P["SECURITY.md"]
B --> Q["v0.2.0"]
C --> Q
E --> Q
F --> Q
G --> Q
H --> Q
J --> Q
K --> Q
L --> Q
N --> Q
O --> Q
P --> Q
File Changes1. src/claudesync/__init__.py
|
Code Review by Qodo
1.
|
|
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:
📝 WalkthroughWalkthroughAdds documentation and version bump to 0.2.0; introduces file‑IO safety and permission‑preserving atomic writes, path‑traversal and JSON‑type validations, manifest rebuild with local→remote path translation, rsync/SSH error handling and keyword-only rsync helpers, and corresponding tests — no behavioral changes outside these areas. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI"
participant LocalFS as "Local FS"
participant Engine as "Engine (rsync/SSH)"
participant Remote as "Remote Host"
participant Manifest as "Local Manifest"
rect rgba(200,230,255,0.5)
CLI->>LocalFS: _collect_local_files() → local paths
CLI->>CLI: _local_to_remote_path(local_path) → remote_path
end
rect rgba(220,255,200,0.5)
CLI->>Engine: request remote hashes for remote_paths
Engine->>Remote: SSH/rsync fetch hashes
Remote-->>Engine: hashes
Engine-->>CLI: remote hashes (remote_path keys)
end
rect rgba(255,230,200,0.5)
CLI->>CLI: remap remote hashes → local-path keys
CLI->>Manifest: update_manifest_for_remote(local_manifest, remote_manifest)
Manifest-->>CLI: manifest saved (mode preserved)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
src/claudesync/backup.py
Outdated
Comment on lines
+89
to
+102
| backup_root = ts_dir.resolve() | ||
| home_root = Path.home().resolve() | ||
| for src in ts_dir.rglob("*"): | ||
| if src.is_file(): | ||
| rel = str(src.relative_to(ts_dir)) | ||
| dest = Path("/" + rel) | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.copy2(src, dest) | ||
| restored.append(dest) | ||
| if not src.is_file(): | ||
| continue | ||
| if not src.resolve().is_relative_to(backup_root): | ||
| raise ValueError(f"Path traversal detected in backup archive: '{src}'") | ||
| rel = str(src.relative_to(ts_dir)) | ||
| dest = Path("/" + rel) | ||
| if not dest.resolve().is_relative_to(home_root): | ||
| raise ValueError(f"Restore destination outside home directory: '{dest}'") | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.copy2(src, dest) | ||
| restored.append(dest) |
There was a problem hiding this comment.
1. Restore-all symlink race 🐞 Bug ⛨ Security
Bulk restore validates the resolved destination is under $HOME, but then creates directories and writes later; a symlink swap between the check and the copy can redirect writes outside $HOME. This undermines the new path-traversal hardening for restore-all.
Agent Prompt
### Issue description
`restore_backup()` (restore-all branch) does a `dest.resolve().is_relative_to(home_root)` check, but then performs `mkdir` + `copy2` later. This leaves a symlink/TOCTOU window where the destination path can be swapped to point outside `$HOME`.
### Issue Context
This code path is explicitly being security-hardened for path traversal. To make the guard robust, the destination write should not depend on a path that can change between validation and use.
### Fix Focus Areas
- src/claudesync/backup.py[89-102]
### Suggested approach (one viable option)
1. Before writing, validate that **no parent component** between `home_root` and `dest.parent` is a symlink.
2. Open the destination file using `os.open` with `O_NOFOLLOW` (prevents following a symlink as the final component), copy bytes from `src` to that fd, and preserve metadata as needed.
3. Alternatively, write into a temp file in the validated directory and `os.replace` into place, again using no-follow checks for the final target.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 133-140: The fenced code block showing the project tree in
README.md is missing a language specifier; update the opening backticks for that
block (the ``` line before the tree listing starting with ".├──
src/claudesync/") to include a language like "text" or "plaintext" (e.g., change
``` to ```text) so the project structure tree is rendered with correct plaintext
formatting.
- Around line 73-83: The fenced code block showing the project tree (the lines
starting with ``` and the block beginning "src/claudesync/") lacks a language
specifier; update the opening fence to include a language such as "text" or
"plaintext" (e.g., change ``` to ```text) so markdown linters recognize it as a
plaintext/code block while leaving the block contents unchanged.
In `@SECURITY.md`:
- Line 14: Replace the placeholder owner in the security advisory URL string
`https://github.com/<owner>/claudesync/security/advisories/new` with the
repository's actual GitHub username or organization (e.g.,
`https://github.com/my-org/claudesync/security/advisories/new`) so the link
points to the real advisory page; update the string in SECURITY.md accordingly.
In `@src/claudesync/backup.py`:
- Around line 89-90: Ensure the code constrains backup_id to BACKUP_DIR before
computing restore roots: build ts_dir by joining BACKUP_DIR and backup_id (not
using backup_id directly if absolute or containing ..), then resolve ts_dir and
verify the resolved path is a child of BACKUP_DIR (e.g., compare
resolved_ts_dir.is_relative_to(resolved_backup_dir) or check str(prefix) match);
if the check fails, raise an error and do not proceed. After this validation set
backup_root = ts_dir.resolve() and home_root = Path.home().resolve() so restore
roots cannot escape the intended BACKUP_DIR boundary.
In `@src/claudesync/cli.py`:
- Around line 230-236: Create a new helper (e.g.,
collect_local_files(project_paths)) that calls get_global_include_paths(),
iterates project_paths and PROJECT_SYNC_ITEMS, appends str(p) for existing
paths, and returns the consolidated list of local file paths; then replace the
duplicated loops in pull and _build_manifests with a call to this helper and
pass its result into build_local_manifest/update_manifest_for_remote so behavior
remains identical (use existing symbols PROJECT_SYNC_ITEMS,
get_global_include_paths, build_local_manifest, update_manifest_for_remote,
pull, and _build_manifests to locate and update the code).
- Around line 383-394: The _local_to_remote_path function currently does string
prefix checks using hardcoded "/" which fails on Windows; change it to operate
with pathlib.Path objects and use Path.relative_to() to compute the relative
path to each project path or to the home directory (catching ValueError when not
relative), then join with remote.remote_home and proj.name using pathlib
semantics (or ensure "/"-normalized remote paths) so Windows backslashes are
handled correctly and the function returns the expected remote path for matches;
keep the fallback of returning the original local_path when no relative match is
found.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
CHANGELOG.mdREADME.mdSECURITY.mdpyproject.tomlsrc/claudesync/__init__.pysrc/claudesync/backup.pysrc/claudesync/cli.pysrc/claudesync/config.pysrc/claudesync/conflicts.pysrc/claudesync/engine.pysrc/claudesync/manifest.pysrc/claudesync/sanitize.pytests/test_backup.pytests/test_cli.pytests/test_conflicts.pytests/test_engine.pytests/test_manifest.pytests/test_sanitize.py
- backup.py: validate backup_id resolves inside BACKUP_DIR before use (prevents path traversal via crafted backup IDs like '../../etc') - backup.py: add symlink check on dest immediately before shutil.copy2 in both single-file and bulk restore branches (mitigates TOCTOU race) - cli.py: rewrite _local_to_remote_path using Path.relative_to() and as_posix() instead of string startswith(); works correctly on all OSes - tests: add test_restore_backup_rejects_traversal_in_backup_id - tests: add test_local_to_remote_path_* unit tests (project, home, unrelated) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- cli.py: extract _collect_local_files(project_paths) helper; replace duplicated local-file collection loops in pull() and _build_manifests() - README.md: add 'text' language specifier to bare code fences in Architecture and Project structure sections (MD040) - SECURITY.md: replace <owner> placeholder with jyshnkr in advisory URL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/claudesync/backup.py`:
- Around line 67-72: The code currently only checks that ts_dir is inside
BACKUP_DIR but allows multi-segment backup_id like "." or "a/.." which can
escape intentions; before constructing ts_dir or resolving, validate that
backup_id is exactly one path segment (e.g. using pathlib.Path(backup_id).parts
has length 1 and the segment is not "." or ".." and does not contain os.sep),
then proceed to build ts_dir = BACKUP_DIR / backup_id and keep the existing
is_relative_to/exists checks; reference the variables backup_id, ts_dir and
constant BACKUP_DIR when adding this validation.
In `@tests/test_cli.py`:
- Line 188: The assertion in tests/test_cli.py is using an unnecessary f-string
(assert result == f"/home/u/MyProject/CLAUDE.md") which triggers ruff F541;
change it to a plain string by removing the leading f so the assertion reads
assert result == "/home/u/MyProject/CLAUDE.md" (keep the same comparison target
and the variable result).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.mdSECURITY.mdsrc/claudesync/backup.pysrc/claudesync/cli.pytests/test_backup.pytests/test_cli.py
Comment on lines
67
to
72
| ts_dir = BACKUP_DIR / backup_id | ||
| # Guard: backup_id must resolve inside BACKUP_DIR | ||
| if not ts_dir.resolve().is_relative_to(BACKUP_DIR.resolve()): | ||
| raise ValueError(f"Invalid backup id outside backup directory: '{backup_id}'") | ||
| if not ts_dir.exists(): | ||
| raise ValueError(f"Backup '{backup_id}' not found in {BACKUP_DIR}") |
There was a problem hiding this comment.
Restrict backup_id to a single directory segment.
Current checks only enforce “inside BACKUP_DIR”, so values like "." or "20260101T000000/.." can still pass and widen restore scope unexpectedly.
🔐 Proposed fix
def restore_backup(backup_id: str, original_path: str | None = None) -> list[Path]:
@@
- ts_dir = BACKUP_DIR / backup_id
+ backup_id_path = Path(backup_id)
+ if backup_id in {"", ".", ".."} or backup_id_path.name != backup_id:
+ raise ValueError(f"Invalid backup id: '{backup_id}'")
+
+ ts_dir = BACKUP_DIR / backup_id
# Guard: backup_id must resolve inside BACKUP_DIR
if not ts_dir.resolve().is_relative_to(BACKUP_DIR.resolve()):
raise ValueError(f"Invalid backup id outside backup directory: '{backup_id}'")
- if not ts_dir.exists():
+ if not ts_dir.exists() or not ts_dir.is_dir():
raise ValueError(f"Backup '{backup_id}' not found in {BACKUP_DIR}")🧰 Tools
🪛 Ruff (0.15.2)
[warning] 70-70: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 72-72: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/claudesync/backup.py` around lines 67 - 72, The code currently only
checks that ts_dir is inside BACKUP_DIR but allows multi-segment backup_id like
"." or "a/.." which can escape intentions; before constructing ts_dir or
resolving, validate that backup_id is exactly one path segment (e.g. using
pathlib.Path(backup_id).parts has length 1 and the segment is not "." or ".."
and does not contain os.sep), then proceed to build ts_dir = BACKUP_DIR /
backup_id and keep the existing is_relative_to/exists checks; reference the
variables backup_id, ts_dir and constant BACKUP_DIR when adding this validation.
|
|
||
| result = _local_to_remote_path(local_path, [proj], remote) | ||
|
|
||
| assert result == f"/home/u/MyProject/CLAUDE.md" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
sed -n '180,195p' tests/test_cli.pyRepository: jyshnkr/claudesync
Length of output: 848
🏁 Script executed:
rg -n "assert result == f" tests/test_cli.pyRepository: jyshnkr/claudesync
Length of output: 117
🏁 Script executed:
# Check if there's a lint/flake8 configuration that enforces F541
find . -maxdepth 2 -name ".flake8" -o -name "pyproject.toml" -o -name "setup.cfg" -o -name "ruff.toml" -o -name ".ruff.toml" | head -20Repository: jyshnkr/claudesync
Length of output: 90
🏁 Script executed:
# Check ruff configuration for F541
cat ruff.tomlRepository: jyshnkr/claudesync
Length of output: 1877
🏁 Script executed:
# Also check pyproject.toml for any linting config
cat pyproject.tomlRepository: jyshnkr/claudesync
Length of output: 658
Remove the unnecessary f-string prefix at this assertion.
Line 188 contains an f-string literal without any placeholders (e.g., {variable}), which triggers ruff's F541 rule. The project's ruff.toml enforces Pyflakes rules and does not ignore F541, so this will fail linting checks.
🧹 Proposed fix
- assert result == f"/home/u/MyProject/CLAUDE.md"
+ assert result == "/home/u/MyProject/CLAUDE.md"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert result == f"/home/u/MyProject/CLAUDE.md" | |
| assert result == "/home/u/MyProject/CLAUDE.md" |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 188-188: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_cli.py` at line 188, The assertion in tests/test_cli.py is using
an unnecessary f-string (assert result == f"/home/u/MyProject/CLAUDE.md") which
triggers ruff F541; change it to a plain string by removing the leading f so the
assertion reads assert result == "/home/u/MyProject/CLAUDE.md" (keep the same
comparison target and the variable result).
Prevents path traversal via crafted backup_id values like "..", ".", or "a/.." that could escape BACKUP_DIR before the ts_dir is resolved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
PR Review Fix: backup_id path traversal validationAddressed the security finding flagged in review. Finding 1 (fixed): Fix (commit 1a92b98):
Finding 2 (not applicable): The stale f-string reference in |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/claudesync/backup.py (1)
77-78:⚠️ Potential issue | 🟠 MajorAlso require
ts_dirto be a directory.Line 77 only checks existence. A file at
BACKUP_DIR/<backup_id>would pass and lead to invalid restore behavior. Validatets_dir.is_dir()before proceeding.🔧 Proposed fix
- if not ts_dir.exists(): + if not ts_dir.exists() or not ts_dir.is_dir(): raise ValueError(f"Backup '{backup_id}' not found in {BACKUP_DIR}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/claudesync/backup.py` around lines 77 - 78, The code currently only checks ts_dir.exists() before proceeding; update the validation to ensure ts_dir is a directory (use ts_dir.is_dir()) and raise the same ValueError if it is missing or not a directory so a regular file at BACKUP_DIR/<backup_id> can't be mistaken for a backup; specifically modify the check around ts_dir (referencing ts_dir, BACKUP_DIR, and backup_id in this function in src/claudesync/backup.py) to validate is_dir() (e.g., if not ts_dir.exists() or not ts_dir.is_dir(): raise ValueError(...)) so invalid restore behavior is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/claudesync/backup.py`:
- Around line 94-99: The current TOCTOU check around dest.is_symlink() and
shutil.copy2(backup_file_path, dest) is unsafe; before writing in the function
that appends to restored, re-validate the parent/resolved-path and perform an
atomic replace: ensure dest.parent.resolve() (or dest.parent.is_symlink()) is
checked immediately before the write, create a temporary file in the same
directory (e.g., use tempfile.NamedTemporaryFile(dir=dest.parent, delete=False)
or similar), copy the backup into that temp file (preserving metadata with
shutil.copy2 to the temp), fsync if needed, then call os.replace(temp_path,
dest) to atomically replace the destination; finally re-check dest.is_symlink()
or parent symlink status where appropriate and append dest to restored only
after successful os.replace. Apply the same pattern to the other block around
shutil.copy2 at lines ~112-117.
In `@tests/test_backup.py`:
- Line 129: The test function test_restore_backup_rejects_malicious_backup_id
declares an unused fixture backup_dir which should be removed to avoid ARG001;
update the function signature to drop the backup_dir parameter (leaving bad_id
only) so the test no longer requests that fixture and rerun tests to ensure no
other references to backup_dir remain in that test.
---
Duplicate comments:
In `@src/claudesync/backup.py`:
- Around line 77-78: The code currently only checks ts_dir.exists() before
proceeding; update the validation to ensure ts_dir is a directory (use
ts_dir.is_dir()) and raise the same ValueError if it is missing or not a
directory so a regular file at BACKUP_DIR/<backup_id> can't be mistaken for a
backup; specifically modify the check around ts_dir (referencing ts_dir,
BACKUP_DIR, and backup_id in this function in src/claudesync/backup.py) to
validate is_dir() (e.g., if not ts_dir.exists() or not ts_dir.is_dir(): raise
ValueError(...)) so invalid restore behavior is avoided.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract _atomic_copy helper that writes to a tempfile in dest.parent, then os.replace() atomically renames it to dest. Checks both dest and dest.parent for symlinks. Replace both single-file and bulk restore write blocks with _atomic_copy calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Validation fires before BACKUP_DIR is ever accessed, so the fixture was never needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A regular file at BACKUP_DIR/<backup_id> would pass exists() but is not a valid backup. is_dir() correctly rejects both non-existent and wrong-type entries. Add test to cover the regular-file case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
PR Review Fix Round 2Addressed three additional review findings: Fix 1 — Atomic restore writes (TOCTOU symlink race)Extracted
Both the single-file and bulk restore paths now use Fix 2 — Unused
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/claudesync/backup.py (1)
117-129:⚠️ Potential issue | 🟠 Major
_atomic_copystill has a parent-path TOCTOU window.
dest.parent.is_symlink()is checked before usingdest.parentinNamedTemporaryFile(...)/os.replace(...). A parent-path swap between check and use can still redirect writes.🔐 Proposed hardening
-def _atomic_copy(src: Path, dest: Path) -> None: +def _atomic_copy(src: Path, dest: Path, home_root: Path) -> None: """Copy src to dest atomically via a temp file to prevent symlink races.""" - if dest.is_symlink(): - raise ValueError(f"Restore destination is a symlink: '{dest}'") - if dest.parent.is_symlink(): - raise ValueError(f"Restore destination parent is a symlink: '{dest.parent}'") - fd = tempfile.NamedTemporaryFile(dir=dest.parent, delete=False) + resolved_parent = dest.parent.resolve() + if not resolved_parent.is_relative_to(home_root): + raise ValueError(f"Restore destination outside home directory: '{dest}'") + target = resolved_parent / dest.name + fd = tempfile.NamedTemporaryFile(dir=resolved_parent, delete=False) tmp = Path(fd.name) fd.close() try: shutil.copy2(src, tmp) - os.replace(tmp, dest) + os.replace(tmp, target) except BaseException: tmp.unlink(missing_ok=True) raise- _atomic_copy(backup_file_path, dest) + _atomic_copy(backup_file_path, dest, Path.home().resolve()) - _atomic_copy(src, dest) + _atomic_copy(src, dest, home_root)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/claudesync/backup.py` around lines 117 - 129, The TOCTOU exists because dest.parent is checked with is_symlink() then used; fix _atomic_copy by opening the parent directory atomically and proving it hasn't changed before creating the temp file: obtain a dirfd via os.open(dest.parent, os.O_RDONLY|os.O_DIRECTORY), compare os.fstat(dirfd).(st_ino,st_dev) to os.lstat(dest.parent).(st_ino,st_dev) to ensure the same directory, then create the temporary file against that opened directory (e.g. use tempfile.NamedTemporaryFile(dir=f"/proc/self/fd/{dirfd}", delete=False) or use os.openat to create the temp file), perform shutil.copy2 to the temp path and os.replace to dest, and finally close the dirfd and ensure the temp file is cleaned up on exceptions; update symbols: _atomic_copy, dest.parent, tempfile.NamedTemporaryFile, os.open/os.openat, os.replace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/claudesync/backup.py`:
- Around line 117-129: The TOCTOU exists because dest.parent is checked with
is_symlink() then used; fix _atomic_copy by opening the parent directory
atomically and proving it hasn't changed before creating the temp file: obtain a
dirfd via os.open(dest.parent, os.O_RDONLY|os.O_DIRECTORY), compare
os.fstat(dirfd).(st_ino,st_dev) to os.lstat(dest.parent).(st_ino,st_dev) to
ensure the same directory, then create the temporary file against that opened
directory (e.g. use tempfile.NamedTemporaryFile(dir=f"/proc/self/fd/{dirfd}",
delete=False) or use os.openat to create the temp file), perform shutil.copy2 to
the temp path and os.replace to dest, and finally close the dirfd and ensure the
temp file is cleaned up on exceptions; update symbols: _atomic_copy,
dest.parent, tempfile.NamedTemporaryFile, os.open/os.openat, os.replace.
Replace naive is_symlink() + NamedTemporaryFile with: 1. os.lstat(dest.parent) to check the link mode and capture inode/dev 2. os.open(dest.parent, O_RDONLY|O_NOFOLLOW|O_DIRECTORY) to get a dirfd that fails immediately if dest.parent is a symlink 3. os.fstat(dirfd) inode/dev comparison to detect a swap between lstat and open (closes the TOCTOU window) 4. os.openat(dirfd, ...) to create the temp file directly inside the verified dirfd; falls back to os.open(full_path) on platforms where openat is unavailable (e.g. some macOS Python builds) Add test_atomic_copy_rejects_symlink_dest and test_atomic_copy_rejects_symlink_parent to cover both rejection paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
PR Review Fix Round 3 — TOCTOU in
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/claudesync/backup.py`:
- Around line 127-128: The except block catching FileNotFoundError should
preserve the original exception when re-raising as a ValueError for better
traceability; change the handler so it captures the original exception (e.g.,
except FileNotFoundError as e) and re-raises ValueError(f"Restore destination
parent does not exist: '{dest.parent}'") from e — locate the FileNotFoundError
except in src/claudesync/backup.py (the block referencing dest.parent) and make
this single-line change to chain the exceptions.
Preserves the original exception for better traceability in tracebacks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
PR Review Fix Round 4 — Exception chaining in
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v0.2.0, withCHANGELOG.md,README.md, and updatedSECURITY.mdChanges by Phase
Phase 1 — Security Fixes
restore_backup()elsebranch now checks source resolves inside backup archive and destination resolves inside$HOMEmerge_pulled_claude_json(),save_manifest(),save_config()test_restore_all_rejects_dest_outside_home,test_merge_preserves_file_permissionsPhase 2 — Bug Fixes
_local_to_remote_path()helper; fix_build_manifests()to translate paths before querying remote hashesFileNotFoundErroringet_remote_file_hashes()(missing SSH binary) →SyncErrordictinsanitize_claude_json(),merge_pulled_claude_json(), andload_manifest()test_get_remote_file_hashes_raises_on_missing_ssh,test_sanitize_rejects_non_dict_json,test_merge_rejects_non_dict_remote,test_merge_rejects_non_dict_local,test_load_manifest_rejects_non_dict_json,test_pull_rebuilds_manifest_after_syncPhase 3 — Minor Improvements
.claude.jsontransfers inSyncSummary.files_transferredsync.strategyconfig field at load timeSyncedFileEntry.last_syncedis now optional (TypedDict inheritance for Python 3.9 compat)/tmppaths in tests; add exit_code assertions; remove unused fixture paramtest_detect_conflicts_last_sync_none_uses_fallbackPhase 4 — Trivial Cleanup
FileConflict.winnerdry_runkeyword-only on_rsync_global()and_rsync_project()test_rsync_project_aggregates_all_failuresassertion from>= 2to== 3Phase 5 — Release Documentation
0.2.0CHANGELOG.md(Keep a Changelog format)README.mdSECURITY.mdwith actual version support table and reporting instructionsTest plan
pytest— all 97 tests passpython -c "from claudesync import __version__; print(__version__)"→0.2.0git tag --list→v0.1.0,v0.2.0v0.2.0-release🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests