Skip to content

Commit 87543e4

Browse files
abnegateclaude
abnegate
and
claude
committed
(fix): Address code review findings — SSRF bypass, send checks, Console logging, SMTP constants, TLSContext rename
- Block IPv6-mapped IPv4 addresses (::ffff:) in SSRF validation - Check send() return values in TCP fast path, forward goroutine, and SMTP forwarding - Fix goroutine resource leak in TCP SwooleCoroutine error path - Replace all echo/error_log with Utopia\Console (log, success, info, error) - Extract SMTP magic numbers into class constants (RECV_BUFFER, PACKAGE_MAX_LENGTH, etc.) - Rename TlsContext → TLSContext (acronym casing convention) - Rename getTlsContext → getTLSContext in Config and all callers - Add workerStart callback support to HTTP SwooleCoroutine - Normalize timeout types to float across all Config classes - Add utopia-php/console dependency, remove unused utopia-php/cli Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 9ccc5ce commit 87543e4

15 files changed

Lines changed: 307 additions & 161 deletions

‎composer.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,9 @@
1111
],
1212
"require": {
1313
"php": ">=8.4",
14+
"ext-redis": "*",
1415
"ext-swoole": ">=6.0",
15-
"ext-redis": "*"
16+
"utopia-php/console": "^0.1.1"
1617
},
1718
"require-dev": {
1819
"phpunit/phpunit": "12.*",

‎src/Adapter.php‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public function setSkipValidation(bool $skip): static
8585
return $this;
8686
}
8787

88-
public function setCacheTtl(int $seconds): static
88+
public function setCacheTTL(int $seconds): static
8989
{
9090
$this->cacheTTL = $seconds;
9191

@@ -237,7 +237,7 @@ public function route(string $resourceId): ConnectionResult
237237
}
238238

239239
if (!$this->skipValidation) {
240-
$this->validate($endpoint);
240+
$endpoint = $this->validate($endpoint);
241241
}
242242

243243
if ($this->cacheTTL > 0) {
@@ -261,19 +261,23 @@ public function route(string $resourceId): ConnectionResult
261261
}
262262

263263
/**
264-
* Validate backend endpoint to prevent SSRF attacks
264+
* Validate backend endpoint to prevent SSRF attacks.
265+
*
266+
* Returns the validated endpoint with the hostname replaced by the
267+
* resolved IP address to prevent DNS rebinding (TOCTOU) attacks.
265268
*/
266-
protected function validate(string $endpoint): void
269+
protected function validate(string $endpoint): string
267270
{
268271
$parts = \explode(':', $endpoint);
269272
if (\count($parts) > 2) {
270273
throw new ResolverException("Invalid endpoint format: {$endpoint}");
271274
}
272275

273276
$host = $parts[0];
274-
$port = isset($parts[1]) ? (int) $parts[1] : 0;
277+
$hasPort = isset($parts[1]);
278+
$port = $hasPort ? (int) $parts[1] : 0;
275279

276-
if ($port > 65535) {
280+
if ($hasPort && ($port < 1 || $port > 65535)) {
277281
throw new ResolverException("Invalid port number: {$port}");
278282
}
279283

@@ -307,23 +311,48 @@ protected function validate(string $endpoint): void
307311
}
308312
}
309313
} elseif (\filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) {
310-
if ($ip === '::1' || \str_starts_with($ip, 'fe80:') || \str_starts_with($ip, 'fc00:') || \str_starts_with($ip, 'fd00:')) {
314+
if (
315+
$ip === '::1'
316+
|| \str_starts_with($ip, 'fe80:')
317+
|| \str_starts_with($ip, 'fc00:')
318+
|| \str_starts_with($ip, 'fd00:')
319+
|| \str_starts_with(\strtolower($ip), '::ffff:')
320+
) {
311321
throw new ResolverException("Access to private/reserved IPv6 address is forbidden: {$ip}");
312322
}
313323
}
324+
325+
return $hasPort ? "{$ip}:{$port}" : $ip;
314326
}
315327

316328
/**
317329
* Initialize routing cache table
318330
*/
319-
protected function initRouter(): void
331+
protected function initRouter(int $size = 10_000): void
320332
{
321-
$this->router = new Table(200_000);
333+
$this->router = new Table($size);
322334
$this->router->column('endpoint', Table::TYPE_STRING, 256);
323335
$this->router->column('updated', Table::TYPE_INT, 8);
324336
$this->router->create();
325337
}
326338

