Prevent command injection vulnerability #269
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a stdin-based configuration path for the voice automatic agent when no CLI args are provided. Renames pool-related config env vars to AUTOMATIC_* and adds new config constants. Updates main fallback to spawn a module subprocess and pass JSON config via stdin. Documentation reflects env var renames. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Starter as Process (no CLI args)
participant Stdin as stdin
participant Agent as Voice Automatic Module
participant Session as handle_session
Starter->>Agent: start()
Note over Agent: Early mode detection (no args)
Agent->>Stdin: readLineAsync()
alt got JSON config
Agent->>Agent: parse JSON
Agent->>Session: handle_session(config)
Session-->>Agent: complete
Agent-->>Starter: return (early path done)
else empty/invalid
Agent->>Agent: log error
Agent-->>Starter: return (no session)
end
sequenceDiagram
autonumber
participant Main as app/main.py
participant Pool as Resource Pools
participant Sub as Subprocess (python -m ...)
participant Pipe as stdin (PIPE)
Main->>Pool: acquire()
alt acquired
Main->>Main: normal flow
else failed
Main->>Sub: spawn(module, stdin=PIPE)
Main->>Pipe: write JSON config
Main->>Pipe: close()
Note over Main,Sub: Config passed via stdin instead of CLI args
Sub-->>Main: run lifecycle / exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/agents/voice/automatic/__init__.py(1 hunks)app/core/config.py(1 hunks)app/main.py(4 hunks)docs/POOL_IMPLEMENTATION.md(2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
app/agents/voice/automatic/__init__.py
103-103: Do not catch blind exception: Exception
(BLE001)
app/main.py
280-280: subprocess call: check for execution of untrusted input
(S603)
292-292: Do not catch blind exception: Exception
(BLE001)
| if proc.stdin: | ||
| try: | ||
| proc.stdin.write(config_json.encode("utf-8")) | ||
| proc.stdin.close() | ||
| except Exception as e: | ||
| logger.error(f"Failed to write to subprocess stdin: {e}") | ||
| bot_procs[proc.pid] = (proc, room_url, session_id, "direct") |
There was a problem hiding this comment.
Propagate fallback stdin write failures instead of returning success.
Right now if proc.stdin.write(...) blows up (e.g., the agent process crashes before reading), we just log the error and still hand the client a “success” response. That leaves them with a Daily room but no agent ever coming online. Mirror the pool path: kill the subprocess, re-raise (and return the room to the pool if applicable) so the caller gets a failure instead of a silent noop.
if proc.stdin:
try:
proc.stdin.write(config_json.encode("utf-8"))
proc.stdin.close()
except Exception as e:
logger.error(f"Failed to write to subprocess stdin: {e}")
+ proc.kill()
+ raise📝 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.
| if proc.stdin: | |
| try: | |
| proc.stdin.write(config_json.encode("utf-8")) | |
| proc.stdin.close() | |
| except Exception as e: | |
| logger.error(f"Failed to write to subprocess stdin: {e}") | |
| bot_procs[proc.pid] = (proc, room_url, session_id, "direct") | |
| if proc.stdin: | |
| try: | |
| proc.stdin.write(config_json.encode("utf-8")) | |
| proc.stdin.close() | |
| except Exception as e: | |
| logger.error(f"Failed to write to subprocess stdin: {e}") | |
| proc.kill() | |
| raise | |
| bot_procs[proc.pid] = (proc, room_url, session_id, "direct") |
🧰 Tools
🪛 Ruff (0.13.1)
292-292: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In app/main.py around lines 288 to 294, the code currently logs exceptions from
proc.stdin.write/config_json and continues as success; instead, on any exception
when writing to the subprocess stdin you must: stop registering the process,
kill/terminate the subprocess (proc.kill() or proc.terminate() then proc.wait())
to avoid zombie/half-started agents, return the room to the pool if this room
was taken from a pool (restore whatever pool state or call the pool-return
helper), and re-raise the exception so the caller receives a failure; ensure
bot_procs[proc.pid] is only set after a successful write/close and that cleanup
happens inside the except before re-raising.
badri-singhal
changed the title
badri-singhal
force-pushed
the
prevent-command-injection-vulnerability
branch
from
516a90f to
e626198
Compare
1 participant
This commit refactors the fallback mechanism for creating voice agent subprocesses to eliminate a potential command injection vulnerability.
Previously, user-provided data was passed as command-line arguments when launching a new agent process directly. This exposed the application to argument injection risks.
The new implementation aligns the fallback behavior with the more secure pattern used by the process pool:
app.agents.voice.automatic) now handles being launched with no arguments by reading this configuration from stdin.This change ensures that user-controlled data is no longer part of the command line, mitigating the security risk and making the process creation mechanism more robust and consistent.
Summary by CodeRabbit