scriptoriumadmin/strix
1
0
Fork 0
mirror of https://github.com/usestrix/strix.git synced 2026-11-14 09:01:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

fix(cloud): pin selected sources with handle stats instead of dropping ctime

Browse source 
On Windows a path stat reports the creation time as st_ctime_ns while fstat on a handle reports the last metadata change, so pinning files with lstat and checking them with fstat compared two different timestamps and rejected every file modified after it was created. The previous commit excluded st_ctime_ns on Windows, which also stopped a chmod or rename from being detected there.

Selection now takes fstat from the file's own handle, after checking it is the same file lstat saw, and reads the archive header from that handle. Both sides of the comparison mean the same thing on every platform, so st_ctime_ns is part of the identity everywhere again.
This commit is contained in:
itzzdev09 2026-09-15 17:51:38 +05:30
parent 3b63de2ad4
commit 721cf75626
2 changed files with 119 additions and 57 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

85
strix/interface/cloud/source_upload.py
View file

@ -127,24 +127,19 @@ _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
"""Fields that must not change between selecting a file and archiving it.
Both sides of the comparison must come from `fstat` on an open handle.
What `st_ctime_ns` means depends on how a file is stat'ed: on POSIX a path
stat and `fstat` both report the inode-change time, but on Windows a path
stat reports the creation time while `fstat` reports the last metadata
change. Pinning files with a path stat therefore rejected every file
modified after it was created (#1258). With handle stats on both sides the
two agree, and `st_ctime_ns` stays a tamper signal on every platform: a
chmod or a rename that leaves size and mtime alone still moves it.
"""
return (info.st_dev, info.st_ino, info.st_size, info.st_mtime_ns, info.st_ctime_ns)
@dataclass(frozen=True)
@ -159,10 +154,7 @@ class SelectedFile:
@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
return (self.device, self.inode, self.size, self.mtime_ns, self.ctime_ns)
@dataclass(frozen=True)
@ -329,10 +321,21 @@ def select_source(
if not stat.S_ISREG(info.st_mode):
excluded["symlink_or_non_file"] += 1
continue
if not include_archives and _has_archive_magic(path):
# Pin the file with the same kind of stat _write_archive checks it with.
handle_info, header = _inspect_regular_file(
path, archive_name, read_header=not include_archives
)
if not stat.S_ISREG(handle_info.st_mode) or (
handle_info.st_dev,
handle_info.st_ino,
) != (info.st_dev, info.st_ino):
raise http.CloudError(
f"{archive_name} changed while the source was being selected; retry."
)
if not include_archives and _is_archive_header(header):
excluded["nested_archive"] += 1
continue
if info.st_size > MAX_FILE_BYTES:
if handle_info.st_size > MAX_FILE_BYTES:
raise http.CloudError(
f"{archive_name} is larger than the 25 MB per-file limit; exclude it explicitly."
)
@ -340,14 +343,14 @@ def select_source(
SelectedFile(
path=path,
archive_name=archive_name,
size=info.st_size,
device=info.st_dev,
inode=info.st_ino,
mtime_ns=info.st_mtime_ns,
ctime_ns=info.st_ctime_ns,
size=handle_info.st_size,
device=handle_info.st_dev,
inode=handle_info.st_ino,
mtime_ns=handle_info.st_mtime_ns,
ctime_ns=handle_info.st_ctime_ns,
)
)
total_bytes += info.st_size
total_bytes += handle_info.st_size
if len(selected) > MAX_FILES:
raise http.CloudError(
f"source contains more than {MAX_FILES:,} files; narrow --source or add exclusions."
@ -669,12 +672,34 @@ def _has_archive_magic(path: Path) -> bool:
try:
descriptor = os.open(path, flags)
with os.fdopen(descriptor, "rb") as stream:
header = stream.read(512)
header = stream.read(_ARCHIVE_HEADER_BYTES)
except OSError:
return False
return _is_archive_header(header)
_ARCHIVE_HEADER_BYTES = 512
def _is_archive_header(header: bytes) -> bool:
return header.startswith(_ARCHIVE_MAGIC_PREFIXES) or header[257:262] == b"ustar"
def _inspect_regular_file(
path: Path, archive_name: str, *, read_header: bool
) -> tuple[os.stat_result, bytes]:
"""Stat a file through its own handle, reading its header too if asked."""
flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)
try:
descriptor = os.open(path, flags)
with os.fdopen(descriptor, "rb") as stream:
info = os.fstat(stream.fileno())
header = stream.read(_ARCHIVE_HEADER_BYTES) if read_header else b""
except OSError as exc:
raise http.CloudError(f"could not safely read {archive_name}: {exc}") from exc
return info, header
def _load_ignore_patterns(source: Path) -> list[str]:
path = source / ".strixignore"
raw_text = _read_ignore_file(path)

91
tests/test_cloud_source_upload.py
View file

@ -6,6 +6,7 @@ import json
import os
import shutil
import subprocess
import time
import zipfile
from typing import TYPE_CHECKING, Any
@ -698,42 +699,35 @@ def test_oversize_archive_names_largest_files_and_exclude_guidance(
assert not list(tmp_path.glob("strix-source-*.zip"))
class _CtimeDriftedStat:
"""A stat result whose `st_ctime_ns` has moved and nothing else."""
class _StatWith:
"""A stat result with some of its fields replaced."""
def __init__(self, real: os.stat_result, ctime_ns: int) -> None:
def __init__(self, real: Any, **fields: int) -> None:
self._real = real
self.st_ctime_ns = ctime_ns
self.__dict__.update(fields)
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(
def test_archive_accepts_a_path_stat_with_a_different_ctime(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""An unchanged file must still archive when only `st_ctime_ns` moved.
"""Selection must not pin files with what a path stat calls `st_ctime_ns`.
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.
On Windows a path stat reports the creation time there and `fstat` the last
metadata change, so for a file modified after it was created the two
differ for good (#1258). This reproduces that on any platform.
"""
(tmp_path / "app.py").write_bytes(b"safe")
real_lstat = source_upload.Path.lstat
def lstat_reporting_creation_time(self: Path) -> Any:
info = real_lstat(self)
return _StatWith(info, st_ctime_ns=info.st_ctime_ns - 7 * 60 * 1_000_000_000)
monkeypatch.setattr(source_upload.Path, "lstat", lstat_reporting_creation_time)
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)
@ -741,14 +735,57 @@ def test_archive_survives_a_windows_ctime_only_drift(
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(
@pytest.mark.skipif(os.name != "nt", reason="only Windows reports different ctimes")
def test_archive_accepts_a_windows_file_modified_after_creation(tmp_path: Path) -> None:
source_path = tmp_path / "app.py"
source_path.write_bytes(b"safe")
time.sleep(0.1)
with source_path.open("ab") as stream:
stream.write(b" and sound")
with source_path.open("rb") as stream:
# the situation from #1258: the path and the handle disagree on ctime
assert os.fstat(stream.fileno()).st_ctime_ns != source_path.lstat().st_ctime_ns
manifest = source_upload.select_source(tmp_path)
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 and sound"
def test_archive_rejects_a_metadata_only_change_after_selection(tmp_path: Path) -> None:
"""A chmod leaves size and mtime alone but moves the handle ctime, on Windows too."""
source_path = tmp_path / "app.py"
source_path.write_bytes(b"safe")
manifest = source_upload.select_source(tmp_path)
time.sleep(0.1) # step past the filesystem's timestamp resolution
source_path.chmod(0o444)
try:
with pytest.raises(
http.CloudError, match="changed while the source archive was being built"
):
source_upload._write_archive(tmp_path / "source.zip", manifest.files)
finally:
source_path.chmod(0o644)
def test_archive_rejects_a_metadata_change_during_the_read(
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)
real_fstat = os.fstat
calls = 0
def fstat_changing_after_the_first_call(fileno: int) -> Any:
nonlocal calls
calls += 1
info = real_fstat(fileno)
if calls == 1:
return info
return _StatWith(info, st_ctime_ns=info.st_ctime_ns + 1)
monkeypatch.setattr(source_upload.os, "fstat", fstat_changing_after_the_first_call)
with pytest.raises(http.CloudError, match="changed while the source archive was being built"):
source_upload._write_archive(tmp_path / "source.zip", manifest.files)