CFE-4708: Added support for a path-variable in def.json to specify files to ship beyond just extension-matching

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

cfbs/build.py

@@ -287,16 +287,25 @@ 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.
     """
+
     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)
@@ -309,10 +318,66 @@ 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
 
     map_file_responses(input_data, _localize)
+    return localized_paths
+
+
+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.
+    """
+    MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA = (3, 29)
+
+    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' in def.json requires masterfiles %s or later, but version is %s."
+        " Files with extensions not listed in 'input_name_patterns' may silently not sync to clients."
+        % (
+            ".".join(str(n) for n in MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA),
+            version,
+        )
+    )
 
 
 def _perform_input_step(args, name, destination, prefix, build_modules):
@@ -336,14 +401,24 @@ 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:
         raise CFBSExitError(
             "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)
@@ -532,6 +607,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 )")

tests/shell/064_input_file_check_mpf.sh

@@ -0,0 +1,23 @@
+set -e
+set -x
+cd tests/
+mkdir -p ./tmp/
+cd ./tmp/
+touch cfbs.json && rm cfbs.json
+rm -rf .git
+rm -rf copy-a-file out
+
+# A project pinned to a masterfiles version older than 3.29
+# must warn that files may silently not get synced to clients.
+cp ../shell/064_input_file_check_mpf/example-cfbs.json cfbs.json
+echo "some content" > source.txt
+
+mkdir -p copy-a-file
+printf 'bundle agent copy_a_file {\n  reports:\n      "Copying $(cfbs.source)";\n}\n' > copy-a-file/copy_a_file.cf
+
+echo '[{"type": "file", "variable": "source", "namespace": "cfbs", "bundle": "copy_a_file", "label": "Source file", "question": "Which file should be copied?", "filetype": [".txt", ".log"], "response": "./source.txt"}]' > copy-a-file/input.json
+
+# Building against masterfiles 3.27.1 must warn that it predates 3.29.
+cfbs build 2>&1 | grep -q "requires masterfiles 3.29 or later, but version is 3.27.1"
+
+rm -rf copy-a-file out source.txt

tests/shell/064_input_file_check_mpf/example-cfbs.json

@@ -0,0 +1,46 @@
+{
+  "name": "Example project",
+  "description": "Example description",
+  "type": "policy-set",
+  "git": false,
+  "build": [
+    {
+      "name": "masterfiles",
+      "description": "Official CFEngine Masterfiles Policy Framework (MPF).",
+      "tags": ["supported", "base"],
+      "repo": "https://github.com/cfengine/masterfiles",
+      "by": "https://github.com/cfengine",
+      "version": "3.27.1",
+      "commit": "15d8ca2ca951db3ddf197b90c9dea6ef75f5fa89",
+      "added_by": "cfbs add",
+      "steps": [
+        "run EXPLICIT_VERSION=3.27.1 EXPLICIT_RELEASE=1 ./prepare.sh -y",
+        "copy ./ ./"
+      ]
+    },
+    {
+      "name": "./copy-a-file/",
+      "description": "Copy a file into masterfiles via a file input.",
+      "tags": ["local"],
+      "added_by": "cfbs add",
+      "steps": [
+        "directory ./ services/cfbs/copy-a-file/",
+        "policy_files services/cfbs/copy-a-file/",
+        "bundles copy_a_file",
+        "input ./input.json def.json"
+      ],
+      "input": [
+        {
+          "type": "file",
+          "variable": "source",
+          "namespace": "cfbs",
+          "bundle": "copy_a_file",
+          "label": "Source file",
+          "question": "Which file should be copied?",
+          "filetype": [".txt", ".log"],
+          "default": "./source.txt"
+        }
+      ]
+    }
+  ]
+}

tests/shell/all.sh

@@ -107,6 +107,7 @@ run_test tests/shell/060_input_file.sh
 run_test tests/shell/061_set_input_file.sh
 run_test tests/shell/062_input_file_in_list_with_keys.sh
 run_test tests/shell/063_input_string_multiline_in_list.sh
+run_test tests/shell/064_input_file_check_mpf.sh
 
 # Summary
 _suite_end=$(date +%s)

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):
@@ -48,7 +56,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",
@@ -58,6 +68,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(
@@ -119,6 +133,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")
@@ -197,3 +238,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