diff --git a/strix/interface/cloud/source_upload.py b/strix/interface/cloud/source_upload.py index 780a84569..81dfa6486 100644 --- a/strix/interface/cloud/source_upload.py +++ b/strix/interface/cloud/source_upload.py @@ -127,6 +127,26 @@ _ARCHIVE_MAGIC_PREFIXES = ( ) +# Whether `st_ctime_ns` belongs in the identity a source file is pinned to. +# +# On POSIX it is the inode-change time and a real tamper signal: a chmod or a +# rename that leaves size and mtime untouched still moves it. On Windows the +# value does not hold still for a file that was written moments ago. A path +# `stat` can be served from cached directory metadata while `fstat` reads the +# handle directly, and NTFS settles that metadata asynchronously, so the two +# disagree by roughly 1-30 ms with nothing having touched the file. Comparing +# it there rejects unchanged sources instead of catching tampering (#1258). +_IDENTITY_INCLUDES_CTIME = os.name != "nt" + + +def _stat_identity(info: os.stat_result) -> tuple[int, ...]: + """Fields that must not change between selecting a file and archiving it.""" + identity: tuple[int, ...] = (info.st_dev, info.st_ino, info.st_size, info.st_mtime_ns) + if _IDENTITY_INCLUDES_CTIME: + identity = (*identity, info.st_ctime_ns) + return identity + + @dataclass(frozen=True) class SelectedFile: path: Path @@ -137,6 +157,13 @@ class SelectedFile: mtime_ns: int ctime_ns: int + @property + def identity(self) -> tuple[int, ...]: + identity: tuple[int, ...] = (self.device, self.inode, self.size, self.mtime_ns) + if _IDENTITY_INCLUDES_CTIME: + identity = (*identity, self.ctime_ns) + return identity + @dataclass(frozen=True) class SourceManifest: @@ -597,14 +624,7 @@ def _write_archive(destination: Path, files: tuple[SelectedFile, ...]) -> None: raise http.CloudError(f"could not safely read {item.archive_name}: {exc}") from exc with os.fdopen(descriptor, "rb") as source_file: current = os.fstat(source_file.fileno()) - if ( - not stat.S_ISREG(current.st_mode) - or current.st_size != item.size - or current.st_dev != item.device - or current.st_ino != item.inode - or current.st_mtime_ns != item.mtime_ns - or current.st_ctime_ns != item.ctime_ns - ): + if not stat.S_ISREG(current.st_mode) or _stat_identity(current) != item.identity: raise http.CloudError( f"{item.archive_name} changed while the source archive was being built; " "retry." @@ -627,11 +647,7 @@ def _write_archive(destination: Path, files: tuple[SelectedFile, ...]) -> None: if ( source_file.read(1) or not stat.S_ISREG(final.st_mode) - or final.st_size != item.size - or final.st_dev != item.device - or final.st_ino != item.inode - or final.st_mtime_ns != item.mtime_ns - or final.st_ctime_ns != item.ctime_ns + or _stat_identity(final) != item.identity ): raise http.CloudError( f"{item.archive_name} changed while the source archive was being " diff --git a/tests/test_cloud_source_upload.py b/tests/test_cloud_source_upload.py index dcd40baa8..8ad650527 100644 --- a/tests/test_cloud_source_upload.py +++ b/tests/test_cloud_source_upload.py @@ -696,3 +696,59 @@ def test_oversize_archive_names_largest_files_and_exclude_guidance( assert "--exclude" in raised.value.next_step assert "--dry-run --show-files" in raised.value.next_step assert not list(tmp_path.glob("strix-source-*.zip")) + + +class _CtimeDriftedStat: + """A stat result whose `st_ctime_ns` has moved and nothing else.""" + + def __init__(self, real: os.stat_result, ctime_ns: int) -> None: + self._real = real + self.st_ctime_ns = ctime_ns + + def __getattr__(self, name: str) -> Any: + return getattr(self._real, name) + + +def _drift_ctime(monkeypatch: pytest.MonkeyPatch, delta_ns: int) -> None: + """Make every `fstat` in the archiver report a later ctime than selection saw.""" + real_fstat = os.fstat + + def drifted(fileno: int) -> Any: + info = real_fstat(fileno) + return _CtimeDriftedStat(info, info.st_ctime_ns + delta_ns) + + monkeypatch.setattr(source_upload.os, "fstat", drifted) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows is where ctime drifts on its own") +def test_archive_survives_a_windows_ctime_only_drift( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """An unchanged file must still archive when only `st_ctime_ns` moved. + + Windows serves a path `stat` from cached directory metadata and `fstat` + from the handle, and NTFS settles that metadata asynchronously, so the two + disagree for a short window after a write with nothing having touched the + file. That is not tampering and must not fail the upload. + """ + (tmp_path / "app.py").write_bytes(b"safe") + manifest = source_upload.select_source(tmp_path) + _drift_ctime(monkeypatch, 30_000_000) # 30 ms, the worst drift observed + + source_upload._write_archive(tmp_path / "source.zip", manifest.files) + + with zipfile.ZipFile(tmp_path / "source.zip") as archive: + assert archive.read("app.py") == b"safe" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX ctime is a real tamper signal") +def test_archive_still_rejects_a_posix_ctime_change( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """On POSIX `st_ctime_ns` is inode-change time, so a move in it is a change.""" + (tmp_path / "app.py").write_bytes(b"safe") + manifest = source_upload.select_source(tmp_path) + _drift_ctime(monkeypatch, 30_000_000) + + with pytest.raises(http.CloudError, match="changed while the source archive was being built"): + source_upload._write_archive(tmp_path / "source.zip", manifest.files)