Skip to content

Commit ad19410

Browse files
committed
[UPD] tests and security
1 parent ff87001 commit ad19410

16 files changed

Lines changed: 1127 additions & 5 deletions

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@
2525
- **Socket file permission race closed.** `SessionManagerDaemon::run()` now calls `umask(0077)` around the `SocketServer` bind so the Unix socket is created `0600` atomically. Previously a permissive process umask could leave the socket world-readable/writable in the window between `bind()` and the follow-up `chmod()`, and a daemon crash in that window could leave a permissive leftover on disk.
2626
- **`username` no longer leaked via the `list` IPC command.** Added `'username'` to `CommandHandler::SENSITIVE_CONFIG_KEYS`; the session-lookup cache key (`SessionConfig::sanitized()`) still preserves username to keep sessions properly scoped per user. A local peer calling `list` can no longer enumerate `(endpoint → username)` tuples of other sessions, which previously enabled targeted credential-stuffing against those endpoints.
2727
- **Per-frame NDJSON cap on inbound IPC.** Added `SessionManagerDaemon::MAX_FRAME_BYTES = 65_536` and a length check before `json_decode()`. Closes a DoS partiale where a single client could force repeated parsing of ~1 MiB of JSON per connection (MAX_BUFFER_SIZE) × 50 concurrent connections. Legitimate requests are under 2 KiB, 64 KiB is comfortable headroom.
28+
- **IPv4-mapped IPv6 loopback now accepted/rejected consistently.** `TcpLoopbackTransport::isLoopbackAddress()` previously rejected `::ffff:127.0.0.1` (false negative) and would have misclassified `::ffff:192.168.1.10` as non-loopback only by coincidence. Explicit handling added: `::ffff:127.*` accepted, everything else under `::ffff:` rejected at construction.
29+
- **`sanitizeErrorMessage` redacts Windows paths and URLs.** The previous Unix-only regex let `C:\Users\...\secret.pem` and URLs with embedded credentials (`opc.tcp://user:pwd@host`) leak unchanged. Three regexes now run: URL (any scheme), Windows path, Unix path; each emits `[url]` / `[path]`. Regression tests added in `CommandHandlerSecurityTest`.
30+
- **PID check conservative fallback.** `SessionManagerDaemon::isProcessRunning()` now treats "can neither call `posix_kill` nor read `/proc`" as "process alive" instead of "dead". Prevents a new daemon from stealing the PID file from a live instance on sandboxed environments where both introspection paths are denied.
2831
- **Persistent cache hardening.** `opcua-client` v4.3.0 removed `unserialize()` from every cache code path in favour of JSON gated by an allowlist (`Cache\WireCacheCodec`). The daemon is long-running and its per-session caches persist across requests, so upgrade paths that share a cache backend across processes should flush it once on upgrade. No API change for the daemon itself — the new `CacheCodecInterface` is picked up automatically via the default `ClientBuilder`.
2932

3033
## [4.2.0] - 2026-04-17

ROADMAP.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
- [x] Extracted `bin/opcua-session-manager` argv parser into `src/Cli/ArgvParser` (unit-testable; reports missing-value errors instead of silently dropping them).
1414
- [x] Added `tests/Unit/ManagedClientTcpTest.php` to give cross-OS coverage to the ManagedClient IPC error-mapping path (`ManagedClientIpcTest` is still Unix-only via `->skipOnWindows()`).
1515
- [x] Replaced a fragile `basename(str_replace('\\', '/', …))` short-class-name hack in `CommandHandler` with `ReflectionClass::getShortName()`.
16-
- [x] **Security audit findings addressed**: socket-file permission race closed via `umask(0077)` around `SocketServer` bind; `username` stripped from the `list` IPC response (session-lookup cache key unchanged); per-frame 64 KiB cap added on inbound NDJSON.
16+
- [x] **Security audit findings addressed**: socket-file permission race closed via `umask(0077)` around `SocketServer` bind; `username` stripped from the `list` IPC response (session-lookup cache key unchanged); per-frame 64 KiB cap added on inbound NDJSON; IPv4-mapped IPv6 loopback handling added to `TcpLoopbackTransport`; `sanitizeErrorMessage` now redacts Windows paths and URL schemes; `isProcessRunning()` conservative fallback when introspection is unavailable.
1717

1818
## v4.2.0
1919

