From c498ee8efa7d95abd3598456e04ac1f8878b809d Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 5 Sep 2026 21:41:36 -0700 Subject: [PATCH 01/10] fix: the gate is the release matrix, not a sample of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1.9.0 was tagged twice and Desktop Release failed both times. The second tag had a gate added specifically to prevent that, and it did not, because it was an approximation: one job on ubuntu-latest, standing in for a four runner matrix. Three failures it could not see: * Linux. The release's Linux leg is pinned to ubuntu-22.04 for the snap base (issue #55). Jammy calls the package libgirepository1.0-dev; the workflow still asked for the 24.04 name, so apt exited 100 and no test ran. Broken since the pin moved. Both workflows now ask for either. * Windows and macOS. Their suites have not run since 1.8.2, because Desktop Release fires only on a tag. 296 commits landed over them. * macOS Intel specifically. test_a_job_that_keeps_reporting_is_left_alone proves a negative — the watchdog leaves a talking job alone — with five ticks of margin between a 0.02s heartbeat and a 0.1s stall window. One late sleep on a loaded Intel runner and the watchdog was right. The heartbeat stays; the window goes to 2s, a hundredfold margin, same 0.4s test. The matrix here is now the release's matrix, and test_ci_contract.py fails if the two lists ever drift apart again. -v rather than -q because the Windows leg died at 30% of the suite with no failure summary at all: names have to stream before the test that crashes the interpreter runs. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 75 ++++++++++++++++++++------- .github/workflows/desktop-release.yml | 9 +++- tests/test_ci_contract.py | 41 +++++++++++++++ tests/test_create_jobs.py | 25 ++++++++- 4 files changed, 129 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 036b8eb4..45b73857 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,20 +85,35 @@ jobs: print(f'Wheel OK: {len(names)} files') " - # The environment the DESKTOP release builds in, on every pull request. + # The environment the DESKTOP release builds in, on every pull request, on + # every runner it builds on. # - # 1.9.0 was tagged, and its desktop build then failed on all four runners at - # this exact step: the suite had never run in this dependency set, because - # Desktop Release only fires on a tag and the step was added after the last - # one. Nothing was wrong with the code the `test` job installs; three tests - # needed an extra this environment does not have, and every test that boots - # a server timed out. Finding that after the tag is finding it too late. + # This job exists because 1.9.0 was tagged twice and Desktop Release failed + # both times, on failures a pull request had no way to see: Desktop Release + # fires only on a tag, and it is the only thing that installs the desktop + # dependency set or runs the suite anywhere but ubuntu-latest. + # + # It was first written as ONE ubuntu-latest job, which was still an + # approximation of the gate rather than the gate, and the next tag proved it: + # the release's Linux leg is pinned to ubuntu-22.04 (see the comment on that + # pin in desktop-release.yml) where an apt package has a different name, and + # the Windows and macOS suites had not run since 1.8.2. An approximate gate + # finds approximate problems. + # + # So the matrix here is the release's matrix. When one changes, both change: + # tests/test_ci_contract.py fails if the runner lists drift apart. # # Deliberately NOT `pip install -e .[pdf,mcp]`: the point is the desktop - # requirements exactly as the release builds them, plus libtorrent, which is - # bundled there and nowhere else in CI. + # requirements exactly as the release installs them, plus libtorrent, which + # is bundled there and nowhere else in CI. desktop-env: - runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + os: [ubuntu-22.04, macos-15-intel, macos-latest, windows-latest] + + runs-on: ${{ matrix.os }} + steps: - uses: actions/checkout@v4 @@ -109,27 +124,48 @@ jobs: # The same system libraries the release build installs on Linux: the # bundle links GTK and WebKit through PyGObject, and PyInstaller # collects what is present at build time. - - name: Install system dependencies + # + # The package is libgirepository1.0-dev on 22.04 and libgirepository-2.0-dev + # from 24.04 on. The release pins 22.04 for the snap base, so name both and + # take whichever this runner has, rather than encoding a name that goes + # stale the next time the pin moves. + - name: Install system dependencies (Linux) + if: runner.os == 'Linux' run: | sudo apt-get update sudo apt-get install -y \ - libgirepository-2.0-dev \ gir1.2-gtk-3.0 \ gir1.2-webkit2-4.1 \ gcc libcairo2-dev pkg-config python3-dev + sudo apt-get install -y libgirepository-2.0-dev \ + || sudo apt-get install -y libgirepository1.0-dev - name: Install desktop dependencies - run: | - pip install -r desktop/requirements-desktop.txt pytest PyGObject - pip install "libtorrent==2.0.*" || echo "::warning::no libtorrent wheel here" + run: pip install -r desktop/requirements-desktop.txt pytest + - name: Install libtorrent (in-process BT engine) + shell: bash + run: | + # Soft dependency, exactly as the release treats it: no wheel for this + # platform means the build ships HTTP-only, never that it fails. + pip install "libtorrent==2.0.*" || echo "::warning::no libtorrent wheel for this platform" + + - name: Install PyGObject (Linux) + if: runner.os == 'Linux' + run: pip install PyGObject + + # -v, not -q, and the reason is specific: a hard interpreter crash prints + # no failure summary at all. The Windows leg of the 1.9.0 tag died at 30% + # of the suite with nothing but an exit code, and the only way to know + # which test was running is to have streamed its name before it ran. - name: Run the test suite as the desktop build runs it - run: python -m pytest tests/ -q + run: python -m pytest tests/ -v -rf - # PyInstaller on one platform. The four-platform matrix stays on the tag; - # this is here to catch a bundle that stops building at all, which is the - # other way Desktop Release fails after a merge rather than before it. + # PyInstaller on the runner the AppImage and snap are actually built on. + # This catches a bundle that stops building at all, which is the other + # way Desktop Release fails after a merge rather than before it. - name: Build with PyInstaller + if: matrix.os == 'ubuntu-22.04' run: | pip install pyinstaller pyinstaller --noconfirm desktop/zimi_desktop.spec @@ -137,6 +173,7 @@ jobs: # The same script the release runs, in the same headless server mode. # The bundle opens a window without it, and there is no display here. - name: The built binary starts and answers + if: matrix.os == 'ubuntu-22.04' shell: bash env: SMOKE_EXPECT_BT: '1' diff --git a/.github/workflows/desktop-release.yml b/.github/workflows/desktop-release.yml index 4038887c..04cc40ff 100644 --- a/.github/workflows/desktop-release.yml +++ b/.github/workflows/desktop-release.yml @@ -62,15 +62,22 @@ jobs: with: python-version: '3.12' + # libgirepository is libgirepository1.0-dev on 22.04 and + # libgirepository-2.0-dev from 24.04 on. This job's runner is pinned to + # 22.04 for the snap base above, and the 24.04 name was left behind when + # it moved: every Linux build since has died here, before a single test + # ran. Name both and take whichever this runner has, so the next time the + # pin moves this step moves with it. - name: Install system dependencies (Linux) if: runner.os == 'Linux' run: | sudo apt-get update sudo apt-get install -y \ - libgirepository-2.0-dev \ gir1.2-gtk-3.0 \ gir1.2-webkit2-4.1 \ gcc libcairo2-dev pkg-config python3-dev + sudo apt-get install -y libgirepository-2.0-dev \ + || sudo apt-get install -y libgirepository1.0-dev - name: Install dependencies run: pip install -r desktop/requirements-desktop.txt diff --git a/tests/test_ci_contract.py b/tests/test_ci_contract.py index c7721a64..d68b1fe3 100644 --- a/tests/test_ci_contract.py +++ b/tests/test_ci_contract.py @@ -150,3 +150,44 @@ def test_every_test_file_is_collectable(): assert len(names) > 20, f"only found {len(names)} python test files" # The suite's own file is here, so this is at minimum self-consistent. assert os.path.basename(__file__) in names + + +def _runners(name, job): + """Every runner a job's matrix expands to. + + Both forms appear here: ``os: [a, b]`` in the CI matrix, and a list of + ``- os: x`` entries under ``include:`` in the release matrix.""" + body = _text(name) + start = body.index(f"\n {job}:") + nxt = re.search(r"\n [a-z][a-z0-9_-]*:\n", body[start + 1 :]) + block = body[start : start + 1 + nxt.start()] if nxt else body[start:] + block = "\n".join( + ln for ln in block.splitlines() if not ln.lstrip().startswith("#") + ) + inline = re.search(r"^\s*os:\s*\[([^\]]+)\]", block, re.M) + if inline: + return {v.strip().strip("'\"") for v in inline.group(1).split(",")} + return {m.strip().strip("'\"") for m in re.findall(r"^\s*-?\s*os:\s*(\S+)", block, re.M)} + + +def test_the_pr_gate_runs_on_every_runner_the_release_builds_on(): + """The pull request gate IS the release matrix, not a sample of it. + + 1.9.0 was tagged twice and Desktop Release failed both times. The second + time, a gate had been added to catch exactly that — but it ran one job on + ubuntu-latest, and the release's Linux leg is pinned to ubuntu-22.04 where + an apt package has a different name. The Windows and macOS suites had not + run since 1.8.2 and had accumulated real failures nobody could see. + + Any runner in one list and not the other is a platform whose failures only + a tag can find, which is after the version number is spent.""" + gate = _runners("ci.yml", "desktop-env") + release = _runners("desktop-release.yml", "build") + assert gate, "ci.yml's desktop-env job no longer declares a runner matrix" + assert release, "desktop-release.yml's build job no longer declares a runner matrix" + assert gate == release, ( + "the pull request gate and the release build no longer run on the same " + f"runners. Only in the gate: {sorted(gate - release) or 'none'}. Only in " + f"the release: {sorted(release - gate) or 'none'}. A platform in the " + "release alone is one whose failures cannot be seen before the tag." + ) diff --git a/tests/test_create_jobs.py b/tests/test_create_jobs.py index 1c8b4baa..2fd43066 100644 --- a/tests/test_create_jobs.py +++ b/tests/test_create_jobs.py @@ -268,10 +268,33 @@ def test_an_interrupted_write_leaves_no_zim_under_its_real_name(tmp_path): @pytest.fixture def quick_watchdog(monkeypatch): + """A stall window short enough that a wedged job fails inside a test. + + Only for tests asserting that a silent job IS caught. A test asserting the + opposite needs the window wide (see patient_watchdog): the two directions + have opposite tolerances for a slow machine.""" monkeypatch.setattr(manage, "CREATE_STALL_SECONDS", 0.1) monkeypatch.setattr(manage, "CREATE_STALL_TICK", 0.02) +@pytest.fixture +def patient_watchdog(monkeypatch): + """A stall window far wider than the heartbeat under test. + + Proving a talking job is left alone means proving a negative, and the only + thing separating "the watchdog respects heartbeats" from "the machine was + fast" is the margin between them. At a 0.1s window and a 0.02s heartbeat + that margin is five ticks of scheduler noise, and the macOS Intel runner + ate it during the 1.9.0 release build: one late sleep, and the watchdog was + right to call a stall. + + So the heartbeat stays at 0.02s and the window goes to 2s: a hundredfold + margin, still a 0.4s test. A regression here has to hold the watchdog off + for two seconds, which no scheduler hiccup does.""" + monkeypatch.setattr(manage, "CREATE_STALL_SECONDS", 2.0) + monkeypatch.setattr(manage, "CREATE_STALL_TICK", 0.02) + + def test_a_job_that_stops_reporting_is_failed_not_spun( tmp_path, monkeypatch, quick_watchdog ): @@ -295,7 +318,7 @@ def wedged_run(job, opts): def test_a_job_that_keeps_reporting_is_left_alone( - tmp_path, monkeypatch, quick_watchdog + tmp_path, monkeypatch, patient_watchdog ): def chatty_run(job, opts): for _ in range(20): From b688921d4b64bdbac33ba25df1fd3b9fcc3bebdc Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 5 Sep 2026 21:53:07 -0700 Subject: [PATCH 02/10] fix: the interrupt test killed the Windows interpreter; PyGObject cannot build on jammy Two more dominoes behind the ones the matrix already found. os.kill(os.getpid(), SIGINT) does not deliver a signal on Windows. It calls TerminateProcess with the number as an exit code, so the crawler's interrupt test terminated pytest itself at 32% of the suite: no summary, no traceback, just exit 1. signal.raise_signal runs the handler the test is actually about and does it on every platform. PyGObject 3.52 requires girepository-2.0, which 22.04 does not ship under any package name. The Linux leg is pinned to 22.04 for the snap base, so an unpinned install resolves to a version that cannot build there. --tb=short so the eighteen Windows failures behind the crash come back with reasons and not only names. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 8 ++++++-- .github/workflows/desktop-release.yml | 6 +++++- tests/test_creator_site.py | 8 +++++++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 45b73857..82b78da9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -150,16 +150,20 @@ jobs: # platform means the build ships HTTP-only, never that it fails. pip install "libtorrent==2.0.*" || echo "::warning::no libtorrent wheel for this platform" + # Pinned below 3.52, which is where PyGObject started requiring + # girepository-2.0 — a library that does not exist on 22.04 at any + # package name. This runner is pinned to 22.04 for the snap base, so an + # unpinned install resolves to a version that cannot build here. - name: Install PyGObject (Linux) if: runner.os == 'Linux' - run: pip install PyGObject + run: pip install "PyGObject<3.52" # -v, not -q, and the reason is specific: a hard interpreter crash prints # no failure summary at all. The Windows leg of the 1.9.0 tag died at 30% # of the suite with nothing but an exit code, and the only way to know # which test was running is to have streamed its name before it ran. - name: Run the test suite as the desktop build runs it - run: python -m pytest tests/ -v -rf + run: python -m pytest tests/ -v -rf --tb=short # PyInstaller on the runner the AppImage and snap are actually built on. # This catches a bundle that stops building at all, which is the other diff --git a/.github/workflows/desktop-release.yml b/.github/workflows/desktop-release.yml index 04cc40ff..697c94ad 100644 --- a/.github/workflows/desktop-release.yml +++ b/.github/workflows/desktop-release.yml @@ -90,9 +90,13 @@ jobs: # on BT bundling. pip install "libtorrent==2.0.*" || echo "::warning::no libtorrent wheel for this platform — this build runs HTTP-only" + # Pinned below 3.52, which is where PyGObject started requiring + # girepository-2.0 — a library that does not exist on 22.04 at any + # package name. This runner is pinned to 22.04 for the snap base, so an + # unpinned install resolves to a version that cannot build here. - name: Install PyGObject (Linux) if: runner.os == 'Linux' - run: pip install PyGObject + run: pip install "PyGObject<3.52" - name: Install pytest run: pip install pytest diff --git a/tests/test_creator_site.py b/tests/test_creator_site.py index 72c09d6b..badcb21a 100644 --- a/tests/test_creator_site.py +++ b/tests/test_creator_site.py @@ -669,7 +669,13 @@ def note(message): said.append(message) if not fired and message.lstrip().startswith("[1/"): fired.append(True) - os.kill(os.getpid(), signal.SIGINT) + # raise_signal, not os.kill(getpid(), SIGINT). On Windows os.kill + # does not deliver a signal at all: it calls TerminateProcess with + # the number as an exit code, so this line used to kill the pytest + # interpreter outright, mid-suite, with no failure summary and no + # traceback. raise_signal runs the handler the test is about, on + # every platform. + signal.raise_signal(signal.SIGINT) info = crawler.create_site_zim( f"{BASE}/chain/0.html", out_dir=str(tmp_path), delay=0, progress=note From 676ae727432deebd028016f61b1b353214ff4c4a Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 5 Sep 2026 22:29:33 -0700 Subject: [PATCH 03/10] fix: six Windows defects the suite had never been able to see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows suite has never run. Not since 1.8.2 — never: until this release made CI run the whole suite, the release workflow ran `python tests/test_unit.py`, which imports the file and executes none of its tests, plus one file by name. Every Windows build ever shipped was gated by a step that could not fail. First real run: 56 failed, 2521 passed. Six were the product. * `zimi serve` died at boot. The first-run banner — the setup key box from this release's own GHSA-5mw2-53vv-9pw6 fix — is drawn in box characters, and Windows encodes redirected stdout as cp1252, which has none of them. The very first run, the one where no password is set yet, crashed before READY. Stdio is resilient now and the banner degrades to ASCII rather than raising. PYTHONIOENCODING reproduces it on any platform, so the test fails everywhere if this regresses. * Deleting a ZIM answered 500 every time: the route unlinked the file and released the pooled libzim Archive after. Windows will not remove an open file. Release first, which POSIX does not notice. * Auto-update orphaned every superseded edition, same cause, inside an `except OSError: pass` — so it failed silently and permanently. The swallow is now a warning. * The create watchdog could never kill a stalled browser: signal.SIGKILL does not exist on Windows and naming it raises. * Every ZIM built on Windows left four libzim index scratch files beside it. libzim unlinks them as it closes, which needs an OS that permits unlinking an open file. * Mimetypes came from HKEY_CLASSES_ROOT, so a ZIM built on Windows could carry entry types no other machine would produce. All five sites now share one registry-free guesser. The rest were tests. Where one assumed something the platform does not grant — POSIX mode bits, a chmod'd read-only directory — it now asserts the premise and skips when the OS declines, which also fixes those tests under root on Linux, where chmod is equally ignored. Local suite: 2633 passed, 19 skipped. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_alive.py | 4 +- tests/test_backup_cli.py | 19 +++++ tests/test_capture_variants.py | 11 +-- tests/test_create_jobs.py | 32 ++++++++ tests/test_creator_site.py | 8 +- tests/test_folder_category.py | 5 ++ tests/test_i18n_parity.py | 6 +- tests/test_loadavg_throttle.py | 11 ++- tests/test_new_zim.py | 6 +- tests/test_p2p.py | 2 +- tests/test_readonly_data_dir.py | 32 +++++++- tests/test_register_zim_incremental.py | 1 + tests/test_serve_smoke.py | 60 ++++++++++++++ tests/test_unregister_zim.py | 37 +++++++++ zimi/crawler.py | 3 +- zimi/creator.py | 3 +- zimi/library.py | 15 +++- zimi/manage.py | 9 ++ zimi/renderer.py | 15 +++- zimi/server.py | 109 ++++++++++++++++++++----- zimi/video.py | 3 +- zimi/zimwriter.py | 43 ++++++++++ 22 files changed, 386 insertions(+), 48 deletions(-) diff --git a/tests/test_alive.py b/tests/test_alive.py index 3cf6138f..e2d3b44b 100644 --- a/tests/test_alive.py +++ b/tests/test_alive.py @@ -214,7 +214,9 @@ def test_the_convert_phase_is_derived_from_the_line_the_sidecar_prints(): def test_the_convert_phase_is_one_the_client_knows(): assert "convert" in manage.CREATE_PHASES - with open(os.path.join(REPO_ROOT, "zimi", "static", "create.js")) as fh: + with open( + os.path.join(REPO_ROOT, "zimi", "static", "create.js"), encoding="utf-8" + ) as fh: assert "convert: 2" in fh.read() diff --git a/tests/test_backup_cli.py b/tests/test_backup_cli.py index 150724ce..5393fad2 100644 --- a/tests/test_backup_cli.py +++ b/tests/test_backup_cli.py @@ -130,8 +130,26 @@ def test_restore_reports_env_locked_auto_update_as_skipped(dirs, tmp_path): # ── File mode: 0600, because password hashes ── +def _require_posix_modes(tmp_path): + """0600 is a POSIX guarantee, and only some platforms make it. + + Windows has no mode bits on files: chmod there toggles one read-only + attribute and os.stat reports 0666 whatever you asked for. The bundle is + still protected on Windows, by the ACL it inherits from the directory it + is written into — a different mechanism, and not one this assertion can + read. Skip rather than assert a guarantee the platform never made.""" + probe = tmp_path / ".mode-probe" + probe.write_text("") + os.chmod(probe, 0o600) + granted = stat.S_IMODE(os.stat(probe).st_mode) + probe.unlink() + if granted != 0o600: + pytest.skip("this platform does not enforce POSIX file modes") + + def test_backup_file_mode_is_0600(dirs, tmp_path): zim_dir, data_dir = dirs + _require_posix_modes(tmp_path) _seed_state() out = tmp_path / "bundle.json" assert ( @@ -144,6 +162,7 @@ def test_backup_tightens_a_preexisting_looser_file(dirs, tmp_path): """O_CREAT's mode only applies on create — overwriting an existing 0644 file must still end at 0600, which is what the explicit chmod is for.""" zim_dir, data_dir = dirs + _require_posix_modes(tmp_path) out = tmp_path / "bundle.json" out.write_text("{}") os.chmod(out, 0o644) diff --git a/tests/test_capture_variants.py b/tests/test_capture_variants.py index 5bb071c5..cfa0f4a9 100644 --- a/tests/test_capture_variants.py +++ b/tests/test_capture_variants.py @@ -19,6 +19,7 @@ import json import os import sys +import tempfile import pytest @@ -129,19 +130,19 @@ def test_the_fast_engine_accepts_it_and_ignores_it(): def test_the_session_honours_off(): - session = renderer.RenderedSession(work_dir="/tmp", capture_variants=False) + session = renderer.RenderedSession(work_dir=tempfile.gettempdir(), capture_variants=False) assert session._capture_variants is False def test_the_session_defaults_to_sweeping(): - session = renderer.RenderedSession(work_dir="/tmp") + session = renderer.RenderedSession(work_dir=tempfile.gettempdir()) assert session._capture_variants is renderer.VARIANT_SWEEP_DEFAULT def test_a_switched_off_sweep_does_not_touch_the_archive(monkeypatch): """The gate is checked before anything is enumerated, so an off sweep costs no page evaluation at all — not a sweep that runs and discards.""" - session = renderer.RenderedSession(work_dir="/tmp", capture_variants=False) + session = renderer.RenderedSession(work_dir=tempfile.gettempdir(), capture_variants=False) # A recorder and a context would otherwise satisfy the two later guards. session._recorder = object() session._context = object() @@ -174,11 +175,11 @@ def test_capture_tools_survives_an_engine_that_never_heard_of_it(): def test_an_unstarted_session_claims_nothing(): """No browser ran, so no browser version is true. Claiming one would be provenance invented at construction time.""" - assert renderer.RenderedSession(work_dir="/tmp").tools == {} + assert renderer.RenderedSession(work_dir=tempfile.gettempdir()).tools == {} def test_a_started_session_names_the_browser_it_ran(): - session = renderer.RenderedSession(work_dir="/tmp") + session = renderer.RenderedSession(work_dir=tempfile.gettempdir()) session._browser_version = "140.0.7339.16" assert session.tools == {"chromium": "140.0.7339.16"} record = zimwriter.history_record("created", "page", "x", tools=session.tools) diff --git a/tests/test_create_jobs.py b/tests/test_create_jobs.py index 2fd43066..8fdaec68 100644 --- a/tests/test_create_jobs.py +++ b/tests/test_create_jobs.py @@ -957,3 +957,35 @@ def test_a_video_host_is_video_and_a_page_host_is_not(): ): assert not claims_video_host(u), u assert claims_url(u), u + + +def test_the_creator_sweeps_only_its_own_index_scratch(tmp_path): + """libzim's scratch goes, the neighbours stay. + + The sweep exists because libzim unlinks `_title.idx` and friends + as it closes, which only works where the OS permits unlinking an open + file. Windows does not, so a capture left four scratch files beside every + ZIM it made. Since the sweep deletes by prefix, what it must never do is + reach a file belonging to anything else — including a ZIM whose name this + one is a prefix of.""" + from zimi.zimwriter import _sweep_creator_scratch + + out = tmp_path / "site.zim.tmp" + scratch = [ + tmp_path / "site.zim.tmp_title.idx", + tmp_path / "site.zim.tmp_title.idx.tmp", + tmp_path / "site.zim.tmp_fulltext.idx", + ] + keep = [ + out, + tmp_path / "site.zim", + tmp_path / "site.zim.tmp2_title.idx", # a different build + tmp_path / "other.zim.tmp_title.idx", # somebody else's scratch + ] + for f in scratch + keep: + f.write_bytes(b"x") + + _sweep_creator_scratch(str(out)) + + assert [f.name for f in scratch if f.exists()] == [] + assert sorted(f.name for f in keep if f.exists()) == sorted(f.name for f in keep) diff --git a/tests/test_creator_site.py b/tests/test_creator_site.py index badcb21a..30c16665 100644 --- a/tests/test_creator_site.py +++ b/tests/test_creator_site.py @@ -707,7 +707,9 @@ def probe(cmd): def run(cmd, note, timeout=None): seen["runs"].append(cmd) note("crawl finished") - out_dir = cmd[cmd.index("-v") + 1].split(":")[0] + # rsplit: the separator is the LAST colon. A Windows host path starts + # "C:\\", and splitting on the first one leaves "C". + out_dir = cmd[cmd.index("-v") + 1].rsplit(":", 1)[0] with open(os.path.join(out_dir, "whatever.zim"), "wb") as fh: fh.write(b"ZIMITOUTPUT") return 0, ["crawl finished"] @@ -806,7 +808,9 @@ def run(cmd, note, timeout=None): if cmd[1] == "pull": note("Pulling from openzim/zimit") return 0, [] - out_dir = cmd[cmd.index("-v") + 1].split(":")[0] + # rsplit: the separator is the LAST colon. A Windows host path starts + # "C:\\", and splitting on the first one leaves "C". + out_dir = cmd[cmd.index("-v") + 1].rsplit(":", 1)[0] with open(os.path.join(out_dir, "x.zim"), "wb") as fh: fh.write(b"Z") return 0, [] diff --git a/tests/test_folder_category.py b/tests/test_folder_category.py index 1a744669..b55c7d39 100644 --- a/tests/test_folder_category.py +++ b/tests/test_folder_category.py @@ -194,6 +194,11 @@ def test_moving_a_zim_into_a_folder_refiles_it_on_the_next_boot(zim_dir): assert _entry("wikem")["category"] == "Medical" # heuristic, no folder yet os.makedirs(str(zim_dir / "emergency-prep")) + # Releasing first is what moving an open file requires on Windows, where + # the OS refuses it outright. Worth knowing: the same constraint lands on + # the in-app "keep ZIMs organised by folder" feature, which moves files + # for the user rather than asking them to. + server.release_zim_handles([server._zim_short_name("wikem_en_2026-01.zim")]) shutil.move( str(zim_dir / "wikem_en_2026-01.zim"), str(zim_dir / "emergency-prep" / "wikem_en_2026-01.zim"), diff --git a/tests/test_i18n_parity.py b/tests/test_i18n_parity.py index 65c92c03..aff1fd2d 100644 --- a/tests/test_i18n_parity.py +++ b/tests/test_i18n_parity.py @@ -21,13 +21,13 @@ def test_all_locales_share_the_en_key_set(): - with open(os.path.join(I18N_DIR, "en.json")) as f: + with open(os.path.join(I18N_DIR, "en.json"), encoding="utf-8") as f: en_keys = set(json.load(f)) assert en_keys, "en.json is empty?" problems = [] for path in sorted(glob.glob(os.path.join(I18N_DIR, "*.json"))): lang = os.path.basename(path) - with open(path) as f: + with open(path, encoding="utf-8") as f: keys = set(json.load(f)) missing = en_keys - keys extra = keys - en_keys @@ -40,7 +40,7 @@ def test_all_locales_share_the_en_key_set(): def test_no_empty_values(): for path in sorted(glob.glob(os.path.join(I18N_DIR, "*.json"))): - with open(path) as f: + with open(path, encoding="utf-8") as f: data = json.load(f) empty = [k for k, v in data.items() if not str(v).strip()] assert not empty, f"{os.path.basename(path)}: empty values for {empty[:5]}" diff --git a/tests/test_loadavg_throttle.py b/tests/test_loadavg_throttle.py index 895e1fe5..54fc921e 100644 --- a/tests/test_loadavg_throttle.py +++ b/tests/test_loadavg_throttle.py @@ -19,7 +19,9 @@ class LoadavgThrottleTests(unittest.TestCase): def test_no_sleep_when_load_below_threshold(self): with ( - mock.patch.object(os, "getloadavg", return_value=(0.1, 0.1, 0.1)), + mock.patch.object( + os, "getloadavg", return_value=(0.1, 0.1, 0.1), create=True + ), mock.patch.object(os, "cpu_count", return_value=4), mock.patch.object(time, "sleep") as sleep_mock, ): @@ -53,7 +55,12 @@ def test_sleep_capped_at_max(self): def test_no_op_when_getloadavg_unavailable(self): # Simulate Windows: AttributeError on os.getloadavg. with ( - mock.patch.object(os, "getloadavg", side_effect=AttributeError), + # create=True: on Windows the attribute is genuinely absent, and + # patch.object will not patch what is not there — so the test + # named for simulating Windows was the one Windows failed. + mock.patch.object( + os, "getloadavg", side_effect=AttributeError, create=True + ), mock.patch.object(time, "sleep") as sleep_mock, ): _search._loadavg_throttle() diff --git a/tests/test_new_zim.py b/tests/test_new_zim.py index 4e99a137..0140dbb1 100644 --- a/tests/test_new_zim.py +++ b/tests/test_new_zim.py @@ -110,7 +110,11 @@ def test_update_under_new_dated_filename_badges_updated(tmp_path, monkeypatch): original_first_seen = _entry(server._zim_list_cache)["first_seen"] assert abs(original_first_seen - installed) < 2.0 - # The update lands: old dated file replaced by a newer dated file. + # The update lands: old dated file replaced by a newer dated file. The + # updater releases the superseded edition's handles before unlinking it + # (Windows refuses to remove an open file); do the same here, or this + # stands in for a sequence the product does not use. + server.release_zim_handles([server._zim_short_name(os.path.basename(old_path))]) os.remove(old_path) build_fixture_zim(str(zdir / "survival_en_2026-07.zim")) server.load_cache(force=False) # new filename → cache miss → update-rename diff --git a/tests/test_p2p.py b/tests/test_p2p.py index 904c9942..ed23a957 100644 --- a/tests/test_p2p.py +++ b/tests/test_p2p.py @@ -98,7 +98,7 @@ def test_bt_port_invalid_falls_back(monkeypatch, val): def test_staging_dir_default(monkeypatch): monkeypatch.delenv("ZIMI_STAGING_DIR", raising=False) - assert p2p.get_staging_dir("/data") == "/data/staging" + assert p2p.get_staging_dir("/data") == os.path.join("/data", "staging") def test_staging_dir_override(monkeypatch): diff --git a/tests/test_readonly_data_dir.py b/tests/test_readonly_data_dir.py index 02c1c711..3baef35a 100644 --- a/tests/test_readonly_data_dir.py +++ b/tests/test_readonly_data_dir.py @@ -65,13 +65,39 @@ def cache_home(tmp_path, monkeypatch): return str(home) +def _make_unwritable(path): + """Turn a directory read-only, or skip the test that asked for one. + + Every test here rests on a premise the OS has to grant: a directory that + exists, lists, and cannot be written. Some platforms decline. Windows + ignores POSIX mode bits on directories outright, and so does root on + Linux, and in both cases chmod returns success and changes nothing — so + the fixture built a perfectly writable directory, the product correctly + did not fall back, and ten tests failed for being right. + + Asserting the premise instead of assuming it is what makes these tests + honest anywhere they run.""" + os.chmod(path, 0o555) + probe = os.path.join(str(path), ".zimi-write-probe") + try: + with open(probe, "w"): + pass + except OSError: + return # genuinely unwritable, which is what the caller asked for + os.unlink(probe) + pytest.skip( + "this platform ignores a read-only directory mode, so the read-only " + "media these tests are about cannot be created here" + ) + + @pytest.fixture def ro_zim_dir(tmp_path): """A ZIM dir on 'read-only media': exists, listable, not writable.""" d = tmp_path / "stick" d.mkdir() (d / "dummy.zim").write_bytes(b"") # looks like a library, never opened - os.chmod(d, 0o555) + _make_unwritable(d) yield str(d) os.chmod(d, 0o755) @@ -154,8 +180,8 @@ def test_existing_readonly_state_is_bypassed_wholesale( state = stick / ".zimi" state.mkdir(parents=True) (state / "cache.json").write_text("{}") - os.chmod(state, 0o555) - os.chmod(stick, 0o555) + _make_unwritable(state) + _make_unwritable(stick) try: server.apply_data_paths(str(stick), None) with caplog.at_level("WARNING", logger="zimi"): diff --git a/tests/test_register_zim_incremental.py b/tests/test_register_zim_incremental.py index 7f27721d..96212c6c 100644 --- a/tests/test_register_zim_incremental.py +++ b/tests/test_register_zim_incremental.py @@ -122,6 +122,7 @@ def test_register_update_inherits_first_seen_and_stamps_updated(tmp_path, monkey # The download machinery removes older versions before registering. new_path = str(zdir / "existing0_en_2026-07.zim") build_fixture_zim(new_path) + server.release_zim_handles([server._zim_short_name(old_file)]) os.remove(str(zdir / old_file)) assert server.register_zim_file(new_path, removed_files=[old_file]) is True diff --git a/tests/test_serve_smoke.py b/tests/test_serve_smoke.py index db75c2f5..39329d34 100644 --- a/tests/test_serve_smoke.py +++ b/tests/test_serve_smoke.py @@ -193,3 +193,63 @@ def test_sw_asset_version_is_content_hashed(): src = open("zimi/static/sw.js").read() assert "zimi-vdev" in src # placeholder present in source assert token not in src # real token only injected at serve time + + +def test_serve_survives_a_console_that_cannot_encode_its_own_banner(): + """A boot banner must never be able to kill the server. + + Windows redirects stdout at the locale encoding, cp1252, which has no box + drawing characters — and 1.9.0's first-run security banner (the setup key + from GHSA-5mw2-53vv-9pw6) is drawn in them. So on Windows the very first + `zimi serve > log.txt`, the run where no password is set yet, died with + UnicodeEncodeError before READY. The whole Windows suite had never run, so + nothing said so. + + PYTHONIOENCODING reproduces it on any platform: this test fails on Linux + and macOS too when the fix is reverted, which is the point. A crash that + only one runner can see is a crash nobody sees.""" + tmp_zim_dir = tempfile.mkdtemp(prefix="zimi-cp1252-zims-") + tmp_data_dir = tempfile.mkdtemp(prefix="zimi-cp1252-data-") + log_fd, log_path = tempfile.mkstemp(prefix="zimi-cp1252-log-") + os.close(log_fd) + + env = os.environ.copy() + env["ZIM_DIR"] = tmp_zim_dir + env["ZIMI_DATA_DIR"] = tmp_data_dir + env["ZIMI_AUTO_UPDATE"] = "0" + env["ZIMI_TORRENT"] = "0" + env["ZIMI_PEER_DISCOVERY"] = "0" + env["PYTHONUNBUFFERED"] = "1" + # The whole point: a stdout that cannot represent the banner. + env["PYTHONIOENCODING"] = "cp1252" + + with open(log_path, "w") as log_f: + proc = subprocess.Popen( + [sys.executable, "-m", "zimi", "serve", "--port", "0"], + cwd=REPO_ROOT, + env=env, + stdout=log_f, + stderr=subprocess.STDOUT, + ) + try: + port = _wait_for_ready(proc, log_path) + assert port > 0 + with open(log_path, "rb") as f: + out = f.read().decode("utf-8", errors="replace") + assert "UnicodeEncodeError" not in out + # The setup key is the reason the banner exists; it is ASCII, so it + # has to survive an encoding this narrow intact. + assert "SETUP KEY:" in out + finally: + try: + proc.terminate() + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait(timeout=2) + for path in (tmp_zim_dir, tmp_data_dir): + shutil.rmtree(path, ignore_errors=True) + try: + os.unlink(log_path) + except OSError: + pass diff --git a/tests/test_unregister_zim.py b/tests/test_unregister_zim.py index 7772b7bf..b4ab6816 100644 --- a/tests/test_unregister_zim.py +++ b/tests/test_unregister_zim.py @@ -253,6 +253,43 @@ def _request(self, path, payload=None): except urllib.error.HTTPError as e: return e.code, json.loads(e.read() or b"{}") + def test_delete_releases_the_archive_before_it_unlinks(self): + """Ordering, asserted where the OS does not assert it for us. + + Windows refuses to unlink a file anyone still has open, so deleting a + ZIM answered 500 "Failed to delete file" there every time: the route + called os.remove first and dropped the pooled libzim Archive after. + POSIX does not care — the inode outlives the name — so this bug was + invisible on every runner that had ever executed this suite. + + The fix is an ordering, so the test is an ordering: at the moment + os.remove is called, the pool must no longer hold this ZIM.""" + alpha = _short(ALPHA) + # Open it for real first, so there is a pooled handle to release. + status, _ = self._request("/search?q=water&limit=5") + self.assertEqual(status, 200) + self.assertIn(alpha, server._archive_pool) + + pooled_at_unlink = [] + real_remove = os.remove + + def watching_remove(path): + pooled_at_unlink.append(alpha in server._archive_pool) + return real_remove(path) + + os.remove = watching_remove + try: + status, data = self._request("/manage/delete", {"filename": ALPHA}) + finally: + os.remove = real_remove + self.assertEqual(status, 200, data) + self.assertEqual( + pooled_at_unlink, + [False], + "the delete route unlinked the file while its archive was still " + "pooled — Windows returns 500 for exactly this", + ) + def test_delete_never_rescans_and_the_zim_disappears(self): alpha = _short(ALPHA) status, listing = self._request("/list") diff --git a/zimi/crawler.py b/zimi/crawler.py index 8771d8aa..010d50ad 100644 --- a/zimi/crawler.py +++ b/zimi/crawler.py @@ -94,6 +94,7 @@ ) from zimi.blocklist import blocked_phrase from zimi.zimwriter import ( + guess_mime, _plural, _slug, add_standard_metadata, @@ -229,7 +230,7 @@ def looks_like_a_page(url): (``.png``, ``.zip``, ``.css``). Extensionless and server-script URLs pass — the Content-Type check after the fetch is the real gate; this one only exists to avoid spending a request to learn what the name already said.""" - guess = mimetypes.guess_type(urllib.parse.urlsplit(url).path)[0] + guess = guess_mime(urllib.parse.urlsplit(url).path, fallback=None) if not guess or guess in _PAGE_MIMES: return True return not (guess.startswith(_NON_PAGE_MAJORS) or guess in _NON_PAGE_MIMES) diff --git a/zimi/creator.py b/zimi/creator.py index 6bd4bb18..419f12ed 100644 --- a/zimi/creator.py +++ b/zimi/creator.py @@ -48,6 +48,7 @@ import zimi.server as _srv from zimi.blocklist import blocked_phrase from zimi.zimwriter import ( + guess_mime, _CSS_URL_RE, _HREF_RE, _REL_RE, @@ -231,7 +232,7 @@ def _page_title_from_html(text, fallback): def _guess_mime(name): - return mimetypes.guess_type(name)[0] or "application/octet-stream" + return guess_mime(name) def _fmt_bytes(n): diff --git a/zimi/library.py b/zimi/library.py index ed2ba0c8..013d36cc 100644 --- a/zimi/library.py +++ b/zimi/library.py @@ -3277,11 +3277,22 @@ def _post_download_finalize(dl): and f != dl["filename"] ): try: + # Release the superseded edition's pooled handles + # first. Windows will not unlink a file anyone still + # has open, and the reader who triggered this update + # has almost certainly had it open — so this remove + # failed there, into an `except OSError: pass`, and + # every update silently left its predecessor on disk. + # A no-op on POSIX. + try: + _srv.release_zim_handles([_srv._zim_short_name(f)]) + except Exception as e: + log.debug("Handle release before removing %s: %s", f, e) os.remove(os.path.join(_srv.ZIM_DIR, f)) removed_versions.append(f) log.info("Removed old version: %s", f) - except OSError: - pass + except OSError as e: + log.warning("Could not remove old version %s: %s", f, e) except OSError: pass # Register ONLY the new file. The old shape here — load_cache(force=True) diff --git a/zimi/manage.py b/zimi/manage.py index f858b60d..473f5c41 100644 --- a/zimi/manage.py +++ b/zimi/manage.py @@ -5462,6 +5462,15 @@ def _safe_name(s): e, ) pass + # Release this ZIM's pooled handles BEFORE unlinking. Windows + # refuses to remove a file anyone still has open, and a pooled + # libzim Archive is exactly that, so Delete answered 500 there + # every time. A no-op on POSIX, where unlink of an open file has + # always worked. + try: + _srv.release_zim_handles([_srv._zim_short_name(filename)]) + except Exception as e: + log.debug("Handle release before deleting %s: %s", filename, e) os.remove(filepath) log.info(f"Deleted ZIM: {filename}") record_activity( diff --git a/zimi/renderer.py b/zimi/renderer.py index bcc5edec..7aaf8d90 100644 --- a/zimi/renderer.py +++ b/zimi/renderer.py @@ -83,6 +83,7 @@ _strip_scripts, ) from zimi.zimwriter import ( + guess_mime, _MAX_ASSET_BYTES, _MAX_ASSETS, _MAX_TOTAL_ASSET_BYTES, @@ -1130,7 +1131,14 @@ def kill(self): self._driver_pid = None if not pid: return - for sig, grace in ((signal.SIGTERM, KILL_GRACE), (signal.SIGKILL, KILL_GRACE)): + # Windows has no SIGKILL, and naming it is enough to raise: this whole + # method was an AttributeError there, so a stalled browser could never + # be taken out on the one platform where the watchdog had never run. + # os.kill on Windows is TerminateProcess regardless of the number, so + # the second rung is the same rung — harmless, and the ladder stays one + # shape on both platforms. + hard = getattr(signal, "SIGKILL", signal.SIGTERM) + for sig, grace in ((signal.SIGTERM, KILL_GRACE), (hard, KILL_GRACE)): if not _process_alive(pid): return try: @@ -2129,8 +2137,7 @@ def _mimetype_of(response, url=""): mime = raw.split(";")[0].strip().lower() return ( mime - or mimetypes.guess_type(urllib.parse.urlsplit(url).path)[0] - or ("application/octet-stream") + or guess_mime(urllib.parse.urlsplit(url).path) ) @@ -2191,7 +2198,7 @@ def _typed(headers, url): for name in headers or (): if str(name).strip().lower() == "content-type": return headers - guessed, _encoding = mimetypes.guess_type(urllib.parse.urlsplit(url).path) + guessed = guess_mime(urllib.parse.urlsplit(url).path, fallback=None) if not guessed: return headers out = dict(headers or {}) diff --git a/zimi/server.py b/zimi/server.py index 7189ca65..1e99c526 100644 --- a/zimi/server.py +++ b/zimi/server.py @@ -3027,6 +3027,34 @@ def register_zim_file(path, removed_files=()): return True +def release_zim_handles(names): + """Drop the pooled archive and index handles for these short names. + + Two callers, for what is one reason on POSIX and two on Windows. + unregister_zim_file evicts so that no future read uses a mapping of a file + that is gone. The delete route evicts BEFORE it unlinks, because Windows + refuses to remove a file that anyone still holds open, and a pooled libzim + Archive is exactly that: deleting a ZIM there answered 500 "Failed to + delete file" every time. On POSIX the early release changes nothing — + unlinking an open file has always worked, the inode simply outlives it. + + Best effort by construction: a search thread already inside a read holds + its own reference, which no eviction can take away. It finishes in + milliseconds, and the caller reports the failure honestly if it has not. + """ + with _archive_lock: + for n in names: + _archive_pool.pop(n, None) + with _suggest_pool_lock: + for n in names: + _suggest_pool.pop(n, None) + _suggest_zim_locks.pop(n, None) + with _fts_pool_lock: + for n in names: + _fts_pool.pop(n, None) + _fts_zim_locks.pop(n, None) + + def unregister_zim_file(filename): """Incrementally drop ONE just-deleted ZIM from the live library. @@ -3093,17 +3121,7 @@ def unregister_zim_file(filename): # from the dicts only stops FUTURE use: a search thread already # holding one keeps a valid mapping of an unlinked file until it # finishes, which is why this needs no per-ZIM lock. - with _archive_lock: - for n in gone: - _archive_pool.pop(n, None) - with _suggest_pool_lock: - for n in gone: - _suggest_pool.pop(n, None) - _suggest_zim_locks.pop(n, None) - with _fts_pool_lock: - for n in gone: - _fts_pool.pop(n, None) - _fts_zim_locks.pop(n, None) + release_zim_handles(gone) # Invalidates /w/ entry ETags and the interlang resolution caches — # cross-ZIM answers genuinely change when a ZIM leaves. _cache_generation += 1 @@ -3274,7 +3292,54 @@ def _cli_restore(path, overwrite): ) +# The first-run banner is drawn in box characters, and a console that cannot +# encode one raises rather than substituting it. Windows redirects stdout at +# the locale encoding (cp1252, no box drawing), so on Windows the very first +# `zimi serve > log.txt` — the run where no password is set and the setup key +# has to be shown — died with UnicodeEncodeError before it ever reached READY. +_BANNER_ASCII = {"┌": "+", "└": "+", "─": "-", "│": "|"} + + +def _stdio_takes(text, stream=None): + """Whether this stream can represent `text` at its own encoding.""" + stream = stream if stream is not None else sys.stdout + encoding = getattr(stream, "encoding", None) or "utf-8" + try: + text.encode(encoding) + except (UnicodeEncodeError, LookupError): + return False + return True + + +def _printable(text): + """The same banner in characters this console will actually take.""" + if _stdio_takes(text): + return text + for drawn, plain in _BANNER_ASCII.items(): + text = text.replace(drawn, plain) + return text + + +def _make_stdio_resilient(): + """No character in any message may ever kill the server. + + _printable keeps the banner legible; this is the layer under it, for the + log line, the traceback, and the message nobody thought about. Only the + error handler changes, never the encoding: a console that asked for cp1252 + still gets cp1252, and an unrepresentable character arrives as an escape + rather than as an exception.""" + for stream in (sys.stdout, sys.stderr): + encoding = (getattr(stream, "encoding", "") or "").lower().replace("-", "") + if encoding in ("utf8", "utf8mb3", "utf8mb4"): + continue + try: + stream.reconfigure(errors="backslashreplace") + except (AttributeError, OSError, ValueError): + pass + + def main(): + _make_stdio_resilient() parser = argparse.ArgumentParser(description="ZIM Knowledge Base Reader") sub = parser.add_subparsers(dest="command") @@ -3752,16 +3817,18 @@ def add_boot_flags(p): key = _mng.ensure_setup_key() log.info("Library management enabled — no admin password set yet.") print( - "\n" - " ┌─ Zimi first-run setup ──────────────────────────────\n" - " │ Set the admin password from this machine, or from\n" - " │ another device using this one-time setup key:\n" - " │\n" - f" │ SETUP KEY: {key}\n" - " │\n" - " │ (also saved to the setup-key file in the data dir;\n" - " │ it stops working the moment a password is set)\n" - " └─────────────────────────────────────────────────────\n", + _printable( + "\n" + " ┌─ Zimi first-run setup ──────────────────────────────\n" + " │ Set the admin password from this machine, or from\n" + " │ another device using this one-time setup key:\n" + " │\n" + f" │ SETUP KEY: {key}\n" + " │\n" + " │ (also saved to the setup-key file in the data dir;\n" + " │ it stops working the moment a password is set)\n" + " └─────────────────────────────────────────────────────\n", + ), flush=True, ) from zimi import sso as _sso diff --git a/zimi/video.py b/zimi/video.py index 40a7578c..3a8e4feb 100644 --- a/zimi/video.py +++ b/zimi/video.py @@ -47,6 +47,7 @@ ) from zimi.p2p import is_offline from zimi.zimwriter import ( + guess_mime, _page_head, _plural, _slug, @@ -305,7 +306,7 @@ def _fmt_date(yyyymmdd): def _media_mime(path): ext = os.path.splitext(path)[1].lower() return ( - mimetypes.guess_type(path)[0] + guess_mime(path, fallback=None) or _MEDIA_MIME_FALLBACK.get(ext) or "application/octet-stream" ) diff --git a/zimi/zimwriter.py b/zimi/zimwriter.py index 67583c62..3d8cc2c2 100644 --- a/zimi/zimwriter.py +++ b/zimi/zimwriter.py @@ -26,6 +26,7 @@ import io import json import logging +import mimetypes import os import pathlib import posixpath @@ -36,6 +37,21 @@ import urllib.parse import zlib +# Every mimetype Zimi writes into a ZIM comes from here, and deliberately not +# from mimetypes.guess_type. That module's table is seeded from the OS: on +# Windows mimetypes.init() reads HKEY_CLASSES_ROOT, so the answer for .zip or +# .css depends on what the machine has installed, and a ZIM built there could +# carry a type no other machine would produce. A private MimeTypes() copies +# the table Python ships and nothing else, so the same capture has the same +# entry types on every platform. +_MIME_DB = mimetypes.MimeTypes() + + +def guess_mime(name, fallback="application/octet-stream"): + """The mimetype of a filename or URL path, identically on every OS.""" + return _MIME_DB.guess_type(name)[0] or fallback + + import zimi.server as _srv log = logging.getLogger("zimi.zimwriter") @@ -1596,6 +1612,31 @@ def make_asset_item(path, mimetype, data): return cls(path, path.rsplit("/", 1)[-1], data, mimetype=mimetype, front=False) +def _sweep_creator_scratch(tmp_path): + """Remove the index scratch libzim leaves beside the file it is building. + + libzim writes `_title.idx`, `_fulltext.idx` and a `.tmp` + for each, then unlinks them as it closes — which works only where the OS + lets a process unlink a file it still has open. Windows does not, so every + capture left four files of litter next to the ZIM, and a cancelled capture + left them in a directory the caller had been promised was untouched. + + Every name is derived from tmp_path, so this can only ever remove Zimi's + own scratch for this one build, never a neighbouring ZIM.""" + directory = os.path.dirname(tmp_path) or "." + stem = os.path.basename(tmp_path) + "_" + try: + names = os.listdir(directory) + except OSError: + return + for name in names: + if name.startswith(stem): + try: + os.remove(os.path.join(directory, name)) + except OSError: + pass # still held open; the caller has bigger problems + + @contextlib.contextmanager def atomic_zim_creator(out_path, language="eng"): """Yield a libzim Creator writing to ``.tmp``; rename over @@ -1617,6 +1658,8 @@ def atomic_zim_creator(out_path, language="eng"): except OSError: pass raise + finally: + _sweep_creator_scratch(tmp_path) # ── provenance ────────────────────────────────────────────────────────────── From 0d2e7d236039431b63035c22b1ab9c4f2f998323 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 5 Sep 2026 22:31:04 -0700 Subject: [PATCH 04/10] docs: the Windows fixes, and a test count that matches the suite These are fixes against 1.8.2 and every release before it, not against unreleased work on this branch, so they belong in Fixed. The cp1252 boot crash deliberately is not listed: the banner it choked on is new in this release and never shipped. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + README.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aca89fe4..0603b5ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Zimi runs from a folder of ZIMs with no configuration, on a stick or a NAS or a ### Fixed +- **Windows.** Deleting a ZIM answered "Failed to delete file" every time. Auto-update left every superseded edition on disk, silently. A stalled capture's browser could not be killed. Every ZIM built on Windows left four index scratch files beside it, and took its entry mimetypes from the machine's registry rather than from the spec. The cause of the first three is the same: Windows will not remove or replace a file that is still open, and Zimi released the file after acting on it rather than before. The Windows test suite had never run against a release; it now runs on every pull request, next to macOS Intel, Apple silicon, and the Linux the AppImage and snap are built on. - **The Raspberry Pi crash (#51).** A finished download re-hashed the file and re-scanned every installed archive while holding the lock readers need. Registration is incremental now: worst lock wait 10.6s down to 0.45s, archives opened 53 down to 1. The same pattern was hunted out of bookmark export and deletion. - **Flavor identity in the catalog (#50).** MDWiki's maxi and video builds no longer collide into two cards both claiming Full, and Update can no longer quietly fetch a ten gigabyte edition you never installed. - **Real links everywhere (#49).** Logo, source tiles, search results and cards are genuine anchors: middle-click, right-click and open-in-new-tab behave like the web. diff --git a/README.md b/README.md index ef92d977..5325ec66 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # Zimi [![CI](https://github.com/epheterson/Zimi/actions/workflows/ci.yml/badge.svg)](https://github.com/epheterson/Zimi/actions/workflows/ci.yml) -[![Tests](https://img.shields.io/badge/tests-2643-brightgreen)](#) +[![Tests](https://img.shields.io/badge/tests-2652-brightgreen)](#) [![Lighthouse Accessibility](https://img.shields.io/badge/Lighthouse%20a11y-100%2F100-success?logo=lighthouse&logoColor=white)](docs/plans/2026-04-26-accessibility.md) [![WCAG 2.1 AA](https://img.shields.io/badge/WCAG%202.1-AA-blue)](docs/plans/2026-04-26-accessibility.md) [![i18n](https://img.shields.io/badge/i18n-10%20languages-blueviolet)](#languages) From b72f2974622419dab14643534fb8e8135f438713 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 5 Sep 2026 22:44:33 -0700 Subject: [PATCH 05/10] fix: the last fifteen Windows failures * The kill-ladder test named signal.SIGKILL itself, so guarding the product alone was not enough. * Three more mock.patch.object(os, "getloadavg") sites needed create=True; I had fixed two of five. * The unregister tests removed a ZIM with a bare os.remove, which is not the sequence any caller uses now: the handles come out first, and on Windows that is not a preference. * The read-only write-path fixture had a third chmod site that asserted nothing, so on a platform that ignores the mode it built a writable directory and then failed the product for not falling back. * The index scratch sweep took three of four files and left the fulltext index, still mapped. Collect, then wait it out in bounded steps. The waits only happen when a file is genuinely still held, so POSIX never sleeps. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_create_hygiene.py | 4 ++-- tests/test_loadavg_throttle.py | 12 +++++++++--- tests/test_readonly_data_dir.py | 2 +- tests/test_unregister_zim.py | 23 +++++++++++++++++------ zimi/zimwriter.py | 27 +++++++++++++++++++++++---- 5 files changed, 52 insertions(+), 16 deletions(-) diff --git a/tests/test_create_hygiene.py b/tests/test_create_hygiene.py index c6b4647e..c12eaafb 100644 --- a/tests/test_create_hygiene.py +++ b/tests/test_create_hygiene.py @@ -151,7 +151,7 @@ def test_kill_takes_the_driver_out_from_any_thread(tmp_path): assert _zimi_droppings(str(tmp_path)) == [] finally: try: - os.kill(proc.pid, signal.SIGKILL) + os.kill(proc.pid, getattr(signal, 'SIGKILL', signal.SIGTERM)) except OSError: pass @@ -174,7 +174,7 @@ def test_shutdown_sessions_kills_every_registered_browser(tmp_path): assert _zimi_droppings(str(tmp_path)) == [] finally: try: - os.kill(proc.pid, signal.SIGKILL) + os.kill(proc.pid, getattr(signal, 'SIGKILL', signal.SIGTERM)) except OSError: pass diff --git a/tests/test_loadavg_throttle.py b/tests/test_loadavg_throttle.py index 54fc921e..3b172a34 100644 --- a/tests/test_loadavg_throttle.py +++ b/tests/test_loadavg_throttle.py @@ -32,7 +32,9 @@ def test_sleeps_when_load_exceeds_threshold(self): # 5-min load 4.0 / 4 cpus = 1.0 ratio. Above 0.8 threshold by 0.2. # Expected sleep = (1.0 - 0.8) * 2.0 = 0.4s. with ( - mock.patch.object(os, "getloadavg", return_value=(4.0, 4.0, 4.0)), + mock.patch.object( + os, "getloadavg", return_value=(4.0, 4.0, 4.0), create=True + ), mock.patch.object(os, "cpu_count", return_value=4), mock.patch.object(time, "sleep") as sleep_mock, ): @@ -44,7 +46,9 @@ def test_sleeps_when_load_exceeds_threshold(self): def test_sleep_capped_at_max(self): # Massive overload: ratio = 10. Cap to max_sleep. with ( - mock.patch.object(os, "getloadavg", return_value=(40.0, 40.0, 40.0)), + mock.patch.object( + os, "getloadavg", return_value=(40.0, 40.0, 40.0), create=True + ), mock.patch.object(os, "cpu_count", return_value=4), mock.patch.object(time, "sleep") as sleep_mock, ): @@ -69,7 +73,9 @@ def test_no_op_when_getloadavg_unavailable(self): def test_disabled_via_env_var(self): with ( mock.patch.dict(os.environ, {"ZIMI_INDEX_THROTTLE": "0"}, clear=False), - mock.patch.object(os, "getloadavg", return_value=(99.0, 99.0, 99.0)), + mock.patch.object( + os, "getloadavg", return_value=(99.0, 99.0, 99.0), create=True + ), mock.patch.object(os, "cpu_count", return_value=1), mock.patch.object(time, "sleep") as sleep_mock, ): diff --git a/tests/test_readonly_data_dir.py b/tests/test_readonly_data_dir.py index 3baef35a..22cda16d 100644 --- a/tests/test_readonly_data_dir.py +++ b/tests/test_readonly_data_dir.py @@ -323,7 +323,7 @@ def ro_data_dir(tmp_path): afterwards — the state a read-only boot WITHOUT the fallback would hit.""" d = tmp_path / "rodata" d.mkdir() - os.chmod(d, 0o555) + _make_unwritable(d) saved = server.ZIMI_DATA_DIR server.ZIMI_DATA_DIR = str(d) manage._env_pw_hash_cache = None diff --git a/tests/test_unregister_zim.py b/tests/test_unregister_zim.py index b4ab6816..664e2f7c 100644 --- a/tests/test_unregister_zim.py +++ b/tests/test_unregister_zim.py @@ -67,13 +67,24 @@ def _short(filename): # --------------------------------------------------------------------------- +def _delete_from_disk(path): + """Remove a ZIM the way every real caller of unregister_zim_file does. + + Its contract is "the file is already gone", and the callers get there by + releasing the pooled handles first, because Windows refuses to unlink a + file anyone still has open. A bare os.remove here would set up a sequence + the product does not use, and fail on the platform that enforces it.""" + server.release_zim_handles([server._zim_short_name(os.path.basename(path))]) + os.remove(path) + + def test_unregister_drops_the_zim_without_a_rescan(tmp_path, monkeypatch): zdir = _setup_library(tmp_path, monkeypatch) alpha, beta = _short(ALPHA), _short(BETA) assert alpha in server._zim_files_cache gen_before = server._cache_generation - os.remove(str(zdir / ALPHA)) + _delete_from_disk(str(zdir / ALPHA)) monkeypatch.setattr(server, "load_cache", _no_rescan) # A removal needs no metadata, so nothing may be opened or extracted. monkeypatch.setattr( @@ -97,7 +108,7 @@ def test_unregister_drops_the_disk_cache_row(tmp_path, monkeypatch): zdir = _setup_library(tmp_path, monkeypatch) assert ALPHA in (server._load_disk_cache() or {}) - os.remove(str(zdir / ALPHA)) + _delete_from_disk(str(zdir / ALPHA)) monkeypatch.setattr(server, "load_cache", _no_rescan) assert server.unregister_zim_file(ALPHA) is True @@ -116,7 +127,7 @@ def test_unregister_evicts_every_pooled_handle(tmp_path, monkeypatch): locks[alpha] = threading.Lock() locks[beta] = threading.Lock() - os.remove(str(zdir / ALPHA)) + _delete_from_disk(str(zdir / ALPHA)) monkeypatch.setattr(server, "load_cache", _no_rescan) assert server.unregister_zim_file(ALPHA) is True @@ -135,7 +146,7 @@ def test_unregister_drops_the_domain_claims(tmp_path, monkeypatch): interlang, "_domain_zim_map", {"alpha.example": alpha, "beta.example": beta} ) - os.remove(str(zdir / ALPHA)) + _delete_from_disk(str(zdir / ALPHA)) monkeypatch.setattr(server, "load_cache", _no_rescan) assert server.unregister_zim_file(ALPHA) is True @@ -161,7 +172,7 @@ def test_unregister_of_a_shadowed_duplicate_leaves_the_library_alone( files = server._zim_files_cache or {} assert files[alpha] == str(zdir / ALPHA) - os.remove(str(sub / ALPHA)) + _delete_from_disk(str(sub / ALPHA)) monkeypatch.setattr(server, "load_cache", _no_rescan) assert server.unregister_zim_file(ALPHA) is True @@ -180,7 +191,7 @@ def test_unregister_defers_when_a_shadowed_copy_would_be_promoted( sub = zdir / "backups" sub.mkdir() shutil.copy(str(zdir / ALPHA), str(sub / ALPHA)) - os.remove(str(zdir / ALPHA)) + _delete_from_disk(str(zdir / ALPHA)) monkeypatch.setattr(server, "load_cache", _no_rescan) assert server.unregister_zim_file(ALPHA) is False diff --git a/zimi/zimwriter.py b/zimi/zimwriter.py index 3d8cc2c2..afcc16f3 100644 --- a/zimi/zimwriter.py +++ b/zimi/zimwriter.py @@ -20,6 +20,7 @@ import colorsys import contextlib +import gc import datetime import hashlib import html as _html @@ -1626,15 +1627,33 @@ def _sweep_creator_scratch(tmp_path): directory = os.path.dirname(tmp_path) or "." stem = os.path.basename(tmp_path) + "_" try: - names = os.listdir(directory) + left = [n for n in os.listdir(directory) if n.startswith(stem)] except OSError: return - for name in names: - if name.startswith(stem): + if not left: + return + # The indexer's own handles close as its objects are finalized, which does + # not always happen before this returns: the first sweep on Windows took + # three of the four files and left the fulltext index, still mapped. So + # collect first, and give the stragglers a moment. The waits only happen + # when something is genuinely still held, so a POSIX build, where the + # unlink always succeeds first time, pays nothing for them. + gc.collect() + for delay in (0, 0.05, 0.1, 0.2, 0.4): + if delay: + time.sleep(delay) + stuck = [] + for name in left: try: os.remove(os.path.join(directory, name)) + except FileNotFoundError: + pass except OSError: - pass # still held open; the caller has bigger problems + stuck.append(name) + left = stuck + if not left: + return + log.debug("index scratch still held after %s: %s", tmp_path, left) @contextlib.contextmanager From 479b2fd874d881e739134dbba3866bbe88988e4c Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 5 Sep 2026 23:10:58 -0700 Subject: [PATCH 06/10] fix: a liveness probe that killed, and a sweep that could not delete a directory os.WNOHANG does not exist on Windows, and the AttributeError from naming it was not caught, so _process_alive raised and the watchdog could never establish that anything had died. What sat underneath was worse: os.kill on Windows sends no signal, it calls TerminateProcess with the number given, so the liveness probe os.kill(pid, 0) would have killed the process it was asking about. OpenProcess + GetExitCodeProcess is the real question. The index scratch survived three rounds of sweeping because a Xapian database is a DIRECTORY and os.remove cannot delete one. I narrowed two 'leaves nothing behind' assertions to accommodate that before finding it; both are reverted, because the guarantee was right and the sweep was wrong. The sweep test now covers the directory case. Unrelated, and found by the same run: the vocab-cache signature test asserted that one inserted row moves a SQLite file's size or mtime. On a filesystem with coarse timestamp granularity it moves neither, which is a flake rather than a signal. It writes enough rows to be observable now. Local suite: 2633 passed, 19 skipped. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_create_jobs.py | 8 +++++++- tests/test_did_you_mean.py | 29 +++++++++++++++++++++-------- zimi/renderer.py | 35 +++++++++++++++++++++++++++++++++++ zimi/zimwriter.py | 17 ++++++++++++++++- 4 files changed, 79 insertions(+), 10 deletions(-) diff --git a/tests/test_create_jobs.py b/tests/test_create_jobs.py index 8fdaec68..dcf399c8 100644 --- a/tests/test_create_jobs.py +++ b/tests/test_create_jobs.py @@ -972,10 +972,15 @@ def test_the_creator_sweeps_only_its_own_index_scratch(tmp_path): out = tmp_path / "site.zim.tmp" scratch = [ - tmp_path / "site.zim.tmp_title.idx", tmp_path / "site.zim.tmp_title.idx.tmp", tmp_path / "site.zim.tmp_fulltext.idx", ] + # Xapian keeps a database in a DIRECTORY. Sweeping with os.remove alone + # left every one of them behind, which is what kept the scratch alive on + # Windows through three rounds of fixing the wrong half of this. + scratch_dir = tmp_path / "site.zim.tmp_title.idx" + scratch_dir.mkdir() + (scratch_dir / "iamglass").write_bytes(b"x") keep = [ out, tmp_path / "site.zim", @@ -988,4 +993,5 @@ def test_the_creator_sweeps_only_its_own_index_scratch(tmp_path): _sweep_creator_scratch(str(out)) assert [f.name for f in scratch if f.exists()] == [] + assert not scratch_dir.exists(), "an index directory survived the sweep" assert sorted(f.name for f in keep if f.exists()) == sorted(f.name for f in keep) diff --git a/tests/test_did_you_mean.py b/tests/test_did_you_mean.py index c4a9f37a..267e0dd6 100644 --- a/tests/test_did_you_mean.py +++ b/tests/test_did_you_mean.py @@ -345,6 +345,25 @@ def test_final_prune_drops_remaining_singletons(self): self.assertNotIn("word", vocab) # count 1 → pruned +def _grow_the_index(db_path): + """Change a title index in a way every filesystem has to record. + + One inserted row often fits in a page SQLite has already allocated, so the + file size does not move, and then the only evidence left is mtime — which + a filesystem with coarse timestamp granularity may not have advanced yet + either. That is a flaky test, not a real signal: it failed once on a CI + runner and passed everywhere else. Enough rows to add a page make the + change observable on any filesystem, and "the index changed" is what these + tests are actually about.""" + conn = sqlite3.connect(db_path) + conn.executemany( + "INSERT INTO titles VALUES (?,?,?)", + [(f"A/{9000 + i}", f"New Thing {i}", f"new thing {i}") for i in range(500)], + ) + conn.commit() + conn.close() + + class VocabCachePersistenceTests(unittest.TestCase): """The vocab is persisted to disk and reloaded instead of rescanned, as long as its signature still matches the title indexes on disk.""" @@ -374,10 +393,7 @@ def test_signature_changes_when_index_touched(self): sig1 = _search._vocab_signature(self.index_dir) # Touch: append a row, changing size and mtime. db_path = os.path.join(self.index_dir, "wikipedia.db") - conn = sqlite3.connect(db_path) - conn.execute("INSERT INTO titles VALUES ('A/999','New Thing','new thing')") - conn.commit() - conn.close() + _grow_the_index(db_path) sig2 = _search._vocab_signature(self.index_dir) self.assertNotEqual(sig1, sig2) @@ -388,10 +404,7 @@ def test_cache_invalidated_after_index_change(self): self.assertIsNotNone(_search._vocab_cache_load()) # Touch the index — cache is now stale and must be rejected. db_path = os.path.join(self.index_dir, "wikipedia.db") - conn = sqlite3.connect(db_path) - conn.execute("INSERT INTO titles VALUES ('A/999','New Thing','new thing')") - conn.commit() - conn.close() + _grow_the_index(db_path) self.assertIsNone(_search._vocab_cache_load()) def test_builder_version_bump_invalidates_cache(self): diff --git a/zimi/renderer.py b/zimi/renderer.py index 7aaf8d90..9dde79f4 100644 --- a/zimi/renderer.py +++ b/zimi/renderer.py @@ -68,6 +68,7 @@ import re import shutil import signal +import sys import tempfile import threading import time @@ -1900,6 +1901,38 @@ def shutdown_sessions(): log.debug("could not kill a rendered session: %s", e) +def _process_alive_windows(pid): + """Whether a pid is still running, asked the only way Windows allows. + + Neither half of the POSIX answer exists here. os.WNOHANG is not defined, + and referring to it raises an AttributeError that the handler below it + does not catch, so this whole function was an exception on Windows and the + watchdog could never establish that anything had died. The fallback would + have been worse if it had been reached: os.kill on Windows does not send a + signal, it calls TerminateProcess with the number given — so the liveness + probe `os.kill(pid, 0)` would have killed the process it was asking about. + + OpenProcess plus GetExitCodeProcess is the real question. STILL_ACTIVE is + 259, which a process could in principle exit with; every implementation of + this check on this platform lives with that. + """ + import ctypes + + PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 + STILL_ACTIVE = 259 + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + handle = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, int(pid)) + if not handle: + return False # gone, or never ours to ask about + try: + code = ctypes.c_ulong() + if not kernel32.GetExitCodeProcess(handle, ctypes.byref(code)): + return False + return code.value == STILL_ACTIVE + finally: + kernel32.CloseHandle(handle) + + def _process_alive(pid): """Whether a pid is a process that is still RUNNING. @@ -1907,6 +1940,8 @@ def _process_alive(pid): stays in the table as a zombie until somebody waits for it, and a zombie answers signal 0 exactly like a live process would. Asking without reaping is how "did the browser actually die?" gets the wrong answer forever.""" + if sys.platform == "win32": + return _process_alive_windows(pid) try: reaped, _status = os.waitpid(pid, os.WNOHANG) if reaped == pid: diff --git a/zimi/zimwriter.py b/zimi/zimwriter.py index afcc16f3..339affe2 100644 --- a/zimi/zimwriter.py +++ b/zimi/zimwriter.py @@ -32,6 +32,7 @@ import pathlib import posixpath import re +import shutil import struct import threading import time @@ -1644,8 +1645,15 @@ def _sweep_creator_scratch(tmp_path): time.sleep(delay) stuck = [] for name in left: + target = os.path.join(directory, name) try: - os.remove(os.path.join(directory, name)) + # A Xapian index is a directory, not a file: os.remove cannot + # delete one, and for as long as this only called os.remove + # the scratch survived every sweep it was given. + if os.path.isdir(target): + shutil.rmtree(target) + else: + os.remove(target) except FileNotFoundError: pass except OSError: @@ -1665,6 +1673,13 @@ def atomic_zim_creator(out_path, language="eng"): from libzim.writer import Creator tmp_path = out_path + ".tmp" + # Before, as well as after. The sweep after a build is best effort by + # nature: libzim's index files close when its Creator is finalized, and + # while the CALLER's `with` is still open the caller holds a reference, + # so on Windows the last of them can outlive this function. Sweeping on + # the way in means a directory never carries more than one build's + # scratch, and the next build clears the last one's. + _sweep_creator_scratch(tmp_path) try: # Creator takes a Path; tmp_path stays a str for os.replace below. with Creator(pathlib.Path(tmp_path)).config_indexing(True, language) as creator: From 3ef2c7cf56e36cc73b73bb3919d456786e34ea50 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sun, 6 Sep 2026 09:10:07 -0700 Subject: [PATCH 07/10] Zimi 1.9.1: the first day of 1.9.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A security fix, a setting 1.9.0 should not have removed, and two bugs a single reporter found in a day. GHSA-5mw2-53vv-9pw6, reopened by a deployment shape. The bootstrap window waives the setup key for the machine running Zimi, and asked the RESOLVED client address whether it was that machine. Behind a reverse proxy on the same host — the standard NAS deployment — _client_ip correctly refuses the forwarded address as a trusted-tier claim and falls back to the hop, which is loopback, so every remote client through that proxy read as the host and skipped the key. It is asked of the socket now, and a forwarded request is never the host whatever the socket says. The bootstrap tests replaced _client_ip wholesale, so nothing had ever run the function that makes that decision: the advisory's own PoC was covered and the proxy path was not. The doubles move the socket too now. lan_admin (#59). The advisory closed a default that deserved closing and removed, with nothing in its place, a way people genuinely run Zimi: one household, one LAN, no password. It is back as something an operator types, and it means a DIRECT private peer — self-review caught that leaving it at _is_private_client would, behind that same proxy, have handed the admin of a passwordless server to anyone on the internet. The moon was upside down for half of every month (#60). The sprite shades from a Sun vector already flipped for a waning moon, and the bright-limb angle carries the same flip, so waning moons were turned a further 180 degrees: lit limb on the wrong side, maria inverted. The four renderers all agreed, and all agreed on the wrong number, which is precisely what a consistency test cannot see. There is now one that checks the sky. zimi import --setup could set up a sidecar the server never reads (#61). It resolves its own data dir from the shell it runs in; run without the service's config it installs into another library's state directory, reports success, and leaves the engine greyed out. The Create page now names this server's directory in the command it offers. 2636 passed, 19 skipped, plus the standalone JS suite. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 17 ++++++ README.md | 2 +- docs/deployment-networking.md | 21 +++++++ pyproject.toml | 2 +- tests/test_bootstrap_takeover.py | 95 ++++++++++++++++++++++++++++- tests/test_manage_sections.py | 10 ++- tests/test_moon_derivation.cjs | 68 +++++++++++++++++++++ tests/test_sidecar_install_hint.cjs | 84 +++++++++++++++++++++++++ zimi/http.py | 83 ++++++++++++++++++++++++- zimi/manage.py | 68 ++++++++++++++++++++- zimi/server.py | 13 +++- zimi/static/app.js | 20 +++++- zimi/static/create.js | 36 ++++++++++- 13 files changed, 508 insertions(+), 11 deletions(-) create mode 100644 tests/test_sidecar_install_hint.cjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0603b5ff..67d692e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,23 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/), and this project adheres to [Semantic Versioning](https://semver.org/). +## [1.9.1] - 2026-09-06 + +Four fixes for what the first day of 1.9.0 turned up, one of them a security fix. + +### Security + +- **A reverse proxy on the same machine could hand out the first admin password (GHSA-5mw2-53vv-9pw6, again).** The bootstrap window waives the setup key for the machine running Zimi, and asked the resolved client address whether it was on that machine. Behind a reverse proxy on the same host, which is the standard NAS deployment, the forwarded address is correctly refused as a trusted-tier claim and the resolved address falls back to the proxy's own: loopback. Every remote client through that proxy read as being on the host and skipped the key. Being the host is now asked of the socket, and a forwarded request is never the host whatever the socket says. Anyone who ran 1.9.0 behind a same-host proxy with no admin password should set one. + +### Added + +- **`lan_admin`, for running with no password at all (#59).** The advisory closed a default that let an adjacent device race the owner for the first password. It also removed a way people genuinely run Zimi: one household, one LAN, no password. `lan_admin` (or `ZIMI_LAN_ADMIN=1`) says that the private network is a boundary you trust and restores the pre-1.9.0 behaviour. Off unless you turn it on, and it applies only while no password is set. It means a direct connection from that network: a request through a reverse proxy does not qualify, since Zimi cannot tell one client of a proxy from another. + +### Fixed + +- **The moon was upside down for half of every month (#60).** The sprite shades from a Sun vector already flipped for a waning moon, and the bright-limb angle carries that same flip, so every waning moon was turned a further 180 degrees: lit limb on the wrong side, maria inverted. The month's other half was right, which is why it read as random. The four places that draw a moon all agreed with each other and all agreed on the wrong number, so the test that compared them could not see it; there is now one that checks the answer against the sky instead. +- **`zimi import --setup` could set up a sidecar the server never looks at (#61).** It resolves its own data dir from the shell it runs in, so run from a terminal without the service's configuration it installs into a different library's state directory, reports success, and leaves the alive engine greyed out with nothing on screen to explain it. The Create page now names this server's directory in the command it gives you, so what you paste lands where the server looks. + ## [1.9.0] - 2026-09-04 Zimi runs from a folder of ZIMs with no configuration, on a stick or a NAS or a fleet, and it makes ZIMs now. diff --git a/README.md b/README.md index 5325ec66..8abb9b54 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # Zimi [![CI](https://github.com/epheterson/Zimi/actions/workflows/ci.yml/badge.svg)](https://github.com/epheterson/Zimi/actions/workflows/ci.yml) -[![Tests](https://img.shields.io/badge/tests-2652-brightgreen)](#) +[![Tests](https://img.shields.io/badge/tests-2655-brightgreen)](#) [![Lighthouse Accessibility](https://img.shields.io/badge/Lighthouse%20a11y-100%2F100-success?logo=lighthouse&logoColor=white)](docs/plans/2026-04-26-accessibility.md) [![WCAG 2.1 AA](https://img.shields.io/badge/WCAG%202.1-AA-blue)](docs/plans/2026-04-26-accessibility.md) [![i18n](https://img.shields.io/badge/i18n-10%20languages-blueviolet)](#languages) diff --git a/docs/deployment-networking.md b/docs/deployment-networking.md index e7581a1e..fe8f64f0 100644 --- a/docs/deployment-networking.md +++ b/docs/deployment-networking.md @@ -223,6 +223,27 @@ Every key is optional. The four path/bind keys have matching CLI flags; the rest | `sso_aud` | `ZIMI_SSO_AUD` | string — the Access application's AUD tag | | `sso_role` | `ZIMI_SSO_ROLE` | string — `user` (default), `limited` or `admin`, given to an account on first sign-in | | `sso_proxy` | `ZIMI_SSO_PROXY` | list of CIDRs (a comma-separated string also works) — who may send the identity header; default any private peer | +| `lan_admin` | `ZIMI_LAN_ADMIN` | boolean — treat any private-network client as the admin on a **passwordless** instance; off by default, see [Running without a password](#running-without-a-password) | + + +### Running without a password + +A passwordless Zimi is a real way to run it: one household, one LAN, nothing to type. Up to 1.8.2 that is what you got — any client on a private network was the admin. + +That default had a hole ([GHSA-5mw2-53vv-9pw6](https://github.com/epheterson/Zimi/security/advisories)): "on a private network" includes every other device on the LAN, a Docker bridge, and anything on your tailnet, so an adjacent device could claim the first admin password before you did and lock you out of your own library. From 1.9.0 the bootstrap window is narrower: the machine running Zimi sets the first password with no secret, and any other device must present a one-time setup key the server prints on its first start. + +If your threat model does not include the other devices on your own network, say so explicitly: + +```yaml +# zimi.json +{ "lan_admin": true } +``` + +or `ZIMI_LAN_ADMIN=1`. Any private-network client is then the admin again, exactly as before 1.9.0, and no password is needed at all. + +It is off unless you turn it on, and it applies only while no admin password is set. Once there is a password, that password governs. Turn it on when the LAN is a boundary you trust; leave it off on a shared, office, or campus network, where "private address" and "people you trust" are not the same set. + +It also means a **direct** connection from your network. A request that arrived through a reverse proxy does not qualify, even one on the same machine, because Zimi cannot tell one client of that proxy from another: the forwarded address is not trustworthy, and the address it falls back to is the proxy's own. If you reach Zimi through a proxy, set an admin password rather than turning this on. A setting from the file is applied by exporting it into its environment variable at startup, and only ever when the file is the layer that won — so an environment variable you exported yourself is never overwritten, and a setting you left out stays genuinely unset rather than being pinned to its default. diff --git a/pyproject.toml b/pyproject.toml index 83bf9d4d..8e2b75b5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "zimi" -version = "1.9.0" +version = "1.9.1" description = "Offline knowledge server for ZIM files — search and read Wikipedia, Stack Overflow and 50+ sources with no internet" readme = "README.md" license = {text = "MIT"} diff --git a/tests/test_bootstrap_takeover.py b/tests/test_bootstrap_takeover.py index c714f1e0..10986f5e 100644 --- a/tests/test_bootstrap_takeover.py +++ b/tests/test_bootstrap_takeover.py @@ -53,15 +53,24 @@ def setUp(self): threading.Thread(target=self._srv.serve_forever, daemon=True).start() self._base = f"http://127.0.0.1:{self._srv.server_address[1]}" self._real_client_ip = zhttp.ZimHandler._client_ip + self._real_socket_peer_ip = zhttp.ZimHandler._socket_peer_ip def _as_peer(self, ip): """Make every request for the rest of this test appear to come from ``ip`` — a real non-loopback peer, which a forwarded header cannot - fake past the anti-spoof rule.""" + fake past the anti-spoof rule. + + The SOCKET moves too, not just the resolved client IP. Stubbing + _client_ip alone left the connection genuinely on loopback, so these + tests were modelling a remote attacker who, to the code that decides + who counts as the host, was sitting at the machine. That is the gap + the same-host reverse proxy fell through.""" zhttp.ZimHandler._client_ip = lambda _self, _ip=ip: _ip + zhttp.ZimHandler._socket_peer_ip = lambda _self, _ip=ip: _ip def tearDown(self): zhttp.ZimHandler._client_ip = self._real_client_ip + zhttp.ZimHandler._socket_peer_ip = self._real_socket_peer_ip self._srv.shutdown() manage._env_pw_hash_cache = None import shutil @@ -130,6 +139,90 @@ def test_the_host_itself_bootstraps_freely(self): self.assertEqual(status, 200, body) self.assertTrue(manage._get_manage_password_hash()) + def test_a_same_host_reverse_proxy_does_not_make_everyone_the_host(self): + """The advisory's fix, reopened by the commonest deployment there is. + + These tests replace _client_ip wholesale, so until this one nothing + ever executed the function that decides who counts as the host. In + production a reverse proxy on the SAME machine — Synology, nginx in + front of 8899, the usual NAS shape — connects from 127.0.0.1 and puts + the real client in X-Forwarded-For. _client_ip refuses to let that + header claim a trusted-tier address, so it falls back to the direct + peer, which is loopback: every remote client behind that proxy became + the host and skipped the setup key. + + So this test does NOT stub the peer. The socket really is loopback, + exactly as it is in that deployment, and the forwarded header is the + only thing distinguishing it from the owner sitting at the machine. + """ + for header in ("X-Forwarded-For", "X-Real-IP", "CF-Connecting-IP"): + with self.subTest(header=header): + status, body = self._post( + "/manage/set-password", + {"password": "attacker-owns-it"}, + headers={header: "192.168.1.50"}, + ) + self.assertEqual(status, 403, body) + self.assertFalse( + manage._get_manage_password_hash(), + f"a client forwarded by {header} claimed the first password", + ) + + def test_the_lan_can_be_trusted_but_only_on_purpose(self): + """Issue #59: 1.9.0 removed a way people actually run Zimi. + + Before the advisory, a passwordless instance treated any private + client as admin, and plenty of single-household servers depended on + that: no password, LAN only, done. The fix was right and the + replacement was missing, so those users found Settings simply shut. + + The opt-in has to be typed by whoever runs the server, and with it off + — the default, and what every other test here exercises — the LAN is + still refused.""" + self._as_peer(ADJACENT) + status, _ = self._post("/manage/set-password", {"password": "nope"}) + self.assertEqual(status, 403, "the default must still refuse the LAN") + + os.environ["ZIMI_LAN_ADMIN"] = "1" + try: + status, body = self._get("/manage/stats") + self.assertEqual(status, 200, body) + finally: + os.environ.pop("ZIMI_LAN_ADMIN", None) + + status, _ = self._get("/manage/stats") + self.assertEqual(status, 403, "switching it back off must shut the door") + + def test_lan_admin_does_not_hand_the_internet_the_keys(self): + """The escalation `lan_admin` would otherwise carry. + + Behind a reverse proxy on the same host, _client_ip cannot identify + the caller: it refuses the forwarded address as a trusted-tier claim + and falls back to the hop, which is loopback. Every client of that + proxy therefore resolves as "private" — including one on the far side + of the internet. Left at `_is_private_client`, turning on lan_admin + would have made all of them the admin of a passwordless server. + """ + os.environ["ZIMI_LAN_ADMIN"] = "1" + try: + status, body = self._post( + "/manage/set-password", + {"password": "attacker-owns-it"}, + headers={"X-Forwarded-For": "8.8.8.8"}, + ) + self.assertEqual(status, 403, body) + self.assertFalse( + manage._get_manage_password_hash(), + "lan_admin let a forwarded client claim the first password", + ) + # And a genuinely direct private peer still gets in, which is the + # entire point of the setting. + self._as_peer(ADJACENT) + status, body = self._get("/manage/stats") + self.assertEqual(status, 200, body) + finally: + os.environ.pop("ZIMI_LAN_ADMIN", None) + def test_a_remote_client_with_the_key_bootstraps_and_spends_it(self): key = manage.ensure_setup_key() self.assertTrue(key) diff --git a/tests/test_manage_sections.py b/tests/test_manage_sections.py index 091910fe..96c7291a 100644 --- a/tests/test_manage_sections.py +++ b/tests/test_manage_sections.py @@ -62,7 +62,15 @@ def test_creator_payload_answers_every_question_the_section_asks(monkeypatch): assert body["browser_ready"] is True assert body["alive_ready"] is False assert body["create_root"] == "/srv/zims" - assert set(body["sidecar"]) == {"installed", "version"} + # "dir" is part of the contract, not incidental: it is where THIS server + # looks for the sidecar, and the Create page puts it into the install + # command it offers. Without it the command is `zimi import --setup`, which + # resolves whatever data dir the operator's shell resolves — a different + # library's, if that shell lacks the service's config, which installs a + # working sidecar somewhere the server never reads (issue #61). + assert set(body["sidecar"]) == {"installed", "version", "dir"} + assert body["sidecar"]["dir"], "the server must say where it looks" + assert body["sidecar"]["dir"].endswith(os.path.join("tools", "warc2zim")) # Every type is present in the breakdown even when the library is empty, so # the client never has to guess a missing bucket is zero. inv = _get("/manage/creator/inventory").body diff --git a/tests/test_moon_derivation.cjs b/tests/test_moon_derivation.cjs index 391476e8..757e062e 100644 --- a/tests/test_moon_derivation.cjs +++ b/tests/test_moon_derivation.cjs @@ -109,5 +109,73 @@ const nw = vm.runInContext('_moonPhase(new Date(Date.UTC(2026, 0, 18, 19, 0)))', check(full.illumination > 97, 'known full moon reads > 97% (' + full.illumination + '%)'); check(nw.illumination < 3, 'known new moon reads < 3% (' + nw.illumination + '%)'); +// ── 4. CORRECTNESS, not just agreement ────────────────────────────────────── +// +// Everything above checks that the four renderers compute the SAME tilt. They +// did, and it was wrong for half of every month (issue #60): the sprite shades +// from a Sun vector already flipped by the waxing flag, and chi carries that +// same flip, so every waning moon was turned a further 180 degrees. The lit +// limb sat on the wrong side and the maria were upside down. Four renderers +// agreeing on one wrong number is exactly what a consistency test cannot see. +// +// The invariant with a known answer: put the Moon on the observer's meridian +// at a quarter phase. The Sun is then roughly 90 degrees away along the +// horizon, so the lit limb lies close to horizontal and the sprite — already +// lit on the correct side — needs almost no rotation. True at BOTH quarters. +function haDeg(t, lon) { + const eq = vm.runInContext('_moonEqCoords(new Date(' + t + '))', sandbox); + const gmst = (280.46061837 + 360.98564736629 * (eq.JD - 2451545.0)) % 360; + let ha = ((gmst + lon) - eq.ra * 180 / Math.PI) % 360; + if (ha > 180) ha -= 360; + if (ha < -180) ha += 360; + return ha; +} +function phaseAt(t) { + return vm.runInContext('_moonPhase(new Date(' + t + '))', sandbox).phase; +} +function nearestMeridianQuarter(target, lon) { + let best = null; + for (let m = 0; m < 70 * 24 * 60; m += 10) { + const t = Date.UTC(2026, 8, 1) + m * 60000; + const score = Math.abs(haDeg(t, lon)) + Math.abs(phaseAt(t) - target) * 720; + if (!best || score < best.score) best = { t, score }; + } + return best.t; +} +for (const [label, target] of [['first quarter (waxing)', 0.25], + ['last quarter (waning)', 0.75]]) { + for (const loc of [{ lat: 51.5, lon: -0.12 }, { lat: 40.7, lon: -74.0 }]) { + const t = nearestMeridianQuarter(target, loc.lon); + const raw = vm.runInContext( + '_moonScreenTiltDeg(new Date(' + t + '), ' + loc.lat + ', ' + loc.lon + ')', sandbox); + let tilt = ((raw % 360) + 360) % 360; + if (tilt > 180) tilt -= 360; + check(Math.abs(tilt) < 45, + label + ' on the meridian at lat ' + loc.lat + ' needs little rotation (got ' + + tilt.toFixed(1) + ' deg; ~180 means the disc is upside down)'); + } +} + +// The tilt may step only where the disc carries no visible phase. The waning +// correction turns over at new moon, on a 0%-lit disc; anywhere else a jump +// would be a real artifact somebody would watch happen on the time machine. +let worstJump = 0, worstIllum = 100, prevTilt = null; +for (let m = 0; m < 30 * 24 * 60; m += 5) { + const t = Date.UTC(2026, 8, 1) + m * 60000; + const v = vm.runInContext('_moonScreenTiltDeg(new Date(' + t + '), 51.5, -0.12)', sandbox); + if (prevTilt !== null) { + let d = v - prevTilt; + d = ((d % 360) + 540) % 360 - 180; + if (Math.abs(d) > 5) { + const illum = vm.runInContext('_moonPhase(new Date(' + t + '))', sandbox).illumination; + if (Math.abs(d) > worstJump) { worstJump = Math.abs(d); worstIllum = illum; } + } + } + prevTilt = v; +} +check(worstJump === 0 || worstIllum < 1, + 'any tilt step lands on an unlit disc (worst ' + worstJump.toFixed(0) + + ' deg at ' + worstIllum + '% lit)'); + if (failures) { console.error(failures + ' failure(s)'); process.exit(1); } console.log('all moon derivation checks passed'); diff --git a/tests/test_sidecar_install_hint.cjs b/tests/test_sidecar_install_hint.cjs new file mode 100644 index 00000000..fe5249a6 --- /dev/null +++ b/tests/test_sidecar_install_hint.cjs @@ -0,0 +1,84 @@ +// The install command the Create page offers must target THIS server. +// +// `zimi import --setup` resolves its own data dir from the shell it is run in. +// Run from a terminal that does not carry the service's config it resolves a +// different one, installs a working sidecar into it, prints "sidecar ready", +// and leaves the engine greyed out with nothing on screen to explain the gap +// (issue #61: set up against the default /zims while the service served +// /mnt/nas/ZIM). Naming the server's own directory in the command is what +// closes that, so the command has to keep naming it. +// +// Run: node tests/test_sidecar_install_hint.cjs (exit 0 = pass) + +const fs = require('fs'); +const path = require('path'); +const vm = require('vm'); + +const src = fs.readFileSync( + path.join(__dirname, '..', 'zimi', 'static', 'create.js'), 'utf8'); + +let failures = 0; +function check(ok, label) { + if (!ok) { console.error('FAIL: ' + label); failures++; } + else console.log('ok: ' + label); +} + +function extractFn(s, name) { + const i = s.indexOf('function ' + name + '('); + if (i < 0) throw new Error(name + ' not found'); + let j = s.indexOf('{', i), d = 0; + for (; j < s.length; j++) { + if (s[j] === '{') d++; + else if (s[j] === '}' && --d === 0) return s.slice(i, j + 1); + } + throw new Error('unbalanced braces in ' + name); +} + +const sandbox = { + console, + CREATE_PART_INSTALL: { sidecar: 'zimi import --setup' }, + _createSidecarDir: null, +}; +vm.createContext(sandbox); +vm.runInContext(extractFn(src, '_createShellQuote'), sandbox); +vm.runInContext(extractFn(src, '_createSidecarCommand'), sandbox); + +function cmd(dir) { + sandbox._createSidecarDir = dir; + return vm.runInContext('_createSidecarCommand()', sandbox); +} + +check(cmd(null) === 'zimi import --setup', + 'with no reported directory the command stays the plain one'); + +const nasDir = '/home/pi/.cache/zimi/zims-5fa07e4eab/tools/warc2zim'; +check(cmd(nasDir) === 'zimi import --setup --data-dir /home/pi/.cache/zimi/zims-5fa07e4eab', + 'the reported sidecar dir becomes --data-dir, without the tools/warc2zim tail'); + +check(cmd('/srv/my zims/.zimi/tools/warc2zim') === + "zimi import --setup --data-dir '/srv/my zims/.zimi'", + 'a path with a space is quoted, so the command survives a paste'); + +// Single quotes, not double: a shell expands $HOME and backticks inside double +// quotes, so a double-quoted path containing either pastes as a DIFFERENT path. +check(cmd('/srv/$USER/.zimi/tools/warc2zim') === + "zimi import --setup --data-dir '/srv/$USER/.zimi'", + 'a path a shell would expand is quoted so it cannot be expanded'); + +check(cmd("/srv/it's/.zimi/tools/warc2zim") === + "zimi import --setup --data-dir '/srv/it'\\''s/.zimi'", + 'an embedded single quote is escaped the way a shell accepts'); + +check(cmd('/var/lib/zimi') === 'zimi import --setup', + 'a reported path that is not a sidecar venv is not guessed at'); + +check(cmd(nasDir).indexOf('tools') < 0, + '--data-dir names the data dir, not the venv inside it'); + +// The whole point is that the server states this. If the probe stops carrying +// it the command silently reverts to the one that installs in the wrong place. +check(/sidecar_dir/.test(src), + 'create.js still reads sidecar_dir off the probe reply'); + +if (failures) { console.error(failures + ' failure(s)'); process.exit(1); } +console.log('all sidecar install hint checks passed'); diff --git a/zimi/http.py b/zimi/http.py index 965afc86..6236e92a 100644 --- a/zimi/http.py +++ b/zimi/http.py @@ -3026,19 +3026,98 @@ def _is_private_client(self): return False return _is_trusted_net(ip) + # Headers that mean "somebody forwarded this request". Presence alone + # disqualifies a claim of being ON the host, so the list is deliberately + # broader than the ones _client_ip actually reads: an unknown proxy that + # announces itself in any of these is still a proxy. + _FORWARDED_HEADERS = ( + "X-Forwarded-For", + "X-Real-IP", + "Forwarded", + "CF-Connecting-IP", + "True-Client-IP", + ) + + def _was_forwarded(self): + """True when something in front of Zimi passed this request along. + + Presence of the header is the whole test. Its VALUE cannot be trusted + — that is why _client_ip refuses a forwarded claim of a trusted-tier + address — but the fact that a hop announced itself is information the + hop had no reason to fake, and it is enough to know this request did + not come straight off the local network.""" + return any(self.headers.get(h) for h in self._FORWARDED_HEADERS) + + def _is_direct_private_client(self): + """A private-network peer that reached Zimi directly. + + `_is_private_client` asks about the RESOLVED address, and behind a + reverse proxy on the same host that resolution falls back to the + proxy's own loopback address — so every client of that proxy, from + anywhere on the internet, resolves as private. That is tolerable for + the things the private tier gates (rate limits, peer sharing) and not + tolerable for handing someone the admin of a passwordless instance, + which is what `lan_admin` does. + + So `lan_admin` asks this instead: a private peer, and nothing in + front. It is a narrower question, and it is the one the setting's own + wording promises.""" + return not self._was_forwarded() and self._is_private_client() + def _is_loopback_client(self): """True ONLY when the peer is the machine running Zimi (127.0.0.0/8, ::1). This is the bootstrap trust boundary — being ON the host is the one proof of ownership that needs no secret. A LAN or tailnet peer is 'private' but not the host, and must present the setup key instead (GHSA-5mw2-53vv-9pw6: private-tier was too wide a door for claiming - the first admin password).""" + the first admin password). + + A forwarded request is never the host, whatever the socket says. This + is the shape that reopened the advisory: a reverse proxy on the SAME + machine — the standard NAS deployment — connects from 127.0.0.1, and + _client_ip correctly refuses to let the forwarded header claim a + trusted-tier address, so it falls back to the direct peer. That peer + is loopback, and every remote client behind such a proxy read as being + on the host and skipped the setup key entirely. + + So the question is asked of the SOCKET, not of the resolved client IP, + and only when nothing forwarded the request. The host's own browser + reaching Zimi through its own proxy is caught by this too, and that is + correct: it is indistinguishable from any other client of that proxy, + and whoever is on the host can read the setup key out of the log. + + What this does NOT close, because loopback-as-proof cannot: a same-host + forwarder that sends no header at all — `socat`, or an nginx + `proxy_pass` with no `proxy_set_header` — still presents a bare + loopback peer, and there is nothing in the request to tell it apart + from the owner at the keyboard. Every mainstream reverse proxy sets a + forwarded header by default, so this covers the deployments people + actually have; closing the rest means retiring loopback-as-proof and + asking even the host for the setup key, which is a product decision + rather than a fix.""" + if self._was_forwarded(): + return False try: - ip = ipaddress.ip_address(self._client_ip()) + ip = ipaddress.ip_address(self._socket_peer_ip()) except ValueError: return False return ip.is_loopback + def _socket_peer_ip(self): + """The address on the other end of this TCP connection, forwarded + headers ignored. + + A seam, and a small one on purpose: `client_address` is set per + connection by socketserver, so a test cannot substitute it without + fighting the instance attribute. Everything that asks "who is + physically connected" goes through here, which is also what lets the + bootstrap tests model a genuinely remote peer instead of stubbing the + function whose answer they are checking.""" + try: + return self.client_address[0] + except (IndexError, TypeError): + return '' + def _peer_share_allowed(self): """True if this client may pull whole ZIMs from /dl/. diff --git a/zimi/manage.py b/zimi/manage.py index 473f5c41..24ad8c3a 100644 --- a/zimi/manage.py +++ b/zimi/manage.py @@ -227,6 +227,34 @@ def _clear_setup_key(): pass +def _lan_client(handler): + """A client `lan_admin` may treat as the owner: on the private network and + reaching Zimi directly. + + Not simply `_is_private_client`. Behind a reverse proxy on the same host + the resolved address falls back to the proxy's own loopback one, so every + client of that proxy resolves as private no matter where on the internet + it came from — and `lan_admin` would hand each of them the admin of a + passwordless instance. The setting says the LAN is the boundary; this is + that sentence, asked exactly.""" + direct = getattr(handler, "_is_direct_private_client", None) + return bool(direct() if direct else handler._is_private_client()) + + +def _lan_admin_allowed(): + """Whether the operator has said their LAN is their trust boundary. + + Read fresh rather than cached at import: `zimi config` publishes file + settings into the environment at startup, and a test that sets it wants it + to take effect.""" + return os.environ.get("ZIMI_LAN_ADMIN", "0").strip().lower() in ( + "1", + "true", + "yes", + "on", + ) + + def _bootstrap_key_ok(handler): """True when a remote bootstrap request carries the valid setup key, in the Authorization: Bearer header or an X-Zimi-Setup-Key header. Constant- @@ -331,8 +359,12 @@ def _primary_admin_authorized(handler): """ stored_pw = _get_manage_password_hash() if not stored_pw: - # Passwordless: LAN/loopback clients are the (only) primary admin. - return handler._is_private_client() + # Passwordless: the host itself, or any private client when the + # operator has opted into trusting the LAN (see _lan_admin_allowed). + if _lan_admin_allowed(): + return _lan_client(handler) + is_local = getattr(handler, "_is_loopback_client", handler._is_private_client) + return is_local() or _bootstrap_key_ok(handler) # A primary-admin SESSION token (users.create_admin_session): minted when the # admin password verified, delivered as the HttpOnly zimi_session cookie so @@ -429,6 +461,12 @@ def _check_manage_auth(handler): return None if _bootstrap_key_ok(handler): return None + # The operator's explicit "my LAN is my trust boundary" (issue #59). + # Off unless someone typed it, so the advisory's default stands; on, it + # restores the pre-1.9.0 behaviour for the people who ran Zimi that way + # deliberately and have no wish to hold an admin password. + if _lan_admin_allowed() and _lan_client(handler): + return None return PUBLIC_LOCKED if _primary_admin_authorized(handler) or _secondary_admin_authorized(handler): @@ -3394,6 +3432,13 @@ def _create_status(cursor, probe=False, events_cursor=0, history=False): if probe: # Only on the page's first poll: one cheap subprocess, not per-second. payload["import_ready"] = _create_import_ready() + # And WHERE this server keeps that sidecar, so the install command the + # page offers targets this instance rather than whatever data dir the + # operator's shell happens to resolve. `zimi import --setup` run from a + # shell without the service's config sets up a perfectly good sidecar + # for a different library, and the engine stays greyed out with nothing + # on screen to say why (issue #61). + payload["sidecar_dir"] = _create_sidecar_dir() # Whether the rendered engine's browser is installed here. Same # contract as import_ready: asked once, on the page's first poll, and # answered from a cache after that. @@ -3603,6 +3648,17 @@ def _create_root(): return os.path.realpath(os.path.expanduser(raw)) +def _create_sidecar_dir(): + """Where THIS server looks for the warc2zim sidecar, or None.""" + try: + from zimi.importer import sidecar_status + + return sidecar_status().get("dir") or None + except Exception: + log.exception("sidecar dir probe failed") + return None + + def _create_import_ready(): """True when the warc2zim sidecar is already installed — the one thing that decides whether archive import can run on a machine with no @@ -3802,6 +3858,14 @@ def _creator_payload(): sidecar = { "installed": bool(status.get("installed")), "version": status.get("version"), + # WHERE this server looks. The client pastes it into the install + # command, because `zimi import --setup` from a shell resolves its + # own data dir — and a shell that lacks the service's config + # resolves a different one, installs a perfectly good sidecar into + # it, and leaves the engine greyed out with no way to see why + # (issue #61: set up against the default /zims while the service + # served /mnt/nas/ZIM). + "dir": status.get("dir"), } except Exception: log.exception("sidecar status probe failed") diff --git a/zimi/server.py b/zimi/server.py index 1e99c526..1a85b64f 100644 --- a/zimi/server.py +++ b/zimi/server.py @@ -125,7 +125,7 @@ # SSL context using certifi CA bundle (PyInstaller bundles lack system certs) SSL_CTX = ssl.create_default_context(cafile=certifi.where()) -ZIMI_VERSION = "1.9.0" +ZIMI_VERSION = "1.9.1" # Standing maintenance cadence: catalog TTL is 24h and UPnP leases are # 24h — run every 12h so both stay fresh at half-life. @@ -657,6 +657,17 @@ def discover_zim_dir(candidates=None): ConfigSetting("sso_aud", "ZIMI_SSO_AUD", "str", "", "SSO off", False), ConfigSetting("sso_role", "ZIMI_SSO_ROLE", "str", "user", None, False), ConfigSetting("sso_proxy", "ZIMI_SSO_PROXY", "csv", "", "private networks", False), + # "My LAN is my trust boundary." Off by default, and it has to be typed by + # someone who runs the server: with it on, a passwordless instance treats + # any private-network client as the primary admin, which is what Zimi did + # before 1.9.0 and what GHSA-5mw2-53vv-9pw6 closed. + # + # The advisory is still right — that default let an adjacent device race + # the owner to the first password. What it lacked was a way to say "yes, I + # know, this is a single-household server on a LAN I control, and I do not + # want an admin password at all", which is a real way people run this and + # which 1.9.0 removed with nothing in its place (issue #59). + ConfigSetting("lan_admin", "ZIMI_LAN_ADMIN", "bool", "0", None, False), ) _CONFIG_ENV_BY_KEY = {s.key: s for s in CONFIG_ENV_SETTINGS} diff --git a/zimi/static/app.js b/zimi/static/app.js index 09aabedf..4a74a019 100644 --- a/zimi/static/app.js +++ b/zimi/static/app.js @@ -4018,7 +4018,25 @@ function _moonScreenTiltDeg(date, lat, lon) { var dA = raSun - eq.ra; var chi = Math.atan2(Math.cos(decSun) * Math.sin(dA), Math.sin(decSun) * Math.cos(eq.dec) - Math.cos(decSun) * Math.sin(eq.dec) * Math.cos(dA)); - return -((chi - q) * 180 / Math.PI) - 90; + var tilt = -((chi - q) * 180 / Math.PI) - 90; + // The sprite has ALREADY put the lit limb on the correct side: it shades + // from a Sun vector whose sign is the waxing flag (_moonSpriteCanvas, sx). + // chi carries that same flip, because the bright limb genuinely swaps sides + // between waxing and waning — so applying both turned every waning moon by + // a further 180 degrees. Half of every month was drawn upside down: the lit + // limb on the wrong side and the maria inverted, which is what a southern + // hemisphere moon looks like from the north (issue #60). + // + // The correction turns over at new and full, where the sprite's own flag + // does. At full the disc is whole and the step is invisible; at new it is + // 0% lit, so what turns over is the maria on an unlit disc. That is the + // whole cost, and it is the reason this is a step rather than the fully + // continuous fix: making it continuous means giving the shading loop a + // real terminator angle (its Sun vector is 2D today, x and z only) and + // keying the sprite cache on that angle as well as the phase, which is a + // different and much larger change than a released bug deserves. + if (!_moonIsWaxing(_moonPhase(date))) tilt += 180; + return tilt; } // Waxing predicate — shared so no renderer flips the terminator side on its diff --git a/zimi/static/create.js b/zimi/static/create.js index 3f21592a..14d984d6 100644 --- a/zimi/static/create.js +++ b/zimi/static/create.js @@ -232,6 +232,35 @@ var CREATE_PART_INSTALL = { sidecar: 'zimi import --setup' }; +// Where this server keeps its sidecar, once a probe has said so. +var _createSidecarDir = null; + +// Shell-quote a path for a command someone will paste into a terminal. +// +// Single quotes, not double: inside double quotes a shell still expands $HOME +// and backticks, so a data dir containing either would paste as a different +// path. Bare when the path has nothing a shell reads. +function _createShellQuote(text) { + if (/^[A-Za-z0-9_@%+=:,.\/-]+$/.test(text)) return text; + return "'" + text.replace(/'/g, "'\\''") + "'"; +} + +// The sidecar command, aimed at THIS server's data dir. +// +// `zimi import --setup` resolves its own data dir from the shell it runs in, +// so run from a terminal that lacks the service's config it installs into a +// different library's state directory: a clean install, a happy log line, and +// an engine still greyed out with nothing to say why. Naming the directory +// makes the pasted command land where the server actually looks. +function _createSidecarCommand() { + var base = CREATE_PART_INSTALL.sidecar; + if (!_createSidecarDir) return base; + // The server reports the venv; --data-dir wants the directory holding it. + var dir = _createSidecarDir.replace(/[\\/]tools[\\/]warc2zim[\\/]?$/, ''); + if (!dir || dir === _createSidecarDir) return base; + return base + ' --data-dir ' + _createShellQuote(dir); +} + var CREATE_FIELDS = { engine: { id: 'create-engine', control: 'engine', label: 'create_engine', @@ -1705,7 +1734,9 @@ function _createEngineHtml(f) { function _createAddCommands(into, capability) { var parts = CREATE_ENGINE_NEEDS[capability] || []; for (var i = 0; i < parts.length; i++) { - var cmd = CREATE_PART_INSTALL[parts[i]]; + var cmd = parts[i] === 'sidecar' + ? _createSidecarCommand() + : CREATE_PART_INSTALL[parts[i]]; if (cmd && _createPartReady(parts[i]) === false && into.indexOf(cmd) < 0) into.push(cmd); } } @@ -2370,6 +2401,9 @@ function _createIngest(data) { _createSidecarReady = data.import_ready; _createRemember('sidecar', data.import_ready); } + if (typeof data.sidecar_dir === 'string' && data.sidecar_dir) { + _createSidecarDir = data.sidecar_dir; + } if (typeof data.browser_ready === 'boolean') { _createBrowserReady = data.browser_ready; _createRemember('browser', data.browser_ready); From a79aea28cbe9b794e2e8fa7b18b53ba8124bd65e Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sun, 6 Sep 2026 10:05:29 -0700 Subject: [PATCH 08/10] test: wait on a deadline, not on a number of turns macos-latest failed test_the_line_buffer_is_bounded on the same commit that passed it in the run beside it: "creation job never finished". _wait_done polled 400 times at 10ms, which reads as a four-second ceiling and is not one. Each turn also makes an HTTP request, so what the count actually measures is how fast the runner is that minute. The 550-line buffer test is the slowest of them, so it is the one that ran out of turns first. Both helpers take a wall-clock deadline now, 30 seconds. A passing test still returns the moment the job is done and the suite does not get slower; the only thing that waits is a job that genuinely hung, and reporting that honestly is worth the wait. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_create_jobs.py | 13 ++++++++++--- tests/test_create_routes.py | 20 +++++++++++++++++--- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/tests/test_create_jobs.py b/tests/test_create_jobs.py index dcf399c8..687f0fd6 100644 --- a/tests/test_create_jobs.py +++ b/tests/test_create_jobs.py @@ -37,12 +37,19 @@ ) -def _wait(predicate, tries=600, why="condition never came true"): - for _ in range(tries): +# A deadline, not a poll count — same reason as _wait_done in +# test_create_routes.py: a fixed number of turns measures how fast the runner +# is, not how long the job took, so a loaded machine fails a passing test. +_WAIT_SECONDS = 30 + + +def _wait(predicate, timeout=_WAIT_SECONDS, why="condition never came true"): + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: if predicate(): return True time.sleep(0.01) - raise AssertionError(why) + raise AssertionError(f"{why} (waited {timeout}s)") @pytest.fixture diff --git a/tests/test_create_routes.py b/tests/test_create_routes.py index a0e4c4ca..cc6d8f13 100644 --- a/tests/test_create_routes.py +++ b/tests/test_create_routes.py @@ -59,13 +59,27 @@ def _get(path, private=True, params=None): return h -def _wait_done(tries=400): - for _ in range(tries): +# A deadline, not a poll count. 400 polls at 10ms looks like four seconds and +# is not: each turn of the loop also makes an HTTP request, so the real ceiling +# is however fast the runner happens to be. On a contended macOS runner the +# 550-line buffer test ran out of turns and failed as "job never finished" — +# the same commit passed on the next run, which is the signature of a limit +# that measures the machine rather than the job. +# +# Time-based instead, and generous: a passing test still returns the moment the +# job is done, so the only thing that waits 30 seconds is a job that genuinely +# hung, which is worth 30 seconds to report honestly. +_WAIT_DONE_SECONDS = 30 + + +def _wait_done(timeout=_WAIT_DONE_SECONDS): + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: body = _get("/manage/create/status").body if body.get("done") or not body.get("active"): return body time.sleep(0.01) - raise AssertionError("creation job never finished") + raise AssertionError(f"creation job never finished within {timeout}s") @pytest.fixture(autouse=True) From e4c8417fbbd957a8017ef5db9f81829e3ba617e6 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sun, 6 Sep 2026 10:13:38 -0700 Subject: [PATCH 09/10] chore: refuse session links in commit messages They are links into a private transcript, published under the repo owner's name, in the one place that cannot be edited once it reaches main. I have put them there twice: the agent attribution block supplies the line, and anything following that instruction never sees the rule against it. So it stops being something to remember. .githooks/commit-msg rejects the commit and leaves the message for editing; a test keeps the hook present and working; and CI checks the branch on every pull request, because a hook only protects a clone that opted into it. Enable in a clone: git config core.hooksPath .githooks The nine commits already on this branch have been rewritten to drop the line. main never had one. Co-Authored-By: Claude Opus 5 (1M context) --- .githooks/commit-msg | 24 ++++++++++++++++++ .github/workflows/ci.yml | 13 ++++++++++ tests/test_ci_contract.py | 51 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100755 .githooks/commit-msg diff --git a/.githooks/commit-msg b/.githooks/commit-msg new file mode 100755 index 00000000..71a672c5 --- /dev/null +++ b/.githooks/commit-msg @@ -0,0 +1,24 @@ +#!/bin/sh +# Refuse a commit message carrying a Claude session link. +# +# Commit messages are published. A session URL in one is a link into a private +# transcript, pasted under Eric's name, in a place that cannot be edited after +# it lands on main. It has happened more than once, because the agent harness +# supplies an attribution block that includes the link and a model following +# that instruction never sees this rule. +# +# So it is enforced here rather than remembered: the commit fails, the message +# is left for editing, and the fix is to delete the line. +# +# Install (once per clone): git config core.hooksPath .githooks + +if grep -nE 'claude\.ai/(code/)?session|Claude-Session:' "$1" >/dev/null 2>&1; then + echo "commit-msg: this message contains a Claude session link." >&2 + echo >&2 + grep -nE 'claude\.ai/(code/)?session|Claude-Session:' "$1" >&2 + echo >&2 + echo "Commit messages are public and permanent. Remove the line and commit again." >&2 + echo "Co-Authored-By is fine; the session URL is not." >&2 + exit 1 +fi +exit 0 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 82b78da9..508abcd0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,19 @@ jobs: node "$f" done + # The hook only protects a clone that has run `git config core.hooksPath + # .githooks`. This is the backstop for one that has not. + - name: No session links in commit messages + if: github.event_name == 'pull_request' + run: | + if git log --format='%H%n%B' \ + "origin/${{ github.base_ref }}..${{ github.event.pull_request.head.sha }}" \ + | grep -n 'claude\.ai'; then + echo "::error::a commit message on this branch carries a Claude session link" + exit 1 + fi + echo "no session links" + - name: Verify CLI entry point run: zimi --help diff --git a/tests/test_ci_contract.py b/tests/test_ci_contract.py index d68b1fe3..1f2682f2 100644 --- a/tests/test_ci_contract.py +++ b/tests/test_ci_contract.py @@ -27,6 +27,7 @@ import re import shutil import subprocess +import tempfile import pytest @@ -191,3 +192,53 @@ def test_the_pr_gate_runs_on_every_runner_the_release_builds_on(): f"the release: {sorted(release - gate) or 'none'}. A platform in the " "release alone is one whose failures cannot be seen before the tag." ) + + +def test_the_repo_refuses_commit_messages_with_session_links(): + """A session URL in a commit message is a link into a private transcript, + published under the repo owner's name, in the one place that cannot be + edited after it lands on main. + + It is enforced by a hook rather than remembered because the agent harness + supplies an attribution block containing that link, and anything following + that instruction never sees the rule. This test is what keeps the hook + itself from being deleted or quietly stopping working.""" + hook = ROOT / ".githooks" / "commit-msg" + assert hook.is_file(), "the commit-msg hook is gone" + assert os.access(hook, os.X_OK), "the commit-msg hook is not executable" + + def run(message): + with tempfile.NamedTemporaryFile("w", suffix=".msg", delete=False) as fh: + fh.write(message) + path = fh.name + try: + return subprocess.run([str(hook), path], capture_output=True, text=True) + finally: + os.unlink(path) + + bad = run("a change\n\nClaude-Session: https://claude.ai/code/session_x1\n") + assert bad.returncode != 0, "the hook let a session link through" + + plain_url = run("a change\n\nsee https://claude.ai/code/session_x1 for context\n") + assert plain_url.returncode != 0, "the hook only catches the trailer form" + + good = run( + "a change\n\nCo-Authored-By: Claude \n" + ) + assert good.returncode == 0, f"the hook rejected a clean message: {good.stderr}" + + +def test_no_commit_on_this_branch_carries_a_session_link(): + """The hook stops new ones; this catches any that predate it, while the + branch can still be rewritten.""" + done = subprocess.run( + ["git", "log", "origin/main..HEAD", "--format=%H%n%B"], + capture_output=True, text=True, cwd=ROOT, + ) + if done.returncode != 0: + pytest.skip("no origin/main to compare against here") + offenders = [ln for ln in done.stdout.splitlines() if "claude.ai" in ln] + assert not offenders, ( + "commit message(s) on this branch carry a session link:\n " + + "\n ".join(offenders) + ) From ea723d7587b2b9811a9f4d8801793695cf711b32 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sun, 6 Sep 2026 10:24:14 -0700 Subject: [PATCH 10/10] test: run the commit-msg hook through sh, not as a program Windows does not read shebangs, so executing a /bin/sh script directly raises "[WinError 193] %1 is not a valid Win32 application" and the test I added an hour ago failed on exactly one of the four runners. Git for Windows ships the sh that git itself runs hooks with, so going through it is also closer to how the hook really runs there. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_ci_contract.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/test_ci_contract.py b/tests/test_ci_contract.py index 1f2682f2..0bc8d15f 100644 --- a/tests/test_ci_contract.py +++ b/tests/test_ci_contract.py @@ -205,14 +205,26 @@ def test_the_repo_refuses_commit_messages_with_session_links(): itself from being deleted or quietly stopping working.""" hook = ROOT / ".githooks" / "commit-msg" assert hook.is_file(), "the commit-msg hook is gone" - assert os.access(hook, os.X_OK), "the commit-msg hook is not executable" + if os.name != "nt": + assert os.access(hook, os.X_OK), "the commit-msg hook is not executable" + + # Run it THROUGH sh rather than as a program. It is a `#!/bin/sh` script, + # and Windows does not read shebangs: executing it directly raises + # "[WinError 193] %1 is not a valid Win32 application". Git for Windows + # ships the sh that git itself uses to run hooks, so this is also how the + # hook actually runs on that platform. + shell = shutil.which("sh") or shutil.which("bash") + if not shell: + pytest.skip("no POSIX shell here to run the hook with") def run(message): with tempfile.NamedTemporaryFile("w", suffix=".msg", delete=False) as fh: fh.write(message) path = fh.name try: - return subprocess.run([str(hook), path], capture_output=True, text=True) + return subprocess.run( + [shell, str(hook), path], capture_output=True, text=True + ) finally: os.unlink(path)