From 8e326585f3a33217f8fffab58405213f29012377 Mon Sep 17 00:00:00 2001 From: Brad Butler Date: Fri, 2 Oct 2026 09:38:29 +1000 Subject: [PATCH 1/2] fix: keep file permission bits when rewriting ComicInfo Embedding ComicInfo.xml rewrites the archive through a temp file created with tempfile.mkstemp, which is always 0600. The rewritten archive then replaced the original, so a file that a media server could read before a Mass Convert or metadata embed was owner-only afterwards. Imports that write ComicInfo while moving a file into the library had the same result: the library copy came out 0600 whatever the source file allowed. Copy the permission bits from the file being replaced (in-place embed) or from the source file (materialize), which is what a plain move or copy would have preserved. The chmod is best effort so mounts that reject it do not fail the rewrite. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01E9gUFQEryh4gDJRKvjNUZM --- src/pullbox/utilities/comicinfo.py | 24 +++++++++--- tests/unit/test_comicinfo_embed.py | 63 ++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/src/pullbox/utilities/comicinfo.py b/src/pullbox/utilities/comicinfo.py index ccf13884..68379a3b 100644 --- a/src/pullbox/utilities/comicinfo.py +++ b/src/pullbox/utilities/comicinfo.py @@ -9,7 +9,10 @@ from __future__ import annotations +import contextlib +import os import shutil +import stat import tempfile import xml.etree.ElementTree as ET import zipfile @@ -278,6 +281,18 @@ def _find_comicinfo_member(source: zipfile.ZipFile) -> zipfile.ZipInfo | None: return None +def _copy_permission_bits(reference: Path, target: Path) -> None: + """Give a rewritten archive the permission bits of the file it stands in for. + + Rewrites go through ``tempfile.mkstemp``, which creates the file as 0600. Left + alone, an archive that was readable by a media server before the rewrite is + owner-only afterwards. Best effort: filesystems that reject chmod, such as some + SMB/CIFS mounts, must not fail the rewrite. + """ + with contextlib.suppress(OSError): + os.chmod(target, stat.S_IMODE(reference.stat().st_mode)) + + def embed_comicinfo_in_cbz( cbz_path: Path, data: dict[str, Any], @@ -371,6 +386,7 @@ def embed_comicinfo_in_cbz( ) # Atomic replace + _copy_permission_bits(cbz_path, tmp_path) shutil.move(str(tmp_path), str(cbz_path)) logger.info( @@ -388,9 +404,6 @@ def embed_comicinfo_in_cbz( tmp_path.unlink() raise finally: - import contextlib - import os - # Close the file descriptor if still open if tmp_fd >= 0: with contextlib.suppress(OSError): @@ -481,6 +494,8 @@ def materialize_cbz_with_comicinfo( "entries", ) + # A plain move or copy would carry the source's mode to the target. + _copy_permission_bits(source_path, temp_output_path) shutil.move(str(temp_output_path), str(target_path)) if transfer_method == "move" and source_path.resolve(strict=False) != target_path.resolve( strict=False @@ -516,9 +531,6 @@ def materialize_cbz_with_comicinfo( temp_output_path.unlink() raise finally: - import contextlib - import os - if tmp_fd >= 0: with contextlib.suppress(OSError): os.close(tmp_fd) diff --git a/tests/unit/test_comicinfo_embed.py b/tests/unit/test_comicinfo_embed.py index 284daa18..8c513c8c 100644 --- a/tests/unit/test_comicinfo_embed.py +++ b/tests/unit/test_comicinfo_embed.py @@ -286,3 +286,66 @@ def test_materialize_cbz_with_comicinfo_move_deletes_source_after_success( assert not source.exists() assert target.exists() assert _read_comicinfo(target)["Series"] == "Moved Series" + + +def _mode(path: Path) -> int: + return path.stat().st_mode & 0o777 + + +def test_embed_comicinfo_keeps_the_archive_permission_bits(tmp_path: Path) -> None: + archive_path = tmp_path / "issue.cbz" + _write_cbz(archive_path, "Old Series") + archive_path.chmod(0o664) + + assert embed_comicinfo_in_cbz(archive_path, {"Series": "New Series"}) is True + + assert _read_comicinfo(archive_path)["Series"] == "New Series" + assert _mode(archive_path) == 0o664 + + +def test_embed_comicinfo_keeps_permission_bits_with_caller_temp_path(tmp_path: Path) -> None: + archive_path = tmp_path / "issue.cbz" + _write_cbz(archive_path, "Old Series") + archive_path.chmod(0o600) + + embed_comicinfo_in_cbz( + archive_path, + {"Series": "New Series"}, + temp_path=tmp_path / "work" / "issue.tmp.cbz", + ) + + assert _mode(archive_path) == 0o600 + + +def test_materialize_cbz_with_comicinfo_carries_source_permission_bits(tmp_path: Path) -> None: + source_path = tmp_path / "downloads" / "issue.cbz" + source_path.parent.mkdir() + _write_cbz(source_path, "Old Series") + source_path.chmod(0o644) + target_path = tmp_path / "library" / "Series" / "issue.cbz" + + materialize_cbz_with_comicinfo( + source_path, + target_path, + {"Series": "New Series"}, + transfer_method="copy", + ) + + assert _read_comicinfo(target_path)["Series"] == "New Series" + assert _mode(target_path) == 0o644 + + +def test_embed_comicinfo_survives_a_filesystem_that_rejects_chmod( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + archive_path = tmp_path / "issue.cbz" + _write_cbz(archive_path, "Old Series") + + def reject_chmod(*_args: object, **_kwargs: object) -> None: + raise PermissionError("chmod is not permitted on this mount") + + monkeypatch.setattr("pullbox.utilities.comicinfo.os.chmod", reject_chmod) + + assert embed_comicinfo_in_cbz(archive_path, {"Series": "New Series"}) is True + assert _read_comicinfo(archive_path)["Series"] == "New Series" From 90683b7067430e6ba0b1c1399ab0693fa493d19c Mon Sep 17 00:00:00 2001 From: Brad Butler Date: Sat, 3 Oct 2026 12:06:06 +1000 Subject: [PATCH 2/2] fix: log when a rewritten archive's permission bits cannot be restored Restoring the bits stays best effort, so filesystems that reject chmod still complete the rewrite, but the failure is now logged as a warning naming the archive, so an owner-only file can be traced back to it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01E9gUFQEryh4gDJRKvjNUZM --- src/pullbox/utilities/comicinfo.py | 12 ++++++++++-- tests/unit/test_comicinfo_embed.py | 16 +++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/pullbox/utilities/comicinfo.py b/src/pullbox/utilities/comicinfo.py index 68379a3b..cf5eae0e 100644 --- a/src/pullbox/utilities/comicinfo.py +++ b/src/pullbox/utilities/comicinfo.py @@ -287,10 +287,18 @@ def _copy_permission_bits(reference: Path, target: Path) -> None: Rewrites go through ``tempfile.mkstemp``, which creates the file as 0600. Left alone, an archive that was readable by a media server before the rewrite is owner-only afterwards. Best effort: filesystems that reject chmod, such as some - SMB/CIFS mounts, must not fail the rewrite. + SMB/CIFS mounts, must not fail the rewrite, but the failure is logged so an + owner-only archive can be traced back to it. """ - with contextlib.suppress(OSError): + try: os.chmod(target, stat.S_IMODE(reference.stat().st_mode)) + except OSError as exc: + logger.warning( + "comicinfo_permission_bits_not_restored", + reference=str(reference), + target=str(target), + error=str(exc), + ) def embed_comicinfo_in_cbz( diff --git a/tests/unit/test_comicinfo_embed.py b/tests/unit/test_comicinfo_embed.py index 8c513c8c..a116844f 100644 --- a/tests/unit/test_comicinfo_embed.py +++ b/tests/unit/test_comicinfo_embed.py @@ -6,6 +6,9 @@ import zipfile from typing import TYPE_CHECKING +import structlog +from structlog.testing import capture_logs + from pullbox.utilities.comicinfo import embed_comicinfo_in_cbz, materialize_cbz_with_comicinfo if TYPE_CHECKING: @@ -346,6 +349,17 @@ def reject_chmod(*_args: object, **_kwargs: object) -> None: raise PermissionError("chmod is not permitted on this mount") monkeypatch.setattr("pullbox.utilities.comicinfo.os.chmod", reject_chmod) + # A logger cached before app reconfiguration would bypass capture_logs. + monkeypatch.setattr( + "pullbox.utilities.comicinfo.logger", + structlog.wrap_logger(None, cache_logger_on_first_use=False), + ) - assert embed_comicinfo_in_cbz(archive_path, {"Series": "New Series"}) is True + with capture_logs() as logs: + assert embed_comicinfo_in_cbz(archive_path, {"Series": "New Series"}) is True assert _read_comicinfo(archive_path)["Series"] == "New Series" + warnings = [log for log in logs if log["event"] == "comicinfo_permission_bits_not_restored"] + assert len(warnings) == 1 + assert warnings[0]["log_level"] == "warning" + assert warnings[0]["reference"] == str(archive_path) + assert "chmod is not permitted" in warnings[0]["error"]