diff --git a/modules/zim/test/test_read_config_value.py b/modules/zim/test/test_read_config_value.py index 41ce7f44..831e8558 100644 --- a/modules/zim/test/test_read_config_value.py +++ b/modules/zim/test/test_read_config_value.py @@ -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' diff --git a/modules/zim/test/test_start_kiwix_serve.py b/modules/zim/test/test_start_kiwix_serve.py index c9d13d67..12f95e13 100644 --- a/modules/zim/test/test_start_kiwix_serve.py +++ b/modules/zim/test/test_start_kiwix_serve.py @@ -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. diff --git a/scripts/start_kiwix_serve.sh b/scripts/start_kiwix_serve.sh index b837d1e9..b9f3d2ba 100755 --- a/scripts/start_kiwix_serve.sh +++ b/scripts/start_kiwix_serve.sh @@ -14,6 +14,14 @@ DEFAULT_ZIM="${DEFAULT_ZIM:-/opt/wrolpi-blobs/default.zim}" # Honor the configured Zim directory (wrolpi.yaml `zims_destination`, always # relative to the media directory), defaulting to `zims`. ZIMS_DEST="$(MEDIA_DIRECTORY="${MEDIA_DIRECTORY}" "${PROJECT_DIR}/wrolpi/scripts/read_config_value.sh" zims_destination zims)" +# Defense in depth: never let an absolute path, traversal, or empty value escape +# the media root (the settings API also rejects these). Fall back to `zims`. +case "${ZIMS_DEST}" in + ''|/*|*..*) + echo "Ignoring unsafe zims_destination '${ZIMS_DEST}'; using 'zims'" >&2 + ZIMS_DEST=zims + ;; +esac ZIM_DIRECTORY="${MEDIA_DIRECTORY}/${ZIMS_DEST}" LIBRARY="${ZIM_DIRECTORY}/library.xml" diff --git a/wrolpi/root_api.py b/wrolpi/root_api.py index 55650815..52afc5ce 100644 --- a/wrolpi/root_api.py +++ b/wrolpi/root_api.py @@ -245,6 +245,9 @@ async def update_settings(_: Request, body: schema.SettingsRequest): if body.zims_destination and pathlib.Path(body.zims_destination).is_absolute(): raise InvalidConfig('Zims directory must be relative to media directory') + elif body.zims_destination and '..' in pathlib.Path(body.zims_destination).parts: + # Kiwix serves and rebuilds library.xml in this directory; traversal must not escape media. + raise InvalidConfig('Zims directory must not contain ".."') elif not body.zims_destination: new_config['zims_destination'] = wrolpi_config.default_config['zims_destination'] @@ -305,6 +308,15 @@ async def update_settings(_: Request, body: schema.SettingsRequest): # without waiting for a reboot. Local import avoids a circular import # (modules depend on wrolpi, not the reverse). if wrolpi_config.zims_destination != old_zims_destination: + # Persist synchronously first: update() only queues an async background_save, + # and that switch has no ordering guarantee against the restart switch below. + # start_kiwix_serve.sh reads zims_destination from disk, so the file must be + # current before Kiwix restarts. A save failure is non-fatal (background_save + # still runs); log and continue. + try: + wrolpi_config.save() + except Exception as e: + logger.warning(f'Could not synchronously save config before restarting Kiwix: {e}') from modules.zim.lib import restart_kiwix_handler restart_kiwix_handler.activate_switch() diff --git a/wrolpi/scripts/default_zim/build_default_zim.sh b/wrolpi/scripts/default_zim/build_default_zim.sh index 2de45c60..410a3ad6 100755 --- a/wrolpi/scripts/default_zim/build_default_zim.sh +++ b/wrolpi/scripts/default_zim/build_default_zim.sh @@ -9,7 +9,9 @@ set -euo pipefail HERE="$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &>/dev/null && pwd )" BLOBS="$( cd -- "${HERE}/../../blobs" &>/dev/null && pwd )" -IMAGE="ghcr.io/openzim/zim-tools:latest" +# Pinned (not :latest) so the committed default.zim is reproducible: identical +# source assets always build with the same zimwriterfs release. +IMAGE="ghcr.io/openzim/zim-tools:3.7.0" echo "Building default.zim from ${HERE}/src -> ${BLOBS}/default.zim ..." rm -f "${BLOBS}/default.zim" # zimwriterfs refuses to overwrite an existing file. diff --git a/wrolpi/scripts/read_config_value.sh b/wrolpi/scripts/read_config_value.sh index 57dfd60d..c9f55f7f 100755 --- a/wrolpi/scripts/read_config_value.sh +++ b/wrolpi/scripts/read_config_value.sh @@ -35,6 +35,12 @@ if [ -f "${CONFIG}" ]; then sub(/^[[:space:]]+/, "", v) # leading whitespace sub(/[[:space:]]+#.*$/, "", v) # trailing inline comment sub(/[[:space:]]+$/, "", v) # trailing whitespace + # Unquoted YAML null means "unset"; return empty so the caller uses its + # default, matching how PyYAML/the Python config layer treat null. + if (v == "null" || v == "Null" || v == "NULL" || v == "~") { + print "" + exit + } # Strip a matching pair of surrounding quotes. if (v ~ /^".*"$/ || v ~ /^'\''.*'\''$/) v = substr(v, 2, length(v) - 2) diff --git a/wrolpi/test/test_root_api.py b/wrolpi/test/test_root_api.py index 25b7ebdf..e11bb199 100644 --- a/wrolpi/test/test_root_api.py +++ b/wrolpi/test/test_root_api.py @@ -104,6 +104,15 @@ async def test_changing_zims_destination_restarts_kiwix(test_session, async_clie mock_activate.assert_called_once() +@pytest.mark.asyncio +async def test_zims_destination_rejects_traversal(test_session, async_client): + """zims_destination must be relative and must not contain traversal ('..') or be absolute.""" + for bad in ('../../escape', 'a/../../b', '/abs/path'): + request, response = await async_client.patch('/api/settings', + content=json.dumps({'zims_destination': bad})) + assert response.status_code == HTTPStatus.BAD_REQUEST, f'{bad!r} should be rejected' + + @pytest.mark.asyncio async def test_log_level(async_client): """Log level must be between 0 and 40."""