zim: address PR review — config race, YAML null, path traversal, pinned build

Triage of Greptile findings on the default-Zim / configurable-directory PR:

- Stale config on restart (P1): settings update() only queues an async
  background_save, and the restart switch has no ordering guarantee (the switch
  worker pops LIFO), so start_kiwix_serve.sh could read the old zims_destination.
  Save the config synchronously before activating the restart switch.
- YAML null (P1): read_config_value.sh now treats an unquoted null/~ as unset
  (returns the default), matching PyYAML/the Python config layer.  A quoted
  "null" remains the literal string.
- Path traversal (P1/security): the settings API now rejects a zims_destination
  containing "..", and start_kiwix_serve.sh defends in depth by falling back to
  `zims` for any absolute/traversal/empty value, so Kiwix can never scan or
  rewrite library.xml outside the media root.
- Reproducible build (P2): pin the zim-tools image to 3.7.0 instead of :latest.

Tests added for null handling, traversal rejection (API + script).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland 2026-07-20 18:44:19 -06:00
parent 76aafc5da9
commit aeebe8d833
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
7 changed files with 68 additions and 1 deletions

View file

@ -57,6 +57,21 @@ def test_empty_value_returns_default(media_with_config):
assert _read(media, 'zims_destination', 'zims') == 'zims'
@pytest.mark.parametrize('null_repr', ['null', 'Null', 'NULL', '~'])
def test_yaml_null_returns_default(media_with_config, null_repr):
"""An unquoted YAML null means "unset" -> default, matching the Python config layer."""
media, write = media_with_config
write(f'zims_destination: {null_repr}\n')
assert _read(media, 'zims_destination', 'zims') == 'zims'
def test_quoted_null_is_literal_string(media_with_config):
"""A quoted "null" is the string null, not YAML null."""
media, write = media_with_config
write('zims_destination: "null"\n')
assert _read(media, 'zims_destination', 'zims') == 'null'
def test_missing_config_file_returns_default(tmp_path):
# No config directory/file at all (fresh install) -> default, no error.
assert _read(tmp_path / 'media', 'zims_destination', 'zims') == 'zims'

View file

@ -159,6 +159,21 @@ def test_honors_configured_zims_destination(kiwix_env):
assert 'mycustomzims/wikipedia.zim' in library.read_text()
def test_traversal_zims_destination_is_rejected(kiwix_env):
"""A traversal zims_destination must not let Kiwix escape the media root; it falls back to `zims`."""
kiwix_env['write_config']('zims_destination: ../../escape\n')
_write_zim(kiwix_env['zims'] / 'wikipedia.zim') # a valid zim in the safe default dir
result = kiwix_env['run']()
assert result.returncode == 0, result.stderr
assert kiwix_env['started_marker'].exists(), 'kiwix-serve was not started'
# Served from the safe `zims` directory, and no library.xml was created outside media.
assert kiwix_env['library'].exists()
assert 'wikipedia.zim' in kiwix_env['library'].read_text()
assert not (kiwix_env['media'].parent / 'escape').exists()
def test_falls_back_to_default_zim_when_no_usable_zim(kiwix_env):
"""When no functioning zim is present, the default Zim is served."""
# No zims in the directory; create the default Zim the script should fall back to.