fix(migrate): reject execute() plans whose destination equals the source - #22
Merged
2 commits merged intoSep 14, 2026
Merged
2 commits merged into
2 commits merged into
Conversation
plan() refuses dest==src, but execute() re-reads the plan JSON from disk, so a hand-edited plan file can still pair a directory with itself: an empty source dir slips past the non-empty-destination check, the copy trivially verifies (0==0), and rmtree then deletes it as its own destination. The guard compares resolve_path()+normcase() on both sides, so trailing separators and Windows case variants are covered too. Surfaced by a Socket supply-chain audit anomaly (LOW) on core/migrate.py.
…POSIX CI caught the guard's blind spot: os.path.normcase lowercases on Windows but returns POSIX paths unchanged, while macOS's default APFS volumes are case-insensitive. A tampered plan pairing '/Users/me/models' with an upper-cased spelling slipped past the guard on macOS and ran the self- migration flow. Compare resolved paths, fold case additionally on darwin; on Linux the comparison stays exact (case-sensitive filesystems keep distinct Foo/foo as distinct directories).
tzzs
added a commit
that referenced
this pull request
Sep 13, 2026
execute() was only covered by the dest==source guard tests; its whole status matrix ran untested. Drive it through the real rules engine (a custom rules dir marks the source as migratable FakeApp data) against fake copy/link engines and get_work_dir pointed at tmp_path: - succeeded via link method and via manual method (hint recorded, no link created) - verification-failed leaves the original untouched - copy-ok-source-not-removed when rmtree raises OSError - dry-run copies nothing and writes no result file - RequiresAppClosed blocks until --app-closed is attested - stale-plan gates: vanished source, non-empty destination verify() had no coverage at all: passing report writes LastVerification back into the result file, missing destination / drifted file counts / broken link each fail their own check, and a missing result file raises FileNotFoundError. Full suite: 156 passed, 39 skipped -- skip count matches the known Windows baseline (39). Stacked on #22.
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.
Background
Triage result of the Socket supply-chain audit (2026-09-11, LOW anomaly on
src/storops/core/migrate.py):verify()NameError claim: false.return reportis correctly spelled andreportis defined at migrate.py:246; a regex pickaxe over all history (git log -G"return repor([^t]|$)") finds no commit ever contained the typo. Artifact of Socket's fragment analysis.execute()"trusts a mutable JSON plan" claim: overstated.execute()already re-runsrules.identify_path()+risk.assert_not_critical()+ themigratablecheck at execution time against the current rules DB (the plan's stored risk field is ignored), and deletion only happens after a verified copy. Socket's operational recommendation is largely covered by existing code.dest == srcguard inexecute().plan()refuses it (migrate.py:33), butexecute()re-reads the plan JSON from disk, so a hand-edited plan can pair a directory with itself. An empty source dir slips past the non-empty-destination check, the copy degenerates to a no-op that trivially verifies (0==0), andshutil.rmtree()then deletes the directory as its own destination (and a self-pointing link may be created afterwards).Change
execute()now rejects plans whose resolved destination equals the resolved source, raisingUnsupportedOperationError(same error class asplan()'s identical condition). Comparison isresolve_path()+os.path.normcase()on both sides, covering trailing separators and case-insensitive filesystems.tests/unit/test_migrate_execute.py(3 tests: exact match, trailing-separator normalization, case-insensitive match — the last skipped on case-sensitive platforms).Verification
143 passed, 39 skipped— skip count matches the known Windows baseline (39).