Conversation
Contributor
Review Summary by QodoAdd labscript MCP server for experiment script execution
WalkthroughsDescription• Add labscript MCP server for running experiment scripts • Implement run_labscript tool supporting inline code and file paths • Support globals injection into script namespace before execution • Include comprehensive documentation and project configuration Diagramflowchart LR
A["User Input<br/>script/script_path/globals"] --> B["run_labscript Tool"]
B --> C["Create Temp Files<br/>if needed"]
C --> D["Inject Globals<br/>via wrapper"]
D --> E["Execute Python<br/>subprocess"]
E --> F["Return Output<br/>exit code/stdout/stderr"]
File Changes1. servers/labscript/src/labscript_server/__init__.py
|
Contributor
Code Review by Qodo
1. No run_labscript audit logs
|
📝 WalkthroughWalkthroughAdds a new Labscript MCP Server: packaging, manifest, README, and a FastMCP-based Python server exposing a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Server as FastMCP Server
participant Validator as Validator
participant Resolver as Script Resolver
participant Executor as Subprocess Executor
participant Cleaner as Temp Cleanup
Client->>Server: call run_labscript(script?, script_path?, globals?)
Server->>Validator: check parameters (mutual exclusivity / presence)
Validator-->>Server: validation result
Server->>Resolver: resolve script source
alt inline script
Resolver->>Resolver: write temporary .py file
else file path
Resolver->>Resolver: validate path exists
end
Resolver-->>Server: resolved script path
alt globals provided
Server->>Resolver: create wrapper injecting globals and Qt stub
Resolver-->>Server: wrapper path
end
Server->>Executor: run wrapper (timeout=60s), capture stdout/stderr, exit code
alt timeout
Executor-->>Server: timeout (exit -1)
else complete
Executor-->>Server: exit code, stdout, stderr
end
Server->>Cleaner: remove temporary files
Cleaner-->>Server: cleanup done
Server-->>Client: return {exit_code, stdout, stderr}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+103
to
+119
| # Step 3: execute | ||
| try: | ||
| result = subprocess.run( | ||
| [sys.executable, run_file], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=TIMEOUT, | ||
| ) | ||
| except subprocess.TimeoutExpired: | ||
| return f"Exit code: -1\n--- stdout ---\n\n--- stderr ---\nExecution timed out after {TIMEOUT} seconds.\n" | ||
|
|
||
| # Step 4: format output | ||
| return ( | ||
| f"Exit code: {result.returncode}\n" | ||
| f"--- stdout ---\n{result.stdout}\n" | ||
| f"--- stderr ---\n{result.stderr}" | ||
| ) |
Contributor
There was a problem hiding this comment.
1. No run_labscript audit logs 📘 Rule violation ✧ Quality
The tool executes scripts without recording an audit event (who invoked it, what was run, and the outcome). This prevents reconstructing critical actions and troubleshooting/security investigations.
Agent Prompt
## Issue description
`run_labscript` executes experiment scripts but does not emit audit-trail logs containing sufficient context (who/what/outcome). This makes it difficult to reconstruct critical actions.
## Issue Context
The tool runs arbitrary experiment code via a subprocess and returns stdout/stderr/exit code to the caller. This should be treated as a critical action and logged with safe, minimal context.
## Fix Focus Areas
- servers/labscript/src/labscript_server/main.py[21-119]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+70
to
+110
| if script is not None: | ||
| tmp_script = tempfile.NamedTemporaryFile( | ||
| suffix=".py", mode="w", delete=False, encoding="utf-8" | ||
| ) | ||
| tmp_script.write(script) | ||
| tmp_script.flush() | ||
| tmp_script.close() | ||
| labscript_file = tmp_script.name | ||
| else: | ||
| labscript_file = os.path.abspath(script_path) | ||
| if not os.path.isfile(labscript_file): | ||
| raise ValueError(f"script_path does not exist: {labscript_file}") | ||
|
|
||
| # Step 2: build wrapper if globals provided | ||
| if globals: | ||
| globals_repr = repr(globals) | ||
| wrapper_code = ( | ||
| "import builtins as _builtins\n" | ||
| f"_globals_dict = {globals_repr}\n" | ||
| "for _k, _v in _globals_dict.items():\n" | ||
| " setattr(_builtins, _k, _v)\n" | ||
| f"exec(open({repr(labscript_file)}).read(), {{'__file__': {repr(labscript_file)}}})\n" | ||
| ) | ||
| tmp_wrapper = tempfile.NamedTemporaryFile( | ||
| suffix=".py", mode="w", delete=False, encoding="utf-8" | ||
| ) | ||
| tmp_wrapper.write(wrapper_code) | ||
| tmp_wrapper.flush() | ||
| tmp_wrapper.close() | ||
| run_file = tmp_wrapper.name | ||
| else: | ||
| run_file = labscript_file | ||
|
|
||
| # Step 3: execute | ||
| try: | ||
| result = subprocess.run( | ||
| [sys.executable, run_file], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=TIMEOUT, | ||
| ) |
Contributor
There was a problem hiding this comment.
2. Unvalidated script execution 📘 Rule violation ⛨ Security
The tool directly executes inline code or any filesystem script_path without allowlisting, authentication/authorization context, or sandboxing. This enables untrusted code execution and access to local files/resources.
Agent Prompt
## Issue description
The server executes untrusted Python from `script` or `script_path` with minimal validation (only mutual exclusivity and file existence). This is a security risk because it allows arbitrary code execution and filesystem access.
## Issue Context
`run_labscript` is exposed as an MCP tool and will be invoked from external clients. Treat all provided inputs as untrusted.
## Fix Focus Areas
- servers/labscript/src/labscript_server/main.py[60-110]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+114
to
+119
| # Step 4: format output | ||
| return ( | ||
| f"Exit code: {result.returncode}\n" | ||
| f"--- stdout ---\n{result.stdout}\n" | ||
| f"--- stderr ---\n{result.stderr}" | ||
| ) |
Contributor
There was a problem hiding this comment.
3. Returns raw stderr output 📘 Rule violation ⛨ Security
The tool returns raw stderr from executed scripts to the caller, which may include stack traces, paths, and other internal details. This can leak sensitive implementation/environment information through user-facing errors.
Agent Prompt
## Issue description
`run_labscript` returns raw stderr to the caller, which can expose stack traces, file paths, and internal details.
## Issue Context
Executed scripts may fail with Python tracebacks and environment-specific information. Compliance requires user-facing errors be generic while detailed errors go to secure internal logs.
## Fix Focus Areas
- servers/labscript/src/labscript_server/main.py[111-119]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+84
to
+110
| if globals: | ||
| globals_repr = repr(globals) | ||
| wrapper_code = ( | ||
| "import builtins as _builtins\n" | ||
| f"_globals_dict = {globals_repr}\n" | ||
| "for _k, _v in _globals_dict.items():\n" | ||
| " setattr(_builtins, _k, _v)\n" | ||
| f"exec(open({repr(labscript_file)}).read(), {{'__file__': {repr(labscript_file)}}})\n" | ||
| ) | ||
| tmp_wrapper = tempfile.NamedTemporaryFile( | ||
| suffix=".py", mode="w", delete=False, encoding="utf-8" | ||
| ) | ||
| tmp_wrapper.write(wrapper_code) | ||
| tmp_wrapper.flush() | ||
| tmp_wrapper.close() | ||
| run_file = tmp_wrapper.name | ||
| else: | ||
| run_file = labscript_file | ||
|
|
||
| # Step 3: execute | ||
| try: | ||
| result = subprocess.run( | ||
| [sys.executable, run_file], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=TIMEOUT, | ||
| ) |
Contributor
There was a problem hiding this comment.
4. Wrapper breaks script context 🐞 Bug ✓ Correctness
When globals are provided, execution switches to a temp wrapper that exec()s the real script with a minimal globals dict; this changes __name__ and the effective import search path compared to running the script directly, potentially breaking scripts that rely on `if __name__ == "__main__"` or local/relative imports.
Agent Prompt
### Issue description
When `globals` are provided, `run_labscript()` creates a temp wrapper file and executes the target script with `exec()` using a minimal globals dict. This can break labscript scripts that rely on normal script semantics (e.g., `__name__ == '__main__'`) or that import other modules located next to the labscript file.
### Issue Context
The wrapper currently executes the target script with:
- `exec(open(labscript_file).read(), {'__file__': labscript_file})`
- `subprocess.run([sys.executable, run_file])` where `run_file` is the wrapper path
This changes the executed entrypoint from the labscript itself to a temp wrapper.
### Fix Focus Areas
- servers/labscript/src/labscript_server/main.py[84-110]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@servers/labscript/src/labscript_server/main.py`:
- Around line 60-110: The run_labscript tool currently allows arbitrary code
execution via the script and script_path parameters (see run_labscript, script,
script_path, subprocess.run, and the tmp_wrapper/tmp_script logic); to fix,
disallow direct inline execution (reject non-empty script) unless an explicit
opt-in guard is present (e.g. require an environment variable like
ENABLE_LABSCRIPT or a server-side permission/token) and validate script_path
against a strict allowlist of base directories (resolve with os.path.realpath
and ensure it is under an allowed root) and that it is a regular file;
additionally run the subprocess with a locked-down execution context (use a
minimal env, set cwd to a safe temp dir, close file descriptors, and drop
privileges / run as a non-privileged user) and remove or restrict the
globals-injection wrapper unless explicitly permitted.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
servers/labscript/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
servers/labscript/README.mdservers/labscript/manifest.jsonservers/labscript/pyproject.tomlservers/labscript/src/labscript_server/__init__.pyservers/labscript/src/labscript_server/main.py
Comment on lines
+60
to
+110
| if script is not None and script_path is not None: | ||
| raise ValueError("Provide either 'script' or 'script_path', not both.") | ||
| if script is None and script_path is None: | ||
| raise ValueError("Provide either 'script' or 'script_path'.") | ||
|
|
||
| tmp_script = None | ||
| tmp_wrapper = None | ||
|
|
||
| try: | ||
| # Step 1: resolve the labscript file path | ||
| if script is not None: | ||
| tmp_script = tempfile.NamedTemporaryFile( | ||
| suffix=".py", mode="w", delete=False, encoding="utf-8" | ||
| ) | ||
| tmp_script.write(script) | ||
| tmp_script.flush() | ||
| tmp_script.close() | ||
| labscript_file = tmp_script.name | ||
| else: | ||
| labscript_file = os.path.abspath(script_path) | ||
| if not os.path.isfile(labscript_file): | ||
| raise ValueError(f"script_path does not exist: {labscript_file}") | ||
|
|
||
| # Step 2: build wrapper if globals provided | ||
| if globals: | ||
| globals_repr = repr(globals) | ||
| wrapper_code = ( | ||
| "import builtins as _builtins\n" | ||
| f"_globals_dict = {globals_repr}\n" | ||
| "for _k, _v in _globals_dict.items():\n" | ||
| " setattr(_builtins, _k, _v)\n" | ||
| f"exec(open({repr(labscript_file)}).read(), {{'__file__': {repr(labscript_file)}}})\n" | ||
| ) | ||
| tmp_wrapper = tempfile.NamedTemporaryFile( | ||
| suffix=".py", mode="w", delete=False, encoding="utf-8" | ||
| ) | ||
| tmp_wrapper.write(wrapper_code) | ||
| tmp_wrapper.flush() | ||
| tmp_wrapper.close() | ||
| run_file = tmp_wrapper.name | ||
| else: | ||
| run_file = labscript_file | ||
|
|
||
| # Step 3: execute | ||
| try: | ||
| result = subprocess.run( | ||
| [sys.executable, run_file], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=TIMEOUT, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "main.py" -path "*/labscript_server/*" | head -5Repository: pathintegral-institute/mcp.science
Length of output: 126
🏁 Script executed:
git ls-files | grep -E "labscript.*main\.py"Repository: pathintegral-institute/mcp.science
Length of output: 124
🏁 Script executed:
fd "main.py" | grep labscriptRepository: pathintegral-institute/mcp.science
Length of output: 124
🏁 Script executed:
cat -n servers/labscript/src/labscript_server/main.pyRepository: pathintegral-institute/mcp.science
Length of output: 5829
🏁 Script executed:
find . -name "README*" -o -name "*SECURITY*" -o -name "*security*" | grep -i labscript | head -10Repository: pathintegral-institute/mcp.science
Length of output: 107
🏁 Script executed:
find servers/labscript -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | head -10Repository: pathintegral-institute/mcp.science
Length of output: 105
🏁 Script executed:
cat servers/labscript/README.md 2>/dev/null | head -100Repository: pathintegral-institute/mcp.science
Length of output: 1591
🏁 Script executed:
find . -name "*.md" | xargs grep -l "security\|threat\|authentication" | head -10Repository: pathintegral-institute/mcp.science
Length of output: 160
🏁 Script executed:
cat servers/labscript/pyproject.toml 2>/dev/nullRepository: pathintegral-institute/mcp.science
Length of output: 492
🏁 Script executed:
find . -type f -name "SECURITY*" -o -name "THREAT*"Repository: pathintegral-institute/mcp.science
Length of output: 60
🏁 Script executed:
cat README.md | head -150Repository: pathintegral-institute/mcp.science
Length of output: 5848
🏁 Script executed:
cat servers/python-code-execution/README.md | head -100Repository: pathintegral-institute/mcp.science
Length of output: 1385
🏁 Script executed:
cat servers/ssh-exec/README.md | head -100Repository: pathintegral-institute/mcp.science
Length of output: 3745
Arbitrary Python code execution with no opt-in guard or path boundaries.
The run_labscript function is exposed as an MCP tool (line 21: @mcp.tool()) and accepts arbitrary Python code via the script parameter (lines 70–77) or file paths via script_path (line 79), executing both under the server process's privileges via subprocess.run (lines 105–110). No environment variable checks, permission gates, or path restrictions exist. Any MCP client can trigger unrestricted code execution.
The suggested hardening aligns with security patterns used in similar tools like ssh-exec (which restricts commands and paths via environment variables):
Suggested hardening baseline
def run_labscript(
script: Optional[str] = None,
script_path: Optional[str] = None,
globals: Optional[dict] = None,
) -> str:
+ if os.getenv("MCP_LABSCRIPT_ALLOW_UNSAFE_EXEC") != "1":
+ raise PermissionError(
+ "Set MCP_LABSCRIPT_ALLOW_UNSAFE_EXEC=1 to enable script execution."
+ )
+
if script is not None and script_path is not None:
raise ValueError("Provide either 'script' or 'script_path', not both.")
@@
else:
labscript_file = os.path.abspath(script_path)
+ allowed_root = os.getenv("MCP_LABSCRIPT_ALLOWED_ROOT")
+ if allowed_root:
+ root = os.path.realpath(allowed_root)
+ real_file = os.path.realpath(labscript_file)
+ if real_file != root and not real_file.startswith(root + os.sep):
+ raise PermissionError(f"script_path must be inside {root}")
if not os.path.isfile(labscript_file):
raise ValueError(f"script_path does not exist: {labscript_file}")🧰 Tools
🪛 Ruff (0.15.2)
[warning] 61-61: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 63-63: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 81-81: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 91-91: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 91-91: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[error] 105-105: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/labscript/src/labscript_server/main.py` around lines 60 - 110, The
run_labscript tool currently allows arbitrary code execution via the script and
script_path parameters (see run_labscript, script, script_path, subprocess.run,
and the tmp_wrapper/tmp_script logic); to fix, disallow direct inline execution
(reject non-empty script) unless an explicit opt-in guard is present (e.g.
require an environment variable like ENABLE_LABSCRIPT or a server-side
permission/token) and validate script_path against a strict allowlist of base
directories (resolve with os.path.realpath and ensure it is under an allowed
root) and that it is a regular file; additionally run the subprocess with a
locked-down execution context (use a minimal env, set cwd to a safe temp dir,
close file descriptors, and drop privileges / run as a non-privileged user) and
remove or restrict the globals-injection wrapper unless explicitly permitted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
servers/labscript/src/labscript_server/main.py (1)
69-129:⚠️ Potential issue | 🔴 CriticalAdd an explicit execution gate and path boundary before running scripts.
At Line 69 onward, the tool still executes caller-provided code/path under server privileges without an opt-in guard or root restriction. That leaves a high-risk remote code execution surface.
🔒 Proposed hardening baseline
`@mcp.tool`() def run_labscript( script: Optional[str] = None, script_path: Optional[str] = None, globals: Optional[dict] = None, ) -> str: + if os.getenv("MCP_LABSCRIPT_ALLOW_UNSAFE_EXEC") != "1": + raise PermissionError( + "Set MCP_LABSCRIPT_ALLOW_UNSAFE_EXEC=1 to enable run_labscript." + ) + if script is not None and script_path is not None: raise ValueError("Provide either 'script' or 'script_path', not both.") if script is None and script_path is None: raise ValueError("Provide either 'script' or 'script_path'.") @@ - else: - labscript_file = os.path.abspath(script_path) - if not os.path.isfile(labscript_file): + else: + labscript_file = os.path.abspath(script_path) + allowed_root = os.getenv("MCP_LABSCRIPT_ALLOWED_ROOT") + if not allowed_root: + raise PermissionError("MCP_LABSCRIPT_ALLOWED_ROOT must be configured.") + root = os.path.realpath(allowed_root) + real_file = os.path.realpath(labscript_file) + if real_file != root and not real_file.startswith(root + os.sep): + raise PermissionError(f"script_path must be inside: {root}") + if not os.path.isfile(real_file): raise ValueError(f"script_path does not exist: {labscript_file}") + labscript_file = real_file @@ - result = subprocess.run( + result = subprocess.run( [sys.executable, tmp_wrapper.name], capture_output=True, text=True, timeout=TIMEOUT, + cwd=tempfile.gettempdir(), )Also applies to: 157-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/labscript/src/labscript_server/main.py` around lines 69 - 129, run_labscript currently executes caller-provided code/paths with server privileges; add an explicit opt-in gate and path boundary to prevent accidental RCE: require a new boolean flag (e.g. allow_execution) or an environment/config check before any execution and raise if not set, enforce that the effective UID is non-root (os.geteuid() != 0) and refuse to run as root, and validate script_path is inside an allowed base directory (canonicalize with os.path.realpath and ensure it startswith an allowed root) before creating tmp_script/tmp_wrapper or executing; update run_labscript, and any helper that invokes the temp wrapper, to perform these checks and fail early with a clear error when the gate/constraints are not met.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@servers/labscript/src/labscript_server/main.py`:
- Around line 157-170: The subprocess.run call currently uses
capture_output=True which can buffer unlimited stdout/stderr and cause OOM;
change the execution to capture pipes but bound their size: use stdout=PIPE and
stderr=PIPE (or subprocess.Popen with streaming reads) and enforce a
MAX_OUTPUT_BYTES constant, then after the process completes (or on timeout)
truncate result.stdout and result.stderr to MAX_OUTPUT_BYTES and append a clear
"[truncated]" notice before returning; update references around the
subprocess.run call and the return that uses result.returncode, result.stdout
and result.stderr (the invocation using tmp_wrapper.name and TIMEOUT) so the
server never holds arbitrarily large child output in memory.
---
Duplicate comments:
In `@servers/labscript/src/labscript_server/main.py`:
- Around line 69-129: run_labscript currently executes caller-provided
code/paths with server privileges; add an explicit opt-in gate and path boundary
to prevent accidental RCE: require a new boolean flag (e.g. allow_execution) or
an environment/config check before any execution and raise if not set, enforce
that the effective UID is non-root (os.geteuid() != 0) and refuse to run as
root, and validate script_path is inside an allowed base directory (canonicalize
with os.path.realpath and ensure it startswith an allowed root) before creating
tmp_script/tmp_wrapper or executing; update run_labscript, and any helper that
invokes the temp wrapper, to perform these checks and fail early with a clear
error when the gate/constraints are not met.
Comment on lines
+157
to
+170
| result = subprocess.run( | ||
| [sys.executable, tmp_wrapper.name], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=TIMEOUT, | ||
| ) | ||
| except subprocess.TimeoutExpired: | ||
| return f"Exit code: -1\n--- stdout ---\n\n--- stderr ---\nExecution timed out after {TIMEOUT} seconds.\n" | ||
|
|
||
| # Step 4: format output | ||
| return ( | ||
| f"Exit code: {result.returncode}\n" | ||
| f"--- stdout ---\n{result.stdout}\n" | ||
| f"--- stderr ---\n{result.stderr}" |
There was a problem hiding this comment.
Bound captured stdout/stderr to prevent memory blowups.
At Line 157, capture_output=True keeps all child output in memory. A noisy script can exhaust memory and take down the server process.
📉 Suggested capped-output pattern
TIMEOUT = 60
+MAX_OUTPUT_BYTES = 1_000_000
@@
- result = subprocess.run(
- [sys.executable, tmp_wrapper.name],
- capture_output=True,
- text=True,
- timeout=TIMEOUT,
- )
+ with tempfile.TemporaryFile() as out_f, tempfile.TemporaryFile() as err_f:
+ result = subprocess.run(
+ [sys.executable, tmp_wrapper.name],
+ stdout=out_f,
+ stderr=err_f,
+ timeout=TIMEOUT,
+ )
+ out_f.seek(0)
+ err_f.seek(0)
+ stdout = out_f.read(MAX_OUTPUT_BYTES + 1).decode("utf-8", errors="replace")
+ stderr = err_f.read(MAX_OUTPUT_BYTES + 1).decode("utf-8", errors="replace")
+ if len(stdout.encode("utf-8")) > MAX_OUTPUT_BYTES:
+ stdout = stdout[:MAX_OUTPUT_BYTES] + "\n...[truncated]"
+ if len(stderr.encode("utf-8")) > MAX_OUTPUT_BYTES:
+ stderr = stderr[:MAX_OUTPUT_BYTES] + "\n...[truncated]"
@@
return (
f"Exit code: {result.returncode}\n"
- f"--- stdout ---\n{result.stdout}\n"
- f"--- stderr ---\n{result.stderr}"
+ f"--- stdout ---\n{stdout}\n"
+ f"--- stderr ---\n{stderr}"
)🧰 Tools
🪛 Ruff (0.15.2)
[error] 157-157: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/labscript/src/labscript_server/main.py` around lines 157 - 170, The
subprocess.run call currently uses capture_output=True which can buffer
unlimited stdout/stderr and cause OOM; change the execution to capture pipes but
bound their size: use stdout=PIPE and stderr=PIPE (or subprocess.Popen with
streaming reads) and enforce a MAX_OUTPUT_BYTES constant, then after the process
completes (or on timeout) truncate result.stdout and result.stderr to
MAX_OUTPUT_BYTES and append a clear "[truncated]" notice before returning;
update references around the subprocess.run call and the return that uses
result.returncode, result.stdout and result.stderr (the invocation using
tmp_wrapper.name and TIMEOUT) so the server never holds arbitrarily large child
output in memory.
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 by CodeRabbit
New Features
Documentation