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