[codex] require Telegram chat allowlist for bridge access#1218
[codex] require Telegram chat allowlist for bridge access#121813ernkastel wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Telegram allowlist management and discovery mode, persists allowed chat IDs to the credentials store, enforces allowlist/NVIDIA API key for normal bridge startup, introduces reserved sandbox names with validation and a sandbox-target override, refactors CLI/start dispatch, updates bridge/start scripts, documentation, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "nemoclaw CLI"
participant Creds as "Credentials Store"
participant StartSvc as "start-services.sh"
participant Bridge as "telegram-bridge.js"
participant Telegram as "Telegram API"
User->>CLI: nemoclaw telegram allow 123456
CLI->>CLI: normalizeTelegramChatIds("123456")
CLI->>Creds: saveCredential(ALLOWED_CHAT_IDS="123456")
Creds-->>CLI: saved
User->>CLI: nemoclaw start
CLI->>Creds: read ALLOWED_CHAT_IDS
CLI->>StartSvc: launch with ALLOWED_CHAT_IDS, NEMOCLAW_TELEGRAM_DISCOVERY=0
StartSvc->>Bridge: start with env
Bridge->>Telegram: connect with token
Telegram-->>Bridge: ready
Telegram->>Bridge: message from chat 123456
Bridge->>Bridge: check ALLOWED_CHAT_IDS
Bridge->>User: forward to agent
sequenceDiagram
actor User
participant CLI as "nemoclaw CLI"
participant StartSvc as "start-services.sh"
participant Bridge as "telegram-bridge.js"
participant Telegram as "Telegram API"
User->>CLI: nemoclaw start --discover-chat-id
CLI->>StartSvc: launch with NEMOCLAW_TELEGRAM_DISCOVERY=1
StartSvc->>Bridge: start with discovery mode
Bridge->>Telegram: connect
Telegram->>Bridge: message from chat 789012
Bridge->>Bridge: DISCOVERY_ONLY = true (no allowlist)
Bridge->>User: reply with chat ID and allow instructions
Note over Bridge: message is NOT forwarded to agent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/nemoclaw.js (1)
51-58:⚠️ Potential issue | 🔴 CriticalReserve global command names to prevent sandbox shadowing.
A sandbox can be created with the name
telegram(it passes RFC 1123 validation invalidateName()), but that sandbox becomes permanently inaccessible via the CLI because command dispatch checksGLOBAL_COMMANDSbeforeregistry.getSandbox(). This blocks users from accessing a legitimately created resource.Add reserved-keyword validation to
registerSandbox()orvalidateName()to reject sandbox names that match entries inGLOBAL_COMMANDS, or implement an escape hatch (e.g.,nemoclaw -- <sandbox-name> <action>) for sandbox-scoped access when a name conflicts.Also applies to: 1277–1296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 51 - 58, The CLI reserves command names in GLOBAL_COMMANDS which can shadow sandbox names and make them unreachable; update validation to prevent this by either adding a reserved-name check in validateName() or registerSandbox() to reject any sandbox name that exists in GLOBAL_COMMANDS, or implement an escape hatch in CLI dispatch (e.g., support a "--" separator) so registry.getSandbox() is consulted when the user explicitly escapes global parsing; modify the code paths that build/dispatch commands (where GLOBAL_COMMANDS is checked and where registry.getSandbox() is called) to apply the chosen fix and include clear error messaging instructing users about reserved names or how to use the escape hatch.
🧹 Nitpick comments (3)
docs/deployment/deploy-to-remote-gpu.md (1)
66-67: One sentence per line and inline code formatting.This paragraph has multiple sentences on a single line (line 66), which makes diffs harder to read. Additionally,
ALLOWED_CHAT_IDSshould use inline code formatting for consistency.As per coding guidelines: "One sentence per line in source (makes diffs readable)."
📝 Suggested fix
-If you configured a Telegram bot token but not an allowlist yet, the bridge stays disabled until you either save `ALLOWED_CHAT_IDS` with `nemoclaw telegram allow <chat-id>` or run discovery mode with `nemoclaw start --discover-chat-id`. +If you configured a Telegram bot token but not an allowlist yet, the bridge stays disabled. +Save `ALLOWED_CHAT_IDS` with `nemoclaw telegram allow <chat-id>` or run discovery mode with `nemoclaw start --discover-chat-id` to enable it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/deploy-to-remote-gpu.md` around lines 66 - 67, Split the long paragraph into separate lines so each sentence is on its own line and apply inline code formatting to configuration and command tokens; specifically, change the sentence containing ALLOWED_CHAT_IDS to use `ALLOWED_CHAT_IDS` (wrap in backticks) and ensure the two actions are separate sentences/lines mentioning `nemoclaw telegram allow <chat-id>` and `nemoclaw start --discover-chat-id` as inline code examples (one sentence per line) for readability and diff friendliness.docs/reference/commands.md (1)
184-193: Consider adding brief descriptions for each subcommand.The code block shows the commands but doesn't explain what each one does. A brief table or description list would improve usability, especially for
telegram discoverwhich isn't self-explanatory.📝 Suggested addition
### `nemoclaw telegram` Manage the Telegram bridge allowlist. +| Subcommand | Description | +|------------|-------------| +| `allow <id>` | Add one or more comma-separated chat IDs to the allowlist | +| `show` | Display the current allowlist | +| `clear` | Remove all chat IDs from the allowlist | +| `discover` | Start the bridge in discovery-only mode (alias for `start --discover-chat-id`) | ```console $ nemoclaw telegram allow 123456789 $ nemoclaw telegram show $ nemoclaw telegram clear $ nemoclaw telegram discover</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/reference/commands.mdaround lines 184 - 193, Update the "nemoclaw
telegram" docs block to include a one-line description for each subcommand so
readers know their purpose; for example, add short phrases after or beneath the
listed commands for "nemoclaw telegram allow" (add a user/chat ID to the bridge
allowlist), "nemoclaw telegram show" (display current allowlist entries),
"nemoclaw telegram clear" (remove all entries from the allowlist), and "nemoclaw
telegram discover" (scan/identify Telegram chats or users available to add).
Reference the exact command names shown (nemoclaw telegram allow, show, clear,
discover) when inserting the descriptions.</details> </blockquote></details> <details> <summary>bin/nemoclaw.js (1)</summary><blockquote> `814-825`: **Expose the `telegram` command group from top-level help.** `telegramHelp()` is thorough, but `nemoclaw help` still advertises only `telegram allow`. Adding a `nemoclaw telegram [help]` line, or listing the full group, would make `show`, `clear`, and `discover` discoverable from the main entry point. Also applies to: 1241-1245 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@bin/nemoclaw.jsaround lines 814 - 825, The top-level help is missing the
full telegram command group; update the main help output to expose the telegram
subcommands by either adding a "nemoclaw telegram [help]" line or listing the
full group (allow, show, clear, discover) so they are discoverable; modify the
main help generator (the code that builds the overall CLI help text) to include
the same entries as telegramHelp() (or a single pointer to it) and ensure
consistency with telegramHelp() and the related block around the other help
entries referenced in the PR.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@bin/nemoclaw.js:
- Around line 827-869: The "show", "clear" and "discover" branches in
telegramCommand ignore extra operands (rest) and should reject them; update
telegramCommand so that in the "show" case (case "show"), "clear" case (case
"clear") and "discover" case (case "discover") you check if rest.length > 0 and
if so print a concise error/usage message (e.g., "Unknown telegram operands:
...; valid usage: telegram show|clear|discover") and exit with non-zero
(process.exit(1)); keep existing behavior when rest is empty (call
getCredential/saveCredential/ start as currently implemented). Ensure the checks
reference the existing rest variable and use process.exit(1) for failure.- Around line 724-725: The env writer currently only appends ALLOWED_CHAT_IDS
and NEMOCLAW_TELEGRAM_DISCOVERY when truthy, which leaves stale values intact;
change the logic around getCredential("ALLOWED_CHAT_IDS") and the code that sets
NEMOCLAW_TELEGRAM_DISCOVERY so both variables are always written into envLines
(and the local-startup section) with explicit assignments: write
ALLOWED_CHAT_IDS=<shellQuote(allowedChatIds)> when present and ALLOWED_CHAT_IDS=
(empty) when not, and write NEMOCLAW_TELEGRAM_DISCOVERY=1 (or =0 depending on
the discovery flag) when enabled and NEMOCLAW_TELEGRAM_DISCOVERY= (or =0) when
disabled, ensuring you update the code that constructs envLines and the local
startup env block to unconditionally emit these keys.In
@test/cli.test.js:
- Around line 154-155: The test assertions in test/cli.test.js are checking for
quoted env values but the bash stub (which prints SANDBOX_NAME and
ALLOWED_CHAT_IDS without quotes) produces unquoted output; update the two
assertions that reference marker (the expect calls checking
"SANDBOX_NAME='alpha'" and "ALLOWED_CHAT_IDS='12345,67890'") to remove the
single quotes so they assert "SANDBOX_NAME=alpha" and
"ALLOWED_CHAT_IDS=12345,67890" to match the stub output.
Outside diff comments:
In@bin/nemoclaw.js:
- Around line 51-58: The CLI reserves command names in GLOBAL_COMMANDS which can
shadow sandbox names and make them unreachable; update validation to prevent
this by either adding a reserved-name check in validateName() or
registerSandbox() to reject any sandbox name that exists in GLOBAL_COMMANDS, or
implement an escape hatch in CLI dispatch (e.g., support a "--" separator) so
registry.getSandbox() is consulted when the user explicitly escapes global
parsing; modify the code paths that build/dispatch commands (where
GLOBAL_COMMANDS is checked and where registry.getSandbox() is called) to apply
the chosen fix and include clear error messaging instructing users about
reserved names or how to use the escape hatch.
Nitpick comments:
In@bin/nemoclaw.js:
- Around line 814-825: The top-level help is missing the full telegram command
group; update the main help output to expose the telegram subcommands by either
adding a "nemoclaw telegram [help]" line or listing the full group (allow, show,
clear, discover) so they are discoverable; modify the main help generator (the
code that builds the overall CLI help text) to include the same entries as
telegramHelp() (or a single pointer to it) and ensure consistency with
telegramHelp() and the related block around the other help entries referenced in
the PR.In
@docs/deployment/deploy-to-remote-gpu.md:
- Around line 66-67: Split the long paragraph into separate lines so each
sentence is on its own line and apply inline code formatting to configuration
and command tokens; specifically, change the sentence containing
ALLOWED_CHAT_IDS to useALLOWED_CHAT_IDS(wrap in backticks) and ensure the
two actions are separate sentences/lines mentioningnemoclaw telegram allow <chat-id>andnemoclaw start --discover-chat-idas inline code examples (one
sentence per line) for readability and diff friendliness.In
@docs/reference/commands.md:
- Around line 184-193: Update the "nemoclaw telegram" docs block to include a
one-line description for each subcommand so readers know their purpose; for
example, add short phrases after or beneath the listed commands for "nemoclaw
telegram allow" (add a user/chat ID to the bridge allowlist), "nemoclaw telegram
show" (display current allowlist entries), "nemoclaw telegram clear" (remove all
entries from the allowlist), and "nemoclaw telegram discover" (scan/identify
Telegram chats or users available to add). Reference the exact command names
shown (nemoclaw telegram allow, show, clear, discover) when inserting the
descriptions.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `0569923d-bdb1-4e4d-8103-1e59825cc98d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c63d37b93a98ac2d9149014280e41decc1e68662 and 92732d0bfa63cfcc047c16cc4001d3a7eb5aeb5c. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `bin/nemoclaw.js` * `docs/deployment/deploy-to-remote-gpu.md` * `docs/deployment/set-up-telegram-bridge.md` * `docs/reference/architecture.md` * `docs/reference/commands.md` * `scripts/start-services.sh` * `scripts/telegram-bridge.js` * `test/cli.test.js` * `test/service-env.test.js` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Addressed the outstanding review feedback in
I resolved the inline review threads tied to these changes. The remaining CodeRabbit summary comment should be superseded by the next review pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
49-50: Extract sandbox-name resolution/error handling into a helper.The shared validator is the right move, but these changes now split sandbox-name policy across two call sites and one regex over
err.message. Pulling that into a small helper would make the contract explicit and keepcreateSandbox()from getting any denser.As per coding guidelines,
**/*.{js,ts,tsx}: Maintain cyclomatic complexity limit of 20 (target: ratchet down to 15) for all functions.Also applies to: 2270-2306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 49 - 50, The sandbox-name validation and error-resolution logic is split between validateSandboxName and createSandbox and even inspects err.message with a regex; extract that logic into a single helper (e.g., resolveSandboxNameOrThrow) that encapsulates: invoking validateSandboxName, normalizing/deriving the sandbox name, and producing clear, structured errors (not relying on regexes over err.message); replace the inline handling in createSandbox and any other call sites (reference validateSandboxName and createSandbox) to call this helper so all policy and error messages live in one function and reduce createSandbox cyclomatic complexity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cli.test.js`:
- Around line 112-117: The tests call runWithEnv("start", {...}) but only blank
TELEGRAM_BOT_TOKEN, leaving ALLOWED_CHAT_IDS from the outer environment to leak
into subprocesses because getCredential() reads process.env; update every
runWithEnv env object used in start-path tests (the calls around the existing
runWithEnv invocation and the other start tests referenced) to include
ALLOWED_CHAT_IDS: "" alongside TELEGRAM_BOT_TOKEN: "" (i.e., change the env
object passed to runWithEnv to { HOME: home, PATH:
`${localBin}:${process.env.PATH || ""}`, TELEGRAM_BOT_TOKEN: "",
ALLOWED_CHAT_IDS: "" }) so start subprocesses are hermetic when getCredential()
is called.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 49-50: The sandbox-name validation and error-resolution logic is
split between validateSandboxName and createSandbox and even inspects
err.message with a regex; extract that logic into a single helper (e.g.,
resolveSandboxNameOrThrow) that encapsulates: invoking validateSandboxName,
normalizing/deriving the sandbox name, and producing clear, structured errors
(not relying on regexes over err.message); replace the inline handling in
createSandbox and any other call sites (reference validateSandboxName and
createSandbox) to call this helper so all policy and error messages live in one
function and reduce createSandbox cyclomatic complexity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dff0ad90-b6b4-4877-b595-848bbb22b865
📒 Files selected for processing (7)
bin/lib/onboard.jsbin/lib/sandbox-names.jsbin/nemoclaw.jsdocs/deployment/deploy-to-remote-gpu.mddocs/reference/commands.mdtest/cli.test.jstest/runner.test.js
✅ Files skipped from review due to trivial changes (2)
- docs/deployment/deploy-to-remote-gpu.md
- docs/reference/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/nemoclaw.js
wscurran
added
status: triage
|
✨ Thanks for submitting this pull request, which proposes a way to require a Telegram chat allowlist for bridge access. Possibly related open issues: |
wscurran
added
the
Integration: Telegram
2 participants
Summary
nemoclaw telegramCLI commands to save, inspect, clear, and discover the Telegram allowlistnemoclaw startand remote deploy flows, and document the secure setup pathRoot Cause
The Telegram bridge accepted messages from any chat when
ALLOWED_CHAT_IDSwas unset. That made the bridge fail open: once an operator enabled the bot, any Telegram user who could reach it could send prompts into the sandboxed agent runtime.Impact
The bridge now fails closed by default. Normal forwarding requires an allowlist, while discovery mode replies with the sender chat ID and does not invoke the agent.
Validation
npm test -- test/service-env.test.js test/cli.test.jsnpm exec eslint -- bin/nemoclaw.js scripts/telegram-bridge.js test/service-env.test.js test/cli.test.jsnpm exec prettier -- --check bin/nemoclaw.js scripts/telegram-bridge.js test/service-env.test.js test/cli.test.jsSummary by CodeRabbit
New Features
Bug Fixes / UX
Documentation
Tests