Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
Latest
------

## Fixes
* [#240](https://github.com/cleverage/ui-process-bundle/issues/240) `ProcessExecutionCrudController::downloadLogFile()` streams the log file (`BinaryFileResponse`) instead of loading it in memory: a log file bigger than the memory limit can be downloaded. The extension of a file uploaded with the launch form is reduced to its letters and digits (at most 16), and the client file name of a file uploaded with the HTTP API is truncated to keep the written file name under 255 bytes: a long name could not be written (500 for the launch form, 422 for the HTTP API as if the process code was missing). `ProcessEventSubscriber` detaches the ended process execution from the entity manager (new `ProcessExecutionManager::detachLastProcessExecution()` and `ProcessExecutionRepository::detach()`): without a reset of the services (processes executed in a loop, tests, FrankenPHP worker mode), the entity manager kept every ended execution. Update documentation, add tests.

v3.4.1
------

Expand Down
2 changes: 1 addition & 1 deletion docs/reference/02-process_ui_options.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ The launch form (`/process/launch?process=<process_code>`) contains:

When `entrypoint_type` is `file`, the uploaded file is saved to the `cleverage_ui_process.upload_directory` directory (see
[bundle configuration](01-bundle_configuration.md#container-parameters)) with a random name keeping the original
extension, and its absolute path is passed as process input. The file is deleted once the process has been executed,
extension (its letters and digits only, at most 16), and its absolute path is passed as process input. The file is deleted once the process has been executed,
unless `cleverage_ui_process.delete_uploaded_files` is `false`. When `entrypoint_type` is `text`, a file path given as
input is never deleted.

Expand Down
2 changes: 1 addition & 1 deletion docs/reference/04-process_executions_and_logs.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ Each execution writes its own log file:
%kernel.logs_dir%/<process_code>/<uuid>.log
```

It can be downloaded from the executions list.
It can be downloaded from the executions list (streamed: its size is not limited by the PHP memory limit).

### Database logs

Expand Down
2 changes: 1 addition & 1 deletion docs/reference/06-http_api.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Parameters can be sent either as a JSON body, or as form data (`application/x-ww
| Parameter | Type | Required | Default | Description |
|-----------|---------------------------|:--------:|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `code` | `string` | **X** | | Code of the process. It must exist and be public. |
| `input` | `string` or file | | `null` | Process input. With form data, it can be an uploaded file: the file is saved in the `cleverage_ui_process.upload_directory` directory (see [bundle configuration](01-bundle_configuration.md#container-parameters)) and its path is passed as input; the file is deleted once the process has been executed, or when the request is rejected (422), unless `cleverage_ui_process.delete_uploaded_files` is `false`. A file given by its path is never deleted. |
| `input` | `string` or file | | `null` | Process input. With form data, it can be an uploaded file: the file is saved in the `cleverage_ui_process.upload_directory` directory (see [bundle configuration](01-bundle_configuration.md#container-parameters)) and its path is passed as input (its name is the date, a unique id and the original file name, truncated to keep it under 255 bytes); the file is deleted once the process has been executed, or when the request is rejected (422), unless `cleverage_ui_process.delete_uploaded_files` is `false`. A file given by its path is never deleted. |
| `context` | `object` or JSON `string` | | `{}` | Process context, as key/value pairs. A JSON string must decode to an object or an array (`422` otherwise). With form data, send one field per value: `context[key]=value`. |
| `queue` | `bool` | | `true` | `true`: the process is queued to the `execute_process` transport (see [messenger](08-messenger.md)). `false`: the process is executed during the HTTP request. |

Expand Down
5 changes: 4 additions & 1 deletion src/Controller/Admin/Process/LaunchAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use CleverAge\UiProcessBundle\Entity\User;
use CleverAge\UiProcessBundle\Form\Type\LaunchType;
use CleverAge\UiProcessBundle\Manager\ProcessConfigurationsManager;
use CleverAge\UiProcessBundle\Manager\UploadedFileManager;
use CleverAge\UiProcessBundle\Message\ProcessExecuteMessage;
use EasyCorp\Bundle\EasyAdminBundle\Config\Asset;
use EasyCorp\Bundle\EasyAdminBundle\Contracts\Context\AdminContextInterface;
Expand Down Expand Up @@ -116,7 +117,9 @@ public function __invoke(
// Only the file written here is deleted once executed, never a file given by its path (text input)
$deleteInput = false;
if ($input instanceof UploadedFile) {
$filename = \sprintf('%s/%s.%s', $uploadDirectory, Uuid::v4(), $input->getClientOriginalExtension());
// The extension sent by the browser is reduced: a long one made the writing fail (500)
$extension = UploadedFileManager::getSafeExtension($input);
$filename = \sprintf('%s/%s%s', $uploadDirectory, Uuid::v4(), '' === $extension ? '' : '.'.$extension);
(new Filesystem())->dumpFile($filename, $input->getContent());
$input = $filename;
$deleteInput = true;
Expand Down
13 changes: 6 additions & 7 deletions src/Controller/Admin/ProcessExecutionCrudController.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@
use EasyCorp\Bundle\EasyAdminBundle\Field\TextField;
use EasyCorp\Bundle\EasyAdminBundle\Filter\DateTimeFilter;
use EasyCorp\Bundle\EasyAdminBundle\Router\AdminUrlGenerator;
use Symfony\Component\HttpFoundation\BinaryFileResponse;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\ResponseHeaderBag;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Symfony\Component\Security\Http\Attribute\IsGranted;
use Symfony\Contracts\Translation\TranslatorInterface;
Expand Down Expand Up @@ -240,15 +242,12 @@ public function downloadLogFile(): Response
/** @var ProcessExecution $processExecution */
$processExecution = $this->getContext()?->getEntity()->getInstance();
$filepath = $this->getLogFilePath($processExecution);
$basename = basename($filepath);
// file_get_contents() emits a warning (converted to an exception) before returning false on a missing file
$content = is_file($filepath) ? file_get_contents($filepath) : false;
if (false === $content) {
if (!is_file($filepath) || !is_readable($filepath)) {
throw new NotFoundHttpException('Log file not found.');
}
$response = new Response($content);
$response->headers->set('Content-Type', 'text/plain; charset=utf-8');
$response->headers->set('Content-Disposition', "attachment; filename=\"$basename\"");
// Streamed: the log file of a long process can be bigger than the memory limit
$response = new BinaryFileResponse($filepath, headers: ['Content-Type' => 'text/plain; charset=utf-8']);
$response->setContentDisposition(ResponseHeaderBag::DISPOSITION_ATTACHMENT, basename($filepath));

return $response;
}
Expand Down
8 changes: 7 additions & 1 deletion src/EventSubscriber/ProcessEventSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,13 @@ private function endProcessExecution(ProcessEvent $event): void
$this->processExecutionManager->unsetProcessExecution($event->getProcessCode());
$this->processHandler->close();
}
$this->dispatchEnded($event);
try {
$this->dispatchEnded($event);
} finally {
// No longer changed: the entity manager of a process running in a loop (without reset of the services)
// would keep every ended process execution
$this->processExecutionManager->detachLastProcessExecution();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace CleverAge\UiProcessBundle\Http\ValueResolver;

use CleverAge\UiProcessBundle\Http\Model\HttpProcessExecution;
use CleverAge\UiProcessBundle\Manager\UploadedFileManager;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\HttpFoundation\File\UploadedFile;
use Symfony\Component\HttpFoundation\Request;
Expand Down Expand Up @@ -65,7 +66,12 @@ public function resolve(Request $request, ArgumentMetadata $argument): iterable
$input ??= $request->files->get('input');

if ($input instanceof UploadedFile) {
$uploadFileName = $this->storageDir.\DIRECTORY_SEPARATOR.date('YmdHis').'_'.uniqid().'_'.$input->getClientOriginalName();
$prefix = date('YmdHis').'_'.uniqid().'_';
// A client file name too long for the file system made the writing fail
$uploadFileName = $this->storageDir.\DIRECTORY_SEPARATOR.$prefix.UploadedFileManager::getSafeName(
$input,
UploadedFileManager::MAX_FILENAME_LENGTH - \strlen($prefix)
);
(new Filesystem())->dumpFile($uploadFileName, $input->getContent());
$request->attributes->set(self::UPLOADED_INPUT_ATTRIBUTE, $uploadFileName);
$input = $uploadFileName;
Expand Down
10 changes: 10 additions & 0 deletions src/Manager/ProcessExecutionManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ public function unsetProcessExecution(string $processCode): self
return $this;
}

/**
* Detach the last process execution, which has ended, from the entity manager.
*/
public function detachLastProcessExecution(): void
{
if ($this->lastProcessExecution instanceof ProcessExecution) {
$this->processExecutionRepository->detach($this->lastProcessExecution);
}
}

public function save(): self
{
if (($processExecution = $this->getManagedProcessExecution()) instanceof ProcessExecution) {
Expand Down
39 changes: 39 additions & 0 deletions src/Manager/UploadedFileManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace CleverAge\UiProcessBundle\Manager;

use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\HttpFoundation\File\UploadedFile;

/**
* Deletes the files sent as input of a process (launch form, HTTP API) once the process has been executed, so that the
Expand All @@ -23,6 +24,16 @@
*/
final readonly class UploadedFileManager
{
/**
* Maximum length of a file name, in bytes, on most file systems (e.g. ext4, XFS).
*/
public const MAX_FILENAME_LENGTH = 255;

/**
* Maximum length of the extension kept from the client file name.
*/
private const MAX_EXTENSION_LENGTH = 16;

public function __construct(
private string $uploadDirectory,
private bool $deleteUploadedFiles = true,
Expand All @@ -49,4 +60,32 @@ public function deleteInput(mixed $input): void

(new Filesystem())->remove($input);
}

/**
* The extension of the client file name, reduced to its letters and digits and to 16 characters (empty when none
* is left): the client file name is chosen by the sender, neither its length nor its characters are checked.
*/
public static function getSafeExtension(UploadedFile $file): string
{
$extension = preg_replace('/[^a-z0-9]/i', '', $file->getClientOriginalExtension()) ?? '';

return substr($extension, 0, self::MAX_EXTENSION_LENGTH);
}

/**
* The client file name, unchanged when it fits in $maxLength bytes; otherwise its name truncated (on a character
* boundary) and its extension reduced by getSafeExtension(), so that the written file name stays under the limit
* of the file system.
*/
public static function getSafeName(UploadedFile $file, int $maxLength): string
{
$name = $file->getClientOriginalName();
if (\strlen($name) <= $maxLength) {
return $name;
}
$extension = self::getSafeExtension($file);
$suffix = '' === $extension ? '' : '.'.$extension;

return mb_strcut(pathinfo($name, \PATHINFO_FILENAME), 0, max(0, $maxLength - \strlen($suffix)), 'UTF-8').$suffix;
}
}
10 changes: 10 additions & 0 deletions src/Repository/ProcessExecutionRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,16 @@ public function save(ProcessExecution $processExecution): void
$this->getEntityManager()->flush();
}

/**
* Detach the process execution from the entity manager if it manages it: its pending changes are not written.
*/
public function detach(ProcessExecution $processExecution): void
{
if ($this->getEntityManager()->isOpen() && $this->getEntityManager()->contains($processExecution)) {
$this->getEntityManager()->detach($processExecution);
}
}

/**
* The process execution managed by the entity manager: the given one, or the persisted one, updated with the state
* of the given one, when it has been detached (e.g. the entity manager, shared with the process tasks, has been
Expand Down
21 changes: 19 additions & 2 deletions tests/Functional/HttpProcessExecuteTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,23 @@ public function testUploadedFileQueued(): void
self::assertFileExists($messages[0]->input);
}

/**
* A client file name too long for the file system: the file was not written, the request answered 422 as if the
* process code was missing.
*/
public function testUploadedFileWithALongName(): void
{
$this->upload(['code' => 'test.process'], str_repeat('é', 200).'.csv');

self::assertResponseIsSuccessful();
$messages = $this->getDispatchedMessages();
self::assertCount(1, $messages);
self::assertIsString($messages[0]->input);
self::assertStringEndsWith('é.csv', $messages[0]->input);
self::assertLessThanOrEqual(255, \strlen(basename($messages[0]->input)));
self::assertSame('line1', file_get_contents($messages[0]->input));
}

public function testUploadedFileSynchronous(): void
{
$this->upload(['code' => 'test.process', 'queue' => '0']);
Expand Down Expand Up @@ -358,7 +375,7 @@ public static function provideQueue(): iterable
/**
* @param array<string, mixed> $parameters
*/
private function upload(array $parameters): void
private function upload(array $parameters, string $clientName = 'data.csv'): void
{
(new Filesystem())->remove($this->getUploadDirectory());
$file = tempnam(sys_get_temp_dir(), 'upload');
Expand All @@ -368,7 +385,7 @@ private function upload(array $parameters): void
'POST',
'/http/process/execute',
$parameters,
['input' => new UploadedFile((string) $file, 'data.csv', 'text/csv', null, true)],
['input' => new UploadedFile((string) $file, $clientName, 'text/csv', null, true)],
['HTTP_AUTHORIZATION' => 'Bearer '.self::TOKEN]
);
unlink((string) $file);
Expand Down
43 changes: 43 additions & 0 deletions tests/Functional/LaunchActionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use CleverAge\UiProcessBundle\Form\Type\ProcessContextType;
use CleverAge\UiProcessBundle\Manager\ProcessConfigurationsManager;
use CleverAge\UiProcessBundle\Manager\ProcessExecutionManager;
use CleverAge\UiProcessBundle\Manager\UploadedFileManager;
use CleverAge\UiProcessBundle\Message\ProcessExecuteMessage;
use CleverAge\UiProcessBundle\Monolog\Handler\DoctrineProcessHandler;
use CleverAge\UiProcessBundle\Monolog\Handler\ProcessHandler;
Expand All @@ -46,6 +47,7 @@
use CleverAge\UiProcessBundle\Twig\Runtime\ProcessExecutionExtensionRuntime;
use CleverAge\UiProcessBundle\Twig\Runtime\ProcessExtensionRuntime;
use PHPUnit\Framework\Attributes\CoversClass;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\UsesClass;
use Symfony\Component\HttpFoundation\File\UploadedFile;

Expand All @@ -60,6 +62,7 @@
#[UsesClass(ProcessExecutionRepository::class)]
#[UsesClass(HttpProcessExecutionAuthenticator::class)]
#[UsesClass(LogLevelExtension::class)]
#[UsesClass(UploadedFileManager::class)]
#[UsesClass(MD5Extension::class)]
#[UsesClass(ProcessExecutionExtension::class)]
#[UsesClass(ProcessExtension::class)]
Expand Down Expand Up @@ -229,6 +232,46 @@ public function testLaunchWithFileUpload(): void
unlink((string) $file);
}

/**
* The extension of the client file name is reduced to its letters and digits: a long one made the writing fail (500).
*/
#[DataProvider('provideClientNames')]
public function testLaunchWithFileUploadKeepsASafeExtension(string $clientName, string $expectedSuffix): void
{
$this->login();
$file = tempnam(sys_get_temp_dir(), 'upload');
file_put_contents((string) $file, 'line1');
$crawler = $this->client->request('GET', '/process?routeName=cleverage_ui_process_launch&process=test.upload');
$form = $crawler->selectButton('Launch')->form();

$this->client->request(
$form->getMethod(),
$form->getUri(),
$form->getPhpValues(),
['launch' => ['input' => new UploadedFile((string) $file, $clientName, 'text/csv', null, true)]]
);
unlink((string) $file);

self::assertResponseRedirects();
$messages = $this->getDispatchedMessages();
self::assertCount(1, $messages);
self::assertIsString($messages[0]->input);
self::assertMatchesRegularExpression('#/[0-9a-f-]{36}'.preg_quote($expectedSuffix, '#').'$#', $messages[0]->input);
self::assertSame('line1', file_get_contents($messages[0]->input));
}

/**
* @return iterable<string, array{string, string}>
*/
public static function provideClientNames(): iterable
{
yield 'extension' => ['data.CSV', '.CSV'];
yield 'long extension' => ['data.'.str_repeat('x', 300), '.xxxxxxxxxxxxxxxx'];
yield 'special characters' => ['data.c"s v', '.csv'];
yield 'no extension' => ['data', ''];
yield 'no letter or digit' => ['data.é', ''];
}

/**
* A context row without key, on a process with constraints on the context: the empty key broke the property paths
* of the constraints (500 "Could not parse property path").
Expand Down
5 changes: 4 additions & 1 deletion tests/Functional/ProcessExecutionCrudTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
use PHPUnit\Framework\Attributes\CoversClass;
use PHPUnit\Framework\Attributes\UsesClass;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\HttpFoundation\BinaryFileResponse;

#[CoversClass(ProcessExecutionCrudController::class)]
#[UsesClass(ContextField::class)]
Expand Down Expand Up @@ -198,9 +199,11 @@ public function testDownloadLogFile(): void
$this->client->request('GET', '/process/process-execution/download-logs?entityId='.$execution->getId());

self::assertResponseIsSuccessful();
// Streamed: not loaded in memory
self::assertInstanceOf(BinaryFileResponse::class, $this->client->getResponse());
self::assertSame('log file content', $this->client->getInternalResponse()->getContent());
self::assertResponseHeaderSame('Content-Type', 'text/plain; charset=utf-8');
self::assertResponseHeaderSame('Content-Disposition', 'attachment; filename="'.$execution->logFilename.'"');
self::assertResponseHeaderSame('Content-Disposition', 'attachment; filename='.$execution->logFilename);
}

public function testDownloadMissingLogFile(): void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

use CleverAge\UiProcessBundle\Http\Model\HttpProcessExecution;
use CleverAge\UiProcessBundle\Http\ValueResolver\HttpProcessExecuteValueResolver;
use CleverAge\UiProcessBundle\Manager\UploadedFileManager;
use PHPUnit\Framework\Attributes\CoversClass;
use PHPUnit\Framework\Attributes\UsesClass;
use PHPUnit\Framework\TestCase;
Expand All @@ -28,6 +29,7 @@

#[CoversClass(HttpProcessExecuteValueResolver::class)]
#[UsesClass(HttpProcessExecution::class)]
#[UsesClass(UploadedFileManager::class)]
class HttpProcessExecuteValueResolverTest extends TestCase
{
private string $storageDir;
Expand Down
Loading
Loading