diff --git a/CHANGELOG.md b/CHANGELOG.md index 57be826..51d4ae3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,6 @@ semantic versioning. ### Fixed -- Refactor response_template parsing to use finite state machine when processing - templates adding additional flexibility such as the use of nested fields. Added - support for shlink to External_Data configuration. Added url_shortener filter and - if_notempty filter to response_template. - - `path` no longer answers "No path information available in current message" on a busy mesh (#255). Verifying a channel message against the RF cache only ever checked the newest row, which assumes the RF log row and the decoded CHAN event @@ -159,6 +154,11 @@ semantic versioning. ### Changed +- Response templates are parsed by a character-by-character state machine rather + than by splitting on delimiters. Placeholders can now nest (`{"Dist: {d|hops_min:1}"}`) + and filter arguments can be quoted. Field values are substituted into the output + and never re-scanned, so a sender-supplied phrase still cannot inject a placeholder. + - Web viewer navigation is grouped: Radio, Scheduled Messages, Greeter, Feeds, Plugins and Configuration now sit under a single **Settings** gear menu, leaving Dashboard, Real-time, Contacts, Mesh Graph and Logs on the bar. The current page is highlighted, @@ -166,6 +166,28 @@ semantic versioning. ### Added +- Shlink is now supported as a URL shortener alongside v.gd / is.gd, selected with + `short_url_website_service = shlink` under `[External_Data]`. It authenticates with + `short_url_website_api_key` in an `X-Api-Key` header and needs `short_url_website` + set to your own instance — there is no default, and the bot skips shortening rather + than sending the key to a host you did not configure. + +- `shorten` and `if_nonempty` response-template filters. `shorten` runs a value + through the configured shortener and falls back to the original URL when shortening + fails, so a clause is never lost to a network error. `if_nonempty:L` replaces a + non-empty value with literal `L` and clears otherwise, which is how a whole clause + is hidden rather than labelled: `{packet_hash|if_nonempty:"https://…/{packet_hash}"|shorten}` + prints nothing at all when RF correlation fails, instead of a broken link. Both + filters also answer to their other spellings — `shorten_url` in a template, + `shorten_url` in a feed format, `if_notempty` — so a chain copied between a feed + format and a command `response_format` works unchanged either way. + +- Response-template filter arguments may be double-quoted, and a quoted argument may + contain nested `{field}` placeholders: `{d|prefix_if_nonempty:"Dist {sender}: "}`. + The quote ends the argument, so further filters can follow it. An unquoted + `prefix_if_nonempty` argument still consumes the rest of the placeholder, which is + what lets its literal contain `|`, so that form must stay last in its chain. + - `mqttN_keepalive` (default 60) sets the MQTT PINGREQ interval per broker. It was hardcoded at 60 before, which is long for websockets through a proxy that drops idle connections. diff --git a/config.ini.example b/config.ini.example index f1d19b9..ecd2f8d 100644 --- a/config.ini.example +++ b/config.ini.example @@ -644,12 +644,15 @@ log_max_bytes = 5242880 # log_backup_count: number of rotated backup files to keep (e.g. meshcore_bot.log.1 … .3) log_backup_count = 3 [External_Data] -# URL shortener Service (gd,shlink). Default: gd +# URL shortener service: gd (v.gd / is.gd-compatible) or shlink. Default: gd short_url_website_service = gd -# URL shortener API base (v.gd / is.gd-compatible create.php). Default: https://v.gd -# See https://v.gd/apishorteningreference.php +# URL shortener API base. For gd this is the create.php host (default https://v.gd, +# see https://v.gd/apishorteningreference.php). For shlink it is your own instance's +# base URL and is REQUIRED -- there is no default, and the bot refuses to shorten +# rather than send your API key to a host you did not configure. short_url_website = https://v.gd -# Optional API key for alternate shortener hosts (unused for public v.gd/is.gd) +# API key. Optional for gd (unused for public v.gd/is.gd, appended as ?key= for +# alternate hosts). Required for shlink, where it is sent as an X-Api-Key header. short_url_website_api_key = # Weather API key (future feature) @@ -1045,6 +1048,8 @@ enable_p_shortcut = true # Only the first RF chunk includes the prefix when the reply is split. # Example: @[{sender}] # Example: {path_distance|prefix_if_nonempty:\U0001F4CF } +# Filter arguments may be double-quoted to end them explicitly and allow further +# filters afterwards: {path_distance|prefix_if_nonempty:"\U0001F4CF "|hops_min:1} reply_prefix = # Bytes per hop required before resolving repeater names from the database. @@ -1559,7 +1564,9 @@ require_path_bytes_failure_response = # {firstlast_distance} requires at least two path nodes with stored repeater/roomserver coordinates. # Pipe filters (feed-style), e.g. only show cumulative path distance when hops are multibyte (2+ bytes per hop): # response_format = ack @[{sender}]{phrase_part} | {connection_info}{path_distance|pathbytes_min:2|prefix_if_nonempty: | Path Dist: }| F/L Dist: {firstlast_distance} | {elapsed} | Rec: {timestamp} -# Filters: pathbytes_min:N (alias pathbytes:N) clears the field unless bytes_per_hop >= N; hops_min:N clears it unless the message travelled at least N hops; prefix_if_nonempty:L prepends literal L when value non-empty after prior filters. If L contains |, put prefix_if_nonempty last in that placeholder (it consumes the rest of the chain as its literal). +# Filters: pathbytes_min:N (alias pathbytes:N) clears the field unless bytes_per_hop >= N; hops_min:N clears it unless the message travelled at least N hops; prefix_if_nonempty:L prepends literal L when value non-empty after prior filters; if_nonempty:L (alias if_notempty:L) replaces the value with literal L when non-empty; shorten (alias shorten_url) runs the value through the [External_Data] shortener, falling back to the original on failure. +# A filter argument may be double-quoted, which ends it at the closing quote so more filters can follow, and may contain nested {field} placeholders: prefix_if_nonempty:"Dist {sender}: "|hops_min:1 +# An UNQUOTED prefix_if_nonempty:L consumes the rest of the chain as its literal, so L may contain | but the filter must come last in that placeholder. # hops_min asks about the route, pathbytes_min about how it is encoded. Use hops_min:1 to drop a # distance clause on a direct message without also dropping a measurable one-byte multi-hop path. # The distance placeholders render "N/A" on a direct message, which is non-empty, so diff --git a/docs/FEEDS.md b/docs/FEEDS.md index 5969a8a..d0bbe41 100644 --- a/docs/FEEDS.md +++ b/docs/FEEDS.md @@ -67,7 +67,7 @@ shorten_urls = false # Or shorten only where the format says {link|shorten} (see placeholders below) ``` -Per-output-format URL shortening: use `{link|shorten}` for a single shortened link, or `{link|shorten|truncate:N}` to shorten then cap length. `shorten_urls = true` shortens every plain `{link}`. +Per-output-format URL shortening: use `{link|shorten}` for a single shortened link, or `{link|shorten|truncate:N}` to shorten then cap length. `shorten_urls = true` shortens every plain `{link}`. `shorten_url` is accepted as an alias, so a filter chain copied from a command `response_format` works here unchanged. ## RSS Feed Configuration diff --git a/docs/path-command-config.md b/docs/path-command-config.md index c0324d6..b48be68 100644 --- a/docs/path-command-config.md +++ b/docs/path-command-config.md @@ -27,17 +27,18 @@ These options only affect the **path** command’s reply text and whether repeat reply_prefix = "{path_distance|prefix_if_nonempty:📏 }\n" ``` - `hops_min:N` clears a field unless the message actually travelled at least N hops. `{path_distance}` renders `N/A` on a direct message, which `prefix_if_nonempty` treats as a value, so gate it first: `{path_distance|hops_min:1|prefix_if_nonempty:📏 }`. Unlike `pathbytes_min:N`, which asks how the path is *encoded*, this keeps a measurable one-byte multi-hop path. -- `if_notempty:LITERAL` renders `LITERAL` when the value is non-empty after prior filters, and clears entirely otherwise — the opposite pairing of `prefix_if_nonempty`, useful when the whole output should be a fixed (or field-built) literal rather than the value with a label prepended. Since `{packet_hash}` is empty whenever RF correlation fails, gating on it hides the whole clause instead of printing a broken link: +- `if_nonempty:LITERAL` (alias `if_notempty`) renders `LITERAL` when the value is non-empty after prior filters, and clears entirely otherwise — the opposite pairing of `prefix_if_nonempty`, useful when the whole output should be a fixed (or field-built) literal rather than the value with a label prepended. Since `{packet_hash}` is empty whenever RF correlation fails, gating on it hides the whole clause instead of printing a broken link: ```ini -reply_prefix = {packet_hash|if_notempty:"https://scope.example.net/#/packets/{packet_hash}"} +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. -- `shorten_url` runs the value through the shared 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` where required). It falls back to the original, unshortened value if shortening fails or isn't configured, so a clause never silently disappears because of a network error. Chain it after building the link so only the final URL is sent over RF: + 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. Quoting also ends the argument at the closing quote, so more filters can follow it — including after `prefix_if_nonempty`, whose *unquoted* argument still swallows the rest of the chain so that a literal may contain `|`. +- `shorten` (alias `shorten_url`, the same filter feed formats document) runs the value through the shared 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` where required). It falls back to the original, unshortened value if shortening fails or isn't configured, so a clause never silently disappears because of a network error. Chain it after building the link so only the final URL is sent over RF: ```ini -reply_prefix = {packet_hash|if_notempty:"https://scope.example.net/#/packets/{packet_hash}"|shorten_url} +reply_prefix = {packet_hash|if_nonempty:"https://scope.example.net/#/packets/{packet_hash}"|shorten} ``` + For `shlink`, `short_url_website` is required — with no base configured the bot skips shortening rather than sending your API key to the default `gd` host. **`minimum_path_bytes`** (integer `0`–`3`, default `0`) diff --git a/modules/commands/path_command.py b/modules/commands/path_command.py index b390e6c..be39552 100644 --- a/modules/commands/path_command.py +++ b/modules/commands/path_command.py @@ -15,7 +15,7 @@ from ..path_inference import ( select_node_repeater, select_repeater_by_graph, ) -from ..response_template import format_piped_template +from ..response_template import format_piped_template_async from ..utils import ( bytes_per_hop_from_routing_and_nodes, calculate_distance, @@ -415,19 +415,24 @@ class PathCommand(BaseCommand): return '' return f"{distance:.1f}km" - def _format_path_reply_prefix(self, message: MeshMessage) -> str: + async def _format_path_reply_prefix(self, message: MeshMessage) -> str: + """Render the configured reply prefix off the event loop. + + Async because the prefix may carry a ``shorten`` filter, whose HTTP call + would otherwise block radio RX and every other handler for its full timeout. + """ if not self.path_reply_prefix: return '' fields = self.get_standard_placeholder_fields(message) fields['path_distance'] = self._format_path_distance(message) - formatted = format_piped_template( + formatted = (await format_piped_template_async( self.path_reply_prefix, {k: str(v) for k, v in fields.items()}, message=message, logger=self.logger, config=self.bot.config, prefix_hex_chars=getattr(self.bot, 'prefix_hex_chars', 2), - ).rstrip() + )).rstrip() if not formatted: return '' return formatted + '\n' @@ -1057,7 +1062,7 @@ class PathCommand(BaseCommand): async def _send_path_response(self, message: MeshMessage, response: str): """Send path response, splitting into multiple messages if necessary""" - prefix = self._format_path_reply_prefix(message) + prefix = await self._format_path_reply_prefix(message) self.last_response = prefix + response if prefix else response max_length = self.get_max_message_length(message) diff --git a/modules/feed_format.py b/modules/feed_format.py index c983e09..a3f26c1 100644 --- a/modules/feed_format.py +++ b/modules/feed_format.py @@ -107,6 +107,20 @@ def clean_feed_html_body(body: str) -> str: return body.strip() +def _canonical_shorten_name(function: str) -> str: + """Accept the response_template spelling ``shorten_url`` for ``shorten``. + + Feed formats and response templates are one operator-facing DSL as far as anyone + configuring the bot is concerned; the same operation answering to a different + name in each is a standing source of config mistakes. + """ + if function == "shorten_url": + return "shorten" + if function.startswith("shorten_url|"): + return "shorten|" + function.split("|", 1)[1] + return function + + def apply_feed_field_function( text: str, function: str, @@ -117,7 +131,7 @@ def apply_feed_field_function( """Apply a shortening, parsing, or conditional function to text. Supported functions: - - shorten - URL-shorten via [External_Data] short_url_website (v.gd / is.gd API) + - shorten (alias shorten_url) - URL-shorten via [External_Data] short_url_website - shorten|truncate:N (etc.) - shorten first, then apply the rest - truncate:N / truncate_hard:N / substr:N[,M] / word_wrap:N / first_words:N - regex:… / if_regex:… / switch:… / regex_cond:… @@ -125,6 +139,7 @@ def apply_feed_field_function( if not function or not str(function).strip(): return text or "" function = str(function).strip() + function = _canonical_shorten_name(function) def _debug(msg: str) -> None: if logger is not None: @@ -441,7 +456,7 @@ def format_feed_message( if field_name == "link": value = link_original - fn = function + fn = _canonical_shorten_name(function) if shorten_feed_urls and fn != "shorten" and not fn.startswith("shorten|"): s = shorten_url_sync( link_original, diff --git a/modules/response_template.py b/modules/response_template.py index 4dbc691..36030ca 100644 --- a/modules/response_template.py +++ b/modules/response_template.py @@ -11,6 +11,8 @@ chain, evaluated left to right. from __future__ import annotations +import asyncio +from functools import partial from typing import Any, Callable from .url_shortener import shorten_url_sync @@ -70,19 +72,52 @@ def _filter_prefix_if_nonempty(value: str, ctx: dict[str, Any], args: str) -> st return '' return args + value + +# The event-loop warning below is worth saying once, not once per reply. +_warned_blocking_render = False + + +def _on_event_loop() -> bool: + """True when called on a thread with a running asyncio loop. + + Rendering in the default executor (see :func:`format_piped_template_async`) puts + the work on a worker thread, where this is False. It is True only when a blocking + filter is about to stall the bot, which is worth saying out loud. + """ + try: + asyncio.get_running_loop() + except RuntimeError: + return False + return True + + def _filter_shorten_url(value: str, ctx: dict[str, Any], args: str) -> str: - """Shorten *value* URL using configured URL shortener (v.gd / is.gd compatible).""" + """Shorten *value* through the configured shortener (v.gd / is.gd, or Shlink). + + Falls back to *value* unchanged whenever shortening is unavailable or fails, so a + misconfigured shortener costs a longer message rather than a broken one. Needs + ``config`` on the render call; without it the value passes through untouched. + + Blocking: this issues an HTTP request. Async callers must reach it through + :func:`format_piped_template_async`, not :func:`format_piped_template`. + """ logger = ctx.get('logger') config = ctx.get('config') - if logger is not None: - logger.debug("Shortening URL %r", value) if config is None or value == '': if logger is not None: - logger.debug("Abandoning shorten url due to empty value or config") + logger.debug("Not shortening: no config on the render call, or empty value") return value + global _warned_blocking_render + if logger is not None and not _warned_blocking_render and _on_event_loop(): + _warned_blocking_render = True + logger.warning( + "Rendering a shorten filter on the event loop; the bot will stall for up " + "to the shortener timeout on each reply. Render via " + "format_piped_template_async. This is logged once." + ) return shorten_url_sync(value, config=config, logger=logger) or value -def _filter_if_notempty(value: str, ctx: dict[str, Any], args: str) -> str: +def _filter_if_nonempty(value: str, ctx: dict[str, Any], args: str) -> str: """Return *args* literal only when *value* is non-empty after prior filters.""" if not value: return '' @@ -93,14 +128,22 @@ RESPONSE_TEMPLATE_FILTERS: dict[str, FilterFn] = { 'pathbytes': _filter_pathbytes_min, 'hops_min': _filter_hops_min, 'prefix_if_nonempty': _filter_prefix_if_nonempty, - 'if_notempty': _filter_if_notempty, + 'if_nonempty': _filter_if_nonempty, + # `if_notempty` and `shorten_url` are aliases. One operation should not have two + # names in an operator-facing DSL, but feed formats already document `shorten` + # (docs/FEEDS.md) and this module already ships `prefix_if_nonempty`, so both + # spellings resolve rather than silently passing the value through. + 'if_notempty': _filter_if_nonempty, + 'shorten': _filter_shorten_url, 'shorten_url': _filter_shorten_url, } -# prefix_if_nonempty's literal argument may itself contain '|', so once the parser -# sees this filter name it stops splitting on '|' and takes everything up to the -# placeholder's closing '}' as one literal argument. It must therefore be last in -# a chain whenever its literal needs a pipe. +# prefix_if_nonempty's literal argument may itself contain '|', and shipped configs +# rely on that (config.ini.example: `prefix_if_nonempty: | Path Dist: `), so for an +# *unquoted* argument the parser stops splitting on '|' and takes everything up to +# the placeholder's closing '}' as one literal. Such a filter must be last in its +# chain. Quoting the argument (`prefix_if_nonempty:"L | "`) is the way to keep +# filtering afterwards; a quote immediately after the ':' selects that branch. _GREEDY_ARG_FILTERS = frozenset({'prefix_if_nonempty'}) @@ -226,6 +269,14 @@ class _TemplateParser: return name, '', i i += 1 # consume ':' if name in _GREEDY_ARG_FILTERS: + # A quote immediately after ':' opts into the quoted-argument grammar. + # Anything else stays greedy, so existing unquoted literals keep their + # pipes and their leading/trailing whitespace exactly as written. + if i < self.n and self.s[i] == '"': + value, j = self._parse_quoted_string(i) + if value is None: + return None, '', j + return name, value, j close = self.s.find('}', i) if close == -1: return None, '', self.n @@ -268,10 +319,15 @@ def format_piped_template( lets ``prefix_if_nonempty`` drop its literal label too. message: Triggering mesh message; required for ``pathbytes`` / ``pathbytes_min`` filters. logger: Optional logger for unknown filter warnings. + config: Bot config, required by ``shorten`` / ``shorten_url``. Without it those + filters pass the long URL through unchanged. prefix_hex_chars: Bot prefix width for inferring bytes per hop from legacy path text. Returns: Fully expanded string. + + Blocking: ``shorten`` issues an HTTP request. Call + :func:`format_piped_template_async` from async code so the event loop keeps running. """ ctx: dict[str, Any] = { 'message': message, @@ -279,6 +335,39 @@ def format_piped_template( 'prefix_hex_chars': prefix_hex_chars, 'config': config, } - if (logger is not None) and (config is not None): - logger.debug("Rendering response template %r with fields %r", template, fields) + if logger is not None: + # Field values carry sender IDs and user-supplied phrases; log the template + # being rendered and the field names, not the values. + logger.debug( + "Rendering response template %r with fields %s", template, sorted(fields) + ) return _TemplateParser(template, fields, ctx, logger).render() + + +async def format_piped_template_async( + template: str, + fields: dict[str, Any], + *, + message: Any = None, + logger: Any = None, + config: Any = None, + prefix_hex_chars: int = 2, +) -> str: + """Async wrapper for :func:`format_piped_template`, run in the default executor. + + The ``shorten`` filter makes a blocking HTTP call with a 5s timeout. Rendering a + template inline on the event loop stalls radio RX, other command handlers, MQTT + and heartbeats for that whole timeout, and the reply misses its RF window. + """ + return await asyncio.get_running_loop().run_in_executor( + None, + partial( + format_piped_template, + template, + fields, + message=message, + logger=logger, + config=config, + prefix_hex_chars=prefix_hex_chars, + ), + ) diff --git a/modules/url_shortener.py b/modules/url_shortener.py index b76de44..6d95839 100644 --- a/modules/url_shortener.py +++ b/modules/url_shortener.py @@ -2,8 +2,11 @@ """ Shared URL shortening for MeshCore Bot and web viewer. -Uses the v.gd / is.gd-compatible API (GET .../create.php?format=simple&url=...). -Configure base URL and optional API key under [External_Data] in config.ini. +Two backends are supported, selected by ``short_url_website_service``: +``gd`` (default) uses the v.gd / is.gd-compatible API +(GET .../create.php?format=simple&url=...), and ``shlink`` POSTs to a self-hosted +Shlink instance's /rest/v3/short-urls with an ``X-Api-Key`` header. Configure the +base URL, service, and optional API key under [External_Data] in config.ini. """ from __future__ import annotations @@ -66,10 +69,25 @@ def _normalize_base(base: str) -> str: return b if b else DEFAULT_SHORT_URL_BASE +def _is_vgd_compat_host(host: str) -> bool: + """True for the public v.gd / is.gd hosts, which take no API key.""" + return (host or "").lower().split(":")[0] in _VGD_COMPAT_HOSTS + + def _host_allows_key_in_query(host: str) -> bool: """True if we may append api_key for this host. v.gd/is.gd public API: False.""" - h = (host or "").lower().split(":")[0] - return h not in _VGD_COMPAT_HOSTS + return not _is_vgd_compat_host(host) + + +def _base_host(base: str) -> str: + """Hostname of *base*, tolerating a scheme-less value like ``example.com/x``.""" + from urllib.parse import urlparse + + root = (base or "").strip() + if "://" not in root: + root = f"https://{root}" + parsed = urlparse(root) + return (parsed.hostname or "").lower() def _parse_simple_response(body: str) -> str | None: @@ -104,7 +122,12 @@ def _build_create_gd_url(long_url: str, base: str, api_key: str) -> str: return rebuilt -def _build_create_shlink_url(long_url: str, base: str, api_key: str) -> str: +def _build_create_shlink_url(base: str) -> str: + """Build the Shlink create endpoint from *base*. + + Only the base is needed: the long URL travels in the POST body and the API key + in an ``X-Api-Key`` header, never in the URL. + """ from urllib.parse import urlparse, urlunparse root = _normalize_base(base) @@ -115,11 +138,7 @@ def _build_create_shlink_url(long_url: str, base: str, api_key: str) -> str: if not netloc and parsed.path: netloc = parsed.path.split("/")[0] path = (parsed.path or "").rstrip("/") + "/rest/v3/short-urls" - if not path.startswith("/"): - path = "/" + path - query = "" - rebuilt = urlunparse((parsed.scheme or "https", netloc, path, "", query, "")) - return rebuilt + return urlunparse((parsed.scheme or "https", netloc, path, "", "", "")) def _shorten_url_with_shlink( @@ -133,7 +152,7 @@ def _shorten_url_with_shlink( """Shorten a URL using Shlink API.""" import json - shortener_url = _build_create_shlink_url(long_url, base, api_key) + shortener_url = _build_create_shlink_url(base) headers = { "Content-Type": "application/json", "X-Api-Key": api_key, @@ -142,16 +161,36 @@ def _shorten_url_with_shlink( {"longUrl": long_url, "findIfExists": True, "tags": ["meshcore-bot"]} ) - get = session.post if session is not None else requests.post - response = get(shortener_url, headers=headers, data=payload, timeout=timeout) + post = session.post if session is not None else requests.post + response = post(shortener_url, headers=headers, data=payload, timeout=timeout) if logger: logger.debug("Shlink response: %s", response.text) - data = response.json() - short_url = data.get("shortUrl") or data.get("shortUrlSlug") + # Shlink reports failures as RFC 7807 problem details, which parse as JSON just + # fine and simply lack shortUrl. Without this check a bad API key looks + # identical to a URL that could not be shortened, at DEBUG only. + if not response.ok: + if logger: + logger.warning( + "Shlink shortener returned HTTP %s: %s", + getattr(response, "status_code", "?"), + (response.text or "")[:200], + ) + return "" + + try: + data = response.json() + except ValueError: + if logger: + logger.warning("Shlink shortener returned a non-JSON body; not shortening.") + return "" + + short_url = (data or {}).get("shortUrl") if short_url: - return short_url + return str(short_url) + if logger: + logger.warning("Shlink response carried no shortUrl; not shortening.") return "" @@ -169,6 +208,15 @@ def _shorten_url_with_gd( get = session.get if session is not None else requests.get response = get(shortener_url, timeout=timeout) + # A failing proxy or maintenance page can return a body that looks like a URL. + # Without this check that body is returned as the short link and transmitted. + if not response.ok: + if logger: + logger.warning( + "URL shortener returned HTTP %s", getattr(response, "status_code", "?") + ) + return "" + short = _parse_simple_response(response.text) if short: return short @@ -193,7 +241,7 @@ def shorten_url_sync( if not url_str: return "" - base = _safe_config_get(config, "External_Data", "short_url_website", "") + raw_base = _safe_config_get(config, "External_Data", "short_url_website", "") service = ( _safe_config_get(config, "External_Data", "short_url_website_service", "gd") .strip() @@ -203,15 +251,32 @@ def shorten_url_sync( _safe_config_get(config, "External_Data", "short_url_website_api_key", "") or "" ).strip() - base = _normalize_base(base) if service == "shlink": + # Deliberately not _normalize_base: its v.gd fallback would POST the + # operator's API key to an unrelated third party when the base is unset. + base = (raw_base or "").strip().rstrip("/") + if not base: + if logger: + logger.warning( + "short_url_website_service=shlink requires short_url_website " + "(there is no default Shlink instance); skipping." + ) + return "" if not api_key: if logger: logger.warning( "short_url_website_service=shlink requires short_url_website_api_key; skipping." ) return "" + if _is_vgd_compat_host(_base_host(base)): + if logger: + logger.warning( + "short_url_website_service=shlink points at the public %s API, " + "which is not Shlink; skipping rather than sending the API key there.", + _base_host(base), + ) + return "" return _shorten_url_with_shlink( url_str, base, @@ -225,13 +290,19 @@ def shorten_url_sync( # only appended for self-hosted alternates via _host_allows_key_in_query). return _shorten_url_with_gd( url_str, - base, + _normalize_base(raw_base), api_key, session=session, timeout=timeout, logger=logger, ) + except (requests.exceptions.Timeout, requests.exceptions.ConnectionError) as e: + # Routine on a mesh node with an intermittent uplink. Logging these at ERROR + # as "unexpected" floods the log and buries the errors that do need triage. + if logger: + logger.debug("URL shortener unreachable: %s", e) + return "" except Exception as e: if logger: logger.error("Unexpected error shortening URL: %s", e) diff --git a/tests/test_url_shortener.py b/tests/test_url_shortener.py index 8829a91..72f92fc 100644 --- a/tests/test_url_shortener.py +++ b/tests/test_url_shortener.py @@ -43,21 +43,20 @@ class TestBuildCreateGdUrl: class TestBuildCreateShlinkUrl: def test_appends_rest_v3_short_urls_path(self): - u = _build_create_shlink_url("http://a.com", "https://short.example", "k1") + u = _build_create_shlink_url("https://short.example") assert u == "https://short.example/rest/v3/short-urls" def test_strips_trailing_slash_on_base(self): - u = _build_create_shlink_url("http://a.com", "https://short.example/", "k1") + u = _build_create_shlink_url("https://short.example/") assert u == "https://short.example/rest/v3/short-urls" def test_bare_hostname_gets_https_scheme(self): - u = _build_create_shlink_url("http://a.com", "short.example", "k1") + u = _build_create_shlink_url("short.example") assert u == "https://short.example/rest/v3/short-urls" - def test_api_key_never_appears_in_the_url(self): - """Shlink authenticates via the X-Api-Key header, not a query param.""" - u = _build_create_shlink_url("http://a.com", "https://short.example", "super-secret") - assert "super-secret" not in u + def test_preserves_a_path_prefix(self): + u = _build_create_shlink_url("https://short.example/shlink") + assert u == "https://short.example/shlink/rest/v3/short-urls" class TestCoerceUrlString: @@ -185,15 +184,47 @@ class TestShortenUrlSync: assert call_url.startswith("https://v.gd/create.php") def test_http_error_returns_empty(self): + """A failing response body must never be returned as the short URL. + + The body here is what a reverse proxy in front of a self-hosted is.gd-compatible + shortener actually serves on 502 -- it parses as a URL, so without the + response.ok guard it would be transmitted over RF as the shortened link. + """ cfg = _minimal_config() mock_resp = MagicMock() mock_resp.ok = False - mock_resp.status_code = 503 + mock_resp.status_code = 502 + mock_resp.text = "http://short.example/maintenance" session = MagicMock() session.get.return_value = mock_resp assert shorten_url_sync("http://a.com", config=cfg, session=session) == "" + def test_http_error_is_logged_at_warning(self): + cfg = _minimal_config() + mock_resp = MagicMock() + mock_resp.ok = False + mock_resp.status_code = 502 + mock_resp.text = "http://short.example/maintenance" + session = MagicMock() + session.get.return_value = mock_resp + logger = MagicMock() + + shorten_url_sync("http://a.com", config=cfg, session=session, logger=logger) + + assert logger.warning.called + + def test_timeout_is_not_logged_as_an_unexpected_error(self): + """A shortener timeout is expected on an intermittent uplink, not an error.""" + cfg = _minimal_config() + session = MagicMock() + session.get.side_effect = requests.exceptions.Timeout("timed out") + logger = MagicMock() + + assert shorten_url_sync("http://a.com", config=cfg, session=session, logger=logger) == "" + logger.error.assert_not_called() + assert logger.debug.called + def test_default_base_when_keys_missing(self): cfg = _minimal_config() mock_resp = MagicMock() @@ -243,6 +274,7 @@ class TestShortenUrlSyncShlink: def test_success_returns_short_url(self): cfg = self._shlink_config() mock_resp = MagicMock() + mock_resp.ok = True mock_resp.json.return_value = {"shortUrl": "https://short.example/abc123"} session = MagicMock() session.post.return_value = mock_resp @@ -255,6 +287,7 @@ class TestShortenUrlSyncShlink: def test_posts_to_rest_v3_short_urls_with_api_key_header(self): cfg = self._shlink_config() mock_resp = MagicMock() + mock_resp.ok = True mock_resp.json.return_value = {"shortUrl": "https://short.example/abc123"} session = MagicMock() session.post.return_value = mock_resp @@ -269,19 +302,85 @@ class TestShortenUrlSyncShlink: assert payload["longUrl"] == "https://example.com/long/path" assert payload["findIfExists"] is True - def test_falls_back_to_short_url_slug(self): + def test_bare_slug_in_response_is_not_treated_as_a_url(self): + """Shlink's create response carries shortUrl; a slug is not a link. + + Returning one would put an unclickable `abc123` where the reply expects a URL. + """ cfg = self._shlink_config() mock_resp = MagicMock() - mock_resp.json.return_value = {"shortUrlSlug": "abc123"} + mock_resp.ok = True + mock_resp.json.return_value = {"shortUrlSlug": "abc123", "shortCode": "abc123"} session = MagicMock() session.post.return_value = mock_resp - out = shorten_url_sync("http://a.com", config=cfg, session=session) - assert out == "abc123" + assert shorten_url_sync("http://a.com", config=cfg, session=session) == "" + + def test_missing_base_does_not_send_the_api_key_anywhere(self): + """Regression: an unset base must not fall back to the public v.gd default. + + _normalize_base defaults to https://v.gd, so without an explicit guard a + shlink deployment with no short_url_website POSTs the operator's API key to + an unrelated third party. + """ + cfg = self._shlink_config(short_url_website="") + session = MagicMock() + logger = MagicMock() + + out = shorten_url_sync("http://a.com", config=cfg, session=session, logger=logger) + + assert out == "" + session.post.assert_not_called() + assert logger.warning.called + + def test_base_pointing_at_the_public_vgd_api_is_refused(self): + """v.gd is not Shlink; sending it an X-Api-Key only discloses the key.""" + cfg = self._shlink_config(short_url_website="https://v.gd") + session = MagicMock() + + assert shorten_url_sync("http://a.com", config=cfg, session=session) == "" + session.post.assert_not_called() + + def test_http_error_returns_empty_and_warns(self): + """A bad API key must be diagnosable above DEBUG. + + Shlink reports failures as RFC 7807 problem details, which parse as JSON and + simply lack shortUrl -- indistinguishable from an unshortenable URL without + checking the status. + """ + cfg = self._shlink_config() + mock_resp = MagicMock() + mock_resp.ok = False + mock_resp.status_code = 401 + mock_resp.text = '{"title": "Invalid API key", "status": 401}' + session = MagicMock() + session.post.return_value = mock_resp + logger = MagicMock() + + out = shorten_url_sync("http://a.com", config=cfg, session=session, logger=logger) + + assert out == "" + assert logger.warning.called + + def test_http_error_does_not_leak_the_api_key_into_the_log(self): + cfg = self._shlink_config() + mock_resp = MagicMock() + mock_resp.ok = False + mock_resp.status_code = 401 + mock_resp.text = "denied" + session = MagicMock() + session.post.return_value = mock_resp + logger = MagicMock() + + shorten_url_sync("http://a.com", config=cfg, session=session, logger=logger) + + logged = " ".join(str(c) for c in logger.warning.call_args_list) + assert "test-api-key" not in logged def test_missing_short_url_in_response_returns_empty(self): cfg = self._shlink_config() mock_resp = MagicMock() + mock_resp.ok = True mock_resp.json.return_value = {"unexpected": "shape"} session = MagicMock() session.post.return_value = mock_resp @@ -309,6 +408,7 @@ class TestShortenUrlSyncShlink: def test_malformed_json_response_returns_empty(self): cfg = self._shlink_config() mock_resp = MagicMock() + mock_resp.ok = True mock_resp.json.side_effect = ValueError("not json") session = MagicMock() session.post.return_value = mock_resp @@ -319,6 +419,7 @@ class TestShortenUrlSyncShlink: def test_no_session_uses_requests_post(self, mock_post): cfg = self._shlink_config() mock_resp = MagicMock() + mock_resp.ok = True mock_resp.json.return_value = {"shortUrl": "https://short.example/xyz"} mock_post.return_value = mock_resp @@ -352,6 +453,7 @@ async def test_shorten_url_async_shlink(): short_url_website_api_key="test-api-key", ) mock_resp = MagicMock() + mock_resp.ok = True mock_resp.json.return_value = {"shortUrl": "https://short.example/async1"} session = MagicMock() session.post.return_value = mock_resp diff --git a/tests/unit/test_response_template.py b/tests/unit/test_response_template.py index 56a9e9b..9bb63b2 100644 --- a/tests/unit/test_response_template.py +++ b/tests/unit/test_response_template.py @@ -8,7 +8,7 @@ import pytest from modules.commands.test_command import TestCommand as MeshTestCommand from modules.models import MeshMessage -from modules.response_template import format_piped_template +from modules.response_template import format_piped_template, format_piped_template_async from modules.utils import message_path_bytes_per_hop @@ -402,3 +402,181 @@ def test_quoted_filter_argument_with_a_nested_placeholder_does_not_close_early() assert format_piped_template(template, {"packet_hash": "ABCDEF12"}) == ( "https://analyzer.example.net/#/packets/ABCDEF12?obs=1620457" ) + + +def _shortener_config(**external_data): + """Config a real shorten_url_sync call will accept, defaulting to v.gd.""" + c = configparser.ConfigParser() + c["External_Data"] = {} + for k, v in external_data.items(): + c["External_Data"][k] = v + return c + + +@pytest.mark.unit +def test_prefix_if_nonempty_accepts_a_quoted_argument_with_a_nested_placeholder(): + """Regression: the greedy branch used to win over the quoted-argument grammar. + + prefix_if_nonempty is the one filter already in shipped configs, so without this + the documented quoted syntax emitted raw template text over RF instead. + """ + out = format_piped_template( + '{path_distance|prefix_if_nonempty:"Dist {sender}: "}', + {"path_distance": "5km", "sender": "y"}, + ) + assert out == "Dist y: 5km" + + +@pytest.mark.unit +def test_prefix_if_nonempty_with_a_quoted_argument_can_be_chained(): + """A quoted arg ends at its closing quote, so a later filter is not swallowed.""" + out = format_piped_template( + '{d|prefix_if_nonempty:"L "|if_nonempty:Z}', + {"d": "5km"}, + ) + assert out == "Z" + + +@pytest.mark.unit +def test_prefix_if_nonempty_keeps_greedy_parsing_for_unquoted_literals(): + """config.ini.example ships `prefix_if_nonempty: | Path Dist: ` -- a literal + containing a pipe, which only parses if unquoted args stay greedy.""" + out = format_piped_template( + "ack{path_distance|pathbytes_min:2|prefix_if_nonempty: | Path Dist: }", + {"path_distance": "12.4km"}, + message=_msg(path="0102 (1 hop)", hops=1, + routing_info={"path_length": 1, "bytes_per_hop": 2}), + ) + assert out == "ack | Path Dist: 12.4km" + + +@pytest.mark.unit +def test_shorten_url_filter_shortens_through_the_configured_service(): + """End-to-end: the filter reaches shorten_url_sync via ctx['config'].""" + cfg = _shortener_config(short_url_website="https://v.gd") + resp = MagicMock() + resp.ok = True + resp.text = "https://v.gd/abc123" + + with pytest.MonkeyPatch.context() as mp: + import modules.url_shortener as us + mp.setattr(us.requests, "get", lambda *a, **k: resp) + out = format_piped_template( + "{link|shorten_url}", + {"link": "https://example.com/a/very/long/path"}, + config=cfg, + ) + assert out == "https://v.gd/abc123" + + +@pytest.mark.unit +def test_shorten_is_an_alias_for_shorten_url(): + """Feed formats document `shorten`; the same name must work here.""" + cfg = _shortener_config(short_url_website="https://v.gd") + resp = MagicMock() + resp.ok = True + resp.text = "https://v.gd/abc123" + + with pytest.MonkeyPatch.context() as mp: + import modules.url_shortener as us + mp.setattr(us.requests, "get", lambda *a, **k: resp) + out = format_piped_template( + "{link|shorten}", {"link": "https://example.com/long"}, config=cfg + ) + assert out == "https://v.gd/abc123" + + +@pytest.mark.unit +def test_shorten_url_falls_back_to_the_long_url_when_shortening_fails(): + """A failing shortener costs a longer message, never a broken one.""" + cfg = _shortener_config(short_url_website="https://v.gd") + resp = MagicMock() + resp.ok = False + resp.status_code = 502 + resp.text = "http://short.example/maintenance" + + with pytest.MonkeyPatch.context() as mp: + import modules.url_shortener as us + mp.setattr(us.requests, "get", lambda *a, **k: resp) + out = format_piped_template( + "{link|shorten_url}", {"link": "https://example.com/long"}, config=cfg + ) + assert out == "https://example.com/long" + + +@pytest.mark.unit +def test_shorten_url_without_a_config_passes_the_value_through(): + out = format_piped_template("{link|shorten_url}", {"link": "https://example.com/long"}) + assert out == "https://example.com/long" + + +@pytest.mark.unit +def test_if_notempty_is_an_alias_for_if_nonempty(): + assert format_piped_template("{d|if_nonempty:Z}", {"d": "x"}) == "Z" + assert format_piped_template("{d|if_notempty:Z}", {"d": "x"}) == "Z" + assert format_piped_template("{d|if_nonempty:Z}", {"d": ""}) == "" + assert format_piped_template("{d|if_notempty:Z}", {"d": ""}) == "" + + +@pytest.mark.asyncio +async def test_format_piped_template_async_matches_the_sync_render(): + """The async wrapper exists so a shorten filter cannot block the event loop.""" + cfg = _shortener_config(short_url_website="https://v.gd") + resp = MagicMock() + resp.ok = True + resp.text = "https://v.gd/abc123" + + with pytest.MonkeyPatch.context() as mp: + import modules.url_shortener as us + mp.setattr(us.requests, "get", lambda *a, **k: resp) + out = await format_piped_template_async( + "{link|shorten_url}", {"link": "https://example.com/long"}, config=cfg + ) + assert out == "https://v.gd/abc123" + + +@pytest.mark.asyncio +async def test_shorten_url_warns_when_rendered_on_the_event_loop(): + """A blocking HTTP call on the loop stalls radio RX and every other handler.""" + cfg = _shortener_config(short_url_website="https://v.gd") + resp = MagicMock() + resp.ok = True + resp.text = "https://v.gd/abc123" + logger = MagicMock() + + with pytest.MonkeyPatch.context() as mp: + import modules.response_template as rt + import modules.url_shortener as us + mp.setattr(us.requests, "get", lambda *a, **k: resp) + mp.setattr(rt, "_warned_blocking_render", False) + format_piped_template( + "{link|shorten_url}", + {"link": "https://example.com/long"}, + config=cfg, + logger=logger, + ) + + warned = " ".join(str(c) for c in logger.warning.call_args_list) + assert "event loop" in warned + + +@pytest.mark.asyncio +async def test_async_render_does_not_warn_about_the_event_loop(): + cfg = _shortener_config(short_url_website="https://v.gd") + resp = MagicMock() + resp.ok = True + resp.text = "https://v.gd/abc123" + logger = MagicMock() + + with pytest.MonkeyPatch.context() as mp: + import modules.url_shortener as us + mp.setattr(us.requests, "get", lambda *a, **k: resp) + out = await format_piped_template_async( + "{link|shorten_url}", + {"link": "https://example.com/long"}, + config=cfg, + logger=logger, + ) + + assert out == "https://v.gd/abc123" + logger.warning.assert_not_called()