phpunit.xml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44
bootstrap="vendor/autoload.php"
55
colors="true"
66
>
7+
<php>
8+
<ini name="memory_limit" value="512M"/>
9+
</php>
710
<testsuites>
811
<testsuite name="Test Suite">
912
<directory suffix="Test.php">./tests</directory>
@@ -13,5 +16,15 @@
1316
<include>
1417
<directory>src</directory>
1518
</include>
19+
<exclude>
20+
<file>src/Daemon/SessionManagerDaemon.php</file>
21+
</exclude>
1622
</source>
23+
<coverage>
24+
<report>
25+
<html outputDirectory="coverage/html"/>
26+
<clover outputFile="coverage/clover.xml"/>
27+
<text outputFile="coverage/coverage.txt" showOnlySummary="true"/>
28+
</report>
29+
</coverage>
1730
</phpunit>

src/Daemon/CommandHandler.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -723,6 +723,8 @@ private function sanitizeConfig(array $config): array
723723

724724
private function sanitizeErrorMessage(string $message): string
725725
{
726+
$message = preg_replace('#[a-z][a-z0-9+.-]*://[^\s]+#i', '[url]', $message);
727+
$message = preg_replace('#[A-Za-z]:\\\\[^\s]+#', '[path]', $message);
726728
$message = preg_replace('#/[^\s:]+/[^\s:]+#', '[path]', $message);
727729

728730
if (strlen($message) > self::MAX_ERROR_MESSAGE_LENGTH) {

src/Daemon/SessionManagerDaemon.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,10 @@ private function isProcessRunning(int $pid): bool
446446
return posix_kill($pid, 0);
447447
}
448448

449-
return file_exists("/proc/{$pid}");
449+
if (is_dir('/proc')) {
450+
return file_exists("/proc/{$pid}");
451+
}
452+
453+
return true;
450454
}
451455
}

src/Ipc/TcpLoopbackTransport.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,14 @@ private static function isLoopbackAddress(string $host): bool
100100
return true;
101101
}
102102

103+
$normalized = strtolower($host);
104+
if (str_starts_with($normalized, '::ffff:')) {
105+
$mapped = substr($normalized, 7);
106+
if ($mapped === '127.0.0.1' || str_starts_with($mapped, '127.')) {
107+
return filter_var($mapped, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) !== false;
108+
}
109+
}
110+
103111
return false;
104112
}
105113
}

