diff --git a/src/jinjaturtle/cli.py b/src/jinjaturtle/cli.py index 65dfa72..bdabc18 100644 --- a/src/jinjaturtle/cli.py +++ b/src/jinjaturtle/cli.py @@ -12,7 +12,6 @@ from .core import ( flatten_config, generate_ansible_yaml, generate_jinja2_template, - ConfigParseError, ) from .multi import process_directory @@ -77,12 +76,6 @@ 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 de34735..d86992b 100644 --- a/src/jinjaturtle/core.py +++ b/src/jinjaturtle/core.py @@ -305,66 +305,6 @@ 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. @@ -373,24 +313,7 @@ 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}") - 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 + parsed = handler.parse(path) # Make sure datetime objects are treated as strings (TOML, YAML) parsed = _stringify_timestamps(parsed) diff --git a/src/jinjaturtle/handlers/base.py b/src/jinjaturtle/handlers/base.py index 6be91f0..14aaec7 100644 --- a/src/jinjaturtle/handlers/base.py +++ b/src/jinjaturtle/handlers/base.py @@ -1,6 +1,5 @@ from __future__ import annotations -import re from pathlib import Path from typing import Any, Iterable @@ -58,20 +57,8 @@ 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 = re.sub(r"_+", "_", role_prefix.strip().lower()) + role_prefix = role_prefix.strip().lower() clean_parts: list[str] = [] for part in path: @@ -83,9 +70,7 @@ class BaseHandler: cleaned_chars.append(c.lower()) else: cleaned_chars.append("_") - # 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("_") + cleaned_part = "".join(cleaned_chars).strip("_") if cleaned_part: clean_parts.append(cleaned_part) diff --git a/src/jinjaturtle/multi.py b/src/jinjaturtle/multi.py index ce54e1c..c968d61 100644 --- a/src/jinjaturtle/multi.py +++ b/src/jinjaturtle/multi.py @@ -25,20 +25,13 @@ 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, - ConfigParseError, -) +from .core import dump_yaml, flatten_config, make_var_name, parse_config 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 @@ -638,13 +631,7 @@ def process_directory( # Parse and group by format grouped: dict[str, list[tuple[Path, Any]]] = defaultdict(list) for p in files: - 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 + fmt, parsed = parse_config(p, None) 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 deleted file mode 100644 index ab42fe5..0000000 --- a/tests/test_config_parse_errors.py +++ /dev/null @@ -1,123 +0,0 @@ -"""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 diff --git a/tests/test_core_utils.py b/tests/test_core_utils.py index fda6dd0..c8e41e1 100644 --- a/tests/test_core_utils.py +++ b/tests/test_core_utils.py @@ -191,43 +191,3 @@ 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)