339+
/**
340+
* Parse an endpoint string into host and port.
341+
*
342+
* If the endpoint already contains a port, that port is used.
343+
* Otherwise the provided default port is used.
344+
*
345+
* @return array{0: string, 1: int}
346+
*/
347+
public static function parseEndpoint(string $endpoint, int $defaultPort): array
348+
{
349+
$parts = \explode(':', $endpoint, 2);
350+
$host = $parts[0];
351+
$port = isset($parts[1]) && $parts[1] !== '' ? (int) $parts[1] : $defaultPort;
352+
353+
return [$host, $port];
354+
}
355+
327356
/**
328357
* Get routing and connection stats
329358
*

‎src/Adapter/TCP.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,7 @@ public function getConnection(string $data, int $fd): Client
115115

116116
$result = $this->route($data);
117117

118-
[$host, $port] = \explode(':', $result->endpoint . ':' . $this->port);
119-
$port = (int) $port;
118+
[$host, $port] = self::parseEndpoint($result->endpoint, $this->port);
120119

121120
$client = new Client(SWOOLE_SOCK_TCP);
122121

‎src/Server/HTTP/Config.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public function __construct(
2626
public readonly bool $parseFiles = false,
2727
public readonly bool $compression = false,
2828
public readonly int $logLevel = SWOOLE_LOG_ERROR,
29-
public readonly int $timeout = 30,
29+
public readonly float $timeout = 30.0,
3030
public readonly float $connectTimeout = 5.0,
3131
public readonly bool $keepAlive = true,
3232
public readonly int $poolSize = 1024,

‎src/Server/HTTP/Swoole.php‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Swoole\Http\Request;
1010
use Swoole\Http\Response;
1111
use Swoole\Http\Server;
12+
use Utopia\Console;
1213
use Utopia\Proxy\Adapter;
1314
use Utopia\Proxy\Protocol;
1415
use Utopia\Proxy\Resolver;
@@ -84,15 +85,15 @@ protected function configure(): void
8485

8586
public function onStart(Server $server): void
8687
{
87-
echo "HTTP Proxy Server started at http://{$this->config->host}:{$this->config->port}\n";
88-
echo "Workers: {$this->config->workers}\n";
89-
echo "Max connections: {$this->config->maxConnections}\n";
88+
Console::success("HTTP Proxy Server started at http://{$this->config->host}:{$this->config->port}");
89+
Console::log("Workers: {$this->config->workers}");
90+
Console::log("Max connections: {$this->config->maxConnections}");
9091
}
9192

9293
public function onWorkerStart(Server $server, int $workerId): void
9394
{
9495
$this->adapter = new Adapter($this->resolver, name: 'HTTP', protocol: Protocol::HTTP);
95-
$this->adapter->setCacheTtl($this->config->cacheTTL);
96+
$this->adapter->setCacheTTL($this->config->cacheTTL);
9697

9798
if ($this->config->skipValidation) {
9899
$this->adapter->setSkipValidation(true);
@@ -102,7 +103,7 @@ public function onWorkerStart(Server $server, int $workerId): void
102103
($this->config->workerStart)($server, $workerId, $this->adapter);
103104
}
104105

105-
echo "Worker #{$workerId} started (Adapter: {$this->adapter->getName()})\n";
106+
Console::log("Worker #{$workerId} started (Adapter: {$this->adapter->getName()})");
106107
}
107108

108109
/**
@@ -114,7 +115,7 @@ public function onRequest(Request $request, Response $response): void
114115
try {
115116
($this->config->requestHandler)($request, $response, $this->adapter);
116117
} catch (\Throwable $e) {
117-
error_log("Request handler error: {$e->getMessage()}");
118+
Console::error("Request handler error: {$e->getMessage()}");
118119
$response->status(500);
119120
$response->end('Internal Server Error');
120121
}
@@ -171,7 +172,7 @@ public function onRequest(Request $request, Response $response): void
171172
}
172173

173174
} catch (\Exception $e) {
174-
error_log("Proxy error: {$e->getMessage()} in {$e->getFile()}:{$e->getLine()}");
175+
Console::error("Proxy error: {$e->getMessage()} in {$e->getFile()}:{$e->getLine()}");
175176

176177
$response->status(503);
177178
$response->header('Content-Type', 'application/json');
@@ -187,8 +188,7 @@ public function onRequest(Request $request, Response $response): void
187188
*/
188189
protected function forwardRequest(Request $request, Response $response, string $endpoint, ?Telemetry $telemetry = null): void
189190
{
190-
[$host, $port] = explode(':', $endpoint . ':80');
191-
$port = (int) $port;
191+
[$host, $port] = Adapter::parseEndpoint($endpoint, 80);
192192

193193
$poolKey = "{$host}:{$port}";
194194
if (!isset($this->pools[$poolKey])) {
@@ -318,8 +318,7 @@ protected function forwardRawRequest(Request $request, Response $response, strin
318318
return;
319319
}
320320

321-
[$host, $port] = explode(':', $endpoint . ':80');
322-
$port = (int) $port;
321+
[$host, $port] = Adapter::parseEndpoint($endpoint, 80);
323322

324323
$poolKey = "raw:{$host}:{$port}";
325324
if (!isset($this->pools[$poolKey])) {

‎src/Server/HTTP/SwooleCoroutine.php‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Swoole\Coroutine\Http\Server as CoroutineServer;
1010
use Swoole\Http\Request;
1111
use Swoole\Http\Response;
12+
use Utopia\Console;
1213
use Utopia\Proxy\Adapter;
1314
use Utopia\Proxy\Protocol;
1415
use Utopia\Proxy\Resolver;
@@ -84,7 +85,7 @@ protected function configure(): void
8485
protected function initAdapter(): void
8586
{
8687
$this->adapter = new Adapter($this->resolver, name: 'HTTP', protocol: Protocol::HTTP);
87-
$this->adapter->setCacheTtl($this->config->cacheTTL);
88+
$this->adapter->setCacheTTL($this->config->cacheTTL);
8889

8990
if ($this->config->skipValidation) {
9091
$this->adapter->setSkipValidation(true);
@@ -93,21 +94,37 @@ protected function initAdapter(): void
9394

9495
public function onStart(): void
9596
{
96-
echo "HTTP Proxy Server started at http://{$this->config->host}:{$this->config->port}\n";
97-
echo "Workers: {$this->config->workers}\n";
98-
echo "Max connections: {$this->config->maxConnections}\n";
97+
Console::success("HTTP Proxy Server started at http://{$this->config->host}:{$this->config->port}");
98+
Console::log("Workers: {$this->config->workers}");
99+
Console::log("Max connections: {$this->config->maxConnections}");
99100
}
100101

101102
public function onWorkerStart(int $workerId = 0): void
102103
{
103-
echo "Worker #{$workerId} started (Adapter: {$this->adapter->getName()})\n";
104+
if ($this->config->workerStart !== null) {
105+
($this->config->workerStart)(null, $workerId, $this->adapter);
106+
}
107+
108+
Console::log("Worker #{$workerId} started (Adapter: {$this->adapter->getName()})");
104109
}
105110

106111
/**
107112
* Main request handler
108113
*/
109114
public function onRequest(Request $request, Response $response): void
110115
{
116+
if ($this->config->requestHandler !== null) {
117+
try {
118+
($this->config->requestHandler)($request, $response, $this->adapter);
119+
} catch (\Throwable $e) {
120+
Console::error("Request handler error: {$e->getMessage()}");
121+
$response->status(500);
122+
$response->end('Internal Server Error');
123+
}
124+
125+
return;
126+
}
127+
111128
try {
112129
if ($this->config->directResponse !== null) {
113130
$response->status($this->config->directResponseStatus);
@@ -157,7 +174,7 @@ public function onRequest(Request $request, Response $response): void
157174
}
158175

159176
} catch (\Exception $e) {
160-
error_log("Proxy error: {$e->getMessage()} in {$e->getFile()}:{$e->getLine()}");
177+
Console::error("Proxy error: {$e->getMessage()} in {$e->getFile()}:{$e->getLine()}");
161178

162179
$response->status(503);
163180
$response->header('Content-Type', 'application/json');
@@ -173,8 +190,7 @@ public function onRequest(Request $request, Response $response): void
173190
*/
174191
protected function forwardRequest(Request $request, Response $response, string $endpoint, ?Telemetry $telemetry = null): void
175192
{
176-
[$host, $port] = explode(':', $endpoint . ':80');
177-
$port = (int) $port;
193+
[$host, $port] = Adapter::parseEndpoint($endpoint, 80);
178194

179195
$poolKey = "{$host}:{$port}";
180196
if (!isset($this->pools[$poolKey])) {
@@ -304,8 +320,7 @@ protected function forwardRawRequest(Request $request, Response $response, strin
304320
return;
305321
}
306322

307-
[$host, $port] = explode(':', $endpoint . ':80');
308-
$port = (int) $port;
323+
[$host, $port] = Adapter::parseEndpoint($endpoint, 80);
309324

310325
$poolKey = "raw:{$host}:{$port}";
311326
if (!isset($this->pools[$poolKey])) {

‎src/Server/SMTP/Config.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ public function __construct(
1414
public readonly int $bufferOutputSize = 2 * 1024 * 1024,
1515
public readonly bool $enableCoroutine = true,
1616
public readonly int $maxWaitTime = 60,
17-
public readonly int $timeout = 30,
17+
public readonly float $timeout = 30.0,
1818
public readonly float $connectTimeout = 5.0,
1919
public readonly bool $skipValidation = false,
2020
public readonly int $cacheTTL = 60,

‎src/Server/SMTP/Connection.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,14 @@
66

77
class Connection
88
{
9-
public string $state = 'greeting';
9+
public string $state = 'command';
1010

1111
public ?string $domain = null;
1212

1313
public ?Client $backend = null;
14+
15+
public function isData(): bool
16+
{
17+
return $this->state === 'data';
18+
}
1419
}

0 commit comments

Comments
 (0)