src/Serialization/TypeSerializer.php

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,6 @@ public function serializeMonitoredItemModifyResult(MonitoredItemModifyResult $re
335335
public function serializeSetTriggeringResult(SetTriggeringResult $result): array
336336
{
337337
return [
338-
'statusCode' => $result->statusCode,
339338
'addResults' => $result->addResults,
340339
'removeResults' => $result->removeResults,
341340
];
@@ -572,7 +571,6 @@ public function deserializeMonitoredItemModifyResult(array $data): MonitoredItem
572571
public function deserializeSetTriggeringResult(array $data): SetTriggeringResult
573572
{
574573
return new SetTriggeringResult(
575-
(int)$data['statusCode'],
576574
array_map('intval', $data['addResults'] ?? []),
577575
array_map('intval', $data['removeResults'] ?? []),
578576
);

tests/Helpers/FakeTcpDaemon.php

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpOpcua\SessionManager\Tests\Helpers;
6+
7+
use PhpOpcua\SessionManager\Client\ManagedClient;
8+
use ReflectionProperty;
9+
use RuntimeException;
10+
11+
/**
12+
* Cross-OS fake daemon over TCP loopback for unit testing `ManagedClient`.
13+
*
14+
* Uses `proc_open()` + `tcp://127.0.0.1:0` instead of `pcntl_fork()` + `unix://`,
15+
* so it runs on Linux, macOS, and Windows.
16+
*/
17+
final class FakeTcpDaemon
18+
{
19+
/**
20+
* @param array<int, array<string, mixed>> $responses JSON objects emitted in order, one per inbound frame.
21+
* @return array{endpoint: string, process: resource, pipes: array, scriptFile: string}
22+
*/
23+
public static function start(array $responses): array
24+
{
25+
$listener = @stream_socket_server('tcp://127.0.0.1:0', $errno, $errstr);
26+
if ($listener === false) {
27+
throw new RuntimeException("Cannot open listener: [{$errno}] {$errstr}");
28+
}
29+
$name = stream_socket_get_name($listener, false);
30+
fclose($listener);
31+
if ($name === false) {
32+
throw new RuntimeException('Cannot resolve listener endpoint');
33+
}
34+
[$host, $port] = explode(':', $name);
35+
36+
$responsesArg = base64_encode(serialize($responses));
37+
38+
$script = <<<'PHP'
39+
<?php
40+
$responses = unserialize(base64_decode($argv[1]));
41+
$host = $argv[2];
42+
$port = (int) $argv[3];
43+
44+
$server = stream_socket_server("tcp://{$host}:{$port}");
45+
if ($server === false) {
46+
exit(1);
47+
}
48+
49+
// Ready-probe: accept the parent readiness connection and close it without
50+
// consuming an entry from $responses.
51+
$probe = @stream_socket_accept($server, 10);
52+
if ($probe !== false) {
53+
fclose($probe);
54+
}
55+
56+
foreach ($responses as $responseData) {
57+
$conn = @stream_socket_accept($server, 10);
58+
if ($conn === false) {
59+
break;
60+
}
61+
62+
$data = '';
63+
while (!str_contains($data, "\n")) {
64+
$chunk = fread($conn, 65536);
65+
if ($chunk === false || $chunk === '') {
66+
break;
67+
}
68+
$data .= $chunk;
69+
}
70+
71+
fwrite($conn, json_encode($responseData) . "\n");
72+
fclose($conn);
73+
}
74+
75+
fclose($server);
76+
exit(0);
77+
PHP;
78+
79+
$scriptFile = tempnam(sys_get_temp_dir(), 'opcua_fake_tcp_') . '.php';
80+
file_put_contents($scriptFile, $script);
81+
82+
$descriptors = [
83+
0 => ['pipe', 'r'],
84+
1 => ['pipe', 'w'],
85+
2 => ['pipe', 'w'],
86+
];
87+
$process = proc_open(
88+
[PHP_BINARY, $scriptFile, $responsesArg, $host, $port],
89+
$descriptors,
90+
$pipes,
91+
);
92+
if (! is_resource($process)) {
93+
@unlink($scriptFile);
94+
95+
throw new RuntimeException('Cannot spawn fake daemon subprocess');
96+
}
97+
98+
set_error_handler(static fn (): bool => true);
99+
try {
100+
$probeConnected = false;
101+
$deadline = microtime(true) + 3.0;
102+
while (microtime(true) < $deadline) {
103+
$probe = @stream_socket_client("tcp://{$host}:{$port}", $err, $errstr, 0.2);
104+
if ($probe !== false) {
105+
fclose($probe);
106+
$probeConnected = true;
107+
break;
108+
}
109+
usleep(50_000);
110+
}
111+
} finally {
112+
restore_error_handler();
113+
}
114+
115+
if (! $probeConnected) {
116+
@unlink($scriptFile);
117+
proc_terminate($process);
118+
proc_close($process);
119+
120+
throw new RuntimeException("Fake daemon did not bind tcp://{$host}:{$port} within 3s");
121+
}
122+
123+
return [
124+
'endpoint' => "tcp://{$host}:{$port}",
125+
'process' => $process,
126+
'pipes' => $pipes,
127+
'scriptFile' => $scriptFile,
128+
];
129+
}
130+
131+
/**
132+
* @param array{process: resource, pipes: array, scriptFile: string} $daemon
133+
* @return void
134+
*/
135+
public static function stop(array $daemon): void
136+
{
137+
foreach ($daemon['pipes'] as $pipe) {
138+
if (is_resource($pipe)) {
139+
@fclose($pipe);
140+
}
141+
}
142+
if (is_resource($daemon['process'])) {
143+
$status = proc_get_status($daemon['process']);
144+
if ($status['running']) {
145+
proc_terminate($daemon['process']);
146+
usleep(200_000);
147+
$status = proc_get_status($daemon['process']);
148+
if ($status['running']) {
149+
proc_terminate($daemon['process'], 9);
150+
}
151+
}
152+
proc_close($daemon['process']);
153+
}
154+
if (isset($daemon['scriptFile']) && file_exists($daemon['scriptFile'])) {
155+
@unlink($daemon['scriptFile']);
156+
}
157+
}
158+
159+
/**
160+
* @param string $endpoint
161+
* @param string $sessionId
162+
* @param float $timeout
163+
* @return ManagedClient
164+
*/
165+
public static function connectClient(string $endpoint, string $sessionId = 'fake-session-id', float $timeout = 2.0): ManagedClient
166+
{
167+
$client = new ManagedClient($endpoint, timeout: $timeout);
168+
169+
$ref = new ReflectionProperty(ManagedClient::class, 'sessionId');
170+
$ref->setValue($client, $sessionId);
171+
172+
return $client;
173+
}
174+
}
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
use PhpOpcua\Client\Client;
6+
use PhpOpcua\Client\Module\Subscription\SubscriptionResult;
7+
use PhpOpcua\SessionManager\Daemon\CommandHandler;
8+
use PhpOpcua\SessionManager\Daemon\Session;
9+
use PhpOpcua\SessionManager\Daemon\SessionStore;
10+
11+
describe('CommandHandler::autoConnectSession', function () {
12+
13+
beforeEach(function () {
14+
$this->store = new SessionStore();
15+
$this->handler = new CommandHandler(store: $this->store);
16+
});
17+
18+
it('returns null when handleOpen fails (max sessions reached)', function () {
19+
$handler = new CommandHandler(store: $this->store, maxSessions: 1);
20+
$client = $this->createStub(Client::class);
21+
$session = new Session('existing', $client, 'opc.tcp://localhost:4840', [], microtime(true));
22+
$this->store->create($session);
23+
24+
$result = $handler->autoConnectSession(
25+
'opc.tcp://another.example:4840',
26+
[],
27+
[],
28+
);
29+
30+
expect($result)->toBeNull();
31+
});
32+
33+
it('creates subscriptions + monitored items on a pre-existing session', function () {
34+
$client = $this->createMock(Client::class);
35+
$client->expects($this->once())
36+
->method('createSubscription')
37+
->willReturn(new SubscriptionResult(42, 500.0, 2400, 10));
38+
$client->expects($this->once())
39+
->method('createMonitoredItems')
40+
->with(42, $this->callback(fn ($items) => count($items) === 1 && $items[0]['nodeId'] === 'ns=2;i=1001'));
41+
$client->expects($this->once())
42+
->method('createEventMonitoredItem')
43+
->with(42, 'ns=0;i=2253', $this->anything(), 7);
44+
45+
$session = new Session('s1', $client, 'opc.tcp://localhost:4840', [], microtime(true));
46+
$this->store->create($session);
47+
48+
$sessionId = $this->handler->autoConnectSession(
49+
'opc.tcp://localhost:4840',
50+
[],
51+
[
52+
[
53+
'publishing_interval' => 500.0,
54+
'lifetime_count' => 2400,
55+
'max_keep_alive_count' => 10,
56+
'priority' => 1,
57+
'monitored_items' => [
58+
[
59+
'node_id' => 'ns=2;i=1001',
60+
'attribute_id' => 13,
61+
'sampling_interval' => 250.0,
62+
'queue_size' => 1,
63+
'client_handle' => 1,
64+
],
65+
],
66+
'event_monitored_items' => [
67+
[
68+
'node_id' => 'ns=0;i=2253',
69+
'select_fields' => ['EventId', 'Severity'],
70+
'client_handle' => 7,
71+
],
72+
],
73+
],
74+
],
75+
);
76+
77+
expect($sessionId)->toBe('s1');
78+
});
79+
80+
it('skips monitored-items path when none are configured', function () {
81+
$client = $this->createMock(Client::class);
82+
$client->expects($this->once())
83+
->method('createSubscription')
84+
->willReturn(new SubscriptionResult(99, 500.0, 2400, 10));
85+
$client->expects($this->never())->method('createMonitoredItems');
86+
$client->expects($this->never())->method('createEventMonitoredItem');
87+
88+
$session = new Session('s2', $client, 'opc.tcp://localhost:4841', [], microtime(true));
89+
$this->store->create($session);
90+
91+
$sessionId = $this->handler->autoConnectSession(
92+
'opc.tcp://localhost:4841',
93+
[],
94+
[['publishing_interval' => 500.0]],
95+
);
96+
97+
expect($sessionId)->toBe('s2');
98+
});
99+
100+
});

0 commit comments

Comments
 (0)