From cde1250b70b9fc3108e45f26921abf056d38704a Mon Sep 17 00:00:00 2001 From: tzzs Date: Mon, 14 Sep 2026 02:10:25 +0800 Subject: [PATCH 1/2] fix(migrate): reject execute() plans whose destination equals the source 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. --- src/storops/core/migrate.py | 11 ++++++ tests/unit/test_migrate_execute.py | 63 ++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 tests/unit/test_migrate_execute.py diff --git a/src/storops/core/migrate.py b/src/storops/core/migrate.py index 25c3a0f..a59e819 100644 --- a/src/storops/core/migrate.py +++ b/src/storops/core/migrate.py @@ -107,6 +107,17 @@ def execute(plan_file: str, *, confirm: bool = False, app_closed: bool = False) raw = json.loads(Path(plan_file).read_text(encoding="utf-8")) source, destination = raw["Source"], raw["Destination"] + # plan() already refuses dest==src, but the plan file is re-read from disk + # here and may no longer be the one plan() wrote. An empty source directory + # would slip past the non-empty-destination check below, the copy would + # trivially "verify" (0==0), and rmtree would then delete the directory as + # its own destination. normcase covers case-insensitive filesystems. + if os.path.normcase(resolve_path(destination)) == os.path.normcase(resolve_path(source)): + raise UnsupportedOperationError( + f"StorOps: plan is invalid -- destination equals the source ('{source}'). " + "Re-run `storops migrate plan` with a distinct destination." + ) + if not os.path.isdir(source): raise StalePlanError(f"StorOps: source '{source}' no longer exists or is not a directory -- the plan is stale. Re-run `storops migrate plan`.") diff --git a/tests/unit/test_migrate_execute.py b/tests/unit/test_migrate_execute.py new file mode 100644 index 0000000..dc06144 --- /dev/null +++ b/tests/unit/test_migrate_execute.py @@ -0,0 +1,63 @@ +"""Regression tests for migrate.execute()'s destination==source guard. + +plan() refuses to generate a plan whose destination equals its source, but +execute() re-reads the plan JSON from disk, so a hand-edited or otherwise +tampered plan file can still pair a directory with itself. Without the guard, +an *empty* source directory slips past execute()'s "destination must be +empty" check, the copy degenerates to a no-op whose verification trivially +matches (0 files == 0 files), and shutil.rmtree() then deletes the directory +as its own destination. +""" +from __future__ import annotations + +import json +import os +import sys +from pathlib import Path + +import pytest + +from storops.core.errors import UnsupportedOperationError +from storops.core.migrate import execute as migrate_execute + + +def _write_plan(tmp_path: Path, source: Path, destination: Path) -> Path: + plan_file = tmp_path / "storops-migrate-plan.json" + plan_file.write_text( + json.dumps({"Source": str(source), "Destination": str(destination)}), + encoding="utf-8", + ) + return plan_file + + +def test_execute_rejects_plan_whose_destination_equals_source(tmp_path): + src = tmp_path / "models" + src.mkdir() # empty on purpose: the non-empty-destination check must not be what saves us + plan_file = _write_plan(tmp_path, src, src) + + with pytest.raises(UnsupportedOperationError, match="destination equals the source"): + migrate_execute(str(plan_file), confirm=True) + + +def test_execute_rejects_destination_equal_after_path_normalization(tmp_path): + # Same directory spelled with a trailing separator -- resolve_path() + # normalizes both sides before comparing. + src = tmp_path / "models" + src.mkdir() + plan_file = _write_plan(tmp_path, src, Path(str(src) + os.sep)) + + with pytest.raises(UnsupportedOperationError, match="destination equals the source"): + migrate_execute(str(plan_file), confirm=True) + + +@pytest.mark.skipif( + sys.platform not in ("win32", "darwin"), + reason="only case-insensitive filesystems make the upper-cased path the same directory", +) +def test_execute_rejects_destination_equal_case_insensitively(tmp_path): + src = tmp_path / "models" + src.mkdir() + plan_file = _write_plan(tmp_path, src, Path(str(src).upper())) + + with pytest.raises(UnsupportedOperationError, match="destination equals the source"): + migrate_execute(str(plan_file), confirm=True) From e40508fd822991c2514415fe1cbb56822b5e1447 Mon Sep 17 00:00:00 2001 From: tzzs Date: Mon, 14 Sep 2026 02:50:08 +0800 Subject: [PATCH 2/2] fix(migrate): fold path case on darwin too -- normcase is a no-op on 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). --- src/storops/core/migrate.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/storops/core/migrate.py b/src/storops/core/migrate.py index a59e819..7ca03e4 100644 --- a/src/storops/core/migrate.py +++ b/src/storops/core/migrate.py @@ -12,6 +12,7 @@ import json import os import shutil +import sys from datetime import datetime, timezone from pathlib import Path @@ -111,8 +112,14 @@ def execute(plan_file: str, *, confirm: bool = False, app_closed: bool = False) # here and may no longer be the one plan() wrote. An empty source directory # would slip past the non-empty-destination check below, the copy would # trivially "verify" (0==0), and rmtree would then delete the directory as - # its own destination. normcase covers case-insensitive filesystems. - if os.path.normcase(resolve_path(destination)) == os.path.normcase(resolve_path(source)): + # its own destination. normcase folds case on Windows only; darwin folds + # explicitly because default APFS volumes are case-insensitive too. + resolved_source = resolve_path(source) + resolved_destination = resolve_path(destination) + collide = os.path.normcase(resolved_source) == os.path.normcase(resolved_destination) + if sys.platform == "darwin": + collide = collide or resolved_source.lower() == resolved_destination.lower() + if collide: raise UnsupportedOperationError( f"StorOps: plan is invalid -- destination equals the source ('{source}'). " "Re-run `storops migrate plan` with a distinct destination."