Skip to content

feat(blocks): add execute command block for self-hosted shell execution#3426

Open
waleedlatif1 wants to merge 15 commits intostagingfrom
waleedlatif1/execute-command-block
Open

feat(blocks): add execute command block for self-hosted shell execution#3426
waleedlatif1 wants to merge 15 commits intostagingfrom
waleedlatif1/execute-command-block

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • Add Execute Command block that runs shell commands on the host machine via child_process.exec
  • Gated behind EXECUTE_COMMAND_ENABLED feature flag, self-hosted only (blocked on hosted)
  • Supports variable injection (<block.output>, {{ENV_VAR}}, <variable.name>)
  • Configurable working directory and timeout
  • Returns stdout, stderr, exitCode — non-zero exits are data, not errors (matches n8n behavior)
  • Added env vars to Helm chart and .env.example

Type of Change

  • New feature

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@cursor
Copy link

cursor bot commented Mar 5, 2026

PR Summary

High Risk
Introduces a new capability to execute arbitrary host shell commands via child_process.exec; even with self-hosted-only feature gating, misconfiguration or untrusted inputs could lead to severe security impact.

Overview
Adds a new self-hosted-only Execute Command block and tool (execute_command_run) that executes host shell commands via a new /api/tools/execute-command/run endpoint, returning stdout/stderr/exitCode and supporting <block.field>, {{ENV_VAR}}, and <variable.name> interpolation.

Introduces EXECUTE_COMMAND_ENABLED / NEXT_PUBLIC_EXECUTE_COMMAND_ENABLED feature flags (including env schema + Helm + .env.example) to gate both server execution and UI visibility, wires the block into the block/tool registries and executor handler registry, and adds a TerminalIcon. Also fixes workflow variable resolution in function execution to normalize variable name comparisons.

Written by Cursor Bugbot for commit 5782b68. Configure here.

@vercel
Copy link

vercel bot commented Mar 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 5, 2026 11:14pm

Request Review

@waleedlatif1 waleedlatif1 force-pushed the waleedlatif1/execute-command-block branch from 2dd1ff9 to 6802245 Compare March 5, 2026 20:35
@waleedlatif1
Copy link
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Collaborator Author

@greptile

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 5, 2026

Greptile Summary

This PR adds an Execute Command block that runs arbitrary shell commands on the host machine via child_process.exec, gated behind an EXECUTE_COMMAND_ENABLED feature flag that is automatically blocked on hosted environments. It follows the project's established block/tool/handler architecture and integrates well with the existing variable-resolution and executor infrastructure.

Several issues raised in the prior review round appear to have been addressed: the index-based string splicing (slice + splice) eliminates the $-pattern corruption from String.prototype.replace; getSafeBaseEnv() prevents server secrets from leaking to child processes; isMaxBuffer detection is now independent of error.killed; a parsedTimeout > 0 guard replaces the earlier falsy-default gap; and block outputs are now declared including the error field.

Four key concerns remain that should be addressed:

  1. Timeout / MAX_DURATION mismatchMAX_DURATION = 210 s is the hard ceiling for the route, but the block UI allows users to configure any positive timeout. A timeout greater than 210,000 ms will cause the route function to be killed by the platform before exec's own timeout fires, orphaning the child process on the host and leaving the client with an unhandled 504 error.

  2. Silent envVars mismatch for blocked key namesenvVars is used for {{ENV_VAR}} template substitution (unfiltered) and as the child process environment (filtered by filterUserEnv). Workflow env vars whose names collide with BLOCKED_ENV_KEYS (e.g. PATH, HOME) will correctly substitute in templates but will be silently dropped from the child process environment, creating confusing debugging experiences where template resolution and actual runtime behavior diverge.

  3. workingDirectory ignores variable-reference syntax — only command is passed through resolveCommandVariables; the working directory field silently treats <block.output> or {{ENV_VAR}} as literal path strings, despite the command field and wand prompt documenting these syntaxes.

  4. Dead-code throw in the handler — the throw new Error branch when !result.success && !result.output is unreachable because the route always populates output. This diverges from the stated n8n-style philosophy (treat all failures as data) and could mislead future maintainers.

The feature is self-hosted only and behind a feature flag, which limits blast radius. However, the timeout/MAX_DURATION gap and the envVars dual-use mismatch are real operational concerns that should be addressed before wide adoption.

