Skip to content

fix(security): require explicit opt-in for Telegram bridge open chat access#699

Closed
ericksoa wants to merge 1 commit intomainfrom
fix/telegram-allowed-chats
Closed

fix(security): require explicit opt-in for Telegram bridge open chat access#699
ericksoa wants to merge 1 commit intomainfrom
fix/telegram-allowed-chats

Conversation

@ericksoa
Copy link
Copy Markdown
Contributor

Summary

Draft — seeking feedback on approach before finalizing.

The Telegram bridge silently accepts messages from ALL chats when ALLOWED_CHAT_IDS is unset. This PR adds a visible warning and logs new chat IDs so users can build their allowlist, without breaking existing deployments.

Behavior:

  • No ALLOWED_CHAT_IDS, no ALLOW_ALL_CHATS: Bridge starts with a loud warning; logs each new chat ID + username so the user can copy-paste into ALLOWED_CHAT_IDS
  • ALLOW_ALL_CHATS=true: Brief warning, no per-message logging (explicit opt-in)
  • ALLOWED_CHAT_IDS set: Existing filtering, no warnings, no change

Open question

Should we hard-fail instead of warn? Current approach is non-breaking but the bridge still accepts all messages. The alternative (exit with error) is more secure but breaks every existing Telegram bridge deployment that doesn't have ALLOWED_CHAT_IDS set.

Test plan

  • 1 regression test verifies bridge references both ALLOWED_CHAT_IDS and ALLOW_ALL_CHATS
  • All tests pass
  • Manual: start bridge without env vars → verify warning + chat ID logging
  • Manual: start bridge with ALLOW_ALL_CHATS=true → verify brief warning only
  • Manual: start bridge with ALLOWED_CHAT_IDS → verify no warnings

@ericksoa ericksoa requested a review from jacobtomlinson March 23, 2026 05:04
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 23, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d027e01a-03dc-4f6f-b6a5-ce397de3560d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/telegram-allowed-chats

Comment @coderabbitai help to get the list of available commands and usage tips.

…access

When ALLOWED_CHAT_IDS is unset, the bridge now refuses to start unless
ALLOW_ALL_CHATS=true is explicitly set. This changes the default from
open-access to fail-closed, preventing arbitrary Telegram users from
driving the sandboxed agent without operator intent.

Addresses NVBUG 6007062.
@cv cv force-pushed the fix/telegram-allowed-chats branch from b25e151 to 87fdc5f Compare March 23, 2026 20:42
@cv
Copy link
Copy Markdown
Contributor

cv commented Mar 23, 2026

Noticed this is still in draft — flagging a few things for when you pick it back up.

  1. The code warns but doesn't block. The commit message says "refuses to start unless ALLOW_ALL_CHATS=true" but the code prints a warning and continues processing all messages. WARN_OPEN_ACCESS controls log output, not access. Messages from arbitrary chats are still forwarded to the agent regardless of the flag.

  2. userName is referenced before declaration. Line 196 uses userName in the log message, but it's not declared until line 199. This will throw a ReferenceError or log undefined on the first message from any new chat in open-access mode.

  3. The test checks for string presence in source, not behavior. The new test asserts that "ALLOWED_CHAT_IDS" and "ALLOW_ALL_CHATS" appear in the file — these could be in a comment and the test would pass.

  4. Re: the open question in the description — if this addresses NVBUG 6007062 and the threat is arbitrary Telegram users driving the agent, warn-only doesn't mitigate it. process.exit(1) when neither env var is set would match the commit message's stated intent.

@prekshivyas
Copy link
Copy Markdown
Contributor

Closing — superseded by #1081, which removes the host-side Telegram bridge entirely (telegram-bridge.js deleted). Messaging now runs as native OpenClaw channels inside the sandbox, so the ALLOWED_CHAT_IDS / ALLOW_ALL_CHATS opt-in logic this PR added no longer applies.

@prekshivyas prekshivyas closed this Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request security Something isn't secure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants