Feature/ssh mcp#133
Conversation
Expose guarded SSH command execution for Pressable and WordPress.com Atomic sites and document the sanctioned exception. Added run_wpcom_site_ssh_command and run_pressable_site_ssh_command helpers, MCP tool methods (wpcom_run_ssh_command, pressable_run_ssh_command), and server-side guardrails: a catastrophic-command denylist (is_blocked_ssh_command) and an audit logger (audit_ssh_command). Updated docs (.agents/skills/add-mcp-tool.md, AGENTS.md, README.md) to explain the guardrail+audit+destructiveHint pattern and the requirement that clients show per-command human approval (these tools must not be auto-approved). Also updated pressable_open_site_shell error text and bumped the README tool count. Denylist is explicitly defense-in-depth and not a substitute for the human approval prompt.
Accept full URLs, bare domains, or numeric IDs in run_pressable_site_ssh_command and run_wpcom_site_ssh_command by normalizing URL inputs to their host when the string contains 'http'. This mirrors get_site_input() behavior and ensures site resolution works when callers pass a full URL; if parse_url doesn't return a host the original input is preserved.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughTwo new MCP tools — 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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
🧹 Nitpick comments (1)
mcp/Team51McpTools.php (1)
1990-1992: ⚡ Quick winAudit blocked SSH attempts as separate security events.
Both tools return early on denylist hits before calling
audit_ssh_command(), so blocked high-risk attempts currently leave no audit trail.Also applies to: 2306-2308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/Team51McpTools.php` around lines 1990 - 1992, Blocked SSH commands detected by is_blocked_ssh_command() return early without creating an audit trail. At mcp/Team51McpTools.php lines 1990-1992 (the anchor location with the is_blocked_ssh_command check) and lines 2306-2308 (the sibling location with the same check), you must call audit_ssh_command() with the blocked SSH command details BEFORE the early return statement. This ensures that all blocked high-risk SSH attempts are recorded as separate security events in the audit log, preventing security events from being missed due to early returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@includes/functions-wpcom.php`:
- Around line 1005-1010: The setTimeout(0) call disables the timeout
indefinitely, causing the exec() method to block until the remote command
completes or the connection is lost, which can hang worker processes.
Additionally, the output callback appends data to the $output variable without
any size limit, risking memory exhaustion on long-running commands. Replace the
hardcoded setTimeout(0) with a finite configurable timeout value read from an
environment variable (with a sensible default), and implement bounded output
capture by tracking the accumulated output size and stopping the append
operation once a configurable maximum output size is exceeded. Apply the same
timeout and output bounding fix pattern to other locations where exec() is
called with unbounded output streaming.
In `@mcp/Team51McpTools.php`:
- Around line 232-245: The audit_ssh_command method logs raw shell commands
which may contain sensitive credentials or tokens passed as arguments. Create a
new private static helper method called sanitize_command_for_audit that uses
regex pattern matching to redact common credential patterns like --password=,
--token=, and --secret= (replacing the values with [REDACTED]), and also
truncates the command to a maximum of 512 characters for safety. Then, in the
audit_ssh_command method, replace the direct reference to trim($command) in the
'command' array field with a call to this new sanitize_command_for_audit helper
function to ensure sensitive values are redacted before logging.
In `@README.md`:
- Around line 123-124: The README.md statement at line 123-124 claims that
high-risk operations like site creation and deployments are excluded from MCP,
but this contradicts the actual tool surface exposed in mcp/Team51McpTools.php
which still includes these legacy high-risk operations. Reword the README
section to accurately reflect which operations are actually available through
the MCP interface, either by removing the false exclusion claim or by updating
it to match the real tool implementation.
---
Nitpick comments:
In `@mcp/Team51McpTools.php`:
- Around line 1990-1992: Blocked SSH commands detected by
is_blocked_ssh_command() return early without creating an audit trail. At
mcp/Team51McpTools.php lines 1990-1992 (the anchor location with the
is_blocked_ssh_command check) and lines 2306-2308 (the sibling location with the
same check), you must call audit_ssh_command() with the blocked SSH command
details BEFORE the early return statement. This ensures that all blocked
high-risk SSH attempts are recorded as separate security events in the audit
log, preventing security events from being missed due to early returns.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b168c6e-38ee-489c-9607-63c9a2ddccb0
📒 Files selected for processing (6)
.agents/skills/add-mcp-tool.mdAGENTS.mdREADME.mdincludes/functions-pressable.phpincludes/functions-wpcom.phpmcp/Team51McpTools.php
| $ssh->setTimeout( 0 ); // Disable timeout in case the command takes a long time. | ||
| $ssh->exec( | ||
| $ssh_command, | ||
| static function ( string $str ) use ( &$output ): void { | ||
| $output .= $str; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In phpseclib3 Net\SSH2, does setTimeout(0) disable exec() timeout indefinitely, and what timeout strategy is recommended for production command execution?
💡 Result:
In phpseclib3, setting setTimeout(0) or setTimeout(false) indicates that there is no timeout, meaning the exec() call will block indefinitely until the remote command finishes or the underlying connection is lost [1][2][3]. However, "no timeout" at the library level does not protect the process from external constraints. For production command execution, a robust strategy should include the following considerations: 1. Application-Level Timeouts: Always define a reasonable maximum duration for your command execution using setTimeout($seconds). Relying on infinite blocking can lead to hung worker processes, exhausted connection pools, or PHP script execution limit errors if the remote command hangs or the network connection drops silently [3][4]. 2. Keepalives: For long-running commands, use setKeepAlive($interval) to prevent intermediate network infrastructure or the remote sshd from closing the connection due to inactivity [4]. Note that this only maintains the SSH connection; it does not stop the remote command if it runs longer than your intended logic [4]. 3. Remote-Side Timeouts: If you need to enforce a strict duration on the command itself, it is safer to use the remote system's tools, such as the GNU timeout utility (e.g., $ssh->exec('timeout 300 your_command')), rather than relying solely on the client-side library timeout [5]. 4. Resource Cleanup: If a phpseclib timeout occurs, the underlying channel may remain in an inconsistent state. Always call $ssh->reset() if you intend to reuse the SSH connection for subsequent commands after a timeout has been triggered [4][6]. 5. External Monitoring: For critical production tasks, ensure your architecture monitors for hanging processes at the OS level, as killing a phpseclib process may not necessarily terminate the command process currently running on the remote server [6].
Citations:
- 1: https://api.phpseclib.com/3.0/phpseclib3/Net/SSH2.html
- 2: https://api.phpseclib.com/master/phpseclib3/Net/SSH2.html
- 3: Timeout cannot be disabled phpseclib/phpseclib#775
- 4: https://phpseclib.com/docs/commands
- 5: setTimeout doesn't work with the sleep command phpseclib/phpseclib#1440
- 6: SSH2 (branch v3) : exec and timeout : how to stop the command ? phpseclib/phpseclib#2046
Bound SSH runtime and output size to avoid hanging MCP requests.
Line 1005 disables timeouts (setTimeout(0)) and lines 1008-1009 append output without a cap. According to phpseclib3 documentation, setTimeout(0) causes exec() to block indefinitely until the remote command finishes or the connection is lost. A streaming or stuck command can block a worker indefinitely and exhaust memory. Implement a finite timeout (configurable via environment variable) and bounded output capture. Apply the same fix to includes/functions-pressable.php lines 655-660.
Suggested hardening sketch
- $ssh->setTimeout( 0 ); // Disable timeout in case the command takes a long time.
+ $timeout_seconds = (int) ( getenv( 'TEAM51_MCP_SSH_TIMEOUT_SECONDS' ) ?: 300 );
+ $max_output_bytes = (int) ( getenv( 'TEAM51_MCP_SSH_MAX_OUTPUT_BYTES' ) ?: 262144 );
+ $captured_bytes = 0;
+ $truncated = false;
+ $ssh->setTimeout( $timeout_seconds );
$ssh->exec(
$ssh_command,
- static function ( string $str ) use ( &$output ): void {
- $output .= $str;
+ static function ( string $str ) use ( &$output, &$captured_bytes, &$truncated, $max_output_bytes ): void {
+ if ( $captured_bytes >= $max_output_bytes ) {
+ $truncated = true;
+ return;
+ }
+ $remaining = $max_output_bytes - $captured_bytes;
+ $chunk = substr( $str, 0, $remaining );
+ $output .= $chunk;
+ $captured_bytes += strlen( $chunk );
}
);
+ if ( $truncated ) {
+ $output .= "\n[output truncated]";
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@includes/functions-wpcom.php` around lines 1005 - 1010, The setTimeout(0)
call disables the timeout indefinitely, causing the exec() method to block until
the remote command completes or the connection is lost, which can hang worker
processes. Additionally, the output callback appends data to the $output
variable without any size limit, risking memory exhaustion on long-running
commands. Replace the hardcoded setTimeout(0) with a finite configurable timeout
value read from an environment variable (with a sensible default), and implement
bounded output capture by tracking the accumulated output size and stopping the
append operation once a configurable maximum output size is exceeded. Apply the
same timeout and output bounding fix pattern to other locations where exec() is
called with unbounded output streaming.
| private static function audit_ssh_command( string $provider, string $site_id_or_url, string $command ): void { | ||
| $actor = defined( 'OPSOASIS_WP_USERNAME' ) ? OPSOASIS_WP_USERNAME : 'unknown'; | ||
|
|
||
| error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log | ||
| '[MCP SSH AUDIT] ' . ( encode_json_content( | ||
| array( | ||
| 'provider' => $provider, | ||
| 'site' => $site_id_or_url, | ||
| 'command' => trim( $command ), | ||
| 'actor' => $actor, | ||
| 'timestamp' => gmdate( DATE_ATOM ), | ||
| ) | ||
| ) ?? '' ) | ||
| ); |
There was a problem hiding this comment.
Redact sensitive values before writing SSH audit logs.
Line 240 stores the raw command verbatim. Operators often pass credentials/tokens in shell args, so this can leak secrets into persistent logs.
Suggested redaction approach
- 'command' => trim( $command ),
+ 'command' => self::sanitize_command_for_audit( trim( $command ) ),private static function sanitize_command_for_audit( string $command ): string {
$sanitized = preg_replace( '/(--password=|--token=|--secret=)\S+/i', '$1[REDACTED]', $command ) ?? $command;
return mb_substr( $sanitized, 0, 512 );
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/Team51McpTools.php` around lines 232 - 245, The audit_ssh_command method
logs raw shell commands which may contain sensitive credentials or tokens passed
as arguments. Create a new private static helper method called
sanitize_command_for_audit that uses regex pattern matching to redact common
credential patterns like --password=, --token=, and --secret= (replacing the
values with [REDACTED]), and also truncates the command to a maximum of 512
characters for safety. Then, in the audit_ssh_command method, replace the direct
reference to trim($command) in the 'command' array field with a call to this new
sanitize_command_for_audit helper function to ensure sensitive values are
redacted before logging.
| Other high-risk operations (site creation, user deletion, deployments) are intentionally excluded — run them via the CLI. | ||
|
|
There was a problem hiding this comment.
Align exclusion claim with the actual MCP tool surface.
Line 123 says other high-risk operations are excluded from MCP, but mcp/Team51McpTools.php still exposes legacy high-risk tools (e.g., site creation and deployment-related operations). Please reword this to avoid a false security posture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 123 - 124, The README.md statement at line 123-124
claims that high-risk operations like site creation and deployments are excluded
from MCP, but this contradicts the actual tool surface exposed in
mcp/Team51McpTools.php which still includes these legacy high-risk operations.
Reword the README section to accurately reflect which operations are actually
available through the MCP interface, either by removing the false exclusion
claim or by updating it to match the real tool implementation.
Initialize a Symfony Console Application in mcp-server.php and register the internal WP-CLI run commands so run_app_command() does not fatal in MCP mode; add --no-autocomplete option for the commands. Change WP-CLI exec callbacks in both Pressable and WPCOM command classes to reset and append to $GLOBALS['wp_cli_output'] so multi-packet SSH output is captured in full. Replace exit(1) in WPCOM command initialization with throwing \InvalidArgumentException (and add a @throws docblock) to avoid terminating the long-lived MCP server process.
1 participant
Dangerously adds SSH to MCP tooling, needs all the reviews. Calling @ecairol to take a look here since you integrated pretty deeply with this MCP on ContextA8C, here's the ask: https://linear.app/a8c/issue/T51ENG-1873/enable-ssh-command-in-team51-cli-mcp
Summary by CodeRabbit