diff --git a/Makefile b/Makefile index 56635863..fc5431f4 100644 --- a/Makefile +++ b/Makefile @@ -20,8 +20,8 @@ install: check: venv format lint uv run pytest -export COVERAGE_PROCESS_START = $(PWD)/.coveragerc -export COVERAGE_FILE = $(PWD)/.coverage +coverage: export COVERAGE_PROCESS_START = $(PWD)/.coveragerc +coverage: export COVERAGE_FILE = $(PWD)/.coverage coverage: uv run coverage erase uv run coverage run --parallel-mode -m pytest diff --git a/cfbs/build.py b/cfbs/build.py index 1f5fbc6a..0222cc0b 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -286,19 +286,27 @@ def _localize_file_inputs(name, input_data, destination, build_modules): If a response is already shipped by another module's own "directory" build step (e.g. the project author set one up manually), that step's destination is used instead of making a redundant copy. + + Returns the masterfiles-relative destination path of every file that was + localized, so callers can make sure those exact paths get synced by the + policy update mechanism even if their extension isn't one of the ones + normally recognized. """ if not isinstance(input_data, list): - return + return [] module_dir_name = name[2:] if name.startswith("./") else name module_dir_name = os.path.basename(module_dir_name.rstrip("/")) + localized_paths = [] + def _localize(rel_path): if not rel_path or not os.path.isfile(rel_path): return rel_path already_shipped = _path_if_already_shipped(rel_path, build_modules, destination) if already_shipped is not None: + localized_paths.append(strip_left(already_shipped, "$(sys.inputdir)/")) return already_shipped rel_path = os.path.normpath(rel_path) @@ -311,8 +319,22 @@ def _localize(rel_path): "modules" if in_module_dir else "", rel_path, ) + abs_destination = os.path.abspath(destination) + if ( + os.path.commonpath([os.path.abspath(dest), abs_destination]) + != abs_destination + ): + # rel_path contained a ".." segment, or was absolute (which + # discards the destination prefix in os.path.join above) - + # either way it would land outside the built masterfiles. + raise CFBSExitError( + "Input file response '%s' would be placed outside the " + "built masterfiles - refusing to copy it" % rel_path + ) cp(rel_path, dest) - return "$(sys.inputdir)/" + os.path.relpath(dest, destination) + dest_rel = os.path.relpath(dest, destination) + localized_paths.append(dest_rel) + return "$(sys.inputdir)/" + dest_rel for element in input_data: if not isinstance(element, dict) or element.get("type") != "file": @@ -323,6 +345,51 @@ def _localize(rel_path): else: element["response"] = _localize(response) + return localized_paths + + +MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA = (3, 29) + + +def _warn_if_input_paths_extra_unsupported(destination, build_modules): + """input_paths_extra (CFE-4708) is only understood by masterfiles + 3.29.0+ - on an older target it's just an unused variable in def.json, + so the file(s) it names won't actually get synced to clients. + """ + def_json = read_json(os.path.join(destination, "def.json")) + if not def_json: + return + if not def_json.get("vars", {}).get("default:update_def.input_paths_extra"): + return + + masterfiles = next( + (m for m in build_modules if m.get("name") == "masterfiles"), None + ) + version = masterfiles.get("version") if masterfiles else None + if not version: + # Not an index-added "masterfiles" module (local copy) + # nothing to check the version of. + # Assume the user has the latest version of masterfiles + return + + parts = version.split(".") + try: + found = (int(parts[0]), int(parts[1])) + except (IndexError, ValueError): + return + if found >= MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA: + return + + log.warning( + "'input_paths_extra' was written to def.json, but the target masterfiles version (%s) predates %s, " + "which is when it was added - the file(s) dependent on this variable " + "(those whose extensions are not defined in input_name_patterns) may silently not get synced to clients." + % ( + version, + ".".join(str(n) for n in MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA), + ) + ) + def _perform_input_step(args, name, destination, prefix, build_modules): src, dst = args @@ -345,7 +412,7 @@ def _perform_input_step(args, name, destination, prefix, build_modules): ) return extras, original = read_json(src), read_json(dst) - _localize_file_inputs(name, extras, destination, build_modules) + localized_paths = _localize_file_inputs(name, extras, destination, build_modules) extras = generate_augment(name, extras) log.debug("Generated augment: %s", pretty(extras)) if not extras: @@ -353,6 +420,16 @@ def _perform_input_step(args, name, destination, prefix, build_modules): "Input data '%s' is incomplete: Skipping build step." % os.path.basename(src) ) + if localized_paths: + # Files brought in through "file" type inputs aren't necessarily + # matched by the policy update's default `input_name_patterns`. + # Rather than widening that extension-based matching for the whole + # policy set, point at exactly these files, by their literal + # relative path. + relative_paths = [path.replace(os.sep, "/") for path in localized_paths] + extras = merge_json( + extras, {"vars": {"default:update_def.input_paths_extra": relative_paths}} + ) if original: log.debug("Original def.json: %s", pretty(original)) merged = merge_json(original, extras) @@ -541,6 +618,7 @@ def perform_build(config: CFBSConfig, diffs_filename=None) -> int: raise CFBSExitError( "Error parsing JSON in 'out/masterfiles/def.json': %s" % e ) + _warn_if_input_paths_extra_unsupported("out/masterfiles", config["build"]) print("") print("Generating tarball...") sh("( cd out/ && tar -czf masterfiles.tgz masterfiles )") diff --git a/tests/test_build.py b/tests/test_build.py index 56389929..52fe4320 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -1,7 +1,12 @@ import os import copy +import json +import tempfile -from cfbs.build import _localize_file_inputs +import pytest + +from cfbs.build import _localize_file_inputs, _perform_input_step +from cfbs.utils import CFBSExitError def test_localize_file_inputs_copies_single_file(tmp_path, monkeypatch): @@ -18,11 +23,14 @@ def test_localize_file_inputs_copies_single_file(tmp_path, monkeypatch): } ] - _localize_file_inputs("run-a-script", input_data, "out/masterfiles", []) + localized_paths = _localize_file_inputs( + "run-a-script", input_data, "out/masterfiles", [] + ) expected_dest = "out/masterfiles/services/cfbs/deploy.sh" assert os.path.isfile(expected_dest) assert input_data[0]["response"] == "$(sys.inputdir)/services/cfbs/deploy.sh" + assert localized_paths == ["services/cfbs/deploy.sh"] def test_localize_file_inputs_copies_list_of_files(tmp_path, monkeypatch): @@ -42,7 +50,9 @@ def test_localize_file_inputs_copies_list_of_files(tmp_path, monkeypatch): } ] - _localize_file_inputs("run-scripts-module", input_data, "out/masterfiles", []) + localized_paths = _localize_file_inputs( + "run-scripts-module", input_data, "out/masterfiles", [] + ) assert input_data[0]["response"] == [ "$(sys.inputdir)/services/cfbs/one.sh", @@ -52,6 +62,10 @@ def test_localize_file_inputs_copies_list_of_files(tmp_path, monkeypatch): assert os.path.isfile( "out/masterfiles/services/cfbs/modules/run-scripts-module/two.sh" ) + assert localized_paths == [ + "services/cfbs/one.sh", + "services/cfbs/modules/run-scripts-module/two.sh", + ] def test_localize_file_inputs_strips_local_module_prefix_with_dot_slash( @@ -113,6 +127,33 @@ def test_localize_file_inputs_ignores_missing_file(tmp_path, monkeypatch): assert input_data == before +def test_localize_file_inputs_rejects_path_traversal(tmp_path, monkeypatch): + """An absolute (or '..'-laden) response would otherwise let os.path.join + discard the destination prefix, placing the file outside the built + masterfiles entirely - refuse it instead of writing there.""" + monkeypatch.chdir(tmp_path) + os.makedirs("out/masterfiles") + + with tempfile.NamedTemporaryFile(suffix=".txt", delete=False) as f: + f.write(b"pwned\n") + outside_path = f.name + + try: + input_data = [{"type": "file", "variable": "script", "response": outside_path}] + + with pytest.raises(CFBSExitError): + _localize_file_inputs("some-module", input_data, "out/masterfiles", []) + + assert not os.path.exists( + "out/masterfiles/services/cfbs/" + os.path.basename(outside_path) + ) + finally: + try: + os.unlink(outside_path) + except OSError: + pass + + def test_localize_file_inputs_ignores_non_file_types(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) os.makedirs("out/masterfiles") @@ -191,3 +232,52 @@ def test_localize_file_inputs_ignores_non_directory_steps(tmp_path, monkeypatch) "$(sys.inputdir)/services/cfbs/modules/run-scripts/deploy.sh" ) assert os.path.isfile("out/masterfiles/services/cfbs/modules/run-scripts/deploy.sh") + + +def test_perform_input_step_adds_input_paths_extra_for_localized_files( + tmp_path, monkeypatch +): + """A "file" type input should make the build add its exact destination + path to `default:update_def.input_paths_extra`, so the policy update + mechanism syncs it even if its extension isn't in the default + `input_name_patterns` list. + """ + monkeypatch.chdir(tmp_path) + os.makedirs("out/masterfiles") + os.makedirs("run-a-script") + with open("deploy.sh", "w") as f: + f.write("echo hi\n") + with open("run-a-script/input.json", "w") as f: + json.dump([{"type": "file", "variable": "script", "response": "deploy.sh"}], f) + + _perform_input_step( + ["./input.json", "def.json"], "run-a-script", "out/masterfiles", "+", [] + ) + + with open("out/masterfiles/def.json") as f: + result = json.load(f) + + expected_path = "services/cfbs/deploy.sh" + assert result["vars"]["default:update_def.input_paths_extra"] == [expected_path] + + +def test_perform_input_step_skips_input_paths_extra_for_non_file_inputs( + tmp_path, monkeypatch +): + """A build with only "string"-type inputs has nothing to localize, so no + `input_paths_extra` augment should be generated at all. + """ + monkeypatch.chdir(tmp_path) + os.makedirs("out/masterfiles") + os.makedirs("some-module") + with open("some-module/input.json", "w") as f: + json.dump([{"type": "string", "variable": "greeting", "response": "hello"}], f) + + _perform_input_step( + ["./input.json", "def.json"], "some-module", "out/masterfiles", "+", [] + ) + + with open("out/masterfiles/def.json") as f: + result = json.load(f) + + assert "vars" not in result