From 3874b3acf414cdca8f98e205ba5f784404b16bb7 Mon Sep 17 00:00:00 2001 From: Kaloyan Tenchov Date: Sat, 16 May 2026 12:27:33 -0400 Subject: [PATCH] fix(signal): normalize composite sender_ids in is_allowed too The base BaseChannel.is_allowed() does a literal ``sender_id in allow_from`` check, but Signal's sender_id is a pipe-joined composite of phone/UUID parts. After splitting an allowlist entry like ``+phone|uuid`` into two separate entries, the per-DM gate accepted it but the base gate still denied because the composite sender string wasn't literally in the list. Override is_allowed on SignalChannel to delegate to _sender_matches_allowlist, which already splits both sides on ``|`` and normalizes each part. _sender_matches_allowlist itself now also splits allowlist entries on ``|`` so legacy composite entries keep working too. Co-Authored-By: Claude Opus 4.7 --- nanobot/channels/signal.py | 31 +++++++++++--- tests/channels/test_signal_channel.py | 61 +++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 6 deletions(-) diff --git a/nanobot/channels/signal.py b/nanobot/channels/signal.py index c53c6a133..328f82b22 100644 --- a/nanobot/channels/signal.py +++ b/nanobot/channels/signal.py @@ -360,6 +360,23 @@ class SignalChannel(BaseChannel): # Each message is a dict with: sender_name, sender_number, content, timestamp self._group_buffers: dict[str, deque] = {} + def is_allowed(self, sender_id: str) -> bool: + """Override base check to normalize and split pipe-joined identifiers. + + ``sender_id`` from Signal is the pipe-joined composite produced by + ``_collect_sender_id_parts``; allow_from entries may be single + identifiers or composites and may use the ``+`` prefix variant or + not. Delegates to ``_sender_matches_allowlist`` so the base gate + matches the per-policy DM gate. + """ + allow_list = self.config.allow_from + if not allow_list: + self.logger.warning("allow_from is empty — all access denied") + return False + if "*" in allow_list: + return True + return self._sender_matches_allowlist(sender_id, allow_list) + async def start(self) -> None: """Start the Signal channel and connect to signal-cli daemon.""" if not self.config.phone_number: @@ -937,11 +954,12 @@ class SignalChannel(BaseChannel): def _sender_matches_allowlist(cls, sender_id: str, allow_list: list[str]) -> bool: """Return True if any normalized variant of sender_id is on allow_list. - sender_id is the pipe-joined identifier string built by - _collect_sender_id_parts. Each part and each allow_list entry is run - through _normalize_signal_id so an allowlist entry like ``1234567890`` - matches a sender ``+1234567890`` (and vice versa), and case-only - differences in UUIDs/ACIs match too. + Both ``sender_id`` and each allow_list entry can be a single + identifier or a pipe-joined composite of several (e.g. + ``"+1234567890|uuid-abc"``); both sides are split on ``|`` and each + part is run through ``_normalize_signal_id`` so an allowlist entry + like ``1234567890`` matches a sender ``+1234567890`` (and vice + versa), and case-only differences in UUIDs/ACIs match too. """ if not allow_list: return False @@ -952,7 +970,8 @@ class SignalChannel(BaseChannel): return False allow_variants: set[str] = set() for entry in allow_list: - allow_variants.update(cls._normalize_signal_id(entry)) + for part in str(entry).split("|"): + allow_variants.update(cls._normalize_signal_id(part)) return bool(sender_variants & allow_variants) def _remember_account_id_alias(self, value: str | None) -> None: diff --git a/tests/channels/test_signal_channel.py b/tests/channels/test_signal_channel.py index 9444e2218..28433c07a 100644 --- a/tests/channels/test_signal_channel.py +++ b/tests/channels/test_signal_channel.py @@ -493,6 +493,51 @@ class TestGroupBuffer: # --------------------------------------------------------------------------- +class TestIsAllowed: + """The base-channel allowlist gate is overridden to understand Signal's + pipe-joined composite sender_ids and the +/no-+ phone variants. + """ + + def test_denies_when_allowlist_empty(self): + ch = _make_channel(dm_enabled=True, dm_policy="open") # open -> no entries + assert ch.is_allowed("+19995550001") is False + + def test_allows_wildcard(self): + ch = _make_channel(dm_policy="allowlist", dm_allow_from=["*"]) + assert ch.is_allowed("+19995550001|some-uuid") is True + + def test_allows_composite_sender_against_split_allowlist(self): + """Composite sender_id, single-id allow_from — must match either part.""" + ch = _make_channel( + dm_policy="allowlist", + dm_allow_from=["+19995550001"], + ) + assert ch.is_allowed("+19995550001|1872ba20-uuid") is True + + def test_allows_composite_sender_against_composite_allowlist_entry(self): + """Backward compat: pipe-joined composite allowlist entries still match.""" + composite = "+19995550001|1872ba20-uuid" + ch = _make_channel(dm_policy="allowlist", dm_allow_from=[composite]) + assert ch.is_allowed(composite) is True + + def test_allows_when_only_uuid_part_is_listed(self): + ch = _make_channel(dm_policy="allowlist", dm_allow_from=["1872ba20-uuid"]) + assert ch.is_allowed("+19995550001|1872ba20-uuid") is True + + def test_denies_when_no_part_matches(self): + ch = _make_channel(dm_policy="allowlist", dm_allow_from=["+12223334444"]) + assert ch.is_allowed("+19995550001|1872ba20-uuid") is False + + def test_allowlist_union_includes_group_ids(self): + """allow_from is the union of dm.allow_from and group.allow_from.""" + ch = _make_channel( + group_enabled=True, + group_policy="allowlist", + group_allow_from=["group-id-base64=="], + ) + assert "group-id-base64==" in ch.config.allow_from + + class TestCheckInboundPolicy: """Direct tests for the policy gate that _handle_data_message now delegates to.""" @@ -665,6 +710,22 @@ class TestHandleDataMessageDM: await ch._handle_receive_notification(params) assert len(handled) == 1 + @pytest.mark.asyncio + async def test_dm_allowlist_matches_pipe_joined_composite_entry(self): + """Allowlist entries written as ``phone|uuid`` composites still work. + + Some configs pre-date the per-part splitting and store the full + sender_id composite as a single allow_from entry. Keep matching it. + """ + composite = "+19995550001|1872ba20-f52a-4bad-b434-bf7f808c8b22" + ch, handled = self._make_dm_channel(policy="allowlist", allow_from=[composite]) + params = _dm_envelope( + source_number="+19995550001", + source_uuid="1872ba20-f52a-4bad-b434-bf7f808c8b22", + ) + await ch._handle_receive_notification(params) + assert len(handled) == 1 + @pytest.mark.asyncio async def test_dm_disabled_rejected(self): ch = _make_channel(dm_enabled=False)