fix: harden telegram bridge against command injection#1206
Open
jyaunches wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix: harden telegram bridge against command injection#1206jyaunches wants to merge 1 commit intoNVIDIA:mainfrom
jyaunches wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Security fixes: - Pass user messages and API key via stdin instead of shell interpolation - Use 'read -r' and 'MSG=$(cat)' pattern to safely read credentials - Strip leading hyphens in sanitizeSessionId() to prevent option injection - Add message length validation (4096 char limit, matches Telegram) - Use execFileSync instead of execSync for external commands - Use cryptographically random temp file paths (mkdtempSync) - Set restrictive permissions (0o600) on temp SSH config files - Clean up temp files in both success and error paths Testability improvements: - Add exitOnError option to initConfig() for testing without process.exit() - Return configuration object from initConfig() for verification - Export runAgentInSandbox, sanitizeSessionId, initConfig for testing New tests: - Unit tests for sanitizeSessionId (alphanumeric, hyphens, injection payloads) - Unit tests for initConfig validation - Source code pattern tests verifying security measures - E2E tests for command injection prevention (T1-T17) - E2E tests for multi-line message handling - E2E tests for session ID option injection prevention Also adds .specs/ to .gitignore. Fixes NVIDIA#118
Contributor
📝 WalkthroughWalkthroughThis pull request addresses a critical command injection vulnerability in the Telegram bridge by replacing shell string interpolation with stdin-based credential and message passing. It introduces configuration initialization, session ID sanitization, message length enforcement, and comprehensive security test coverage. Changes
Sequence DiagramsequenceDiagram
participant TG as Telegram Bridge
participant SSH as SSH Connection
participant SandBox as Sandbox Env
Note over TG: Old Approach (Vulnerable)
TG->>TG: message = user input
TG->>TG: cmd = "export API_KEY='${key}' && nemoclaw... -m '${escaped}'"
TG->>SSH: execute interpolated command string
SSH->>SandBox: runs command (injection possible)
Note over TG: New Approach (Secure)
TG->>TG: initConfig() loads & validates settings
TG->>TG: sanitizeSessionId() strips metacharacters
TG->>TG: validate message length ≤ 4096
TG->>SSH: spawn with ['nemoclaw-start', 'openclaw', ...] array args
TG->>SSH: stdin line 1: API_KEY
TG->>SSH: stdin line 2+: message content
SSH->>SandBox: read -r API_KEY < stdin
SSH->>SandBox: MSG=$(cat) < stdin
SSH->>SandBox: execute with variables, no string interpolation
SandBox-->>SSH: output
SSH-->>TG: result
TG->>TG: cleanup temp SSH config
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/telegram-bridge.test.js (1)
17-45: Definerequireexplicitly before touchingrequire.cachein this ESM test.This file is authored as ESM, but
createMockBridge()still uses barerequire/require.cache. In Node ESM, CommonJSrequireis obtained viacreateRequire(import.meta.url), so this helper should define its own localrequireinstead of relying on implicit runtime behavior. (nodejs.org)💡 Minimal fix
import fs from "node:fs"; +import { createRequire } from "node:module"; import path from "node:path"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +const require = createRequire(import.meta.url); + const TELEGRAM_BRIDGE_JS = path.join(import.meta.dirname, "..", "scripts", "telegram-bridge.js");As per coding guidelines,
test/**/*.test.js: Use ES modules (import/export) for test files in thetest/directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telegram-bridge.test.js` around lines 17 - 45, The test uses bare require/require.cache inside createMockBridge(), which isn't valid in ESM; import createRequire from "module" and create a local require via createRequire(import.meta.url) at the top of the test, then use that local require for require.resolve, require.cache access and module loading inside createMockBridge(); update references in createMockBridge() (e.g., require.resolve, require.cache, require("../scripts/telegram-bridge.js")) to use the new local require so the mock and cache manipulation work under ESM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/telegram-bridge.js`:
- Around line 168-173: sanitizeSessionId currently strips all leading hyphens
which prevents option-injection but also makes "-123456789" collide with
"123456789"; modify sanitizeSessionId so it only preserves a single leading '-'
when the cleaned token matches a negative numeric chat id (i.e., pattern
/^-\d+$/), but still remove other leading hyphens for non-numeric values to
avoid option injection; keep the same overall flow and return null for empty
results. Ensure you update the logic inside sanitizeSessionId to first apply the
alphanumeric-and-hyphen filter, then conditionally restore/keep a single leading
'-' only when the rest is all digits.
In `@test/e2e/test-telegram-injection.sh`:
- Around line 466-507: The test writes helpers to /tmp but then requires the
bridge relatively (require('./scripts/telegram-bridge.js')), which picks up
/tmp/scripts/telegram-bridge.js and can let sandbox short-circuit; change the
require to load the bridge from the repository root (use
path.join(process.cwd(), 'scripts/telegram-bridge.js') or require.resolve with
process.cwd()) so the test imports $REPO/scripts/telegram-bridge.js instead of
/tmp; update the same change in the other test file that uses the relative
require (test-session-id.js) to keep both tests consistent and avoid false SAFE
results for runAgentInSandbox.
---
Nitpick comments:
In `@test/telegram-bridge.test.js`:
- Around line 17-45: The test uses bare require/require.cache inside
createMockBridge(), which isn't valid in ESM; import createRequire from "module"
and create a local require via createRequire(import.meta.url) at the top of the
test, then use that local require for require.resolve, require.cache access and
module loading inside createMockBridge(); update references in
createMockBridge() (e.g., require.resolve, require.cache,
require("../scripts/telegram-bridge.js")) to use the new local require so the
mock and cache manipulation work under ESM.
🪄 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: 9c954544-9f72-4355-9b27-c72a3611ba4a
📒 Files selected for processing (4)
.gitignorescripts/telegram-bridge.jstest/e2e/test-telegram-injection.shtest/telegram-bridge.test.js
Comment on lines
+168
to
+173
| function sanitizeSessionId(sessionId) { | ||
| // Strip all non-alphanumeric characters except hyphens | ||
| const sanitized = String(sessionId).replace(/[^a-zA-Z0-9-]/g, ""); | ||
| // Remove leading hyphens to prevent option injection (e.g., "--help", "-v") | ||
| const noLeadingHyphen = sanitized.replace(/^-+/, ""); | ||
| return noLeadingHyphen.length > 0 ? noLeadingHyphen : null; |
Contributor
There was a problem hiding this comment.
Preserve the sign for numeric chat IDs instead of deleting it.
Stripping the leading - fixes option injection, but it also maps -123456789 and 123456789 to the same sanitized value. That collapses distinct Telegram chats onto the same tg-${safeSessionId} conversation and can bleed agent state between a group chat and a private chat.
💡 One way to keep the fix without losing uniqueness
function sanitizeSessionId(sessionId) {
- // Strip all non-alphanumeric characters except hyphens
- const sanitized = String(sessionId).replace(/[^a-zA-Z0-9-]/g, "");
- // Remove leading hyphens to prevent option injection (e.g., "--help", "-v")
- const noLeadingHyphen = sanitized.replace(/^-+/, "");
- return noLeadingHyphen.length > 0 ? noLeadingHyphen : null;
+ const raw = String(sessionId).trim();
+ const prefix = /^-\d+$/.test(raw) ? "neg-" : "";
+ const sanitized = raw.replace(/[^a-zA-Z0-9-]/g, "").replace(/^-+/, "");
+ return sanitized.length > 0 ? `${prefix}${sanitized}` : null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function sanitizeSessionId(sessionId) { | |
| // Strip all non-alphanumeric characters except hyphens | |
| const sanitized = String(sessionId).replace(/[^a-zA-Z0-9-]/g, ""); | |
| // Remove leading hyphens to prevent option injection (e.g., "--help", "-v") | |
| const noLeadingHyphen = sanitized.replace(/^-+/, ""); | |
| return noLeadingHyphen.length > 0 ? noLeadingHyphen : null; | |
| function sanitizeSessionId(sessionId) { | |
| const raw = String(sessionId).trim(); | |
| const prefix = /^-\d+$/.test(raw) ? "neg-" : ""; | |
| const sanitized = raw.replace(/[^a-zA-Z0-9-]/g, "").replace(/^-+/, ""); | |
| return sanitized.length > 0 ? `${prefix}${sanitized}` : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/telegram-bridge.js` around lines 168 - 173, sanitizeSessionId
currently strips all leading hyphens which prevents option-injection but also
makes "-123456789" collide with "123456789"; modify sanitizeSessionId so it only
preserves a single leading '-' when the cleaned token matches a negative numeric
chat id (i.e., pattern /^-\d+$/), but still remove other leading hyphens for
non-numeric values to avoid option injection; keep the same overall flow and
return null for empty results. Ensure you update the logic inside
sanitizeSessionId to first apply the alphanumeric-and-hyphen filter, then
conditionally restore/keep a single leading '-' only when the rest is all
digits.
Comment on lines
+466
to
+507
| cat >/tmp/test-real-bridge.js <<'TESTSCRIPT' | ||
| // Test script to invoke the real runAgentInSandbox() function | ||
| const path = require('path'); | ||
|
|
||
| // Mock resolveOpenshell to use the system openshell | ||
| const resolveOpenshellPath = require.resolve(path.join(process.cwd(), 'bin/lib/resolve-openshell')); | ||
| require.cache[resolveOpenshellPath] = { | ||
| exports: { resolveOpenshell: () => process.env.OPENSHELL_PATH || 'openshell' } | ||
| }; | ||
|
|
||
| const bridge = require('./scripts/telegram-bridge.js'); | ||
|
|
||
| async function test() { | ||
| const message = process.argv[2] || 'Hello'; | ||
| const sessionId = process.argv[3] || 'e2e-test'; | ||
| const apiKey = process.env.NVIDIA_API_KEY; | ||
| const sandbox = process.env.NEMOCLAW_SANDBOX_NAME || 'e2e-test'; | ||
| const openshell = process.env.OPENSHELL_PATH || 'openshell'; | ||
|
|
||
| if (!apiKey) { | ||
| console.error('NVIDIA_API_KEY required'); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| try { | ||
| // Call the real runAgentInSandbox with options override for testing | ||
| const result = await bridge.runAgentInSandbox(message, sessionId, { | ||
| apiKey, | ||
| sandbox, | ||
| openshell, | ||
| }); | ||
| console.log('RESULT_START'); | ||
| console.log(result); | ||
| console.log('RESULT_END'); | ||
| } catch (err) { | ||
| console.error('ERROR:', err.message); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| test(); | ||
| TESTSCRIPT |
Contributor
There was a problem hiding this comment.
Require telegram-bridge.js via the repo path, not relative to /tmp.
Both helpers are written into /tmp, so require('./scripts/telegram-bridge.js') resolves to /tmp/scripts/telegram-bridge.js, not $REPO/scripts/telegram-bridge.js. That lets Phase 7/8/9 short-circuit before loading the bridge and still report SAFE, which turns these new security checks into false positives.
💡 Minimal fix
const path = require('path');
...
-const bridge = require('./scripts/telegram-bridge.js');
+const bridge = require(path.join(process.cwd(), 'scripts', 'telegram-bridge.js'));Apply the same change in both /tmp/test-real-bridge.js and /tmp/test-session-id.js.
Also applies to: 700-723
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/test-telegram-injection.sh` around lines 466 - 507, The test writes
helpers to /tmp but then requires the bridge relatively
(require('./scripts/telegram-bridge.js')), which picks up
/tmp/scripts/telegram-bridge.js and can let sandbox short-circuit; change the
require to load the bridge from the repository root (use
path.join(process.cwd(), 'scripts/telegram-bridge.js') or require.resolve with
process.cwd()) so the test imports $REPO/scripts/telegram-bridge.js instead of
/tmp; update the same change in the other test file that uses the relative
require (test-session-id.js) to keep both tests consistent and avoid false SAFE
results for runAgentInSandbox.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a command injection vulnerability in the Telegram bridge where user messages were interpolated into shell command strings, allowing attackers to execute arbitrary commands via
$(), backticks, or quote escapes.Security Fixes
Stdin-Based Message Passing
User messages and API credentials are now passed via stdin instead of shell interpolation:
Option Injection Prevention
sanitizeSessionId()now strips leading hyphens to prevent session IDs like--helpfrom being interpreted as command-line flags:Additional Hardening
execFileSyncinstead ofexecSyncfor external commandsmkdtempSync)New Tests
Unit Tests (
test/telegram-bridge.test.js) - NEW FILEsanitizeSessionId: alphanumeric, hyphens, shell metacharacters, option injectioninitConfig: validation, error handling, return valueE2E Tests (
test/e2e/test-telegram-injection.sh) - ENHANCEDrunAgentInSandbox()code pathFiles Changed
scripts/telegram-bridge.jstest/telegram-bridge.test.jstest/e2e/test-telegram-injection.sh.gitignore.specs/directoryTesting
Fixes #118
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores