From e9852e6c866c0b0fe13aeca872796bd206b776d5 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 13:21:07 +0100 Subject: [PATCH] fix: keep the mirror readable under a umask that masks the owner's read bit The umask handling added for group access cleared only the group bits and left owner and other to the environment. A container whose umask carries 0400 then produces mirror directories of mode 0300: writable and enterable, unreadable to the very run that created them, and unreadable to anything serving the share. Clear the owner read and execute bits from the umask as well. The `other` bits stay where the environment puts them, because whether the mirror is world-readable is a real policy question; being able to read a directory the process itself just created is not. Files were never exposed to this: mkstemp sets 0600 outright and copy2 takes the source file's mode, both ignoring the umask. --- README.md | 11 ++++++++--- music_mirror.py | 19 +++++++++++++------ tests/conftest.py | 15 +++++++++++++++ tests/test_music_mirror.py | 21 +++++++++++++++++++++ 4 files changed, 57 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index dc23bf1..aaae683 100644 --- a/README.md +++ b/README.md @@ -63,9 +63,14 @@ Everything written into the mirror is made group-readable, and its directories group-traversable, so the mirror can be read back by whatever serves it. Neither writer does that unaided: the temporary file an encode renames into place is created `0600` regardless of the umask, and a straight copy of an existing MP3 -inherits the mode of a source file in a library this tool does not own. Only the -group bits are touched; whether the mirror is world-readable stays with the -umask, as does the ownership. +inherits the mode of a source file in a library this tool does not own. + +Directories are handled by clearing the owner and group read/execute bits from +the process umask, once, at startup. Owner as well as group, because a umask +carrying `0400` produces directories of mode `0300` — writable and enterable, +unreadable to the very run that created them. The `other` bits are left where +the umask puts them: whether the mirror is world-readable is a genuine policy +question, and so is its ownership. Mirror files written before this existed are topped up on the next pass. Their mtimes are correct, so nothing else would revisit them — and they are not diff --git a/music_mirror.py b/music_mirror.py index cf8a6b6..7b4c263 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -84,7 +84,12 @@ MTIME_TOLERANCE_SECONDS = 2 # that may be tighter still. Directories need the execute bit too, or the group # cannot enter them to reach the readable files inside. GROUP_READ = 0o040 -GROUP_ENTER = 0o050 + +# Cleared from the umask so directories this run creates can be listed and +# entered. Owner as well as group: a umask carrying 0400 -- which is unusual but +# not ours to assume away -- otherwise produces a mirror tree that not even the +# process that built it can read back. +DIRECTORY_ACCESS = 0o550 @dataclass @@ -505,12 +510,14 @@ def main(argv=None): logging.basicConfig(format="%(asctime)s %(levelname)s %(message)s", level=logging.INFO) args = build_parser().parse_args(argv) - # Directories are created with 0o777 masked by the umask, so clear the group - # bits from it once here rather than chmod'ing every directory the walk - # creates. Files cannot be handled this way -- mkstemp and copy2 both set a - # mode outright -- so they get an explicit chmod instead. + # Directories are created with 0o777 masked by the umask, so clear the bits + # that matter from it once here rather than chmod'ing every directory the + # walk creates. The `other` bits are left alone, since whether the mirror is + # world-readable is a real policy question; owner and group access is not. + # Files cannot be handled this way -- mkstemp and copy2 both set a mode + # outright, ignoring the umask -- so they get an explicit chmod instead. inherited = os.umask(0o077) - os.umask(inherited & ~GROUP_ENTER) + os.umask(inherited & ~DIRECTORY_ACCESS) if not args.source or not args.mirror: logger.error("both --source and --mirror are required") diff --git a/tests/conftest.py b/tests/conftest.py index abcac25..753d029 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -27,6 +27,21 @@ def tight_umask(): os.umask(previous) +@pytest.fixture +def owner_hostile_umask(): + """Return a callable applying a umask that masks off the owner's read bit. + + Unusual, but it is what produces a mirror tree of mode 0300 -- writable and + enterable, unreadable to the very process that built it. Applied on demand + rather than for the whole test, because the source library is built by + something else entirely and the same umask would make the test's own + fixtures unreadable before the run under test even started. + """ + previous = os.umask(0o022) + yield lambda: os.umask(0o477) + os.umask(previous) + + @pytest.fixture def make_flac(): """Return a factory writing a short tagged FLAC file.""" diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index 238302e..b609985 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -222,6 +222,27 @@ def test_mirror_directories_are_group_traversable(tmp_path, make_flac, tight_uma assert mode & stat.S_IXGRP, directory +def test_mirror_directories_survive_an_owner_hostile_umask( + tmp_path, make_flac, owner_hostile_umask +): + """A umask carrying 0400 otherwise builds a tree the run cannot read back.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "Artist" / "Album" / "a.flac") + + # Applied only now: the library already exists, and the umask under test is + # the one the container starts this run with. + owner_hostile_umask() + run(source, mirror) + + for directory in (mirror, mirror / "Artist", mirror / "Artist" / "Album"): + mode = directory.stat().st_mode + assert mode & stat.S_IRUSR, directory + assert mode & stat.S_IXUSR, directory + assert mode & stat.S_IRGRP, directory + assert mode & stat.S_IXGRP, directory + + def test_private_mirror_file_is_repaired_without_re_encoding(tmp_path, make_flac): """A mirror written by an older version has a correct mtime, so nothing else in the pass would revisit it."""