More hardening
This commit is contained in:
parent
c6ce0311ed
commit
f9332879ba
3 changed files with 95 additions and 3 deletions
|
|
@ -375,6 +375,10 @@ def parse_config(path: Path, fmt: str | None = None) -> tuple[str, Any]:
|
||||||
raise ValueError(f"Unsupported config format: {fmt}")
|
raise ValueError(f"Unsupported config format: {fmt}")
|
||||||
try:
|
try:
|
||||||
parsed = handler.parse(path)
|
parsed = handler.parse(path)
|
||||||
|
# Make sure datetime objects are treated as strings (TOML, YAML). This
|
||||||
|
# walks the parsed object recursively, so keep it inside the try where a
|
||||||
|
# RecursionError from a pathological structure is normalised below.
|
||||||
|
parsed = _stringify_timestamps(parsed)
|
||||||
except ConfigParseError:
|
except ConfigParseError:
|
||||||
raise
|
raise
|
||||||
except _MALFORMED_CONFIG_ERRORS as exc:
|
except _MALFORMED_CONFIG_ERRORS as exc:
|
||||||
|
|
@ -391,8 +395,16 @@ def parse_config(path: Path, fmt: str | None = None) -> tuple[str, Any]:
|
||||||
# attempted XXE/entity-expansion attack and must propagate unchanged so
|
# attempted XXE/entity-expansion attack and must propagate unchanged so
|
||||||
# callers (and tests) can distinguish "malformed" from "malicious".
|
# callers (and tests) can distinguish "malformed" from "malicious".
|
||||||
raise ConfigParseError(f"could not parse {path} as {fmt}: {exc}") from exc
|
raise ConfigParseError(f"could not parse {path} as {fmt}: {exc}") from exc
|
||||||
# Make sure datetime objects are treated as strings (TOML, YAML)
|
except RecursionError as exc:
|
||||||
parsed = _stringify_timestamps(parsed)
|
# A deeply-nested or self-referential structure (e.g. a recursive YAML
|
||||||
|
# anchor) can exhaust the Python stack while walking the parsed object.
|
||||||
|
# The YAML handler already rejects reference cycles up front; this is a
|
||||||
|
# format-agnostic backstop so any such input fails closed with a clean
|
||||||
|
# message instead of a stack-overflow traceback.
|
||||||
|
raise ConfigParseError(
|
||||||
|
f"could not parse {path} as {fmt}: input is too deeply nested "
|
||||||
|
"or self-referential"
|
||||||
|
) from exc
|
||||||
|
|
||||||
return fmt, parsed
|
return fmt, parsed
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,36 @@ from ..loop_analyzer import is_safe_loop_field_key
|
||||||
from ..loop_analyzer import LoopCandidate
|
from ..loop_analyzer import LoopCandidate
|
||||||
|
|
||||||
|
|
||||||
|
def _reject_recursive_structure(obj: Any) -> None:
|
||||||
|
"""Raise ``yaml.YAMLError`` if *obj* contains a reference cycle.
|
||||||
|
|
||||||
|
A recursive YAML anchor (``a: &a [*a]``) produces a container that contains
|
||||||
|
itself. Every consumer in JinjaTurtle walks the parsed object depth-first,
|
||||||
|
so a cycle would raise ``RecursionError`` deep in unrelated code. Detect it
|
||||||
|
up front by tracking the ``id()`` of containers on the current descent path;
|
||||||
|
a repeat means a cycle. ``yaml.YAMLError`` is raised so ``parse_config``
|
||||||
|
normalises it into a clean ``ConfigParseError`` like any other malformed
|
||||||
|
input, rather than surfacing a stack-overflow traceback.
|
||||||
|
"""
|
||||||
|
|
||||||
|
on_path: set[int] = set()
|
||||||
|
|
||||||
|
def walk(node: Any) -> None:
|
||||||
|
if isinstance(node, (dict, list)):
|
||||||
|
marker = id(node)
|
||||||
|
if marker in on_path:
|
||||||
|
raise yaml.YAMLError(
|
||||||
|
"recursive/self-referential YAML structure is not supported"
|
||||||
|
)
|
||||||
|
on_path.add(marker)
|
||||||
|
children = node.values() if isinstance(node, dict) else node
|
||||||
|
for child in children:
|
||||||
|
walk(child)
|
||||||
|
on_path.discard(marker)
|
||||||
|
|
||||||
|
walk(obj)
|
||||||
|
|
||||||
|
|
||||||
class YamlHandler(DictLikeHandler):
|
class YamlHandler(DictLikeHandler):
|
||||||
"""
|
"""
|
||||||
YAML handler that can generate both scalar templates and loop-based templates.
|
YAML handler that can generate both scalar templates and loop-based templates.
|
||||||
|
|
@ -21,7 +51,16 @@ class YamlHandler(DictLikeHandler):
|
||||||
|
|
||||||
def parse(self, path: Path) -> Any:
|
def parse(self, path: Path) -> Any:
|
||||||
text = path.read_text(encoding="utf-8")
|
text = path.read_text(encoding="utf-8")
|
||||||
return yaml.safe_load(text) or {}
|
parsed = yaml.safe_load(text) or {}
|
||||||
|
# PyYAML's safe_load happily builds *recursive* structures from an anchor
|
||||||
|
# that references itself (e.g. ``a: &a [*a]``). Downstream flattening,
|
||||||
|
# timestamp-stringifying and template generation all walk the parsed
|
||||||
|
# object recursively and would blow the Python stack (RecursionError) on
|
||||||
|
# such input. JinjaTurtle is regularly pointed at harvested,
|
||||||
|
# attacker-influenceable config, so reject a self-referential document
|
||||||
|
# cleanly here rather than crashing later.
|
||||||
|
_reject_recursive_structure(parsed)
|
||||||
|
return parsed
|
||||||
|
|
||||||
def generate_jinja2_template(
|
def generate_jinja2_template(
|
||||||
self,
|
self,
|
||||||
|
|
|
||||||
|
|
@ -162,3 +162,44 @@ def test_folder_mode_skips_unparseable_file(tmp_path: Path) -> None:
|
||||||
assert "localhost" in defaults_yaml
|
assert "localhost" in defaults_yaml
|
||||||
assert "good.json" in defaults_yaml
|
assert "good.json" in defaults_yaml
|
||||||
assert outputs
|
assert outputs
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"content",
|
||||||
|
[
|
||||||
|
"a: &a [*a]\n", # self-referential sequence alias
|
||||||
|
"root: &r\n child: *r\n", # self-referential mapping alias
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_recursive_yaml_is_rejected_cleanly(tmp_path: Path, content: str) -> None:
|
||||||
|
"""A recursive YAML anchor must fail as a clean ConfigParseError.
|
||||||
|
|
||||||
|
PyYAML's safe_load builds a self-referential object from a recursive anchor;
|
||||||
|
JinjaTurtle's downstream walks (flatten, timestamp-stringify, template gen)
|
||||||
|
would otherwise blow the stack with a RecursionError traceback. Harvested
|
||||||
|
config is attacker-influenceable, so this must fail closed.
|
||||||
|
"""
|
||||||
|
src = tmp_path / "cyclic.yaml"
|
||||||
|
src.write_text(content, encoding="utf-8")
|
||||||
|
with pytest.raises(ConfigParseError):
|
||||||
|
parse_config(src, "yaml")
|
||||||
|
|
||||||
|
|
||||||
|
def test_recursive_yaml_cli_exits_nonzero_without_traceback(tmp_path: Path) -> None:
|
||||||
|
src = tmp_path / "cyclic.yaml"
|
||||||
|
src.write_text("a: &a [*a]\n", encoding="utf-8")
|
||||||
|
res = _run_cli([str(src), "-f", "yaml", "-r", "role"])
|
||||||
|
assert res.returncode != 0
|
||||||
|
assert "Traceback" not in res.stderr
|
||||||
|
assert "RecursionError" not in res.stderr
|
||||||
|
|
||||||
|
|
||||||
|
def test_shared_noncyclic_yaml_aliases_still_work(tmp_path: Path) -> None:
|
||||||
|
"""A shared (acyclic) anchor referenced multiple times is a DAG, not a
|
||||||
|
cycle, and must still parse and template normally."""
|
||||||
|
src = tmp_path / "shared.yaml"
|
||||||
|
src.write_text("base: &b\n x: 1\na: *b\nc: *b\n", encoding="utf-8")
|
||||||
|
fmt, parsed = parse_config(src, "yaml")
|
||||||
|
assert fmt == "yaml"
|
||||||
|
assert parsed["a"] == {"x": 1}
|
||||||
|
assert parsed["c"] == {"x": 1}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue