Fix paired Telegram owner scope routine visibility - #2258
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces identity resolution via a PairingStore in the WASM channel wrapper, allowing paired users to be recognized within the owner's scope. It also updates the HTTP request logic to permit private IP targets when URLs are rewritten for testing purposes. E2E tests were expanded to include Telegram verification handling and a new scenario for paired users accessing owner routines. Feedback was provided to simplify the error handling in the identity resolution logic by removing a redundant function call that always evaluates to a fixed guest state.
| Err(error) => { | ||
| tracing::warn!( | ||
| channel = %channel_name, | ||
| sender_id = %sender_id, | ||
| "Failed to resolve paired sender identity: {}", | ||
| error | ||
| ); | ||
| resolve_message_scope(owner_scope_id, owner_actor_id, sender_id) | ||
| } |
There was a problem hiding this comment.
The fallback to resolve_message_scope in the Err arm is a bit redundant. Since the owner_actor_id == sender_id case is already handled at the beginning of resolve_message_scope_with_pairing, the call to resolve_message_scope here will always evaluate to its else branch, which is (sender_id.to_string(), false). This is the same outcome as the Ok(None) case.
To simplify and make the logic more direct, you can replace the function call with the explicit tuple. This makes it clearer that a failure to resolve a paired identity results in treating the sender as a guest.
| Err(error) => { | |
| tracing::warn!( | |
| channel = %channel_name, | |
| sender_id = %sender_id, | |
| "Failed to resolve paired sender identity: {}", | |
| error | |
| ); | |
| resolve_message_scope(owner_scope_id, owner_actor_id, sender_id) | |
| } | |
| Err(error) => { | |
| tracing::warn!( | |
| channel = %channel_name, | |
| sender_id = %sender_id, | |
| "Failed to resolve paired sender identity: {}", | |
| error | |
| ); | |
| (sender_id.to_string(), false) | |
| } |
|
I dug through the current multi-tenant model ( Short version:
That means the remaining risk is less "obvious cross-user leak in direct replies" and more "incomplete multi-tenant support for proactive/outbound Telegram delivery". Follow-up issue with code anchors, repro direction, and acceptance criteria: #2263 |
henrypark133
left a comment
There was a problem hiding this comment.
Review: Fix paired Telegram owner scope routine visibility
Correct fix for a real identity-normalization bug: paired Telegram senders now resolve to their paired owner_id via PairingStore lookup, so tenant-scoped resources (routines, workspaces) are visible when they should be.
Positives:
resolve_message_scope_with_pairing()cleanly extends the existingresolve_message_scope()with async pairing lookup, falling back to the sync version on error — no regression risk for existing channels- Paired senders get
is_owner_sender = false(commit 2 fix) — they route to owner scope but don't get owner-level privileges. This is the correct semantic: paired users share the scope but aren't the owner should_update_owner_broadcast_metadata()checkssender_id == owner_actor_id— prevents a paired guest's chat_id from overwriting the owner's broadcast routing metadata. Good regression test (test_respond_paired_guest_does_not_overwrite_owner_broadcast_metadata)- Private IP bypass for test-only Telegram URL rewrites is properly gated:
rewrite_telegram_api_url_for_testing()compiles toNonein release builds via#[cfg(not(any(test, debug_assertions)))]— dead code in production - Test isolation: separate
telegram_e2e_server_with_routinesfixture avoids polluting the existing session-scoped fixture (commit 4) - Integration test with real libSQL PairingStore (
make_db_backed_pairing_store) validates the full resolve path
Minor notes:
debug_assertionsgate means the Telegram URL rewrite + private IP bypass is active in debug builds, not just tests — intentional for local dev but worth being aware of- The
activate_telegramhelper now handles the verification challenge flow — this adapts to a staging change, not part of the core fix - E2E test
test_paired_telegram_user_lists_owner_routineshas a 60s timeout forwait_for_sent_messages— generous but appropriate for CI environments
LGTM.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
* Fix paired Telegram owner scope routing * fix: address review findings (iteration 1) * fix: address telegram owner routing feedback * test: isolate telegram routines e2e fixture --------- Co-authored-by: Guille <gagdiez.c@gmail.com>
Problem
Routines are owner-scoped data: they are created and listed under
ctx.user_id. That works across CLI and web because those entrypoints already use the stable owner scope ID as the effective user identity.Telegram was different for paired non-owner senders. After a user was paired, inbound Telegram traffic still entered the WASM channel path with the raw external Telegram sender ID as
user_id. The wrapper only remapped the configured owner actor to owner scope; it did not remap other paired senders.That meant the same owner could end up with two different identities depending on ingress path:
user_id = owner scopeuser_id = raw Telegram sender idRoutines surfaced the bug because listing and creation both key off
ctx.user_id. A routine created from CLI/web was stored under owner scope, but a paired Telegram guest listed routines under their raw external ID and saw an empty set. The underlying problem was broader than routines: it was an ingress identity normalization bug.Why The Fix Lives In The WASM Wrapper
Fixing this in the routine tool would have been the wrong layer. The problem is not that routines use
ctx.user_id; the problem is that paired channel traffic was buildingIncomingMessagewith the wrong effective identity.The wrapper now resolves sender identity before constructing
IncomingMessage:PairingStoreuser_idto the paired owner scopesender_idThat keeps owner-scoped features consistent across entrypoints while still preserving who actually sent the Telegram message. It also means this fix applies to the whole WASM channel ingress path instead of only to routines.
Implementation Details
Wrapper identity remap
src/channels/wasm/wrapper.rsnow usesresolve_message_scope_with_pairing(...)in both emitted-message dispatch paths. That helper checks owner binding first, then consultsPairingStore, and falls back to the raw sender only when no pairing exists.The resulting
IncomingMessagesemantics are now:user_id: stable owner scope for owner-bound or paired senderssender_id: original external channel sender idA paired sender is intentionally not treated as the bound owner actor for outbound Telegram routing metadata. That metadata should only move when the actual owner actor speaks, otherwise a paired guest could steal the owner's broadcast destination.
Test-only Telegram HTTP rewrite
The Telegram E2E stack routes Bot API calls to a local fake Telegram server. The WASM HTTP host already rewrote Telegram URLs for tests, but it still ran the private-IP SSRF guard after rewrite, which blocked the local fake server.
The wrapper now skips that private-IP rejection only for explicit test rewrite targets. Production behavior is unchanged. I added a targeted unit test to cover that path.
E2E setup updates
The Telegram E2E helper needed to reflect current setup behavior:
Tests Added
Rust unit coverage
Added wrapper-level coverage proving that a paired Telegram sender:
user_idsender_idAlso added coverage for the Telegram test rewrite path so local fake Bot API endpoints are allowed in E2E without weakening normal SSRF checks.
E2E regression
Added a Telegram regression that:
This is the failing user path behind #2239.
Testing
cargo test test_dispatch_emitted_messages_ --libcargo test test_http_request_ --lib/tmp/ironclaw-e2e-venv311/bin/python -m pytest tests/e2e/scenarios/test_telegram_e2e.py -k paired_telegram_user_lists_owner_routines -q/tmp/ironclaw-e2e-venv311/bin/python -m pytest tests/e2e/scenarios/test_telegram_e2e.py -k telegram_setup_and_dm_roundtrip -qCloses #2239