diff --git a/scripts/install_service.sh b/scripts/install_service.sh index a349f1d..f1803c6 100755 --- a/scripts/install_service.sh +++ b/scripts/install_service.sh @@ -52,12 +52,19 @@ bot_port() { # Installed here rather than documented, per the repo's own rule that a # script owns its dependencies. ensure_exiftool() { - if command -v exiftool >/dev/null; then - return 0 + if ! command -v exiftool >/dev/null; then + echo "Installing perl-image-exiftool (needed by !add_photo)..." + if ! sudo pacman -S --needed --noconfirm perl-image-exiftool; then + die "could not install perl-image-exiftool; run: sudo pacman -S perl-image-exiftool" + fi fi - echo "Installing perl-image-exiftool (needed by !add_photo)..." - if ! sudo pacman -S --needed --noconfirm perl-image-exiftool; then - die "could not install perl-image-exiftool; run: sudo pacman -S perl-image-exiftool" + # `command -v` answers for *this shell*, which is not who has to find it. + # Arch installs to /usr/bin/vendor_perl, on an interactive PATH and not on + # a systemd unit's -- so on 2026-08-31 the bot refused every upload while + # exiftool sat plainly on the installer's PATH. Ask the bot's own resolver. + if ! env -i "PATH=/usr/local/bin:/usr/bin" "${REPO_ROOT}/.venv/bin/python" -c \ + 'import sys; from signal_bot.media_strip import locate; sys.exit(0 if locate() else 1)'; then + die "exiftool is installed but signal_bot.media_strip.locate() cannot find it under the unit's PATH" fi } diff --git a/signal_bot/app.py b/signal_bot/app.py index 5320fb2..4424c88 100644 --- a/signal_bot/app.py +++ b/signal_bot/app.py @@ -74,10 +74,17 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: class SelfTestRequest(BaseModel): - """One command to type at the bot, and where.""" + """One command to type at the bot, where, and what it carries. + + ``attachments`` are ids the container's own attachment store already + holds. That is how !add_photo gets verified without a second human: the + handler fetches them back over the real API, so the ingest path exercised + is the live one rather than a hand-fed buffer. + """ group: str text: str + attachments: list[str] = [] app = FastAPI(lifespan=lifespan) @@ -125,7 +132,13 @@ async def selftest(request: SelfTestRequest, response: Response) -> dict[str, st response.status_code = status.HTTP_503_SERVICE_UNAVAILABLE return {"status": "connecting", "reason": starter.reason} try: - group_id = await run_selftest(runtime.bot, app.state.client, request.group, request.text) + group_id = await run_selftest( + runtime.bot, + app.state.client, + request.group, + request.text, + attachments=request.attachments, + ) except SelfTestError as refusal: response.status_code = status.HTTP_403_FORBIDDEN return {"status": "refused", "reason": str(refusal)} diff --git a/signal_bot/media_strip.py b/signal_bot/media_strip.py index e5f145d..71fa24b 100644 --- a/signal_bot/media_strip.py +++ b/signal_bot/media_strip.py @@ -26,6 +26,7 @@ shell out, and ``scripts/e2e.sh`` runs the real one against generated media. from __future__ import annotations import logging +import os import re import shutil import tempfile @@ -42,6 +43,21 @@ logger = logging.getLogger(__name__) EXIFTOOL: Final = "exiftool" +# Where to look when PATH does not have it. Arch's perl-image-exiftool +# installs to /usr/bin/vendor_perl, which is on an interactive shell's PATH +# (perl's profile.d adds it) and *not* on a systemd unit's, whose default is +# "/usr/local/bin:/usr/bin". Found the hard way on 2026-08-31: the stripper +# worked in every test and in the shell, and the first live run under the +# unit refused every upload with "exiftool is not installed". Looking in the +# known locations is the fix; setting PATH in the unit would only be a note +# somebody has to remember on the next machine. +FALLBACK_PATHS: Final = ( + "/usr/bin/vendor_perl/exiftool", + "/usr/local/bin/exiftool", + "/usr/bin/exiftool", + "/opt/homebrew/bin/exiftool", +) + # How long either exiftool call may take. A phone video is a few tens of # megabytes and exiftool rewrites the container, not the frames. TIMEOUT_SECONDS: Final = 60 @@ -50,6 +66,18 @@ TIMEOUT_SECONDS: Final = 60 _TAG_LINE: Final = re.compile(r"^\[(\w+)\]\s+(\S+)\s*:\s?(.*)$") +def locate() -> str | None: + """The exiftool binary's absolute path, or None if it is not installed. + + PATH first, then the known install locations -- see FALLBACK_PATHS for + why the second half is not redundant. + """ + found = shutil.which(EXIFTOOL) + if found is not None: + return found + return next((path for path in FALLBACK_PATHS if Path(path).is_file()), None) + + class Stripper(Protocol): """Removes metadata from one upload, or refuses it.""" @@ -66,7 +94,7 @@ class ExiftoolStripper: Args: executable: Override for the binary, used by the e2e script. """ - self._exiftool = executable or shutil.which(EXIFTOOL) + self._exiftool = executable or locate() def strip(self, payload: bytes, kind: MediaKind) -> bytes | None: """Strip one upload and verify the result, or refuse it. @@ -87,7 +115,12 @@ class ExiftoolStripper: logger.warning("storing %s unstripped: exiftool cannot write it", kind.suffix) return payload if self._exiftool is None: - logger.error("exiftool is not installed; refusing to store %s", kind.suffix) + logger.error( + "exiftool not found on PATH (%s) or in %s; refusing to store %s", + os.environ.get("PATH", ""), + FALLBACK_PATHS, + kind.suffix, + ) return None with tempfile.TemporaryDirectory(prefix="signal-bot-strip-") as workspace: return self._run(Path(workspace), payload, kind) diff --git a/signal_bot/selftest.py b/signal_bot/selftest.py index d515764..3cea93f 100644 --- a/signal_bot/selftest.py +++ b/signal_bot/selftest.py @@ -13,6 +13,14 @@ downstream of the websocket read is the code that actually serves the group: parsing, the scoping gate, the tally, trigger resolution, the cooldown, the handler and the real transport. Only the socket read itself is skipped. +A message may carry attachments, which is what lets ``!add_photo`` be +verified this way at all: the ids name files already in the container's own +store, so the handler fetches them back over the real +GET /v1/attachments/{id} rather than being handed bytes. Signal never +delivers an account its own sends -- measured 2026-08-31, and a single-device +account gets no sync copy either -- so this is the only way the bot can put a +photo in front of itself. + Fail-closed: SELFTEST_GROUPS names the groups this may post into, and is empty by default. Without it the endpoint refuses, so a loopback caller cannot make the bot post into the real group. @@ -21,12 +29,13 @@ cannot make the bot post into the real group. from __future__ import annotations import json +import mimetypes import os import time from typing import TYPE_CHECKING, Any, Final if TYPE_CHECKING: - from collections.abc import Mapping + from collections.abc import Mapping, Sequence import httpx @@ -73,14 +82,35 @@ async def resolve_group(client: httpx.AsyncClient, config: Config, name: str) -> raise SelfTestError(msg) -def build_frame(group_id: str, author: str, text: str) -> str: - """Build the websocket frame signal-cli would emit for this message.""" +def attachment_entry(attachment_id: str) -> dict[str, Any]: + """One attachments[] entry, shaped the way signal-cli reports it. + + The type is guessed from the id's suffix because that is all signal-cli + itself has to go on for a stored file -- and it is not load-bearing: + !add_photo sniffs the magic bytes and ignores what the envelope claims. + """ + guessed, _ = mimetypes.guess_type(attachment_id) + return {"contentType": guessed or "application/octet-stream", "id": attachment_id} + + +def build_frame(group_id: str, author: str, text: str, attachments: Sequence[str] = ()) -> str: + """Build the websocket frame signal-cli would emit for this message. + + ``attachments`` names files that are **already in the container's own + attachment store**, exactly as a real inbound envelope does. Nothing here + invents bytes: the handler fetches them back over the real + GET /v1/attachments/{id}, so the ingest path under test is the live one. + """ stamp = int(time.time() * 1000) payload: dict[str, Any] = { "timestamp": stamp, "message": text, "groupInfo": {"groupId": group_id, "type": "DELIVER"}, } + if attachments: + # Top level, never under "quote": a reply carries the *quoted* + # message's media there, and !add_photo must never take that. + payload["attachments"] = [attachment_entry(a) for a in attachments] envelope: dict[str, Any] = { "source": author, "sourceNumber": author, @@ -99,11 +129,12 @@ async def run( group: str, text: str, env: Mapping[str, str] | None = None, + attachments: Sequence[str] = (), ) -> str: - """Feed one synthetic message through the live dispatcher.""" + """Feed one synthetic message, optionally carrying media, to the dispatcher.""" if group not in allowed_groups(os.environ if env is None else env): msg = f"{group!r} is not in {GROUPS_ENV}; refusing to post there" raise SelfTestError(msg) group_id, author = await resolve_group(client, bot.config, group) - await bot.dispatcher.handle(build_frame(group_id, author, text)) + await bot.dispatcher.handle(build_frame(group_id, author, text, attachments)) return group_id diff --git a/tests/test_media_strip.py b/tests/test_media_strip.py index 46d7eb9..2842cf4 100644 --- a/tests/test_media_strip.py +++ b/tests/test_media_strip.py @@ -15,7 +15,7 @@ from typing import TYPE_CHECKING import pytest from signal_bot.media_sniff import JPEG, MP4, SVG, WEBM, MediaKind -from signal_bot.media_strip import ExiftoolStripper +from signal_bot.media_strip import FALLBACK_PATHS, ExiftoolStripper, locate if TYPE_CHECKING: from pathlib import Path @@ -114,8 +114,14 @@ def test_a_missing_binary_refuses_rather_than_raising(tmp_path: Path) -> None: def test_no_exiftool_at_all_refuses_the_upload(monkeypatch: pytest.MonkeyPatch) -> None: - """Never stores it dirty instead: a strip that cannot run is a refusal.""" + """Never stores it dirty instead: a strip that cannot run is a refusal. + + Both lookups have to be blinded, not just PATH -- on a machine that has + perl-image-exiftool the fallback list finds it and this branch is never + reached, which is exactly how it went uncovered until the live run. + """ monkeypatch.setattr("signal_bot.media_strip.shutil.which", lambda _: None) + monkeypatch.setattr("signal_bot.media_strip.Path.is_file", lambda self: False) assert ExiftoolStripper().strip(b"\xff\xd8\xffdata", JPEG) is None @@ -127,3 +133,38 @@ def test_the_two_unwritable_formats_are_stored_as_they_arrived( stripper = ExiftoolStripper(str(tmp_path / "never-invoked")) payload = b"a name" assert stripper.strip(payload, kind) == payload + + +def test_locate_prefers_path(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr("signal_bot.media_strip.shutil.which", lambda _: "/somewhere/exiftool") + assert locate() == "/somewhere/exiftool" + + +def test_locate_falls_back_to_the_known_install_locations( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The live bug of 2026-08-31, pinned. + + Arch's perl-image-exiftool installs to /usr/bin/vendor_perl, which is on + an interactive shell's PATH and not on a systemd unit's -- so the + stripper passed every test and refused every upload in production. + """ + monkeypatch.setattr("signal_bot.media_strip.shutil.which", lambda _: None) + monkeypatch.setattr( + "signal_bot.media_strip.Path.is_file", + lambda self: str(self) == "/usr/bin/vendor_perl/exiftool", + ) + assert locate() == "/usr/bin/vendor_perl/exiftool" + + +def test_locate_returns_none_when_it_really_is_absent( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr("signal_bot.media_strip.shutil.which", lambda _: None) + monkeypatch.setattr("signal_bot.media_strip.Path.is_file", lambda self: False) + assert locate() is None + + +def test_the_arch_vendor_perl_path_is_on_the_fallback_list() -> None: + """Named explicitly: this is the one that actually bit.""" + assert "/usr/bin/vendor_perl/exiftool" in FALLBACK_PATHS diff --git a/tests/test_selftest_attachments.py b/tests/test_selftest_attachments.py new file mode 100644 index 0000000..fc42b30 --- /dev/null +++ b/tests/test_selftest_attachments.py @@ -0,0 +1,116 @@ +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""Self-testing a command that needs a *photo*, not just text. + +Split out of test_selftest.py at the 250-line cap. This is the half that +exists because of !add_photo: Signal never delivers an account its own sends +-- measured against the live container on 2026-08-31, and a single-device +account gets no sync copy either -- so the only way to put a photo in front of +the bot is to name one the container's own attachment store already holds and +let the handler fetch it back over the real API. +""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING + +import httpx +import pytest + +from signal_bot.bot import build_bot +from signal_bot.liveness import Liveness +from signal_bot.selftest import attachment_entry, build_frame, run +from tests.conftest import WATCHED_GROUP + +if TYPE_CHECKING: + from signal_bot.bot import Bot + from signal_bot.config import Config + from signal_bot.discovery import GroupDirectory + from signal_bot.registry import CommandContext + from signal_bot.store import Store + from signal_bot.transport_fake import FakeTransport + +GROUP_ID = "group.internal-id" +OTHER = "+48000000000" + + +def _bot(config: Config, store: Store, transport: FakeTransport, directory: GroupDirectory) -> Bot: + """The production object graph, which is the point of self-testing at all.""" + client = httpx.AsyncClient(transport=httpx.MockTransport(lambda _: httpx.Response(200))) + return build_bot(config, client, store, transport, directory, Liveness(config, client)) + + +def _client(entries: list[dict[str, object]]) -> httpx.AsyncClient: + def handler(_: httpx.Request) -> httpx.Response: + return httpx.Response(200, json=entries) + + return httpx.AsyncClient(transport=httpx.MockTransport(handler)) + + +def test_a_frame_can_carry_an_attachment() -> None: + """What makes !add_photo verifiable without a second human. + + Signal never delivers an account its own sends -- measured against the + live container on 2026-08-31, and a single-device account gets no sync + copy either -- so this is the only way to put a photo in front of the bot. + """ + body = json.loads(build_frame(GROUP_ID, OTHER, "!dodaj alfa", ["abc.jpg"]))["envelope"] + attachments = body["dataMessage"]["attachments"] + assert attachments == [{"contentType": "image/jpeg", "id": "abc.jpg"}] + + +def test_a_frame_without_attachments_has_no_attachments_key() -> None: + """Byte-for-byte what it emitted before, for every existing caller.""" + body = json.loads(build_frame(GROUP_ID, OTHER, "!papież"))["envelope"] + assert "attachments" not in body["dataMessage"] + + +def test_the_attachment_sits_at_the_top_level_not_in_a_quote() -> None: + """quote.attachments is the *quoted* message's media; taking it would let + a self-test re-file a photo somebody else posted.""" + payload = json.loads(build_frame(GROUP_ID, OTHER, "!dodaj alfa", ["a.jpg"])) + message = payload["envelope"]["dataMessage"] + assert "attachments" in message + assert "quote" not in message + + +@pytest.mark.parametrize( + ("attachment_id", "expected"), + [ + ("photo.jpg", "image/jpeg"), + ("clip.mp4", "video/mp4"), + ("shot.png", "image/png"), + # No suffix to go on, and it does not matter: !add_photo sniffs the + # magic bytes and ignores what the envelope claims to be carrying. + ("no-suffix", "application/octet-stream"), + ], +) +def test_the_content_type_is_guessed_from_the_id(attachment_id: str, expected: str) -> None: + assert attachment_entry(attachment_id)["contentType"] == expected + + +async def test_run_passes_attachments_through_to_the_dispatcher( + config: Config, store: Store, transport: FakeTransport, directory: GroupDirectory +) -> None: + """End to end through the real wiring: the id reaches the handler.""" + bot = _bot(config, store, transport, directory) + entries = [ + {"name": "testbot", "internal_id": WATCHED_GROUP, "members": [config.phone_number, OTHER]} + ] + seen: list[tuple[str, ...]] = [] + + async def capture(ctx: CommandContext) -> None: + seen.append(ctx.message.attachment_ids) + + bot.dispatcher.registry.register("probe", ["!probe"], capture, "probe", takes_args=True) + + await run( + bot, + _client(entries), + "testbot", + "!probe", + env={"SELFTEST_GROUPS": "testbot"}, + attachments=["staged.jpg"], + ) + + assert seen == [("staged.jpg",)]