Skip to content

fix(security): bundle Telegram allowlist and update hardening#1500

Closed
13ernkastel wants to merge 5 commits intoNVIDIA:mainfrom
13ernkastel:codex/security-telegram-cleanup-bundle
Closed

fix(security): bundle Telegram allowlist and update hardening#1500
13ernkastel wants to merge 5 commits intoNVIDIA:mainfrom
13ernkastel:codex/security-telegram-cleanup-bundle

Conversation

@13ernkastel
Copy link
Copy Markdown
Contributor

@13ernkastel 13ernkastel commented Apr 5, 2026

Summary

  • group the Telegram allowlist hardening from #1218 with the sandbox self-update-hint fix from #1215
  • keep #1416 and #1499 separate on purpose
  • carry the smaller integration follow-up needed to make the grouped branch fit current main

Related PRs / Issues

  • bundles #1218
  • bundles #1215
  • keeps #1416 separate
  • keeps #1499 separate
  • addresses #896
  • addresses #1029

Why

These two changes both tighten the default runtime posture for operator-managed deployments:

  • #1218 makes Telegram bridge access fail closed unless a chat allowlist is configured, with a safe discovery mode for retrieving chat IDs
  • #1215 removes misleading in-sandbox self-update hints so the supported upgrade path stays image-based instead of mutable in-container updates

Grouping them into one cleanup PR reduces review fragmentation for the remaining security work without collapsing unrelated security follow-ups like #1416 or #1499.

Changes

  • require an explicit Telegram allowlist before the bridge forwards prompts
  • add nemoclaw telegram subcommands and nemoclaw start --discover-chat-id
  • preserve the reserved-sandbox-name guard added during the Telegram review follow-up
  • disable unsupported OpenClaw self-update hints in the generated sandbox config
  • propagate saved Telegram allowlists into the remote deploy env so deployed bridges stay fail-closed too
  • update focused CLI/deploy tests to match the current services-based startup path on main

Follow-up Improvement

  • carry the Telegram allowlist into src/lib/deploy.ts so a locally saved allowlist is not lost when operators use the remote deploy flow

Validation

  • npm run build:cli
  • npx vitest run src/lib/deploy.test.ts test/onboard.test.js test/cli.test.js test/runner.test.js test/service-env.test.js

