diff --git a/scripts/artifacts.py b/scripts/artifacts.py index b233bb2e..e0442d53 100644 --- a/scripts/artifacts.py +++ b/scripts/artifacts.py @@ -50,6 +50,25 @@ def write_if_changed( return True +def plain_file_mode() -> int: + """The mode open() gives a new file: 0o666 less the process umask.""" + # Reading the umask sets it, so it is put back at once. + umask = os.umask(0) + os.umask(umask) + return 0o666 & ~umask + + +def set_plain_mode(path: str | os.PathLike) -> None: + """Give a scratch file the mode a plain open() would have. + + mkstemp creates its file 0600 so no other user reads it while it is + written. Renamed into place, that mode stayed: database.json, the + manifests, the packs and the recipe snapshots came out unreadable to + every other account on the machine and to a web server serving them. + """ + os.chmod(path, plain_file_mode()) + + def write_text_atomic(path: str, content: str) -> None: """Write a whole file or nothing. @@ -65,6 +84,7 @@ def write_text_atomic(path: str, content: str) -> None: try: with os.fdopen(handle, "w", encoding="utf-8") as f: f.write(content) + set_plain_mode(scratch) os.replace(scratch, path) except BaseException: with contextlib.suppress(OSError): @@ -81,6 +101,7 @@ def write_bytes_atomic(path: str, content: bytes) -> None: try: with os.fdopen(handle, "wb") as f: f.write(content) + set_plain_mode(scratch) os.replace(scratch, path) except BaseException: with contextlib.suppress(OSError): diff --git a/scripts/common.py b/scripts/common.py index 79404098..35b53465 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -2805,6 +2805,8 @@ from artifacts import ( # noqa: E402,F401 write_if_changed, write_text_atomic, copy_file_atomic, + plain_file_mode, + set_plain_mode, ArtifactLockBusy, artifact_lock, hold_artifact_lock, diff --git a/scripts/generate_db.py b/scripts/generate_db.py index 398a3b71..855a6fd6 100644 --- a/scripts/generate_db.py +++ b/scripts/generate_db.py @@ -27,6 +27,7 @@ from common import ( list_registered_platforms, load_provenance_snapshots, require_yaml, + set_plain_mode, source_yaml_paths, variant_base_name, write_if_changed, @@ -309,6 +310,7 @@ def _restore(cached: str, path: str) -> None: os.close(fd) try: shutil.copy2(cached, scratch) + set_plain_mode(scratch) os.replace(scratch, path) finally: if os.path.exists(scratch): diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index e1cb1060..2fb7f728 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -14,7 +14,7 @@ from __future__ import annotations from collections.abc import Callable, Iterator -from artifacts import _build_timestamp +from artifacts import _build_timestamp, set_plain_mode import argparse import contextlib import functools @@ -221,6 +221,7 @@ def _writing_pack(zip_path: str) -> Iterator[zipfile.ZipFile]: try: with zipfile.ZipFile(scratch, "w", zipfile.ZIP_DEFLATED) as zf: yield zf + set_plain_mode(scratch) os.replace(scratch, zip_path) except BaseException: Path(scratch).unlink(missing_ok=True) @@ -4573,6 +4574,7 @@ def inject_manifest(zip_path: str, manifest: dict) -> None: continue dst.writestr(item, src.read(item.filename)) _write_generated_member(dst, "manifest.json", manifest_json) + set_plain_mode(scratch) os.replace(scratch, zip_path) except BaseException: Path(scratch).unlink(missing_ok=True) diff --git a/scripts/largefiles.py b/scripts/largefiles.py index 5089f44e..2c54fd43 100644 --- a/scripts/largefiles.py +++ b/scripts/largefiles.py @@ -19,6 +19,7 @@ from collections import Counter from collections.abc import Iterable from pathlib import Path +from artifacts import set_plain_mode from hashing import compute_hashes @@ -219,6 +220,7 @@ def _fetch_asset( compute_hashes(tmp_path), expected_sha1, expected_md5 ): return None + set_plain_mode(tmp_path) os.replace(tmp_path, cached) return cached finally: diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index d39e855d..f1f0c4b5 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -25,7 +25,7 @@ import urllib.request import zipfile from collections.abc import Iterator from pathlib import Path -from artifacts import file_lock as _file_lock +from artifacts import file_lock as _file_lock, set_plain_mode from common import github_token, platform_arg, require_platform, yaml_load try: @@ -133,6 +133,7 @@ def _save_versions( with os.fdopen(handle, "w") as f: json.dump(versions, f, indent=2, sort_keys=True) f.write("\n") + set_plain_mode(scratch) os.replace(scratch, path) except BaseException: with contextlib.suppress(OSError): diff --git a/scripts/upstream.py b/scripts/upstream.py index e5eec95f..025b1de2 100644 --- a/scripts/upstream.py +++ b/scripts/upstream.py @@ -21,6 +21,8 @@ from dataclasses import dataclass from datetime import datetime from pathlib import Path +from artifacts import set_plain_mode + USER_AGENT = "retrobios-profile-sync/1.0" # Where fetched upstream files live, shared by every reader of this cache. CACHE_DIR = ".cache/upstream" @@ -343,6 +345,7 @@ def write_cache(target: Path, text: str) -> None: try: with os.fdopen(fd, "w", encoding="utf-8") as fh: fh.write(text) + set_plain_mode(scratch) os.replace(scratch, target) except OSError: Path(scratch).unlink(missing_ok=True) diff --git a/scripts/ziptools.py b/scripts/ziptools.py index 7e8ab3f1..cb82d41b 100644 --- a/scripts/ziptools.py +++ b/scripts/ziptools.py @@ -14,6 +14,7 @@ import sys import tempfile import zipfile +from artifacts import set_plain_mode from hashing import md5sum @@ -141,6 +142,7 @@ def _write_member_cache(path: str, data: dict[str, list[list]]) -> None: try: with os.fdopen(fd, "w", encoding="utf-8") as fh: json.dump(data, fh, separators=(",", ":")) + set_plain_mode(scratch) os.replace(scratch, path) except OSError as exc: print(f"warning: could not write {path}: {exc}", file=sys.stderr) diff --git a/tests/test_atomic_artifacts.py b/tests/test_atomic_artifacts.py index 497e6650..5da43354 100644 --- a/tests/test_atomic_artifacts.py +++ b/tests/test_atomic_artifacts.py @@ -129,5 +129,90 @@ class TheNativeIndexKeepsEveryPlatform(unittest.TestCase): self.assertEqual(recorded.get("batocera/file"), "https://a") self.assertEqual((root / "batocera" / "file").read_bytes(), b"payload") +class ScratchFilesGetThePlainMode(unittest.TestCase): + """mkstemp makes its file 0600 and the rename kept it: database.json, + the manifests and the packs came out readable by their owner only.""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.dir = Path(self._tmp.name) + previous = os.umask(0o022) + self.addCleanup(os.umask, previous) + + def _mode(self, path: Path) -> int: + return path.stat().st_mode & 0o777 + + def test_text_and_bytes_writers(self): + artifacts.write_text_atomic(str(self.dir / "a.json"), "{}") + artifacts.write_bytes_atomic(str(self.dir / "b.bin"), b"x") + self.assertEqual(self._mode(self.dir / "a.json"), 0o644) + self.assertEqual(self._mode(self.dir / "b.bin"), 0o644) + + def test_the_umask_decides(self): + os.umask(0o002) + artifacts.write_text_atomic(str(self.dir / "a.json"), "{}") + self.assertEqual(self._mode(self.dir / "a.json"), 0o664) + + def test_packs_and_their_manifest(self): + import zipfile # noqa: PLC0415 + + import generate_pack # noqa: PLC0415 + + pack = self.dir / "Pack.zip" + with generate_pack._writing_pack(str(pack)) as zf: + zf.writestr("a.bin", b"x") + self.assertEqual(self._mode(pack), 0o644) + os.chmod(pack, 0o600) + generate_pack.inject_manifest(str(pack), {"files": []}) + self.assertEqual(self._mode(pack), 0o644) + with zipfile.ZipFile(pack) as zf: + self.assertIn("manifest.json", zf.namelist()) + + def test_every_renamed_scratch_gets_the_mode(self): + """A function that renames a mkstemp file into place sets its mode. + + copy_file_atomic is the one exception: copy2 gives the scratch the + source file's own mode. + """ + import ast # noqa: PLC0415 + + def name_of(node: ast.AST) -> str | None: + return node.id if isinstance(node, ast.Name) else None + + def callee(call: ast.Call) -> str: + func = call.func + return func.attr if isinstance(func, ast.Attribute) else name_of(func) or "" + + unset = [] + for path in sorted((REPO_ROOT / "scripts").rglob("*.py")): + tree = ast.parse(path.read_text(encoding="utf-8")) + for func in ast.walk(tree): + if not isinstance(func, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if func.name == "copy_file_atomic": + continue + scratches: set[str] = set() + for node in ast.walk(func): + if (isinstance(node, ast.Assign) and isinstance(node.value, ast.Call) + and callee(node.value) == "mkstemp" + and isinstance(node.targets[0], ast.Tuple)): + scratch = name_of(node.targets[0].elts[1]) + if scratch: + scratches.add(scratch) + calls = [n for n in ast.walk(func) if isinstance(n, ast.Call)] + moded = { + name_of(c.args[0]) for c in calls + if callee(c) in ("set_plain_mode", "chmod") and c.args + } + for call in calls: + if callee(call) != "replace" or not call.args: + continue + first = name_of(call.args[0]) + if first in scratches and first not in moded: + unset.append(f"{path.name}:{call.lineno} {func.name}") + self.assertEqual(unset, []) + + if __name__ == "__main__": unittest.main()