Skip to content

Secure MCP tool execution via Artisan #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Snider wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Secure MCP tool execution via Artisan #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48078c0
Security: Implement timeout and output limits for MCP tool execution
Snider Feb 4, 2026
b890233
Fix CI: Add missing repository for host-uk/core and dev dependencies
Snider Feb 4, 2026
da734b3
Fix CI: Revert to VCS repositories and remove broken lock file
Snider Feb 4, 2026
02f9647
Fix CI: Add missing PHPStan and Psalm dev dependencies
Snider Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,28 @@
"license": "EUPL-1.2",
"require": {
"php": "^8.2",
"host-uk/core": "@dev",
"host-uk/core": "dev-dev",
"host-uk/core-mcp": "dev-dev",
"symfony/yaml": "^7.0"
},
"require-dev": {
"pestphp/pest": "^3.0",
"laravel/pint": "^1.18",
"orchestra/testbench": "^10.0",
"phpstan/phpstan": "^2.1",
"vimeo/psalm": "^6.14",
"phpunit/phpunit": "^11.5"
},
"repositories": [
{
"type": "vcs",
"url": "https://github.com/host-uk/core-php"
},
{
"type": "vcs",
"url": "https://github.com/host-uk/core-mcp"
}
],
"autoload": {
"psr-4": {
"Core\\Api\\": "src/Api/",
Expand All @@ -19,6 +38,11 @@
"providers": []
}
},
"minimum-stability": "stable",
"config": {
"allow-plugins": {
"pestphp/pest-plugin": true
}
},
"minimum-stability": "dev",
"prefer-stable": true
}
62 changes: 60 additions & 2 deletions src/Api/Controllers/McpApiController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Str;
use Symfony\Component\Yaml\Yaml;

/**
Expand Down Expand Up @@ -444,6 +446,9 @@ protected function executeToolViaArtisan(string $server, string $tool, array $ar
throw new \RuntimeException("Unknown server: {$server}");
}

// Add timeout from configuration
$timeout = config('api.mcp.execution_timeout', 30);

// Build MCP request
$mcpRequest = [
'jsonrpc' => '2.0',
Expand Down Expand Up @@ -471,15 +476,68 @@ protected function executeToolViaArtisan(string $server, string $tool, array $ar
throw new \RuntimeException('Failed to start MCP server process');
}

// Write request to stdin
fwrite($pipes[0], json_encode($mcpRequest)."\n");
fclose($pipes[0]);

$output = stream_get_contents($pipes[1]);
// Set non-blocking and use stream_select for timeout and size limits
stream_set_blocking($pipes[1], false);
stream_set_blocking($pipes[2], false);

$output = '';
$errorOutput = '';
$startTime = time();

while (true) {
$read = [$pipes[1], $pipes[2]];
$write = $except = null;

if (stream_select($read, $write, $except, 1) === false) {
break;
}

if (time() - $startTime > $timeout) {
proc_terminate($process);
throw new \RuntimeException('MCP server execution timed out');
}

// Read with size limits
$output .= fread($pipes[1], 1024 * 1024);
$errorOutput .= fread($pipes[2], 1024 * 64);
Comment on lines +505 to +506
Copy link

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

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

medium

The code currently attempts to read from both stdout and stderr pipes on every loop iteration where stream_select doesn't fail. This is inefficient because it doesn't check which stream is actually ready for reading. You should use the $read array, which is modified by stream_select to contain only the ready streams, to conditionally read from the correct pipe.

            if (in_array($pipes[1], $read, true)) {
                $output .= fread($pipes[1], 1024 * 1024);
            }
            if (in_array($pipes[2], $read, true)) {
                $errorOutput .= fread($pipes[2], 1024 * 64);
            }


if (strlen($output) > 10 * 1024 * 1024) { // 10MB absolute max
proc_terminate($process);
throw new \RuntimeException('Output size limit exceeded');
}

if (strlen($errorOutput) > 1024 * 1024) { // 1MB max for stderr
proc_terminate($process);
throw new \RuntimeException('Error output size limit exceeded');
}
Comment on lines +505 to +516
Copy link

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

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

medium

This block of code uses several magic numbers for buffer chunk sizes and maximum output sizes (e.g., 1024 * 1024, 10 * 1024 * 1024). To improve readability and maintainability, it's recommended to define these values as named constants (e.g., as class constants like private const STDOUT_MAX_SIZE = 10 * 1024 * 1024;). This makes the code self-documenting and simplifies future modifications to these limits.


// Check if process is still running
$status = proc_get_status($process);
if (! $status['running']) {
// Read remaining output
$output .= stream_get_contents($pipes[1]);
$errorOutput .= stream_get_contents($pipes[2]);
break;
Comment on lines +521 to +524
Copy link

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

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

high

When the process terminates, the remaining output is read from the pipes using stream_get_contents. However, the size of the output read at this stage is not checked against the defined limits. This could allow a process to bypass the size restrictions by outputting a large amount of data just before exiting. You should add the size limit checks after these final reads and before breaking the loop.

                // Read remaining output
                $output .= stream_get_contents($pipes[1]);
                $errorOutput .= stream_get_contents($pipes[2]);

                if (strlen($output) > 10 * 1024 * 1024) { // 10MB absolute max
                    throw new \RuntimeException('Output size limit exceeded');
                }

                if (strlen($errorOutput) > 1024 * 1024) { // 1MB max for stderr
                    throw new \RuntimeException('Error output size limit exceeded');
                }
                break;

}
}

fclose($pipes[1]);
fclose($pipes[2]);

proc_close($process);

// Log stderr for debugging (sanitized)
if (! empty($errorOutput)) {
Log::warning('MCP server stderr output', [
'server' => $server,
'tool' => $tool,
'error' => Str::limit($errorOutput, 1000),
]);
}

$response = json_decode($output, true);

if (isset($response['error'])) {
Expand Down
14 changes: 14 additions & 0 deletions src/Api/config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,4 +234,18 @@
'max_per_page' => 100,
],

/*
|--------------------------------------------------------------------------
| MCP Settings
|--------------------------------------------------------------------------
|
| Configuration for Model Context Protocol (MCP) servers and execution.
|
*/

'mcp' => [
// Execution timeout in seconds
'execution_timeout' => env('MCP_EXECUTION_TIMEOUT', 30),
],

];
Loading