From 4c532c153b0d26d342bfc703b9647bdc5bc94587 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 29 Mar 2026 12:15:28 +0530 Subject: [PATCH] fix: URL-encode Signal phone numbers and correct attachment RPC parameter (#3670) Fixes two Signal bugs: 1. SSE connection: URL-encode phone numbers so + isn't interpreted as space (400 Bad Request) 2. Attachment fetch: use 'id' parameter instead of 'attachmentId' (NullPointerException in signal-cli) Also refactors Signal tests with shared helpers. --- gateway/platforms/signal.py | 6 +- tests/gateway/test_signal.py | 132 +++++++++++++++++++++++++++-------- 2 files changed, 105 insertions(+), 33 deletions(-) diff --git a/gateway/platforms/signal.py b/gateway/platforms/signal.py index cbe12a87c..95f605c0b 100644 --- a/gateway/platforms/signal.py +++ b/gateway/platforms/signal.py @@ -22,7 +22,7 @@ import time from datetime import datetime, timezone from pathlib import Path from typing import Dict, List, Optional, Any -from urllib.parse import unquote +from urllib.parse import quote, unquote import httpx @@ -253,7 +253,7 @@ class SignalAdapter(BasePlatformAdapter): async def _sse_listener(self) -> None: """Listen for SSE events from signal-cli daemon.""" - url = f"{self.http_url}/api/v1/events?account={self.account}" + url = f"{self.http_url}/api/v1/events?account={quote(self.account, safe='')}" backoff = SSE_RETRY_DELAY_INITIAL while self._running: @@ -521,7 +521,7 @@ class SignalAdapter(BasePlatformAdapter): """Fetch an attachment via JSON-RPC and cache it. Returns (path, ext).""" result = await self._rpc("getAttachment", { "account": self.account, - "attachmentId": attachment_id, + "id": attachment_id, }) if not result: diff --git a/tests/gateway/test_signal.py b/tests/gateway/test_signal.py index 8bf5537f4..acd6513e5 100644 --- a/tests/gateway/test_signal.py +++ b/tests/gateway/test_signal.py @@ -1,11 +1,42 @@ """Tests for Signal messenger platform adapter.""" +import base64 import json import pytest from unittest.mock import MagicMock, patch, AsyncMock +from urllib.parse import quote from gateway.config import Platform, PlatformConfig +# --------------------------------------------------------------------------- +# Shared Helpers +# --------------------------------------------------------------------------- + +def _make_signal_adapter(monkeypatch, account="+15551234567", **extra): + """Create a SignalAdapter with sensible test defaults.""" + monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", extra.pop("group_allowed", "")) + from gateway.platforms.signal import SignalAdapter + config = PlatformConfig() + config.enabled = True + config.extra = { + "http_url": "http://localhost:8080", + "account": account, + **extra, + } + return SignalAdapter(config) + + +def _stub_rpc(return_value): + """Return an async mock for SignalAdapter._rpc that captures call params.""" + captured = [] + + async def mock_rpc(method, params, rpc_id=None): + captured.append({"method": method, "params": dict(params)}) + return return_value + + return mock_rpc, captured + + # --------------------------------------------------------------------------- # Platform & Config # --------------------------------------------------------------------------- @@ -61,48 +92,22 @@ class TestSignalConfigLoading: # --------------------------------------------------------------------------- class TestSignalAdapterInit: - def _make_config(self, **extra): - config = PlatformConfig() - config.enabled = True - config.extra = { - "http_url": "http://localhost:8080", - "account": "+15551234567", - **extra, - } - return config - def test_init_parses_config(self, monkeypatch): - monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "group123,group456") - - from gateway.platforms.signal import SignalAdapter - adapter = SignalAdapter(self._make_config()) - + adapter = _make_signal_adapter(monkeypatch, group_allowed="group123,group456") assert adapter.http_url == "http://localhost:8080" assert adapter.account == "+15551234567" assert "group123" in adapter.group_allow_from def test_init_empty_allowlist(self, monkeypatch): - monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "") - - from gateway.platforms.signal import SignalAdapter - adapter = SignalAdapter(self._make_config()) - + adapter = _make_signal_adapter(monkeypatch) assert len(adapter.group_allow_from) == 0 def test_init_strips_trailing_slash(self, monkeypatch): - monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "") - - from gateway.platforms.signal import SignalAdapter - adapter = SignalAdapter(self._make_config(http_url="http://localhost:8080/")) - + adapter = _make_signal_adapter(monkeypatch, http_url="http://localhost:8080/") assert adapter.http_url == "http://localhost:8080" def test_self_message_filtering(self, monkeypatch): - monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "") - - from gateway.platforms.signal import SignalAdapter - adapter = SignalAdapter(self._make_config()) - + adapter = _make_signal_adapter(monkeypatch) assert adapter._account_normalized == "+15551234567" @@ -189,6 +194,73 @@ class TestSignalHelpers: assert check_signal_requirements() is False +# --------------------------------------------------------------------------- +# SSE URL Encoding (Bug Fix: phone numbers with + must be URL-encoded) +# --------------------------------------------------------------------------- + +class TestSignalSSEUrlEncoding: + """Verify that phone numbers with + are URL-encoded in the SSE endpoint.""" + + def test_sse_url_encodes_plus_in_account(self): + """The + in E.164 phone numbers must be percent-encoded in the SSE query string.""" + encoded = quote("+31612345678", safe="") + assert encoded == "%2B31612345678" + + def test_sse_url_encoding_preserves_digits(self): + """Digits and country codes should pass through URL encoding unchanged.""" + assert quote("+15551234567", safe="") == "%2B15551234567" + + +# --------------------------------------------------------------------------- +# Attachment Fetch (Bug Fix: parameter must be "id" not "attachmentId") +# --------------------------------------------------------------------------- + +class TestSignalAttachmentFetch: + """Verify that _fetch_attachment uses the correct RPC parameter name.""" + + @pytest.mark.asyncio + async def test_fetch_attachment_uses_id_parameter(self, monkeypatch): + """RPC getAttachment must use 'id', not 'attachmentId' (signal-cli requirement).""" + adapter = _make_signal_adapter(monkeypatch) + + png_data = b"\x89PNG\r\n\x1a\n" + b"\x00" * 100 + b64_data = base64.b64encode(png_data).decode() + + adapter._rpc, captured = _stub_rpc({"data": b64_data}) + + with patch("gateway.platforms.signal.cache_image_from_bytes", return_value="/tmp/test.png"): + await adapter._fetch_attachment("attachment-123") + + call = captured[0] + assert call["method"] == "getAttachment" + assert call["params"]["id"] == "attachment-123" + assert "attachmentId" not in call["params"], "Must NOT use 'attachmentId' — causes NullPointerException in signal-cli" + assert call["params"]["account"] == "+15551234567" + + @pytest.mark.asyncio + async def test_fetch_attachment_returns_none_on_empty(self, monkeypatch): + adapter = _make_signal_adapter(monkeypatch) + adapter._rpc, _ = _stub_rpc(None) + path, ext = await adapter._fetch_attachment("missing-id") + assert path is None + assert ext == "" + + @pytest.mark.asyncio + async def test_fetch_attachment_handles_dict_response(self, monkeypatch): + adapter = _make_signal_adapter(monkeypatch) + + pdf_data = b"%PDF-1.4" + b"\x00" * 100 + b64_data = base64.b64encode(pdf_data).decode() + + adapter._rpc, _ = _stub_rpc({"data": b64_data}) + + with patch("gateway.platforms.signal.cache_document_from_bytes", return_value="/tmp/test.pdf"): + path, ext = await adapter._fetch_attachment("doc-456") + + assert path == "/tmp/test.pdf" + assert ext == ".pdf" + + # --------------------------------------------------------------------------- # Session Source # ---------------------------------------------------------------------------