From eb362f309b1a9721c7bf981ef64eecd65d1c6609 Mon Sep 17 00:00:00 2001 From: Adam Gessaman Date: Sat, 29 Aug 2026 15:12:08 -0700 Subject: [PATCH] fix(response_template): address review findings 1-4 and 7 Follow-up to the PR #254 integration fixes, from an xhigh review of the merged branch. Five of fourteen findings; the other nine are written up in .scratch/pr254-deferred-review-findings.md (untracked). - Re-raise CancelledError from resolve_template_async. gather's `return_exceptions=True` captures it like any other exception, so the BaseException branch was swallowing it: a render cancelled during shutdown logged "shortening failed" and carried on to transmit. - Warn once per template, not once per render. The unresolved-template warning runs on the inbound message path, so a shorten_url filter in Test_Command.response_format wrote a WARNING per matching message, unthrottled, to a rotating 5 MB log. - Add a `urlencode` filter. A quoted literal substitutes nested fields verbatim, which is right for prose and wrong for URLs: `sender` is whatever a remote node advertises, and an unencoded `&`, `#` or space rewrote the query, truncated at a fragment, or malformed the link that was then POSTed to the shortener. `/` is encoded too, since an interpolated field is one path segment. - Distinguish the two ways shortening can be unavailable. A template needing resolution with no config now warns at the resolver instead of returning {} indistinguishably from "nothing to do"; a pre-pass that ran but could not shorten logs at debug, since that is transient network trouble rather than a misconfiguration and must not escalate on the message path. - Cover the path_command wiring. _format_path_reply_prefix is the only place the pre-pass is called and had no test, so a dropped `await` or a missing `shortened=` would have disabled shortening silently. The warn-once cache is module state, so its test fixture is autouse: without it a test that renders an unresolved template suppresses the warning in whichever test runs next, which is an order-dependent failure. Every template shipped in config.ini.example still renders identically to the regex parser this replaced. --- .gitignore | 1 + CHANGELOG.md | 3 + docs/path-command-config.md | 5 + modules/response_template.py | 69 ++++++++-- ...st_path_command_reply_prefix_shortening.py | 73 +++++++++++ tests/unit/test_response_template.py | 119 ++++++++++++++++++ 6 files changed, 262 insertions(+), 8 deletions(-) create mode 100644 tests/unit/test_path_command_reply_prefix_shortening.py diff --git a/.gitignore b/.gitignore index 22290d6..fb6ec31 100644 --- a/.gitignore +++ b/.gitignore @@ -188,4 +188,5 @@ local/service_plugins/* # Local debugging scratch (captured configs and logs — may contain real settings) _debug/ +.scratch/ aqi_http_cache.sqlite diff --git a/CHANGELOG.md b/CHANGELOG.md index c833dcc..4cb7079 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ semantic versioning. a packet on a full URL. The request is made off the event loop before rendering starts; if it fails the clause is dropped rather than sent unshortened, so a shortener outage costs the link instead of a second transmission. +- `urlencode` percent-encodes a value before it is interpolated into a URL built by + a quoted literal, for fields a remote node controls (`sender`, and anything derived + from message content) where a stray `&` or `#` would otherwise rewrite the link. - Shlink is supported as a URL shortener backend alongside v.gd / is.gd, selected with `short_url_website_service` under `[External_Data]` (`gd`, the default, or `shlink`). Shlink authenticates with `short_url_website_api_key`. diff --git a/docs/path-command-config.md b/docs/path-command-config.md index 785ebe6..9ea2909 100644 --- a/docs/path-command-config.md +++ b/docs/path-command-config.md @@ -33,6 +33,11 @@ reply_prefix = "{path_distance|prefix_if_nonempty:📏 }\n" reply_prefix = {packet_hash|if_nonempty:"https://scope.example.net/#/packets/{packet_hash}"} ``` The `LITERAL` argument may itself be a double-quoted string containing nested `{field}` placeholders (expanded before the filter runs), so the link above still carries the packet hash even though the field being gated on (`packet_hash`) and the field inside the literal are the same one. +- `urlencode` percent-encodes a value for safe interpolation into a URL. A quoted literal substitutes nested fields verbatim, which is correct for prose and wrong for links: `{sender}` is whatever name a remote node advertises, so an unencoded `&`, `#`, `?` or space rewrites the query, truncates the URL at a fragment, or malforms it. `{packet_hash}` is hex and needs no encoding, but anything a remote node controls does: + +```ini +reply_prefix = {sender|if_nonempty:"https://scope.example.net/#/nodes/{sender|urlencode}"|shorten_url} +``` - `shorten_url` replaces the value with a short link from the shortener configured under `[External_Data]` (`short_url_website`, `short_url_website_service` — `gd` for v.gd/is.gd-compatible or `shlink`, and `short_url_website_api_key`, which shlink requires). Chain it after building the link so only the final URL is sent over RF: ```ini diff --git a/modules/response_template.py b/modules/response_template.py index 535c9e4..37576f7 100644 --- a/modules/response_template.py +++ b/modules/response_template.py @@ -13,6 +13,7 @@ from __future__ import annotations import asyncio from typing import Any, Callable +from urllib.parse import quote from .url_shortener import shorten_url from .utils import message_hop_count, message_path_bytes_per_hop @@ -96,14 +97,22 @@ def _filter_shorten_url(value: str, ctx: dict[str, Any], args: str) -> str: return value if isinstance(resolved, dict): - return resolved.get(value, '') + short = resolved.get(value) + if short: + return short + # A resolved mapping that lacks this URL means the pre-pass ran and the + # shortener could not answer. Debug, not warning: that is a transient + # network condition on the message path, not a misconfiguration. + logger = ctx.get('logger') + if logger is not None: + logger.debug("No shortened form for %r; dropping the clause", value) + return '' - logger = ctx.get('logger') - if logger is not None: - logger.warning( - "shorten_url used in a template rendered without resolve_template_async(); " - "dropping the clause rather than blocking the event loop" - ) + _warn_unresolved_once( + ctx.get('logger'), + str(ctx.get('template') or ''), + 'the caller did not run resolve_template_async()', + ) return '' @@ -114,12 +123,27 @@ def _filter_if_nonempty(value: str, ctx: dict[str, Any], args: str) -> str: return args +def _filter_urlencode(value: str, ctx: dict[str, Any], args: str) -> str: + """Percent-encode *value* for safe interpolation into a URL. + + A quoted literal substitutes nested field values verbatim, which is right for + prose but wrong the moment the literal is a URL: ``sender`` is whatever name a + remote node advertises, so an unencoded ``&``, ``#``, ``?`` or space silently + rewrites the link's query, truncates it at a fragment, or malforms it outright. + Encodes ``/`` too, since an interpolated field is a single path segment. + """ + if not value: + return '' + return quote(value, safe='') + + RESPONSE_TEMPLATE_FILTERS: dict[str, FilterFn] = { 'pathbytes_min': _filter_pathbytes_min, 'pathbytes': _filter_pathbytes_min, 'hops_min': _filter_hops_min, 'prefix_if_nonempty': _filter_prefix_if_nonempty, 'if_nonempty': _filter_if_nonempty, + 'urlencode': _filter_urlencode, 'shorten_url': _filter_shorten_url, } @@ -129,6 +153,25 @@ RESPONSE_TEMPLATE_FILTERS: dict[str, FilterFn] = { # a chain whenever its literal needs a pipe. _GREEDY_ARG_FILTERS = frozenset({'prefix_if_nonempty'}) +# Templates already warned about, so a misconfiguration is reported once rather than +# once per inbound message. Bounded by the number of templates in config. +_UNRESOLVED_WARNED: set[str] = set() + + +def _warn_unresolved_once(logger: Any, template: str, reason: str) -> None: + """Warn that ``shorten_url`` cannot resolve here, at most once per template. + + This filter runs on the inbound message path, so an unconditional warning is one + log line per message forever on a device writing to a rotating 5 MB file. + """ + if logger is None or template in _UNRESOLVED_WARNED: + return + _UNRESOLVED_WARNED.add(template) + logger.warning( + "shorten_url in template %r cannot resolve (%s); dropping the clause rather " + "than blocking the event loop", template, reason, + ) + class _TemplateParser: """Finite-state parser for ``{field|filter:arg|...}``-style placeholders. @@ -310,6 +353,7 @@ def format_piped_template( 'prefix_hex_chars': prefix_hex_chars, 'config': config, 'shortened': shortened, + 'template': template, } if logger is not None: logger.debug("Rendering response template %r with fields %r", template, fields) @@ -345,7 +389,10 @@ async def resolve_template_async( Returns an empty mapping when there is nothing to do, which renders exactly as an unresolved template would. """ - if config is None or not template_needs_resolution(template): + if not template_needs_resolution(template): + return {} + if config is None: + _warn_unresolved_once(logger, template, 'no config was supplied to resolve it') return {} pending: set[str] = set() @@ -355,6 +402,7 @@ async def resolve_template_async( 'prefix_hex_chars': prefix_hex_chars, 'config': config, 'shortened': pending, + 'template': template, } _TemplateParser(template, fields, ctx, logger).render() if not pending: @@ -367,6 +415,11 @@ async def resolve_template_async( ) resolved: dict[str, str] = {} for url, short in zip(urls, results, strict=True): + # Cancellation is not a shortening failure. `return_exceptions=True` captures + # it like any other, so swallowing it here would let a cancelled render carry + # on and transmit during shutdown. + if isinstance(short, asyncio.CancelledError): + raise short if isinstance(short, BaseException): if logger is not None: logger.debug("Shortening %r failed: %s", url, short) diff --git a/tests/unit/test_path_command_reply_prefix_shortening.py b/tests/unit/test_path_command_reply_prefix_shortening.py new file mode 100644 index 0000000..43c303c --- /dev/null +++ b/tests/unit/test_path_command_reply_prefix_shortening.py @@ -0,0 +1,73 @@ +#!/usr/bin/env python3 +"""PathCommand wires the shorten_url pre-pass into its reply prefix. + +_format_path_reply_prefix is the only place resolve_template_async() is called, so +without these a dropped `await` or a missing `shortened=` would disable shortening +silently: the clause just stops appearing and nothing fails. +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from modules.commands.path_command import PathCommand +from modules.models import MeshMessage + +LONG = "https://scope.example.net/#/packets/ABCDEF12" +TEMPLATE = '{packet_hash|if_nonempty:"https://scope.example.net/#/packets/{packet_hash}"|shorten_url}' + + +def _msg(): + return MeshMessage(content="path", sender_id="!aabbccdd", is_dm=True) + + +@pytest.fixture +def cmd(mock_bot): + c = PathCommand(mock_bot) + c.path_reply_prefix = TEMPLATE + c.get_standard_placeholder_fields = MagicMock(return_value={"packet_hash": "ABCDEF12"}) + c._format_path_distance = MagicMock(return_value="") + return c + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_reply_prefix_uses_the_shortened_link(cmd): + with patch( + "modules.response_template.shorten_url", return_value="https://v.gd/abc" + ) as shorten: + out = await cmd._format_path_reply_prefix(_msg()) + + assert out == "https://v.gd/abc\n" + shorten.assert_called_once() + assert shorten.call_args[0][0] == LONG + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_reply_prefix_drops_the_clause_when_shortening_fails(cmd): + """An unreachable shortener costs the link, not a second transmission.""" + with patch("modules.response_template.shorten_url", return_value=""): + assert await cmd._format_path_reply_prefix(_msg()) == "" + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_reply_prefix_never_shortens_from_the_sync_render(cmd): + """The HTTP call must happen in the pre-pass, not inside format_piped_template.""" + with patch("modules.url_shortener.requests.get") as get, \ + patch("modules.url_shortener.requests.post") as post, \ + patch("modules.response_template.shorten_url", return_value="https://v.gd/abc"): + await cmd._format_path_reply_prefix(_msg()) + get.assert_not_called() + post.assert_not_called() + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_prefix_without_shorten_url_makes_no_request(cmd): + cmd.path_reply_prefix = "{packet_hash|prefix_if_nonempty:# }" + with patch("modules.response_template.shorten_url") as shorten: + out = await cmd._format_path_reply_prefix(_msg()) + assert out == "# ABCDEF12\n" + shorten.assert_not_called() diff --git a/tests/unit/test_response_template.py b/tests/unit/test_response_template.py index fa90c64..9842655 100644 --- a/tests/unit/test_response_template.py +++ b/tests/unit/test_response_template.py @@ -493,3 +493,122 @@ async def test_resolve_template_async_skips_a_gated_clause(): @pytest.mark.asyncio async def test_resolve_template_async_is_a_noop_without_config(): assert await resolve_template_async(_LINK_TEMPLATE, {"packet_hash": "A"}) == {} + + +@pytest.fixture(autouse=True) +def _fresh_warn_state(): + """Reset the module-level warn-once cache around every test in this file. + + `_UNRESOLVED_WARNED` deduplicates the unresolved-template warning across the + process, so without this a test that renders such a template silently suppresses + the warning in whichever test runs next — an order-dependent failure. Autouse + because any future test here could trip on it. + """ + from modules import response_template + + response_template._UNRESOLVED_WARNED.clear() + yield + response_template._UNRESOLVED_WARNED.clear() + + +@pytest.mark.unit +def test_urlencode_escapes_a_field_interpolated_into_a_url(): + """`sender` is whatever a remote node advertises; unencoded it rewrites the URL.""" + template = '{sender|if_nonempty:"https://x.example/u/{sender|urlencode}"}' + out = format_piped_template(template, {"sender": "bob&admin=1 #frag"}) + assert out == "https://x.example/u/bob%26admin%3D1%20%23frag" + + +@pytest.mark.unit +def test_urlencode_escapes_slashes_too(): + """An interpolated field is one path segment, not a path.""" + assert format_piped_template('{"p/{a|urlencode}"}', {"a": "x/../y"}) == "p/x%2F..%2Fy" + + +@pytest.mark.unit +def test_urlencode_leaves_an_empty_value_empty(): + assert format_piped_template("{missing|urlencode}", {}) == "" + + +@pytest.mark.unit +def test_unresolved_shorten_url_warns_once_per_template(): + """This runs on the inbound message path; an unconditional warning would be one + log line per message forever.""" + logger = Mock() + for _ in range(5): + assert format_piped_template(_LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger) == "" + assert logger.warning.call_count == 1 + + +@pytest.mark.unit +def test_a_second_distinct_template_still_warns(): + logger = Mock() + format_piped_template(_LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger) + format_piped_template('{a|shorten_url}', {"a": "https://other.example"}, logger=logger) + assert logger.warning.call_count == 2 + + +@pytest.mark.unit +def test_a_resolved_mapping_that_misses_does_not_warn(): + """A pre-pass that ran but could not shorten is a transient network condition, + not a misconfiguration — it must not escalate to WARNING on the message path.""" + logger = Mock() + out = format_piped_template( + _LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger, shortened={} + ) + assert out == "" + logger.warning.assert_not_called() + logger.debug.assert_called() + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_resolve_template_async_warns_when_config_is_missing(): + """Regression: this used to return {} indistinguishably from 'nothing to do', so + the render dropped the clause with no diagnostic anywhere.""" + logger = Mock() + assert await resolve_template_async(_LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger) == {} + logger.warning.assert_called_once() + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_resolve_template_async_does_not_warn_without_shorten_url(): + logger = Mock() + assert await resolve_template_async("{d|hops_min:1}", {"d": "1km"}, logger=logger) == {} + logger.warning.assert_not_called() + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_resolve_template_async_propagates_cancellation(): + """`return_exceptions=True` captures CancelledError like any other exception; + swallowing it would let a cancelled render carry on and transmit at shutdown.""" + import asyncio + + cfg = configparser.ConfigParser() + cfg.add_section("External_Data") + + async def _cancelled(*a, **k): + raise asyncio.CancelledError() + + with patch("modules.response_template.shorten_url", _cancelled): + with pytest.raises(asyncio.CancelledError): + await resolve_template_async( + _LINK_TEMPLATE, {"packet_hash": "AB"}, config=cfg + ) + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_resolve_template_async_still_swallows_ordinary_failures(): + cfg = configparser.ConfigParser() + cfg.add_section("External_Data") + + async def _boom(*a, **k): + raise RuntimeError("shortener exploded") + + with patch("modules.response_template.shorten_url", _boom): + assert await resolve_template_async( + _LINK_TEMPLATE, {"packet_hash": "AB"}, config=cfg + ) == {}