Confidence Score: 3/5

  • Safe to merge for self-hosted users who understand shell execution risks, but orphaned child processes from timeout/MAX_DURATION mismatch and silent envVars filtering require attention before wide adoption.
  • All four verified comments identify real issues in the code: (1) user-configured timeouts exceeding the platform's 210s MAX_DURATION cause orphaned processes, a genuine operational problem; (2) envVars filtering is silent and asymmetric between template resolution and child process environment, causing user confusion; (3) workingDirectory lacks variable syntax support despite command field and wand prompt encouraging it; (4) the handler has unreachable code that violates stated philosophy. However, the feature is gated behind EXECUTE_COMMAND_ENABLED and self-hosted only, significantly limiting blast radius. The security model (getSafeBaseEnv isolation, filterUserEnv protection) is sound. These issues require fixes but are not blockers for a self-hosted-only feature.
  • apps/sim/app/api/tools/execute-command/run/route.ts (timeout ceiling and envVars filtering asymmetry), apps/sim/executor/handlers/execute-command/execute-command-handler.ts (dead-code throw branch), apps/sim/blocks/blocks/execute-command.ts (workingDirectory variable syntax mismatch)

Last reviewed commit: 5782b68

@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

…timeout:0, route path

- Rewrite resolveTagVariables to use index-based back-to-front splicing instead of
  global regex replace, preventing double-substitution when resolved values contain
  tag-like patterns
- Fix timeout:0 bypass by using explicit lower-bound check instead of destructuring default
- Add shell injection warning to bestPractices documentation
- Move API route from api/execute-command/ to api/tools/execute-command/ for consistency
…ead of throwing

When a command times out or exceeds the buffer limit, the route returns
partial stdout/stderr. Previously the handler threw an Error, discarding
that output. Now returns partial output with an error field so downstream
blocks can inspect it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

…n, remove unused userId

Fix asymmetric normalization in resolveWorkflowVariables where the stored
variable name was normalized but the reference name was only trimmed. This
caused <variable.MyVar> to fail matching a variable named "MyVar". Applied
the same fix to the function route which had the identical bug.

Also removed unused userId field from the execute-command tool config
request body — auth identity comes from checkInternalAuth, not the body.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Refactor variable resolution to collect all replacements from all three
patterns (workflow variables, env vars, block references) against the
ORIGINAL command string, then apply them in a single back-to-front pass.

Previously, three sequential passes meant a workflow variable value
containing {{ENV_VAR}} syntax would be further resolved in the env var
pass. Now all patterns are matched before any substitution occurs,
preventing unintended cascading resolution.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Return NodeJS.ProcessEnv from getSafeBaseEnv() and include NODE_ENV
to satisfy the exec() type signature which requires ProcessEnv.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…verride

- Throw a clear error when a block reference cannot be resolved instead
  of leaving raw <blockName.output> in the command (which the shell
  interprets as I/O redirection). Skip special prefixes (variable, loop,
  parallel) which are handled by their own collectors.

- Filter user-supplied env vars to prevent overriding safe base env keys
  (PATH, HOME, SHELL, etc.) and block process-influencing variables
  (LD_PRELOAD, BASH_ENV, DYLD_INSERT_LIBRARIES, etc.).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

… regex

- Extend BLOCKED_ENV_KEYS with NODE_OPTIONS, LD_AUDIT, DYLD_LIBRARY_PATH,
  DYLD_FRAMEWORK_PATH, GLIBC_TUNABLES, NODE_PATH, PYTHONPATH, PERL5LIB,
  RUBYLIB to prevent process-influencing env var injection.

- Add error field to block and tool output schemas so downstream blocks
  can inspect timeout/buffer errors via <executeCommand.error>. Handler
  returns error: null on success for consistency.

- Use permissive tag regex ([^<>]+) matching createReferencePattern() so
  block names with hyphens (e.g. <my-block.output>) resolve correctly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

…list

Add two more process-influencing env vars to the blocklist:
- JAVA_TOOL_OPTIONS: allows JVM agent injection (similar to NODE_OPTIONS)
- PERL5OPT: allows Perl code injection (complements PERL5LIB)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ap guard, shell-safe regex

