fix(cache): refuse a render that captured a template failure

Ansible's marker behaviour replaces an undefined name with the literal
'<< error N - NAME is undefined >>' instead of raising, and it is
inherited ambiently from whatever task is rendering, so code calling
templar.template(fail_on_undefined=True) gets that string back rather
than an exception.

_render_with_templar then returns it, get_merged_applications memoises
it unconditionally under a key that fingerprints only applications,
users and two domains, and hands the same object back without a
deepcopy. The first caller in the process therefore freezes the value
for every later one: a lookup from another role renders a role's addon
config without that role's vars, and the role itself is served the
poisoned result afterwards even though its own read happens in the right
scope.

It reached a running Nextcloud as the stored Collabora WOPI URL. A
configuration key set to an error message is worse than a missing one,
because the application stores it and reports itself configured.

The check sits at the end of the one function every merge render passes
through, so applications, users and domains are covered, and it runs
before any caller can memoise. The path is carried down while
descending, so the message names the offending key rather than the
payload. The pattern requires the ordinal and the dash so prose about
errors does not trip it.

2678090934 made the same choice for the lookup layer; this is the cache
layer.

This raises where the code used to continue. That is the point - a value
nobody can act on should stop a deploy rather than configure an
application - but the blast radius is only measured by the next deploy.
This commit is contained in:
kevinveenbirkenbach committed 2026-08-29 16:06:45 +02:00
1 parent 42c7df9ed9
commit 49f2dd516b
2 files changed
+69

No files matched your search

+34
View File
@@ -182,6 +182,40 @@ class TestResolveOverrideMapping(unittest.TestCase):
self.assertEqual(result, {"web-app-x": {"foo": 1}})
class TestErrorMarkersAreRefused(unittest.TestCase):
"""A captured template failure must not become a cached configuration value.
Ansible's marker behaviour replaces an undefined name with a literal
``<< error N - ... >>`` string instead of raising. Nextcloud stored one as
the Collabora WOPI URL and reported itself configured.
"""
MARKER = "<< error 10 - 'NEXTCLOUD_COLLABORA_URL' is undefined >>"
def test_a_clean_payload_passes(self):
base.assert_no_error_markers({"a": {"b": ["fine", 42, None]}})
def test_a_marker_in_a_nested_list_is_refused(self):
payload = {"applications": {"app": {"config": [{"value": self.MARKER}]}}}
with self.assertRaises(ValueError) as caught:
base.assert_no_error_markers(payload)
self.assertIn("applications.app.config[0].value", str(caught.exception))
def test_the_message_carries_the_offending_value(self):
with self.assertRaises(ValueError) as caught:
base.assert_no_error_markers({"k": self.MARKER})
self.assertIn("NEXTCLOUD_COLLABORA_URL", str(caught.exception))
def test_prose_mentioning_an_error_is_not_a_marker(self):
"""The pattern must not fire on a description that discusses errors."""
base.assert_no_error_markers({"note": "returns << error >> on failure"})
base.assert_no_error_markers({"note": "error 10 - see the log"})
def test_a_marker_anywhere_in_a_longer_string_is_caught(self):
with self.assertRaises(ValueError):
base.assert_no_error_markers({"url": f"https://x/{self.MARKER}/y"})
class TestFingerprintIsContentOnly(unittest.TestCase):
def test_fingerprint_survives_reset_because_it_holds_no_state(self):
before = base._fingerprint_mapping({"a": 1})
+35
View File
@@ -16,6 +16,7 @@ only when actually invoked.
from __future__ import annotations
import copy
import re
import threading
from collections.abc import Mapping
from pathlib import Path
@@ -186,6 +187,39 @@ def _resolve_override_mapping(
return {}
ERROR_MARKER_RE = re.compile(r"<<\s*error\s+\d+\s*-")
def assert_no_error_markers(data: Any, path: str = "") -> None:
"""Refuse a render that captured a template failure instead of raising.
Args:
data: the rendered structure.
path: dotted position reached so far, for the message.
Ansible's marker behaviour, inherited from whatever task is rendering,
replaces an undefined name with the literal ``<< error N - ... >>`` rather
than raising. Left alone that string is memoised and handed to every later
caller, and it reaches applications as a configuration value: a key set to
an error message, which the application stores and reports as configured.
Failing here keeps a value nobody can act on out of the cache.
"""
if isinstance(data, Mapping):
for key, item in data.items():
assert_no_error_markers(item, f"{path}.{key}" if path else str(key))
return
if isinstance(data, (list, tuple)):
for index, item in enumerate(data):
assert_no_error_markers(item, f"{path}[{index}]")
return
if isinstance(data, str) and ERROR_MARKER_RE.search(data):
raise ValueError(
f"{path or '<root>'} rendered to {data!r}. A template failed and "
f"the failure was captured as text; caching it would hand that "
f"string to every later reader and to the application itself."
)
def _render_with_templar(
value: Any,
*,
@@ -274,6 +308,7 @@ def _render_with_templar(
if hasattr(templar, "available_variables"):
templar.available_variables = prev_templar_avail
assert_no_error_markers(data)
return _decrypt_ansible_encrypted_strings(data)