From 9bad17bfc4e169401e56d15ab20084e9fcff6965 Mon Sep 17 00:00:00 2001 From: loks0n <22452787+loks0n@users.noreply.github.com> Date: Wed, 4 Nov 2026 12:43:30 +0000 Subject: [PATCH 1/5] fix(security): do not follow container-planted links in logs, build output or cache The executor reads files that a runtime or build container can write: execution logs in /mnt/logs, build logs in /tmp/logging, the build output and the build cache. Every read followed symlinks, and the execution log id from the x-open-runtimes-log-id header was used unchecked. A function could point its log file at /proc/self/environ and get the executor's environment, including OPR_EXECUTOR_SECRET, back as its logs. RegularFile only reads an entry that lstat says is a regular file and that fstat confirms is the same inode once opened. Build output and cache uploads are re-checked afterwards, and the log id must be alphanumeric. Co-Authored-By: Claude Opus 5.5 --- src/Executor/Logs.php | 8 +- src/Executor/RegularFile.php | 93 +++++++++++++++++ src/Executor/Runner/Docker.php | 77 +++++++------- tests/unit/Executor/RegularFileTest.php | 128 ++++++++++++++++++++++++ 4 files changed, 268 insertions(+), 38 deletions(-) create mode 100644 src/Executor/RegularFile.php create mode 100644 tests/unit/Executor/RegularFileTest.php diff --git a/src/Executor/Logs.php b/src/Executor/Logs.php index 1845359..ce3c73b 100644 --- a/src/Executor/Logs.php +++ b/src/Executor/Logs.php @@ -19,13 +19,13 @@ public static function get(string $containerId): array $logsFile = $dir . "/logs.txt"; $timingsFile = $dir . "/timings.txt"; - if (!\file_exists($logsFile) || !\file_exists($timingsFile)) { + // The build container writes both files, so a symlink or anything else reads as empty. + $logs = RegularFile::read($logsFile); + $timings = RegularFile::read($timingsFile); + if ($logs === '' || $timings === '') { return []; } - $logs = \file_get_contents($logsFile) ?: ''; - $timings = \file_get_contents($timingsFile) ?: ''; - $offset = 0; // Current offset from timing for reading logs content $introOffset = self::getLogOffset($logs); diff --git a/src/Executor/RegularFile.php b/src/Executor/RegularFile.php new file mode 100644 index 0000000..9a306dc --- /dev/null +++ b/src/Executor/RegularFile.php @@ -0,0 +1,93 @@ +|null + */ + public static function stat(string $path): ?array + { + $handle = self::open($path, $stat); + if ($handle === null) { + return null; + } + + \fclose($handle); + + return $stat; + } + + /** + * Whether $path is still the regular file $stat described, with the same size. + * + * @param array $stat + */ + public static function unchanged(string $path, array $stat): bool + { + $now = self::stat($path); + + return $now !== null && $now['dev'] === $stat['dev'] && $now['ino'] === $stat['ino'] && $now['size'] === $stat['size']; + } + + /** + * Up to $length bytes of $path from $offset (all of it when $length is null), or '' when + * $path is not a regular file. + */ + public static function read(string $path, int $offset = 0, ?int $length = null): string + { + $handle = self::open($path); + if ($handle === null) { + return ''; + } + + try { + return (string) \stream_get_contents($handle, $length, $offset); + } finally { + \fclose($handle); + } + } + + /** + * @param array|null $stat + * @return resource|null + */ + private static function open(string $path, ?array &$stat = null) + { + $link = @\lstat($path); + // lstat describes the entry itself, so a symlink is refused rather than followed. + if ($link === false || ($link['mode'] & 0170000) !== 0100000) { + return null; + } + + // 'n' opens with O_NONBLOCK: a FIFO swapped in after the lstat would otherwise block + // this open forever with no writer. It opens at once instead, and the inode check refuses it. + $handle = @\fopen($path, 'rbn'); + if ($handle === false) { + return null; + } + + // Swapped for a symlink between lstat and fopen, the open followed it to another inode. + $opened = \fstat($handle); + if ($opened === false || $opened['dev'] !== $link['dev'] || $opened['ino'] !== $link['ino']) { + \fclose($handle); + return null; + } + + $stat = $link; + + return $handle; + } +} diff --git a/src/Executor/Runner/Docker.php b/src/Executor/Runner/Docker.php index 820c1ce..a725aac 100644 --- a/src/Executor/Runner/Docker.php +++ b/src/Executor/Runner/Docker.php @@ -4,6 +4,7 @@ use OpenRuntimes\Executor\Logs; use OpenRuntimes\Executor\Exception as ExecutorException; +use OpenRuntimes\Executor\RegularFile; use OpenRuntimes\Executor\Runner\Repository\Runtimes; use OpenRuntimes\Executor\StorageFactory; use OpenRuntimes\Executor\Validator\TCP; @@ -82,8 +83,9 @@ public function getLogs(string $runtimeId, int $timeout, Response $response): vo throw new ExecutorException(ExecutorException::LOGS_TIMEOUT); } - if (\file_exists($tmpLogging . '/logs.txt') && \file_exists($tmpLogging . '/timings.txt')) { - $timings = \file_get_contents($tmpLogging . '/timings.txt') ?: ''; + // The build container writes these files, so only regular files are read (see RegularFile). + if (RegularFile::stat($tmpLogging . '/logs.txt') !== null) { + $timings = RegularFile::read($tmpLogging . '/timings.txt'); if ($timings !== '') { break; } @@ -148,7 +150,7 @@ public function getLogs(string $runtimeId, int $timeout, Response $response): vo }); $offset = 0; // Current offset from timing for reading logs content - $tempLogsContent = \file_get_contents($tmpLogging . '/logs.txt') ?: ''; + $tempLogsContent = RegularFile::read($tmpLogging . '/logs.txt'); $introOffset = Logs::getLogOffset($tempLogsContent); $datetime = new \DateTime("now", new \DateTimeZone("UTC")); // Date used for tracking absolute log timing @@ -159,7 +161,7 @@ public function getLogs(string $runtimeId, int $timeout, Response $response): vo $logsProcess = $process; $logsPath = $tmpLogging . '/logs.txt'; - if (!\file_exists($logsPath)) { + if (RegularFile::stat($logsPath) === null) { if (!empty($logsProcess)) { \proc_terminate($logsProcess, 9); } @@ -173,7 +175,7 @@ public function getLogs(string $runtimeId, int $timeout, Response $response): vo $timestamp = $part['timestamp'] ?? ''; $length = \intval($part['length'] ?? '0'); - $logContent = \file_get_contents($tmpLogging . '/logs.txt', false, null, $introOffset + $offset, \abs($length)) ?: ''; + $logContent = RegularFile::read($logsPath, $introOffset + $offset, \abs($length)); $logContent = \str_replace("\n", "\\n", $logContent); @@ -447,13 +449,18 @@ public function createRuntime( * Move built code to expected build directory */ if ($destination !== '' && $destination !== '0') { - // Check if the build was successful by checking if file exists - if (!$localDevice->exists($tmpBuild)) { - throw new \Exception('Something went wrong when starting runtime.'); + // The build container can write this path. Following a symlink there would upload the + // executor's own files (its environment) as the build, for the owner to download. + $stat = RegularFile::stat($tmpBuild); + if ($stat === null) { + if (!\is_link($tmpBuild) && !\file_exists($tmpBuild)) { + throw new \Exception('Something went wrong when starting runtime.'); + } + + throw new ExecutorException(ExecutorException::BUILD_FAILED, 'Build output must be a regular file.'); } - $size = $localDevice->getFileSize($tmpBuild); - $container['size'] = $size; + $container['size'] = $stat['size']; $destinationDevice = StorageFactory::getDevice($destination, System::getEnv('OPR_EXECUTOR_CONNECTION_STORAGE')); $path = $destinationDevice->getPath(\uniqid() . '.' . \pathinfo($tmpBuild, PATHINFO_EXTENSION)); @@ -462,6 +469,12 @@ public function createRuntime( throw new \Exception('Failed to move built code to storage'); }; + // The build container is still running, so the file could have been swapped mid-copy. + if (!RegularFile::unchanged($tmpBuild, $stat)) { + $destinationDevice->delete($path); + throw new ExecutorException(ExecutorException::BUILD_FAILED, 'Build output changed while it was being stored.'); + } + $container['path'] = $path; } @@ -631,7 +644,9 @@ private function storeBuildCacheArtifact(string $cacheKey, string $tmpCache, Loc { $artifact = $tmpCache . '/stores.sqfs'; - if (!$localDevice->exists($artifact)) { + // /cache is mounted into the build container, so anything but a regular file is skipped. + $stat = RegularFile::stat($artifact); + if ($stat === null) { return; } @@ -641,6 +656,11 @@ private function storeBuildCacheArtifact(string $cacheKey, string $tmpCache, Loc if (!$localDevice->copy($artifact, $destinationArtifact, $cacheDevice)) { throw new \Exception('Failed to store build cache artifact'); } + + if (!RegularFile::unchanged($artifact, $stat)) { + $cacheDevice->delete($destinationArtifact); + throw new \Exception('Build cache artifact changed while it was being stored'); + } } private function getBuildCacheDevice(): \Utopia\Storage\Device @@ -1023,34 +1043,23 @@ public function createExecution( $logs = ''; $errors = ''; - if ($fileId !== '' && $fileId !== '0') { + // The runtime sends this id, so it may only name a file directly in its own logs directory. + if ($fileId !== '' && $fileId !== '0' && \ctype_alnum($fileId)) { $logFile = '/tmp/' . $runtimeName . '/logs/' . $fileId . '_logs.log'; $errorFile = '/tmp/' . $runtimeName . '/logs/' . $fileId . '_errors.log'; - $logDevice = new Local(); - - if ($logDevice->exists($logFile)) { - if ($logDevice->getFileSize($logFile) > MAX_LOG_SIZE) { - $maxToRead = MAX_LOG_SIZE; - $logs = (string) $logDevice->read($logFile, 0, $maxToRead); - $logs .= "\nLog file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; - } else { - $logs = (string) $logDevice->read($logFile); - } - - $logDevice->delete($logFile); + // The runtime writes these files, so a symlink is refused, never followed. unlink + // removes a symlink itself, never its target. + $logs = RegularFile::read($logFile, 0, MAX_LOG_SIZE + 1); + @\unlink($logFile); + if (\strlen($logs) > MAX_LOG_SIZE) { + $logs = \substr($logs, 0, MAX_LOG_SIZE) . "\nLog file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; } - if ($logDevice->exists($errorFile)) { - if ($logDevice->getFileSize($errorFile) > MAX_LOG_SIZE) { - $maxToRead = MAX_LOG_SIZE; - $errors = (string) $logDevice->read($errorFile, 0, $maxToRead); - $errors .= "\nError file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; - } else { - $errors = (string) $logDevice->read($errorFile); - } - - $logDevice->delete($errorFile); + $errors = RegularFile::read($errorFile, 0, MAX_LOG_SIZE + 1); + @\unlink($errorFile); + if (\strlen($errors) > MAX_LOG_SIZE) { + $errors = \substr($errors, 0, MAX_LOG_SIZE) . "\nError file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; } } diff --git a/tests/unit/Executor/RegularFileTest.php b/tests/unit/Executor/RegularFileTest.php new file mode 100644 index 0000000..9b6ef5d --- /dev/null +++ b/tests/unit/Executor/RegularFileTest.php @@ -0,0 +1,128 @@ +dir = \sys_get_temp_dir() . '/regular-file-' . \bin2hex(\random_bytes(6)); + \mkdir($this->dir); + + // Stands in for the executor's own files, such as /proc/self/environ. Two lines, because + // build logs drop the first line as the script banner. + $this->secret = $this->dir . '/secret'; + \file_put_contents($this->secret, "first line\nOPR_EXECUTOR_SECRET=leaked"); + } + + protected function tearDown(): void + { + $this->remove($this->dir); + + parent::tearDown(); + } + + public function testReadsARegularFile(): void + { + \file_put_contents($this->dir . '/logs.log', 'hello world'); + + $this->assertSame('hello world', RegularFile::read($this->dir . '/logs.log')); + $this->assertSame('world', RegularFile::read($this->dir . '/logs.log', 6)); + $this->assertSame('hello', RegularFile::read($this->dir . '/logs.log', 0, 5)); + $this->assertNotNull(RegularFile::stat($this->dir . '/logs.log')); + } + + public function testDoesNotFollowASymlinkTheContainerPlanted(): void + { + \symlink($this->secret, $this->dir . '/logs.log'); + + $this->assertSame('', RegularFile::read($this->dir . '/logs.log')); + $this->assertNull(RegularFile::stat($this->dir . '/logs.log')); + $this->assertStringContainsString('leaked', (string) \file_get_contents($this->secret)); + } + + public function testRefusesAFifoWithoutBlocking(): void + { + if (!\function_exists('posix_mkfifo')) { + $this->markTestSkipped('posix_mkfifo is not available'); + } + + \posix_mkfifo($this->dir . '/logs.log', 0600); + + $this->assertSame('', RegularFile::read($this->dir . '/logs.log')); + $this->assertNull(RegularFile::stat($this->dir . '/logs.log')); + } + + public function testMissingPathReadsAsEmpty(): void + { + $this->assertSame('', RegularFile::read($this->dir . '/missing.log')); + $this->assertNull(RegularFile::stat($this->dir . '/missing.log')); + } + + public function testDetectsAFileSwappedForASymlinkAfterTheCheck(): void + { + $path = $this->dir . '/code.tar.gz'; + \file_put_contents($path, 'build'); + $stat = RegularFile::stat($path); + $this->assertNotNull($stat); + $this->assertTrue(RegularFile::unchanged($path, $stat)); + + \unlink($path); + \symlink($this->secret, $path); + + $this->assertFalse(RegularFile::unchanged($path, $stat)); + } + + public function testBuildLogsDoNotFollowASymlinkTheBuildPlanted(): void + { + // Logs::get reads /tmp//logging, the directory the build mounts as /tmp/logging. + // app/init.php defines it in the executor; the unit suite doesn't load that. + if (!\defined('MAX_BUILD_LOG_SIZE')) { + \define('MAX_BUILD_LOG_SIZE', 1000 * 1000); + } + + $container = 'regular-file-test-' . \bin2hex(\random_bytes(6)); + $logging = '/tmp/' . $container . '/logging'; + \mkdir($logging, 0777, true); + + try { + \symlink($this->secret, $logging . '/logs.txt'); + \file_put_contents($logging . '/timings.txt', "0.1 30\n"); + + $output = \json_encode(Logs::get($container)); + + $this->assertIsString($output); + $this->assertStringNotContainsString('leaked', $output); + } finally { + $this->remove('/tmp/' . $container); + } + } + + private function remove(string $path): void + { + if (\is_link($path) || \is_file($path) || \filetype($path) === 'fifo') { + \unlink($path); + return; + } + + foreach (\scandir($path) ?: [] as $entry) { + if ($entry !== '.' && $entry !== '..') { + $this->remove($path . '/' . $entry); + } + } + + \rmdir($path); + } +} From 5f08891bdfe0380009c5d6a7b2255669acc35975 Mon Sep 17 00:00:00 2001 From: loks0n <22452787+loks0n@users.noreply.github.com> Date: Wed, 4 Nov 2026 12:53:21 +0000 Subject: [PATCH 2/5] fix(security): upload build output and cache from a private copy Checking the path and re-checking it after the upload still left a window: the build container is running, so it could swap a symlink in for the upload's own open and put the original back before the re-check. RegularFile::copy now copies from the handle that passed the checks into /tmp/, which no container mounts, and the upload reads that copy. Co-Authored-By: Claude Opus 5.5 --- src/Executor/RegularFile.php | 34 +++++++++++++++++++++---- src/Executor/Runner/Docker.php | 30 ++++++++-------------- tests/unit/Executor/RegularFileTest.php | 34 ++++++++++++++++++------- 3 files changed, 65 insertions(+), 33 deletions(-) diff --git a/src/Executor/RegularFile.php b/src/Executor/RegularFile.php index 9a306dc..53daeac 100644 --- a/src/Executor/RegularFile.php +++ b/src/Executor/RegularFile.php @@ -31,15 +31,39 @@ public static function stat(string $path): ?array } /** - * Whether $path is still the regular file $stat described, with the same size. + * Copy $path to $destination, a new file outside the container's reach, from the handle that + * passed the checks, and return the bytes copied. Null when $path is not a regular file. * - * @param array $stat + * Uploading $path by name would reopen it, and a container still running can swap a symlink + * in for that open and back again. The copy is what gets uploaded instead. */ - public static function unchanged(string $path, array $stat): bool + public static function copy(string $path, string $destination): ?int { - $now = self::stat($path); + $source = self::open($path); + if ($source === null) { + return null; + } + + try { + // 'x' fails if anything already sits at $destination, a symlink included. + $target = @\fopen($destination, 'xb'); + if ($target === false) { + throw new \Exception('Failed to create ' . $destination); + } - return $now !== null && $now['dev'] === $stat['dev'] && $now['ino'] === $stat['ino'] && $now['size'] === $stat['size']; + try { + $copied = \stream_copy_to_stream($source, $target); + if ($copied === false) { + throw new \Exception('Failed to copy ' . $path); + } + + return $copied; + } finally { + \fclose($target); + } + } finally { + \fclose($source); + } } /** diff --git a/src/Executor/Runner/Docker.php b/src/Executor/Runner/Docker.php index a725aac..a8229da 100644 --- a/src/Executor/Runner/Docker.php +++ b/src/Executor/Runner/Docker.php @@ -451,8 +451,10 @@ public function createRuntime( if ($destination !== '' && $destination !== '0') { // The build container can write this path. Following a symlink there would upload the // executor's own files (its environment) as the build, for the owner to download. - $stat = RegularFile::stat($tmpBuild); - if ($stat === null) { + // The upload reads a copy in /tmp/, which no container mounts. + $snapshot = \dirname($tmpBuild, 2) . '/output-' . \basename($tmpBuild); + $size = RegularFile::copy($tmpBuild, $snapshot); + if ($size === null) { if (!\is_link($tmpBuild) && !\file_exists($tmpBuild)) { throw new \Exception('Something went wrong when starting runtime.'); } @@ -460,21 +462,15 @@ public function createRuntime( throw new ExecutorException(ExecutorException::BUILD_FAILED, 'Build output must be a regular file.'); } - $container['size'] = $stat['size']; + $container['size'] = $size; $destinationDevice = StorageFactory::getDevice($destination, System::getEnv('OPR_EXECUTOR_CONNECTION_STORAGE')); $path = $destinationDevice->getPath(\uniqid() . '.' . \pathinfo($tmpBuild, PATHINFO_EXTENSION)); - if (!$localDevice->copy($tmpBuild, $path, $destinationDevice)) { + if (!$localDevice->copy($snapshot, $path, $destinationDevice)) { throw new \Exception('Failed to move built code to storage'); }; - // The build container is still running, so the file could have been swapped mid-copy. - if (!RegularFile::unchanged($tmpBuild, $stat)) { - $destinationDevice->delete($path); - throw new ExecutorException(ExecutorException::BUILD_FAILED, 'Build output changed while it was being stored.'); - } - $container['path'] = $path; } @@ -644,23 +640,19 @@ private function storeBuildCacheArtifact(string $cacheKey, string $tmpCache, Loc { $artifact = $tmpCache . '/stores.sqfs'; - // /cache is mounted into the build container, so anything but a regular file is skipped. - $stat = RegularFile::stat($artifact); - if ($stat === null) { + // /cache is mounted into the build container, so anything but a regular file is skipped, and + // the upload reads a copy in /tmp/, which no container mounts. + $snapshot = \dirname($tmpCache) . '/cache-stores.sqfs'; + if (RegularFile::copy($artifact, $snapshot) === null) { return; } $cacheDevice = $this->getBuildCacheDevice(); $destinationArtifact = $this->getBuildCacheArtifactPath($cacheDevice, $cacheKey); - if (!$localDevice->copy($artifact, $destinationArtifact, $cacheDevice)) { + if (!$localDevice->copy($snapshot, $destinationArtifact, $cacheDevice)) { throw new \Exception('Failed to store build cache artifact'); } - - if (!RegularFile::unchanged($artifact, $stat)) { - $cacheDevice->delete($destinationArtifact); - throw new \Exception('Build cache artifact changed while it was being stored'); - } } private function getBuildCacheDevice(): \Utopia\Storage\Device diff --git a/tests/unit/Executor/RegularFileTest.php b/tests/unit/Executor/RegularFileTest.php index 9b6ef5d..e53f8f4 100644 --- a/tests/unit/Executor/RegularFileTest.php +++ b/tests/unit/Executor/RegularFileTest.php @@ -71,18 +71,34 @@ public function testMissingPathReadsAsEmpty(): void $this->assertNull(RegularFile::stat($this->dir . '/missing.log')); } - public function testDetectsAFileSwappedForASymlinkAfterTheCheck(): void + public function testCopiesARegularFile(): void { - $path = $this->dir . '/code.tar.gz'; - \file_put_contents($path, 'build'); - $stat = RegularFile::stat($path); - $this->assertNotNull($stat); - $this->assertTrue(RegularFile::unchanged($path, $stat)); + \file_put_contents($this->dir . '/code.tar.gz', 'build'); - \unlink($path); - \symlink($this->secret, $path); + $this->assertSame(5, RegularFile::copy($this->dir . '/code.tar.gz', $this->dir . '/output')); + $this->assertSame('build', \file_get_contents($this->dir . '/output')); + } + + public function testDoesNotCopyASymlinkTheBuildPlanted(): void + { + \symlink($this->secret, $this->dir . '/code.tar.gz'); + + $this->assertNull(RegularFile::copy($this->dir . '/code.tar.gz', $this->dir . '/output')); + $this->assertFileDoesNotExist($this->dir . '/output'); + } - $this->assertFalse(RegularFile::unchanged($path, $stat)); + public function testDoesNotCopyOverAnExistingEntry(): void + { + \file_put_contents($this->dir . '/code.tar.gz', 'build'); + \symlink($this->secret, $this->dir . '/output'); + + try { + RegularFile::copy($this->dir . '/code.tar.gz', $this->dir . '/output'); + $this->fail('Copied over an existing entry'); + } catch (\Exception) { + $this->assertStringContainsString('leaked', (string) \file_get_contents($this->secret)); + $this->assertStringNotContainsString('build', (string) \file_get_contents($this->secret)); + } } public function testBuildLogsDoNotFollowASymlinkTheBuildPlanted(): void From 1392190b51291daad196b5ac0e532e327b710285 Mon Sep 17 00:00:00 2001 From: loks0n <22452787+loks0n@users.noreply.github.com> Date: Wed, 4 Nov 2026 13:25:01 +0000 Subject: [PATCH 3/5] refactor: drop RegularFile::stat and the extra build-output error Existence checks only learn whether an entry is there, so they stay plain file_exists. A build output that is missing or not a regular file fails the same way it always failed when missing. Co-Authored-By: Claude Opus 5.5 --- src/Executor/RegularFile.php | 26 ++++--------------------- src/Executor/Runner/Docker.php | 12 ++++-------- tests/unit/Executor/RegularFileTest.php | 4 ---- 3 files changed, 8 insertions(+), 34 deletions(-) diff --git a/src/Executor/RegularFile.php b/src/Executor/RegularFile.php index 53daeac..83f8a76 100644 --- a/src/Executor/RegularFile.php +++ b/src/Executor/RegularFile.php @@ -12,24 +12,6 @@ */ final class RegularFile { - /** - * The lstat of $path when it is a regular file that opens as itself, otherwise null: - * a symlink, FIFO, device, directory or missing path. - * - * @return array|null - */ - public static function stat(string $path): ?array - { - $handle = self::open($path, $stat); - if ($handle === null) { - return null; - } - - \fclose($handle); - - return $stat; - } - /** * Copy $path to $destination, a new file outside the container's reach, from the handle that * passed the checks, and return the bytes copied. Null when $path is not a regular file. @@ -85,10 +67,12 @@ public static function read(string $path, int $offset = 0, ?int $length = null): } /** - * @param array|null $stat + * The open handle when $path is a regular file that opens as itself, otherwise null: + * a symlink, FIFO, device, directory or missing path. + * * @return resource|null */ - private static function open(string $path, ?array &$stat = null) + private static function open(string $path) { $link = @\lstat($path); // lstat describes the entry itself, so a symlink is refused rather than followed. @@ -110,8 +94,6 @@ private static function open(string $path, ?array &$stat = null) return null; } - $stat = $link; - return $handle; } } diff --git a/src/Executor/Runner/Docker.php b/src/Executor/Runner/Docker.php index a8229da..9ebb5d7 100644 --- a/src/Executor/Runner/Docker.php +++ b/src/Executor/Runner/Docker.php @@ -83,8 +83,8 @@ public function getLogs(string $runtimeId, int $timeout, Response $response): vo throw new ExecutorException(ExecutorException::LOGS_TIMEOUT); } - // The build container writes these files, so only regular files are read (see RegularFile). - if (RegularFile::stat($tmpLogging . '/logs.txt') !== null) { + if (\file_exists($tmpLogging . '/logs.txt') && \file_exists($tmpLogging . '/timings.txt')) { + // The build container writes these files, so they are read through RegularFile. $timings = RegularFile::read($tmpLogging . '/timings.txt'); if ($timings !== '') { break; @@ -161,7 +161,7 @@ public function getLogs(string $runtimeId, int $timeout, Response $response): vo $logsProcess = $process; $logsPath = $tmpLogging . '/logs.txt'; - if (RegularFile::stat($logsPath) === null) { + if (!\file_exists($logsPath)) { if (!empty($logsProcess)) { \proc_terminate($logsProcess, 9); } @@ -455,11 +455,7 @@ public function createRuntime( $snapshot = \dirname($tmpBuild, 2) . '/output-' . \basename($tmpBuild); $size = RegularFile::copy($tmpBuild, $snapshot); if ($size === null) { - if (!\is_link($tmpBuild) && !\file_exists($tmpBuild)) { - throw new \Exception('Something went wrong when starting runtime.'); - } - - throw new ExecutorException(ExecutorException::BUILD_FAILED, 'Build output must be a regular file.'); + throw new \Exception('Something went wrong when starting runtime.'); } $container['size'] = $size; diff --git a/tests/unit/Executor/RegularFileTest.php b/tests/unit/Executor/RegularFileTest.php index e53f8f4..ed0dd3e 100644 --- a/tests/unit/Executor/RegularFileTest.php +++ b/tests/unit/Executor/RegularFileTest.php @@ -41,7 +41,6 @@ public function testReadsARegularFile(): void $this->assertSame('hello world', RegularFile::read($this->dir . '/logs.log')); $this->assertSame('world', RegularFile::read($this->dir . '/logs.log', 6)); $this->assertSame('hello', RegularFile::read($this->dir . '/logs.log', 0, 5)); - $this->assertNotNull(RegularFile::stat($this->dir . '/logs.log')); } public function testDoesNotFollowASymlinkTheContainerPlanted(): void @@ -49,7 +48,6 @@ public function testDoesNotFollowASymlinkTheContainerPlanted(): void \symlink($this->secret, $this->dir . '/logs.log'); $this->assertSame('', RegularFile::read($this->dir . '/logs.log')); - $this->assertNull(RegularFile::stat($this->dir . '/logs.log')); $this->assertStringContainsString('leaked', (string) \file_get_contents($this->secret)); } @@ -62,13 +60,11 @@ public function testRefusesAFifoWithoutBlocking(): void \posix_mkfifo($this->dir . '/logs.log', 0600); $this->assertSame('', RegularFile::read($this->dir . '/logs.log')); - $this->assertNull(RegularFile::stat($this->dir . '/logs.log')); } public function testMissingPathReadsAsEmpty(): void { $this->assertSame('', RegularFile::read($this->dir . '/missing.log')); - $this->assertNull(RegularFile::stat($this->dir . '/missing.log')); } public function testCopiesARegularFile(): void From 903116213b95775453c4e5fe21000f33ebe32ed9 Mon Sep 17 00:00:00 2001 From: loks0n <22452787+loks0n@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:03:13 +0000 Subject: [PATCH 4/5] fix: remove upload snapshots, cover the Docker paths end to end The build-output and cache copies are removed after upload, on failure too, so a runtime kept after its build doesn't hold a second copy of its artifact. E2E: a build that leaves a symlink as its output gets no upload, and a function that swaps its log files for symlinks gets none of the executor's environment. Co-Authored-By: Claude Opus 5.5 --- src/Executor/Runner/Docker.php | 43 ++++++++----- tests/e2e/ExecutorTest.php | 62 +++++++++++++++++++ .../functions/node-log-symlink/index.js | 14 +++++ .../functions/node-log-symlink/package.json | 12 ++++ 4 files changed, 114 insertions(+), 17 deletions(-) create mode 100644 tests/resources/functions/node-log-symlink/index.js create mode 100644 tests/resources/functions/node-log-symlink/package.json diff --git a/src/Executor/Runner/Docker.php b/src/Executor/Runner/Docker.php index 9ebb5d7..a3ecce8 100644 --- a/src/Executor/Runner/Docker.php +++ b/src/Executor/Runner/Docker.php @@ -453,19 +453,24 @@ public function createRuntime( // executor's own files (its environment) as the build, for the owner to download. // The upload reads a copy in /tmp/, which no container mounts. $snapshot = \dirname($tmpBuild, 2) . '/output-' . \basename($tmpBuild); - $size = RegularFile::copy($tmpBuild, $snapshot); - if ($size === null) { - throw new \Exception('Something went wrong when starting runtime.'); - } + try { + $size = RegularFile::copy($tmpBuild, $snapshot); + if ($size === null) { + throw new \Exception('Something went wrong when starting runtime.'); + } - $container['size'] = $size; + $container['size'] = $size; - $destinationDevice = StorageFactory::getDevice($destination, System::getEnv('OPR_EXECUTOR_CONNECTION_STORAGE')); - $path = $destinationDevice->getPath(\uniqid() . '.' . \pathinfo($tmpBuild, PATHINFO_EXTENSION)); + $destinationDevice = StorageFactory::getDevice($destination, System::getEnv('OPR_EXECUTOR_CONNECTION_STORAGE')); + $path = $destinationDevice->getPath(\uniqid() . '.' . \pathinfo($tmpBuild, PATHINFO_EXTENSION)); - if (!$localDevice->copy($snapshot, $path, $destinationDevice)) { - throw new \Exception('Failed to move built code to storage'); - }; + if (!$localDevice->copy($snapshot, $path, $destinationDevice)) { + throw new \Exception('Failed to move built code to storage'); + }; + } finally { + // A runtime kept after its build keeps /tmp/, so the copy is removed here. + @\unlink($snapshot); + } $container['path'] = $path; } @@ -639,15 +644,19 @@ private function storeBuildCacheArtifact(string $cacheKey, string $tmpCache, Loc // /cache is mounted into the build container, so anything but a regular file is skipped, and // the upload reads a copy in /tmp/, which no container mounts. $snapshot = \dirname($tmpCache) . '/cache-stores.sqfs'; - if (RegularFile::copy($artifact, $snapshot) === null) { - return; - } + try { + if (RegularFile::copy($artifact, $snapshot) === null) { + return; + } - $cacheDevice = $this->getBuildCacheDevice(); - $destinationArtifact = $this->getBuildCacheArtifactPath($cacheDevice, $cacheKey); + $cacheDevice = $this->getBuildCacheDevice(); + $destinationArtifact = $this->getBuildCacheArtifactPath($cacheDevice, $cacheKey); - if (!$localDevice->copy($snapshot, $destinationArtifact, $cacheDevice)) { - throw new \Exception('Failed to store build cache artifact'); + if (!$localDevice->copy($snapshot, $destinationArtifact, $cacheDevice)) { + throw new \Exception('Failed to store build cache artifact'); + } + } finally { + @\unlink($snapshot); } } diff --git a/tests/e2e/ExecutorTest.php b/tests/e2e/ExecutorTest.php index 54b205c..588c39a 100644 --- a/tests/e2e/ExecutorTest.php +++ b/tests/e2e/ExecutorTest.php @@ -425,6 +425,68 @@ public function testBuildOutputDirectory(): void $this->assertEquals(200, $response['headers']['status-code']); } + public function testBuildOutputSymlinkIsNotUploaded(): void + { + $output = ''; + $stderr = ''; + Console::execute('cd /app/tests/resources/functions/node && tar --exclude code.tar.gz -czf code.tar.gz .', '', $output, $stderr); + + $runtimeId = \bin2hex(\random_bytes(4)); + + // The build leaves a link to /proc/self/environ as its output. Followed, the executor + // uploads its own environment, secret included, as a build the owner can download. + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-symlink-' . $runtimeId, + 'source' => '/storage/functions/node/code.tar.gz', + 'destination' => '/storage/builds/test', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-22', + 'command' => 'ln -sf /proc/self/environ /mnt/code/code.tar.gz', + 'remove' => true + ]); + + $this->assertNotEquals(201, $response['headers']['status-code']); + $this->assertArrayNotHasKey('path', (array) $response['body']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) \json_encode($response['body'])); + } + + public function testExecutionLogSymlinkIsNotFollowed(): void + { + $output = ''; + $stderr = ''; + Console::execute('cd /app/tests/resources/functions/node-log-symlink && tar --exclude code.tar.gz -czf code.tar.gz .', '', $output, $stderr); + + $runtimeId = \bin2hex(\random_bytes(4)); + + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-log-symlink-' . $runtimeId, + 'source' => '/storage/functions/node-log-symlink/code.tar.gz', + 'destination' => '/storage/builds/test', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-22', + 'command' => 'tar -zxf /tmp/code.tar.gz -C /mnt/code && bash helpers/build.sh "npm i"', + 'remove' => true + ]); + $this->assertEquals(201, $response['headers']['status-code']); + + // The function swaps its log files for links to /proc/self/environ. Followed, they return + // the executor's own environment, secret included, as the execution's logs. + $response = $this->client->call(Client::METHOD_POST, '/runtimes/test-exec-log-symlink-' . $runtimeId . '/executions', [], [ + 'source' => $response['body']['path'], + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-22', + 'runtimeEntrypoint' => 'cp /tmp/code.tar.gz /mnt/code/code.tar.gz && nohup helpers/start.sh "bash helpers/server.sh"', + ]); + + $this->assertEquals(200, $response['headers']['status-code']); + $this->assertEquals('OK', $response['body']['body']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) $response['body']['logs']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) $response['body']['errors']); + + $response = $this->client->call(Client::METHOD_DELETE, '/runtimes/test-exec-log-symlink-' . $runtimeId, [], []); + $this->assertEquals(200, $response['headers']['status-code']); + } + public function testBuildUncompressed(): void { $output = ''; diff --git a/tests/resources/functions/node-log-symlink/index.js b/tests/resources/functions/node-log-symlink/index.js new file mode 100644 index 0000000..2e1957f --- /dev/null +++ b/tests/resources/functions/node-log-symlink/index.js @@ -0,0 +1,14 @@ +const fs = require('fs'); + +// Replaces this execution's log files with symlinks to /proc/self/environ. The runtime keeps +// writing to the files it already opened, but the executor reads them by path afterwards. +module.exports = async (context) => { + context.log('planted'); + + for (const file of fs.readdirSync('/mnt/logs')) { + fs.rmSync(`/mnt/logs/${file}`); + fs.symlinkSync('/proc/self/environ', `/mnt/logs/${file}`); + } + + return context.res.send('OK'); +} diff --git a/tests/resources/functions/node-log-symlink/package.json b/tests/resources/functions/node-log-symlink/package.json new file mode 100644 index 0000000..e019f53 --- /dev/null +++ b/tests/resources/functions/node-log-symlink/package.json @@ -0,0 +1,12 @@ +{ + "name": "log-symlink", + "version": "1.0.0", + "description": "", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "keywords": [], + "author": "", + "license": "ISC" +} From a8a8a2fbb3f7c48e3ba3d7655d5141ddf9b3faa1 Mon Sep 17 00:00:00 2001 From: loks0n <22452787+loks0n@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:09:14 +0000 Subject: [PATCH 5/5] test: cover the build-cache path end to end A build swaps its cache artifact for a link to /proc/self/environ, and the next build with the same key prints what was restored. Against main it prints the executor's environment; on this branch nothing is stored. Co-Authored-By: Claude Opus 5.5 --- tests/e2e/ExecutorTest.php | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/e2e/ExecutorTest.php b/tests/e2e/ExecutorTest.php index 588c39a..4b297c9 100644 --- a/tests/e2e/ExecutorTest.php +++ b/tests/e2e/ExecutorTest.php @@ -888,6 +888,44 @@ public function testBuildCache(): void $this->assertEquals(200, $response['headers']['status-code']); } + public function testBuildCacheSymlinkIsNotStored(): void + { + $output = ''; + $stderr = ''; + Console::execute('cd /app/tests/resources/functions/node && tar --exclude code.tar.gz -czf code.tar.gz .', '', $output, $stderr); + + $runtimeId = \bin2hex(\random_bytes(4)); + $cacheKey = 'test-build-cache-symlink-' . $runtimeId; + + // The first build swaps its cache artifact for a link to /proc/self/environ. Followed, the + // executor stores its own environment, secret included, as the project's build cache. + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-cache-plant-' . $runtimeId, + 'source' => '/storage/functions/node/code.tar.gz', + 'destination' => '/storage/builds/test-cache-plant', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-18.0', + 'command' => 'tar -zxf /tmp/code.tar.gz -C /mnt/code && bash helpers/build.sh "npm install" && ln -sf /proc/self/environ /cache/stores.sqfs', + 'cacheKey' => $cacheKey, + 'remove' => true, + ]); + $this->assertEquals(201, $response['headers']['status-code']); + + // The second build prints whatever the executor restored as its cache. + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-cache-read-' . $runtimeId, + 'source' => '/storage/functions/node/code.tar.gz', + 'destination' => '/storage/builds/test-cache-read', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-18.0', + 'command' => 'tar -zxf /tmp/code.tar.gz -C /mnt/code && (cat /cache/stores.sqfs || true) && bash helpers/build.sh "npm install"', + 'cacheKey' => $cacheKey, + 'remove' => true, + ]); + $this->assertEquals(201, $response['headers']['status-code']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) \json_encode($response['body'])); + } + // We also test SSR two Set-cookie here public function testSSRLogs(): void {