Risks / Notes

  • npm run typecheck:cli still hits the repo’s existing src/lib/*.test.ts -> ../../dist/lib/* type-resolution issue in this environment, so validation here relies on the targeted build plus Vitest coverage above
  • #1416 and #1499 are intentionally left out of this bundle

Summary by CodeRabbit

  • New Features

    • Added Telegram discovery mode via nemoclaw start --discover-chat-id to retrieve chat IDs without forwarding messages to the agent.
    • Introduced nemoclaw telegram management commands (allow, show, clear, discover) for Telegram allowlist configuration.
    • Added CLI escape mechanism nemoclaw -- <name> <action> to invoke sandboxes with names matching global commands.
    • Disabled automatic update checking on application startup.
  • Improvements

    • Enhanced Telegram bridge status reporting with specific failure reasons.
    • Improved sandbox name validation with clearer error messages.
  • Documentation

    • Updated Telegram bridge setup guides with discovery workflow and allowlist management instructions.
    • Added troubleshooting guidance for update command behavior in sandboxes.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 5, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR introduces sandbox name validation with reserved-name collision detection, extends the CLI with a -- escape mechanism for sandbox-scoped actions, and adds Telegram bridge discovery mode with CLI-based allowlisting. It disables startup update checking via Docker configuration, updates deployment credential handling, and expands test coverage and documentation accordingly.

Changes

Cohort / File(s) Summary
Sandbox Name Validation
bin/lib/sandbox-names.js, bin/lib/onboard.js, test/runner.test.js
New module exports RESERVED_SANDBOX_NAMES, SANDBOX_ACTIONS, and validateSandboxName() function; onboard imports and uses it with try/catch handling; tests verify reserved name rejection and validation logic.
CLI Dispatch & Telegram Management
bin/nemoclaw.js, test/cli.test.js
Added -- escape mechanism to bypass global-command interception, replaced static GLOBAL_COMMANDS with dynamic set from reserved names, extended start with --discover-chat-id flag, introduced nemoclaw telegram subcommands (allow, show, clear, discover), added collision-detection hints; tests verify dispatch behavior, new commands, and escape-hatch invocation.
Telegram Bridge Discovery Mode
scripts/telegram-bridge.js, scripts/start-services.sh, test/service-env.test.js
Added NEMOCLAW_TELEGRAM_DISCOVERY flag for discovery-only mode (returns chat ID instead of forwarding); updated startup conditions to require ALLOWED_CHAT_IDS or discovery mode; refined status reporting for missing allowlist vs API key; tests verify startup validation and status output.
Deploy Credentials & Config
src/lib/deploy.ts, src/lib/deploy.test.ts, Dockerfile
Added optional ALLOWED_CHAT_IDS to DeployCredentials interface, passthrough of NEMOCLAW_TELEGRAM_DISCOVERY in env lines; Dockerfile adds update.checkOnStart: False to openclaw.json; test validates Telegram credential deployment.
Documentation
docs/deployment/deploy-to-remote-gpu.md, docs/deployment/set-up-telegram-bridge.md, docs/reference/architecture.md, docs/reference/commands.md, docs/reference/troubleshooting.md
Updated Telegram prerequisites to include NVIDIA_API_KEY and ALLOWED_CHAT_IDS; documented --discover-chat-id flag and nemoclaw telegram CLI management commands; clarified allowlist requirement in architecture docs; added troubleshooting note on openclaw update timeout in sandboxes.
Integration Tests
test/e2e-gateway-isolation.sh, test/cli.test.js helpers (runWithEnv, stubbing)
Inserted e2e test for update.checkOnStart config validation; updated CLI test helpers to use process.execPath and capture stub files; added test coverage for forwarding stored Telegram allowlist and discovery-mode startup.

Sequence Diagrams

sequenceDiagram
    participant User
    participant CLI as nemoclaw CLI
    participant Dispatch
    participant Sandbox as Sandbox Actions
    participant TgBridge as Telegram Bridge
    
    User->>CLI: nemoclaw start --discover-chat-id
    CLI->>Dispatch: Parse --discover-chat-id flag
    Dispatch->>Dispatch: Set NEMOCLAW_TELEGRAM_DISCOVERY=1
    Dispatch->>Sandbox: start-services.sh with discovery flag
    Sandbox->>TgBridge: Launch telegram-bridge.js (DISCOVERY_ONLY=true)
    TgBridge->>TgBridge: Receive Telegram message
    TgBridge->>User: Reply with chat ID (discovery mode)
    TgBridge->>TgBridge: Skip OpenClaw forwarding
Loading
sequenceDiagram
    participant User
    participant CLI as nemoclaw CLI
    participant Dispatch
    participant Registry as Sandbox Registry
    participant GlobalCmds as Reserved Names
    
    User->>CLI: nemoclaw telegram start
    CLI->>Dispatch: Dispatch sandbox action
    Dispatch->>GlobalCmds: Check if "telegram" is global/reserved
    GlobalCmds-->>Dispatch: "telegram" is reserved
    Dispatch->>User: Error: collision detected
    User->>CLI: nemoclaw -- telegram start
    CLI->>Dispatch: Parse -- escape sequence
    Dispatch->>Registry: Lookup sandbox "telegram"
    Dispatch->>Dispatch: Invoke "start" action on sandbox "telegram"
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

Suggested labels

security, enhancement: testing

Suggested reviewers

  • kjw3
  • ericksoa
  • cv

Poem

🐰 A sandbox by any other name won't start,
But now with -- our CLI's grown smart!
Chat IDs discovered, allowlists saved with care,
Reserved names escape—fair and square! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(security): bundle Telegram allowlist and update hardening' directly and concisely summarizes the main changes: security hardening for Telegram allowlist requirements and disabling update hints, exactly matching the PR objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@13ernkastel
Copy link
Copy Markdown
Contributor Author

Folding this cleanup PR into #1416 per request. The combined sandbox/Telegram/update hardening now lives there; keeping #1499 separate.

@13ernkastel 13ernkastel closed this Apr 5, 2026
@13ernkastel 13ernkastel deleted the codex/security-telegram-cleanup-bundle branch April 5, 2026 04:25
@13ernkastel
Copy link
Copy Markdown
Contributor Author

Consolidated into the open security PR #1416: #1416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant