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"