- Promote DEFAULT_EXECUTION_TIMEOUT_MS to static top-level import
- Add timeout subBlock so users can configure command timeout
- Add overlapping replacement assertion to prevent corruption
- Tighten tag regex to require non-whitespace start, avoiding shell redirection false matches

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Comment on lines +362 to +366
const result = await executeCommand(resolvedCommand, {
timeout,
cwd: workingDirectory,
env: envVars,
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

envVars used for both template resolution and child process environment — silently filters blocked keys in one path but not the other

The same envVars object serves two purposes: (1) template substitution in resolveCommandVariables (lines 353–360) and (2) as the child process environment via filterUserEnv (line 263). When a workflow env var has a name in BLOCKED_ENV_KEYS (e.g., PATH, HOME), it will:

  • Resolve correctly in template patterns like echo {{PATH}}
  • Be silently dropped from the child process environment

Concrete example: User sets envVars.PATH = "/custom/bin" and writes command echo {{PATH}} && echo $PATH. The resolved command becomes echo /custom/bin && echo $PATH, but when executed, the process inherits the server's PATH from getSafeBaseEnv(), not the user's custom value. This is a confusing contradiction within a single command.

Consider documenting in bestPractices that workflow env vars matching keys in BLOCKED_ENV_KEYS are available for template substitution but will NOT be injected into the child process environment.


export const dynamic = 'force-dynamic'
export const runtime = 'nodejs'
export const MAX_DURATION = 210
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User-configured timeout can exceed MAX_DURATION, orphaning the child process

MAX_DURATION = 210 seconds is the Next.js maximum route duration. However, a user can configure any positive timeout value via the block UI (line 68 in execute-command.ts). When user-configured timeout exceeds MAX_DURATION * 1000:

  1. exec starts the child process with the user's timeout (e.g., 300,000 ms).
  2. At 210 seconds, the Next.js/hosting platform kills the route handler.
  3. The child process continues running on the host as an orphan process.
  4. The client receives a 504 / connection-closed error instead of the clean "Command timed out" response.

This is a real operational problem for self-hosted deployments with long-running build scripts or migrations. Consider enforcing an upper bound on timeout less than MAX_DURATION:

const MAX_ALLOWED_TIMEOUT_MS = (MAX_DURATION - 10) * 1000 // 200 s, leaving buffer
const timeout = Math.min(
  parsedTimeout > 0 ? parsedTimeout : DEFAULT_EXECUTION_TIMEOUT_MS,
  MAX_ALLOWED_TIMEOUT_MS
)

Comment on lines +57 to +62
id: 'workingDirectory',
title: 'Working Directory',
type: 'short-input',
required: false,
placeholder: '/path/to/directory',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workingDirectory silently ignores variable-reference syntax

Unlike the command field, workingDirectory is passed directly to exec as cwd (route.ts line 364) without going through resolveCommandVariables. This means <blockName.output>, {{ENV_VAR}}, and <variable.name> syntax in the working directory field are treated as literal path strings rather than being substituted.

This is asymmetric and confusing because:

  • The command field prominently documents both syntaxes (lines 17–18)
  • The wand prompt instructs the LLM to use these syntaxes (lines 40–41)
  • Users or the LLM are likely to write workingDirectory: <filesBlock.outputPath> and be surprised when it doesn't resolve

Consider either:

  1. Passing workingDirectory through resolveCommandVariables before using it as cwd, or
  2. Adding a note to bestPractices explicitly stating that variable-reference syntax is not supported in the Working Directory field.

Comment on lines +46 to +53
if (!result.success) {
if (result.output) {
return {
...result.output,
error: result.error || 'Command execution failed',
}
}
throw new Error(result.error || 'Command execution failed')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreachable throw branch — handler already returns data on failure

The route handler always returns an output object regardless of the success value (timeout case: route.ts lines 376–386, maxBuffer case: lines 388–398, error case: lines 408–418). This means result.output is never falsy when !result.success, making the throw new Error(...) branch at line 53 unreachable in practice.

The dead branch could mislead future readers into thinking there is a failure mode where no output is returned. More importantly, it diverges from the stated n8n-style philosophy that "non-zero exits are data, not errors" — failures should be returned as structured data, not thrown.

Consider removing the unreachable branch and always returning the output for consistency:

if (!result.success) {
  return {
    ...(result.output ?? { stdout: '', stderr: '', exitCode: 1 }),
    error: result.error || 'Command execution failed',
  }
}

return { ...result.output, error: null }

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

…dler, document workingDirectory

- Enforce upper bound on timeout (MAX_DURATION - 10s) to prevent orphan processes
- Remove unreachable throw branch in handler — always return structured data
- Document that workingDirectory does not support variable references

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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