security: fix command injection in Telegram bridge

[kakuteki]

Summary

Fixes #118 — Command injection vulnerability in scripts/telegram-bridge.js

The Telegram bridge interpolated user messages directly into a shell command string passed to SSH. The existing escaping (single-quote only) did not prevent $() or backtick expansion, allowing attackers to execute arbitrary commands inside the sandbox and potentially exfiltrate the NVIDIA_API_KEY.

Changes

• Pass user message and API key via stdin instead of shell string interpolation — the remote script reads the API key with read -r (line 1) and the message with cat (remaining stdin), then expands them inside double-quoted "$VAR" which is not subject to further shell parsing
• Validate sessionId — strip non-alphanumeric characters (Telegram chat IDs are numeric)
• Validate SANDBOX_NAME at startup — reject names with shell metacharacters to prevent injection in the execSync call
• Change stdio from ["ignore", ...] to ["pipe", ...] so stdin is writable

Before (vulnerable)

const escaped = message.replace(/'/g, "'\''");
const cmd = `export NVIDIA_API_KEY='${API_KEY}' && nemoclaw-start ... -m '${escaped}' ...`;
spawn("ssh", [..., cmd], { stdio: ["ignore", "pipe", "pipe"] });

After (fixed)

const remoteScript = "read -r NVIDIA_API_KEY && export NVIDIA_API_KEY && MSG=$(cat) && exec nemoclaw-start ... -m \"$MSG\" ...";
const proc = spawn("ssh", [..., remoteScript], { stdio: ["pipe", "pipe", "pipe"] });
proc.stdin.write(API_KEY + "\n");
proc.stdin.write(message);
proc.stdin.end();

Test plan

• Send a normal message via Telegram → agent responds correctly
• Send a message containing $(whoami) → treated as literal text, no command execution
• Send a message containing backticks and single quotes → no injection
• Set an invalid SANDBOX_NAME (e.g. foo;rm -rf /) → startup exits with error
• Verify NVIDIA_API_KEY no longer appears in process arguments (ps aux)

Summary by CodeRabbit

• Bug Fixes
  • Enhanced validation of configuration names to prevent invalid entries and improve error handling.
  • Improved sanitization of session identifiers for greater stability.
  • Strengthened security handling for sensitive credentials by preventing direct command-line exposure and reducing potential injection vulnerabilities.
  • Refined process communication to ensure more secure data transmission.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request addresses a command injection vulnerability in the Telegram Bridge by sanitizing inputs (session ID and SANDBOX name), validating characters, and refactoring data passage from command-line embedding to stdin, preventing attackers from executing arbitrary commands via malicious messages.

Changes

Cohort / File(s)

Summary

Telegram Bridge Security Hardening scripts/telegram-bridge.js

Added SANDBOX name validation with invalid character checks. Introduced sessionId sanitization to safeSessionId. Replaced static SSH command construction with a remoteScript that reads NVIDIA_API_KEY and message from stdin instead of embedding them in the shell command string, eliminating command injection attack surface. Updated SSH stdio handling and temp config path to use sanitized session ID.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 With whiskers twitched and paws held high,
A rabbit cheers as injections die!
Through stdin flows the secret key,
Command injection? Never more—we're free!
Sanitized and safe, the bridge now stands,
Guarded by this coder's careful hands. 🔐

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'security: fix command injection in Telegram bridge' clearly and concisely summarizes the primary change—fixing a command injection vulnerability in the Telegram bridge script.
Linked Issues check	✅ Passed	All coding requirements from issue #118 are met: command injection is fixed via stdin-based message passing, sessionId and SANDBOX_NAME are validated, SSH stdio is configured for stdin writability, and double-quoted variable expansion prevents shell parsing.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to addressing the command injection vulnerability in scripts/telegram-bridge.js as specified in issue #118; no unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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

Tip

CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.

OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required.

[coderabbitai]

🧹 Nitpick comments (1)

scripts/telegram-bridge.js (1)

25-27: Consider making ALLOWED_CHAT_IDS mandatory for production deployments.

The optional ALLOWED_CHAT_IDS whitelist is a good defense-in-depth mechanism. For production, enforcing this (or at least emitting a warning when unset) would reduce attack surface by ensuring only trusted Telegram chats can interact with the agent.

 const ALLOWED_CHATS = process.env.ALLOWED_CHAT_IDS
   ? process.env.ALLOWED_CHAT_IDS.split(",").map((s) => s.trim())
   : null;
+
+if (!ALLOWED_CHATS) {
+  console.warn("WARNING: ALLOWED_CHAT_IDS not set — all Telegram chats can interact with this bot");
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/telegram-bridge.js` around lines 25 - 27, Make ALLOWED_CHAT_IDS
mandatory (or emit a clear warning) by checking process.env.ALLOWED_CHAT_IDS at
startup and failing fast or logging a high‑severity warning if it's missing;
update the ALLOWED_CHATS initialization (the variable ALLOWED_CHATS and the
process.env.ALLOWED_CHAT_IDS usage) so that when the env var is absent the
process either exits with a descriptive error message or logs a prominent
warning advising this must be set in production, and ensure the message includes
guidance on the expected comma-separated format.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@scripts/telegram-bridge.js`:
- Around line 25-27: Make ALLOWED_CHAT_IDS mandatory (or emit a clear warning)
by checking process.env.ALLOWED_CHAT_IDS at startup and failing fast or logging
a high‑severity warning if it's missing; update the ALLOWED_CHATS initialization
(the variable ALLOWED_CHATS and the process.env.ALLOWED_CHAT_IDS usage) so that
when the env var is absent the process either exits with a descriptive error
message or logs a prominent warning advising this must be set in production, and
ensure the message includes guidance on the expected comma-separated format.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9730f601-7e91-4bf6-908f-ec6ee23bb938

📥 Commits

Reviewing files that changed from the base of the PR and between 241ffb2 and fa74697.

📒 Files selected for processing (1)

• scripts/telegram-bridge.js

[kakuteki]

Hi maintainers, could you please approve the CI workflow run for this PR? I've rebased onto the latest main. Thank you!

[north-echo]

Security Review, PR #119

The stdin-based approach for passing the API key and user message is the right call here. It eliminates shell interpretation of user input on the remote side entirely. This is the correct fix for the core command injection (issue #118). A few gaps worth addressing:

1. execSync still used for ssh-config (line 95)

const sshConfig = execSync(openshell sandbox ssh-config ${SANDBOX}, { encoding: "utf-8" });

Even though SANDBOX is validated at startup, this is still string interpolation passed to a shell. PR #320 gets this right with execFileSync("openshell", ["sandbox", "ssh-config", SANDBOX]). Defense in depth: the validation is one layer, parameterized execution is another.

2. SANDBOX regex allows leading hyphens

if (!/^[a-zA-Z0-9_-]+$/.test(SANDBOX)) {

This matches -v, --help, etc., which could be interpreted as options by openshell. PR #320's pattern is better:

/^[A-Za-z0-9][A-Za-z0-9_-]*$/

Requiring an alphanumeric first character prevents option injection.

3. Predictable temp file path

const confPath = /tmp/nemoclaw-tg-ssh-${safeSessionId}.conf;

safeSessionId is derived from the Telegram chat ID, a public integer. On multi-user systems this is a symlink race target (CWE-377). PR #320 solves this with crypto.randomBytes(8).toString("hex") and exclusive creation (flag: "wx").

4. No message length cap

Unbounded user input passed to the agent. A reasonable cap (e.g., 4096 chars matching Telegram's own limit) would prevent abuse.

5. Bare openshell vs resolved path

The script imports resolveOpenshell() and validates it at startup, but the execSync call on line 95 uses bare openshell instead of the resolved OPENSHELL path.

Recommendation

This PR and #320 are complementary, not competing. This PR fixes the core injection; #320 fixes the surrounding hardening (execFileSync, temp file races, better SANDBOX regex). Combining both would give a comprehensive fix. Consider adopting #320's hardening into this branch, or coordinating with @pjt222.

The stdin approach itself is sound.

Christopher Lusk (christopherdlusk@gmail.com)

[wscurran]

Thanks for identifying and fixing the command injection vulnerability, this is a critical security fix that will help protect NemoClaw users.

[kakuteki]

@north-echo Thank you for the thorough security review. I'll coordinate with @pjt222 to address the points you raised.

[cv]

Not a duplicate of #617 (bridge framework). Your stdin-based approach (passing the API key and message via stdin instead of shell interpolation) is a different and arguably more robust mitigation than #617's shellQuote() approach. shellQuote can be bypassed if the quoting function has edge cases; stdin avoids shell parsing entirely.

However, #617 rewrites telegram-bridge.js into a backwards-compat wrapper that delegates to bridge.js. If #617 merges first, the file this PR patches will no longer contain the vulnerable code path. The stdin approach would need to be applied to bridge-core.js instead.

Worth coordinating with #617 on merge order.

[jyaunches]

Hi! Thanks for this contribution. It looks like this branch has fallen a bit behind main and has some merge conflicts. Could you please rebase or merge with main to resolve the conflicts? Let me know if you need any help. Thanks!