From bf6d37e6210e06b8b09687052b40d99df6e96beb Mon Sep 17 00:00:00 2001 From: flan Date: Mon, 13 Jul 2026 16:52:23 +0000 Subject: [PATCH] v0.5.1: the update alert could have broken middlewared at startup middlewared's alert.load() imports every file in alert/source/ with NO try/except: def load(self): for module in load_modules(.../alert/source): for cls in load_classes(module, AlertSource, (ThreadedAlertSource,)): ... and it runs during setup. A module that raises on import therefore takes middlewared's startup down with it -- exactly the class of failure this project exists to avoid. apply.sh now COMPILES the substituted alert source and refuses to write it if it does not parse. An uninstalled alert is a missing convenience; a broken one is a broken box. @PATCH_DIR@ is also substituted with repr() rather than raw, so a repository path containing a quote or backslash yields a valid Python literal instead of a syntax error in the installed module. The alert source no longer mutates sys.path. It loaded tools/release_notes.py via sys.path.insert(0, ...), which shadows the stdlib for that interpreter -- and ThreadedAlertSource runs in middlewared's thread pool, so mutating sys.path is a race. It now loads by file path with importlib. New tests guard every import-time failure mode: the module compiles, apply.sh compiles before writing, awkward paths (quotes, backslashes) still produce valid modules, nothing but imports/constants/classes runs at module scope, every AlertClass name ends in "AlertClass" (AlertClassMeta raises NameError otherwise), the alert text placeholders match the args passed, and no git command that writes to .git is ever used. 152 tests, ruff and shellcheck -S style clean. --- CHANGELOG.md | 28 ++++++++ install.sh | 2 +- patch/alert_source.py | 27 ++++++-- patch/apply.sh | 17 ++++- patch/create_task.py | 2 +- recover.sh | 2 +- tests/test_alert_source.py | 132 +++++++++++++++++++++++++++++++++++++ uninstall.sh | 2 +- update.sh | 2 +- 9 files changed, 200 insertions(+), 14 deletions(-) create mode 100644 tests/test_alert_source.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 89a8fbe..6010119 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,33 @@ # Changelog +## v0.5.1 — 2026-07-13 + +### Fixed + +- **The update alert could have broken middlewared at startup.** middlewared's + `alert.load()` imports every file in `alert/source/` with **no try/except**, and + it runs during setup — so a module that raises on import takes middlewared down + with it. `apply.sh` now **compiles the substituted alert source and refuses to + write it** if it does not parse. An uninstalled alert is a missing convenience; + a broken one is a broken box. + +- **`@PATCH_DIR@` is substituted with `repr()`**, so a repository path containing + a quote or a backslash produces a valid Python literal instead of a syntax error + in the installed module. + +- **The alert source no longer mutates `sys.path`.** It loaded + `tools/release_notes.py` via `sys.path.insert(0, …)`, which shadows the stdlib + for that interpreter — and `ThreadedAlertSource` runs in middlewared's thread + pool, so mutating `sys.path` is a race. It now loads the module by file path with + `importlib`. + +### Notes + +Timing, for the record: `process_alerts` is `@periodic(60)` and +`alert_source_last_run` is in-memory, so the check runs **within 60 seconds of any +middlewared restart** (which this patch performs at every boot) and otherwise +**within 24 hours** of a release. + ## v0.5.0 — 2026-07-13 ### Added diff --git a/install.sh b/install.sh index 391682b..f15c581 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ set -euo pipefail -VERSION="0.5.0" +VERSION="0.5.1" # The directory containing install.sh is the permanent install location. PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" diff --git a/patch/alert_source.py b/patch/alert_source.py index ab9b4ed..7980c73 100644 --- a/patch/alert_source.py +++ b/patch/alert_source.py @@ -20,11 +20,11 @@ the way a `git fetch` from middlewared (running as root) would. """ import datetime +import importlib.util import logging import os import re import subprocess -import sys import urllib.request from middlewared.alert.base import ( @@ -102,11 +102,10 @@ class TrueCloudPatchUpdateAlertSource(ThreadedAlertSource): if not latest: return None - sys.path.insert(0, os.path.join(PATCH_DIR, "tools")) - try: - from release_notes import significance, version_tuple - finally: - sys.path.pop(0) + rn = self._release_notes() + if rn is None: + return None + significance, version_tuple = rn.significance, rn.version_tuple if version_tuple(latest) <= version_tuple(current): return None @@ -136,6 +135,22 @@ class TrueCloudPatchUpdateAlertSource(ThreadedAlertSource): ) return Alert(klass, args, key=[current, latest]) + def _release_notes(self): + """Load tools/release_notes.py by path. + + NOT via sys.path: prepending would shadow the stdlib for this interpreter, + and this runs in middlewared's thread pool, so mutating sys.path is a race. + """ + path = os.path.join(PATCH_DIR, "tools", "release_notes.py") + try: + spec = importlib.util.spec_from_file_location("_tc_release_notes", path) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + except Exception: + logger.debug("could not load release_notes", exc_info=True) + return None + def _installed_version(self): """The version of the patch actually checked out here.""" try: diff --git a/patch/apply.sh b/patch/apply.sh index a042eec..fbd185c 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -32,7 +32,7 @@ # Derive PATCH_DIR from this script's location (parent of the patch/ directory). PATCH_DIR="$(cd "$(dirname "$0")/.." && pwd)" LOG="$PATCH_DIR/apply.log" -VERSION="0.5.0" +VERSION="0.5.1" # Rotate log at 512 KB to avoid unbounded growth on a system volume. # Keep two prior generations (.1 and .2) so the last three boots are always available. @@ -602,15 +602,26 @@ else: try: with open(alert_src, encoding='utf-8') as fh: _body = fh.read() - # PATCH_DIR is baked in: the alert source must find the repo it belongs to. + + # repr() so ANY path becomes a valid Python literal -- a directory + # containing a quote or backslash would otherwise produce a syntax error. + _body = _body.replace('"@PATCH_DIR@"', repr(_patch_dir)) + + # COMPILE BEFORE WRITING. middlewared's alert.load() imports every file in + # alert/source/ with NO try/except, and it runs at startup -- a module that + # raises on import takes middlewared's setup down with it. An uninstalled + # alert is a missing convenience; a broken one is a broken box. + compile(_body, alert_dst, 'exec') + with open(alert_dst, 'w', encoding='utf-8') as fh: - fh.write(_body.replace('@PATCH_DIR@', _patch_dir)) + fh.write(_body) alert_ok = True alert_detail = 'update alert installed' print(f'OK: Installed update alert → {alert_dst}') except Exception as e: alert_detail = f'not applied: {e}' print(f'WARNING: could not install update alert: {e}') + print('WARNING: no update alert; everything else is unaffected.') patches = { 'providers': { diff --git a/patch/create_task.py b/patch/create_task.py index bc44258..0d398c4 100755 --- a/patch/create_task.py +++ b/patch/create_task.py @@ -52,7 +52,7 @@ import subprocess import sys import time -__version__ = "0.5.0" +__version__ = "0.5.1" _PATCH_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) _STATUS_FILE = os.path.join(_PATCH_DIR, "hook_status.json") diff --git a/recover.sh b/recover.sh index 08a6c35..3173425 100755 --- a/recover.sh +++ b/recover.sh @@ -17,7 +17,7 @@ # bash /mnt/tank/truenas-truecloud-patch/patch/apply.sh # systemctl restart middlewared -VERSION="0.5.0" +VERSION="0.5.1" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" diff --git a/tests/test_alert_source.py b/tests/test_alert_source.py new file mode 100644 index 0000000..a8d337f --- /dev/null +++ b/tests/test_alert_source.py @@ -0,0 +1,132 @@ +"""Tests for the update-available alert source. + +This file is imported by middlewared's `alert.load()`, which runs at STARTUP and +has **no try/except**: + + def load(self): + for module in load_modules(.../alert/source): + for cls in load_classes(module, AlertSource, (ThreadedAlertSource,)): + source = cls(self.middleware) + if source.name in ALERT_SOURCES: + raise RuntimeError(...) + +So a module that raises on import takes middlewared's setup down with it. These +tests guard the realistic ways that could happen. +""" + +import ast +import os +import re + +import pytest + +ALERT_SRC = os.path.join(os.path.dirname(__file__), "..", "patch", "alert_source.py") +APPLY_SH = os.path.join(os.path.dirname(__file__), "..", "patch", "apply.sh") + + +def source(): + with open(ALERT_SRC, encoding="utf-8") as fh: + return fh.read() + + +def tree(): + return ast.parse(source()) + + +class TestCannotBreakMiddlewaredAtImport: + def test_it_compiles(self): + compile(source(), "alert_source.py", "exec") + + def test_apply_sh_compiles_it_before_writing_it(self): + # The substituted file is what middlewared imports. If it does not compile, + # installing it would break startup — so apply.sh must refuse to write it. + with open(APPLY_SH, encoding="utf-8") as fh: + sh = fh.read() + i = sh.index("alert_dst = os.path.join(mw_dir, 'alert', 'source'") + block = sh[i:i + 1600] + assert "compile(_body, alert_dst, 'exec')" in block + assert block.index("compile(_body") < block.index("open(alert_dst, 'w'") + + def test_patch_dir_is_substituted_with_repr(self): + # A directory containing a quote or backslash would otherwise produce a + # syntax error in the installed module. + with open(APPLY_SH, encoding="utf-8") as fh: + sh = fh.read() + assert "_body.replace('\"@PATCH_DIR@\"', repr(_patch_dir))" in sh + + @pytest.mark.parametrize("path", [ + "/mnt/tank/patch", + '/mnt/we"ird/patch', # a quote in the path + "/mnt/back\\slash/patch", # a backslash + ]) + def test_substituted_module_compiles_for_awkward_paths(self, path): + body = source().replace('"@PATCH_DIR@"', repr(path)) + compile(body, "alert_source.py", "exec") # must not raise + + def test_no_io_at_module_import_time(self): + # Anything at module scope runs during alert.load(). Only imports, + # constants and class definitions are allowed. + allowed = (ast.Import, ast.ImportFrom, ast.Assign, ast.AnnAssign, + ast.ClassDef, ast.FunctionDef, ast.Expr) + for node in tree().body: + assert isinstance(node, allowed), f"module-level {type(node).__name__}" + if isinstance(node, ast.Expr): + assert isinstance(node.value, ast.Constant), "only the docstring" + + +class TestAlertClassNaming: + """middlewared's AlertClassMeta raises NameError unless the name ends in + 'AlertClass' — at import, inside alert.load(), which has no try/except.""" + + def alert_classes(self): + return [n for n in tree().body + if isinstance(n, ast.ClassDef) + and any(getattr(b, "id", "") == "AlertClass" for b in n.bases)] + + def test_there_are_alert_classes(self): + assert self.alert_classes() + + def test_every_alert_class_name_ends_in_AlertClass(self): + for cls in self.alert_classes(): + assert cls.name.endswith("AlertClass"), ( + f"{cls.name}: AlertClassMeta raises NameError on this" + ) + + def test_every_alert_class_defines_the_required_attrs(self): + # category/level/title are NotImplemented on the base; a missing one shows + # up as a broken alert rather than an error. + for cls in self.alert_classes(): + names = {t.id for n in cls.body if isinstance(n, ast.Assign) + for t in n.targets if isinstance(t, ast.Name)} + assert {"category", "level", "title", "text"} <= names, cls.name + + def test_alert_text_placeholders_match_the_args_we_pass(self): + src = source() + placeholders = set(re.findall(r"%\((\w+)\)s", src)) + # These are the keys built in _check(). + assert placeholders <= {"current", "latest", "summary", "dir"} + + +class TestNoSysPathMutation: + def test_release_notes_is_loaded_by_path_not_sys_path(self): + # sys.path.insert(0, ...) would shadow the stdlib for this interpreter, and + # ThreadedAlertSource runs in a thread pool — mutating sys.path is a race. + # + # Check for actual MUTATION, not the string: the docstring legitimately + # mentions sys.path to explain why it is avoided. + src = source() + assert "sys.path.insert" not in src + assert "sys.path.append" not in src + assert not re.search(r"^import sys$", src, re.M), "sys is not needed" + assert "spec_from_file_location" in src + + +class TestNeverWritesToGit: + def test_only_read_only_git_commands(self): + # A `git fetch` from middlewared (running as root) would leave root-owned + # objects in .git and break every later non-root git command — which is + # exactly the breakage this project already hit once. + src = source() + for forbidden in ("fetch", "pull", "checkout", "clone", "reset"): + assert f'"{forbidden}"' not in src, f"git {forbidden} writes to .git" + assert '"ls-remote"' in src diff --git a/uninstall.sh b/uninstall.sh index 827b60b..f70a2cf 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -3,7 +3,7 @@ set -euo pipefail -VERSION="0.5.0" +VERSION="0.5.1" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" _HOOK_COMMENT='TrueCloud provider patch (S3/B2)' diff --git a/update.sh b/update.sh index e1d2264..979d54b 100644 --- a/update.sh +++ b/update.sh @@ -19,7 +19,7 @@ set -euo pipefail -VERSION="0.5.0" +VERSION="0.5.1" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" _PREV_FILE="$PATCH_DIR/.update_previous"