diff --git a/news/525.bugfix.md b/news/525.bugfix.md new file mode 100644 index 00000000..082ad213 --- /dev/null +++ b/news/525.bugfix.md @@ -0,0 +1,4 @@ +Files written by `to_html(path)`, `to_svg(path)`, `write_image`, `write_images` +and facet exports now get the same permissions as any other new file (normally +`rw-r--r--`), and a file that already exists keeps its mode. They were created +owner-only (`rw-------`), so a web server or another user could not read them. diff --git a/python/xy/export.py b/python/xy/export.py index f2f3dec1..0bbb1f87 100644 --- a/python/xy/export.py +++ b/python/xy/export.py @@ -10,7 +10,9 @@ import numbers import os import re as _re +import secrets import shutil +import stat import subprocess import sys import tempfile @@ -216,6 +218,52 @@ def _base64_chunks(blob: bytes) -> list[str]: ) +def _open_file_mode(target: Path) -> int: + """The permission bits `open(target, "w")` would leave on `target`. + + An existing file keeps its own bits (`open` never changes them; the kernel + clears setuid/setgid on write, hence the `0o777` mask). A new file gets + `0o666` filtered by whatever governs creation in that directory — the + umask, or a default ACL — which is read back from an empty probe file + rather than by flipping the process-wide umask. + """ + try: + return stat.S_IMODE(os.stat(target).st_mode) & 0o777 + except OSError: + pass + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_NOFOLLOW", 0) + for _ in range(tempfile.TMP_MAX): + probe = target.parent / f".{target.name}.{secrets.token_hex(4)}.mode" + try: + fd = os.open(probe, flags, 0o666) + except FileExistsError: + continue + try: + return stat.S_IMODE(os.fstat(fd).st_mode) + finally: + os.close(fd) + with suppress(FileNotFoundError): + probe.unlink() + raise FileExistsError(f"no free temporary file name beside {str(target)!r}") + + +def _publish_mode(fd: int, target: Path) -> None: + """Give the finished temp file the mode `open()` would have given the export. + + `tempfile.mkstemp` creates `0o600` so nothing can read the file while it is + being written, but `os.replace` then carries that mode onto the target and + every export ended up owner-only. The final mode is applied through the + descriptor, after the data is flushed and immediately before the replace. + Best effort: if the mode cannot be determined or applied (no free inode for + the probe, a filesystem without chmod), the finished export is still + published, just owner-only as before. + """ + if not hasattr(os, "fchmod"): # POSIX; Windows has no owner-only mode to undo + return + with suppress(OSError): + os.fchmod(fd, _open_file_mode(target)) + + def _atomic_write_bytes(path: str | PathLike[str], data: bytes) -> None: """Write bytes through a same-directory temp file, then replace atomically.""" target = Path(path) @@ -227,6 +275,7 @@ def _atomic_write_bytes(path: str | PathLike[str], data: bytes) -> None: f.write(data) f.flush() os.fsync(f.fileno()) + _publish_mode(f.fileno(), target) os.replace(tmp_path, target) except Exception: if fd != -1: @@ -254,6 +303,7 @@ def _atomic_write_text(path: str | PathLike[str], text: str) -> None: f.write(text) f.flush() os.fsync(f.fileno()) + _publish_mode(f.fileno(), target) os.replace(tmp_path, target) except Exception: if fd != -1: diff --git a/spec/api/export.md b/spec/api/export.md index 259e403d..2471d3b0 100644 --- a/spec/api/export.md +++ b/spec/api/export.md @@ -228,9 +228,12 @@ Two properties are the point of the API: browser at all. Writes are atomic per file (same-directory temp file, fsync, `os.replace`), so a -reader never observes a partial image. Failure mid-batch is not transactional: -files already written stay on disk. The return value is the list of written -byte strings, in input order. +reader never observes a partial image. The temp file stays owner-only while it +is written; just before the replace it takes the mode a plain `open(path, "w")` +would leave (an existing file's permission bits, or for a new file `0o666` +filtered by the umask or the directory's default ACL), so exports are not left +owner-only. Failure mid-batch is not transactional: files already written stay +on disk. The return value is the list of written byte strings, in input order. ## 9. What styling survives which export path XY has five styling mechanisms (`spec/api/styling.md` § The five ways to style) diff --git a/tests/test_figure.py b/tests/test_figure.py index 0149b122..0e3de78c 100644 --- a/tests/test_figure.py +++ b/tests/test_figure.py @@ -5,8 +5,11 @@ from __future__ import annotations import datetime as dt +import errno import html as _html import json +import os +import stat import warnings from pathlib import Path @@ -1253,6 +1256,87 @@ def fail_replace(src: Path, dst: Path) -> None: assert not list(tmp_path.glob(".chart.html.*.tmp")) +@pytest.mark.skipif(os.name != "posix", reason="POSIX permission bits") +@pytest.mark.parametrize("name", ["chart.html", "chart.png", "chart.svg", "chart.pdf", "batch.png"]) +def test_path_exports_get_the_permissions_open_would_give( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, name: str +): + """The atomic temp file must not leak its owner-only mode onto the export: + a new file gets the mode `open(path, "w")` gives beside it, and an existing + file keeps its own mode. The temp file stays private until it is complete.""" + fig = Figure(title="permissions").line([0.0, 1.0], [1.0, 2.0]) + + def export(target: Path) -> None: + if name.startswith("batch"): + export_module.write_images([fig], [target]) + elif target.suffix == ".html": + fig.to_html(target) + elif target.suffix == ".svg": + fig.to_svg(target) + else: + fig.write_image(target) + + written_modes: list[int] = [] + real_fsync = export_module.os.fsync + + def recording_fsync(fd: int) -> None: + written_modes.append(stat.S_IMODE(os.fstat(fd).st_mode)) + real_fsync(fd) + + monkeypatch.setattr(export_module.os, "fsync", recording_fsync) + previous = os.umask(0o022) + try: + reference = tmp_path / "reference" + reference.open("w").close() + expected = stat.S_IMODE(reference.stat().st_mode) + + created = tmp_path / name + export(created) + assert stat.S_IMODE(created.stat().st_mode) == expected + + existing = tmp_path / f"shared-{name}" + existing.write_bytes(b"old") + existing.chmod(0o664) + export(existing) + assert stat.S_IMODE(existing.stat().st_mode) == 0o664 + assert existing.read_bytes() != b"old" + finally: + os.umask(previous) + assert written_modes and all(mode == 0o600 for mode in written_modes) + assert not list(tmp_path.glob(".*.tmp")) and not list(tmp_path.glob(".*.mode")) + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX permission bits") +@pytest.mark.parametrize("failure", ["probe", "fchmod"]) +def test_export_still_lands_when_the_mode_cannot_be_applied( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure: str +): + """Applying the open()-style mode is best effort: a directory that cannot + take one more file (no free inode), or a filesystem without chmod, still + gets the finished export, owner-only as before.""" + fig = Figure(title="probe").line([0.0, 1.0], [1.0, 2.0]) + if failure == "probe": + real_open = export_module.os.open + + def no_probe(path, flags, mode=0o777, *args, **kwargs): + if str(path).endswith(".mode"): + raise OSError(errno.ENOSPC, os.strerror(errno.ENOSPC)) + return real_open(path, flags, mode, *args, **kwargs) + + monkeypatch.setattr(export_module.os, "open", no_probe) + else: + + def no_fchmod(fd, mode): + raise OSError(errno.EPERM, os.strerror(errno.EPERM)) + + monkeypatch.setattr(export_module.os, "fchmod", no_fchmod) + target = tmp_path / "chart.html" + html = fig.to_html(target) + assert target.read_text(encoding="utf-8") == html + assert stat.S_IMODE(target.stat().st_mode) == 0o600 + assert not list(tmp_path.glob(".*.tmp")) and not list(tmp_path.glob(".*.mode")) + + def test_figure_dom_slots_are_validated_before_export(): fig = Figure().line([0.0, 1.0], [1.0, 2.0]) fig.class_names = {"legend": "ok", "legnd": "typo"}