From 3573e8e7501c6a888732bc5f812f4bc12f93e8bc Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 29 Jun 2026 20:46:07 +1000 Subject: [PATCH 1/2] Make var name collapse underscores --- src/jinjaturtle/handlers/base.py | 19 +++++++++++++-- tests/test_core_utils.py | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/jinjaturtle/handlers/base.py b/src/jinjaturtle/handlers/base.py index 14aaec7..6be91f0 100644 --- a/src/jinjaturtle/handlers/base.py +++ b/src/jinjaturtle/handlers/base.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re from pathlib import Path from typing import Any, Iterable @@ -57,8 +58,20 @@ class BaseHandler: role_prefix_section_subsection_key Sanitises parts to lowercase [a-z0-9_] and strips extras. + + Consecutive separators are collapsed to a single underscore. This is + required for correctness, not just aesthetics: a source key such as + ``log..level`` or ``cache--size`` would otherwise sanitise to a name + containing a double underscore (``log__level``). The output safety gate + in ``safety.py`` deliberately rejects *any* ``__`` in a generated + identifier because ``__`` is the gateway to every Jinja2 SSTI gadget + (``__class__``/``__globals__``/...). Emitting a dunder here would make + JinjaTurtle's own gate reject JinjaTurtle's own placeholder, aborting + generation on entirely benign config. Collapsing runs keeps every + generated name a plain single-underscore-delimited identifier that the + gate accepts. """ - role_prefix = role_prefix.strip().lower() + role_prefix = re.sub(r"_+", "_", role_prefix.strip().lower()) clean_parts: list[str] = [] for part in path: @@ -70,7 +83,9 @@ class BaseHandler: cleaned_chars.append(c.lower()) else: cleaned_chars.append("_") - cleaned_part = "".join(cleaned_chars).strip("_") + # Collapse runs of underscores (from adjacent separators) to a + # single "_" so the result can never contain a forbidden "__". + cleaned_part = re.sub(r"_+", "_", "".join(cleaned_chars)).strip("_") if cleaned_part: clean_parts.append(cleaned_part) diff --git a/tests/test_core_utils.py b/tests/test_core_utils.py index c8e41e1..fda6dd0 100644 --- a/tests/test_core_utils.py +++ b/tests/test_core_utils.py @@ -191,3 +191,43 @@ def test_flatten_config_unsupported_format(): flatten_config("bogusfmt", parsed=None) assert "Unsupported format" in str(exc.value) + + +def test_make_var_name_collapses_adjacent_separators(): + """Regression: adjacent separators must not produce a forbidden ``__``. + + A source key such as ``log..level`` or ``cache--size`` previously sanitised + to a name containing a double underscore (``..._log__level``). The output + safety gate in safety.py rejects *any* ``__`` in a generated identifier + (it is the gateway to Jinja2 SSTI gadgets), so emitting one made JinjaTurtle + reject its own placeholder and abort generation on entirely benign config. + make_var_name now collapses runs of underscores to a single ``_``. + """ + for raw_key in ("log..level", "cache--size", "a...b", "x.-.y", "a..b--c"): + name = make_var_name("role", ("main", raw_key)) + assert "__" not in name, f"{raw_key!r} produced {name!r}" + # Still a valid Ansible/Jinja identifier. + assert name.replace("_", "").isalnum() or name == "role" + + # The double-underscore-free collapse is stable and predictable. + assert make_var_name("role", ("main", "log..level")) == "role_main_log_level" + assert make_var_name("role", ("main", "cache--size")) == "role_main_cache_size" + # A leading-digit-free prefix with its own repeated separators is collapsed too. + assert make_var_name("My__Role", ("k",)) == "my_role_k" + + +def test_make_var_name_collapsed_names_pass_output_safety_gate(): + """The names make_var_name emits must be accepted by the safety gate. + + This binds the two modules together: whatever identifier make_var_name + produces for a hostile-looking key must lex as the JinjaTurtle subset, so a + real template built from it is not refused. + """ + from jinjaturtle.safety import verify_jinja2_template_safe + + for raw_key in ("log..level", "cache--size", "a...b", "weird..key..name"): + var = make_var_name("demo", ("section", raw_key)) + # Build the kind of expression a handler would emit for this variable. + template = "{{ " + var + " }}" + # Must not raise TemplateSafetyError. + verify_jinja2_template_safe(template) From 42321a8ec97918853aeb1fe6d438b5bb836703ca Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 29 Jun 2026 20:47:05 +1000 Subject: [PATCH 2/2] Catch malformed config errors gracefully --- src/jinjaturtle/cli.py | 7 ++ src/jinjaturtle/core.py | 79 ++++++++++++++++++- src/jinjaturtle/multi.py | 17 ++++- tests/test_config_parse_errors.py | 123 ++++++++++++++++++++++++++++++ 4 files changed, 223 insertions(+), 3 deletions(-) create mode 100644 tests/test_config_parse_errors.py diff --git a/src/jinjaturtle/cli.py b/src/jinjaturtle/cli.py index bdabc18..65dfa72 100644 --- a/src/jinjaturtle/cli.py +++ b/src/jinjaturtle/cli.py @@ -12,6 +12,7 @@ from .core import ( flatten_config, generate_ansible_yaml, generate_jinja2_template, + ConfigParseError, ) from .multi import process_directory @@ -76,6 +77,12 @@ def _main(argv: list[str] | None = None) -> int: except OutputPathError as exc: print(f"jinjaturtle: refusing unsafe output path: {exc}", file=sys.stderr) return 2 + except ConfigParseError as exc: + # The source file could not be parsed as its (detected or forced) + # format. This is expected for malformed/attacker-influenced input; + # fail cleanly with a non-zero exit code instead of a traceback. + print(f"jinjaturtle: {exc}", file=sys.stderr) + return 1 def _run(argv: list[str] | None = None) -> int: diff --git a/src/jinjaturtle/core.py b/src/jinjaturtle/core.py index d86992b..de34735 100644 --- a/src/jinjaturtle/core.py +++ b/src/jinjaturtle/core.py @@ -305,6 +305,66 @@ def detect_format(path: Path, explicit: str | None = None) -> str: return "ini" +class ConfigParseError(Exception): + """Raised when a source config file cannot be parsed as its format. + + Each underlying parser (json, tomllib, PyYAML, defusedxml/ElementTree, + configparser) raises its own exception type on malformed input. Without a + single normalised error, a malformed file -- which is entirely expected when + JinjaTurtle is pointed at harvested, attacker-influenceable config -- would + escape as an unhandled traceback (e.g. ``xml.etree.ElementTree.ParseError`` + on an XML file whose element name is not well-formed). ``parse_config`` + converts every such failure into this one type so the CLI can fail closed + with a clean message and a non-zero exit code, and so library callers (such + as Enroll, which falls back to copying the raw file) have a single, stable + exception to catch. + + Note: defusedxml's *security* exceptions (``EntitiesForbidden``, + ``DTDForbidden``, ...) are intentionally NOT folded into this type. They + signal an attempted XXE/entity-expansion attack rather than a benign + malformed file, and must propagate unchanged so callers can tell the two + apart. + """ + + +def _build_malformed_config_errors() -> tuple[type[BaseException], ...]: + """Return the concrete "this file is malformed" exception types to catch. + + Deliberately specific. In particular we must avoid catching plain + ``ValueError``: defusedxml's ``EntitiesForbidden``/``DTDForbidden`` subclass + ``ValueError``, and those are security signals that must NOT be swallowed. + """ + + import configparser + import json + from xml.etree.ElementTree import ParseError as _XMLParseError # nosec + + import yaml as _yaml + + errs: list[type[BaseException]] = [ + _XMLParseError, + json.JSONDecodeError, + configparser.Error, + _yaml.YAMLError, + UnicodeDecodeError, + ] + try: + import tomllib + + errs.append(tomllib.TOMLDecodeError) + except ModuleNotFoundError: # pragma: no cover - Python < 3.11 fallback + try: + import tomli # type: ignore + + errs.append(tomli.TOMLDecodeError) + except ModuleNotFoundError: + pass + return tuple(errs) + + +_MALFORMED_CONFIG_ERRORS = _build_malformed_config_errors() + + def parse_config(path: Path, fmt: str | None = None) -> tuple[str, Any]: """ Parse config file into a Python object. @@ -313,7 +373,24 @@ def parse_config(path: Path, fmt: str | None = None) -> tuple[str, Any]: handler = _HANDLERS.get(fmt) if handler is None: raise ValueError(f"Unsupported config format: {fmt}") - parsed = handler.parse(path) + try: + parsed = handler.parse(path) + except ConfigParseError: + raise + except _MALFORMED_CONFIG_ERRORS as exc: + # Normalise the per-parser "this file is malformed" errors into one + # type: json.JSONDecodeError / tomllib.TOMLDecodeError (ValueError + # subclasses), PyYAML's YAMLError, configparser.Error, and + # xml.etree.ElementTree.ParseError (raised by defusedxml on XML whose + # structure/element name is not well-formed). A bad input file is + # expected when parsing harvested config, so fail closed with a clean + # error instead of an unhandled traceback. + # + # IMPORTANT: this deliberately does NOT catch defusedxml's security + # exceptions (EntitiesForbidden, DTDForbidden, ...). Those signal an + # attempted XXE/entity-expansion attack and must propagate unchanged so + # callers (and tests) can distinguish "malformed" from "malicious". + raise ConfigParseError(f"could not parse {path} as {fmt}: {exc}") from exc # Make sure datetime objects are treated as strings (TOML, YAML) parsed = _stringify_timestamps(parsed) diff --git a/src/jinjaturtle/multi.py b/src/jinjaturtle/multi.py index c968d61..ce54e1c 100644 --- a/src/jinjaturtle/multi.py +++ b/src/jinjaturtle/multi.py @@ -25,13 +25,20 @@ from copy import deepcopy import os import configparser import stat +import sys from dataclasses import dataclass from pathlib import Path from typing import Any, Iterable import xml.etree.ElementTree as ET # nosec from . import j2 -from .core import dump_yaml, flatten_config, make_var_name, parse_config +from .core import ( + dump_yaml, + flatten_config, + make_var_name, + parse_config, + ConfigParseError, +) from .handlers.xml import XmlHandler from .safety import verify_jinja2_template_safe, verify_no_live_jinja_in_json_keys from .escape import escape_jinja_literal @@ -631,7 +638,13 @@ def process_directory( # Parse and group by format grouped: dict[str, list[tuple[Path, Any]]] = defaultdict(list) for p in files: - fmt, parsed = parse_config(p, None) + try: + fmt, parsed = parse_config(p, None) + except ConfigParseError as exc: + # One malformed file should not abort processing of an entire + # directory. Skip it with a warning; the rest still generate. + print(f"jinjaturtle: skipping {p}: {exc}", file=sys.stderr) + continue if fmt not in FOLDER_SUPPORTED_FORMATS: # Directory mode only supports a subset of formats for now. continue diff --git a/tests/test_config_parse_errors.py b/tests/test_config_parse_errors.py new file mode 100644 index 0000000..ab42fe5 --- /dev/null +++ b/tests/test_config_parse_errors.py @@ -0,0 +1,123 @@ +"""Regression tests for malformed-input handling (``ConfigParseError``). + +Bug: every parse-layer handler (xml/json/toml/yaml/ini) used to let its +underlying parser's exception escape as an unhandled traceback when given a +malformed file. Pointing JinjaTurtle (or Enroll, which calls it as a library) +at harvested, attacker-influenceable config makes malformed input an entirely +expected condition, so it must fail cleanly instead of crashing. + +These tests pin down that: + + * ``parse_config`` raises the normalised ``ConfigParseError`` for malformed + XML/JSON/TOML/INI; + * defusedxml's *security* exceptions (XXE / ``EntitiesForbidden``) are NOT + swallowed by that normalisation -- they must still propagate so a caller can + distinguish "malformed" from "malicious"; + * an unrelated error (e.g. the TOML "tomllib missing" ``RuntimeError``) is not + captured by the normalisation either; + * the CLI exits non-zero with a clean message (no traceback) on malformed + input; and + * folder mode skips an unparseable file instead of aborting the whole run. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest + +from jinjaturtle.core import ConfigParseError, parse_config + + +def _run_cli(args: list[str]) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, "-m", "jinjaturtle.cli", *args], + capture_output=True, + text=True, + ) + + +@pytest.mark.parametrize( + "fmt,filename,content", + [ + # Element name is not well-formed XML -> expat ParseError historically + # escaped as an uncaught traceback. + ("xml", "bad.xml", "<{{tag}}/>"), + ("xml", "bad2.xml", ""), + ("json", "bad.json", "{not valid json"), + ("toml", "bad.toml", "x = = ="), + # A bare key with no value is invalid INI for configparser. + ("ini", "bad.ini", "[s]\nthis line has no equals and no colon\n"), + ], +) +def test_parse_config_raises_configparseerror_on_malformed( + tmp_path: Path, fmt: str, filename: str, content: str +) -> None: + src = tmp_path / filename + src.write_text(content, encoding="utf-8") + with pytest.raises(ConfigParseError): + parse_config(src, fmt=fmt) + + +def test_parse_config_does_not_swallow_xxe_security_exception(tmp_path: Path) -> None: + """defusedxml's EntitiesForbidden must propagate, not become ConfigParseError. + + EntitiesForbidden subclasses ValueError, so a naive ``except ValueError`` / + ``except Exception`` in parse_config would mask an XXE attempt as a benign + "malformed file". The normalisation is deliberately scoped to exclude it. + """ + from defusedxml.common import EntitiesForbidden + + xxe = ( + '\n' + ' ]>\n' + "&xxe;\n" + ) + src = tmp_path / "xxe.xml" + src.write_text(xxe, encoding="utf-8") + with pytest.raises(EntitiesForbidden): + parse_config(src, fmt="xml") + + +def test_parse_config_does_not_swallow_unrelated_runtimeerror( + tmp_path: Path, monkeypatch +) -> None: + """A non-malformation error (tomllib missing) must propagate unchanged.""" + import jinjaturtle.handlers.toml as toml_module + + monkeypatch.setattr(toml_module, "tomllib", None) + src = tmp_path / "x.toml" + src.write_text('a = "b"\n', encoding="utf-8") + with pytest.raises(RuntimeError) as exc: + parse_config(src, fmt="toml") + assert "tomllib/tomli is required" in str(exc.value) + + +def test_cli_fails_cleanly_on_malformed_xml(tmp_path: Path) -> None: + """The CLI must exit non-zero with a clean message, not a traceback.""" + src = tmp_path / "bad.xml" + src.write_text("<{{tag}}/>", encoding="utf-8") + res = _run_cli([str(src), "-f", "xml", "-r", "demo"]) + assert res.returncode != 0 + # Clean, user-facing message -- not a Python traceback. + assert "could not parse" in res.stderr + assert "Traceback (most recent call last)" not in res.stderr + + +def test_folder_mode_skips_unparseable_file(tmp_path: Path) -> None: + """One malformed file should not abort processing of the whole directory.""" + from jinjaturtle.multi import process_directory + + good = tmp_path / "good.json" + good.write_text('{ "host": "localhost" }', encoding="utf-8") + bad = tmp_path / "bad.json" + bad.write_text("{not valid json", encoding="utf-8") + + defaults_yaml, outputs = process_directory(tmp_path, False, "demo") + # The good file still produced output despite the bad sibling: its content + # and source id appear, and a template was generated. + assert "localhost" in defaults_yaml + assert "good.json" in defaults_yaml + assert outputs