Skip to content

It is possible to upload dangerous files from Web Entry #8573

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

Merged
nolanpro merged 1 commit into from
Oct 30, 2025
Merged

It is possible to upload dangerous files from Web Entry #8573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions ProcessMaker/Http/Controllers/Api/FileController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@
use ProcessMaker\Models\MediaLog;
use ProcessMaker\Models\ProcessRequest;
use ProcessMaker\Models\TaskDraft;
use ProcessMaker\Traits\ValidatesFileTrait;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

class FileController extends Controller
{
use ValidatesFileTrait;

/**
* A whitelist of attributes that should not be
* sanitized by our SanitizeInput middleware.
Expand Down Expand Up @@ -188,7 +191,21 @@ public function store(Request $request)
}

$mediaCollection = $request->input('collection', 'local');

// Validate the file before processing
$uploadedFile = $request->file('file');
if (!$uploadedFile) {
return abort(response(['message' => 'No file provided'], 422));
}

$errors = [];
$this->validateFile($uploadedFile, $errors);
if (count($errors) > 0) {
return abort(response($errors, 422));
}

$file = $model->addMediaFromRequest('file');

$user = pmUser();
$originalCreatedBy = $user ? $user->id : null;
$data_name = $request->input('data_name', '');
Expand Down
130 changes: 3 additions & 127 deletions ProcessMaker/Http/Controllers/Api/ProcessRequestFileController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,30 @@
namespace ProcessMaker\Http\Controllers\Api;

use Exception;
use Illuminate\Contracts\Routing\ResponseFactory;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
use Illuminate\Http\Resources\Json\ResourceCollection;
use Illuminate\Http\Response;
use Illuminate\Http\UploadedFile;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Storage;
use Pion\Laravel\ChunkUpload\Exceptions\UploadMissingFileException;
use Pion\Laravel\ChunkUpload\Handler\AbstractHandler;
use Pion\Laravel\ChunkUpload\Handler\HandlerFactory;
use Pion\Laravel\ChunkUpload\Receiver\FileReceiver;
use ProcessMaker\Events\FilesAccessed;
use ProcessMaker\Events\FilesCreated;
use ProcessMaker\Events\FilesDeleted;
use ProcessMaker\Events\FilesDownloaded;
use ProcessMaker\Http\Controllers\Controller;
use ProcessMaker\Http\Resources\ApiCollection;
use ProcessMaker\Http\Resources\ApiResource;
use ProcessMaker\Models\Media;
use ProcessMaker\Models\ProcessRequest;
use ProcessMaker\Models\TaskDraft;
use ProcessMaker\Traits\ValidatesFileTrait;
use Spatie\MediaLibrary\MediaCollections\Exceptions\FileIsTooBig;

class ProcessRequestFileController extends Controller
{
use ValidatesFileTrait;

/**
* A whitelist of attributes that should not be
* sanitized by our SanitizeInput middleware.
Expand Down Expand Up @@ -439,126 +437,4 @@ public function destroy(Request $laravel_request, ProcessRequest $request, $file

return response([], 204);
}

/**
* Validate uploaded file for security and type restrictions
*
* @param UploadedFile $file
* @param array $errors
* @return array
*/
private function validateFile(UploadedFile $file, &$errors)
{
// Explicitly reject archive files for security
if (config('files.enable_dangerous_validation')) {
$this->rejectArchiveFiles($file, $errors);
}

// Validate file extension if enabled
if (config('files.enable_extension_validation')) {
$this->validateFileExtension($file, $errors);
}

// Validate MIME type vs extension if enabled
if (config('files.enable_mime_validation')) {
$this->validateExtensionMimeTypeMatch($file, $errors);
}

// Validate specific file types (e.g., PDF for JavaScript content)
if (strtolower($file->getClientOriginalExtension()) === 'pdf') {
$this->validatePDFFile($file, $errors);
}

return $errors;
}

/**
* Explicitly reject archive files for security reasons
*
* @param UploadedFile $file
* @param array $errors
* @return void
*/
private function rejectArchiveFiles(UploadedFile $file, &$errors)
{
$dangerousExtensions = config('files.dangerous_extensions');

$fileExtension = strtolower($file->getClientOriginalExtension());

if (in_array($fileExtension, $dangerousExtensions)) {
$errors['message'] = __('Uploaded file type is not allowed');

return;
}

// Also check MIME types for archive files
$dangerousMimeTypes = config('files.dangerous_mime_types');

$fileMimeType = $file->getMimeType();

if (in_array($fileMimeType, $dangerousMimeTypes)) {
$errors['message'] = __('Uploaded mime file type is not allowed');
}
}

/**
* Validate that file extension matches the MIME type
*
* @param UploadedFile $file
* @param array $errors
* @return void
*/
private function validateExtensionMimeTypeMatch(UploadedFile $file, &$errors)
{
$fileExtension = strtolower($file->getClientOriginalExtension());
$fileMimeType = $file->getMimeType();

// Get extension to MIME type mapping from configuration
$extensionMimeMap = config('files.extension_mime_map');

// Check if extension exists in our map
if (!isset($extensionMimeMap[$fileExtension])) {
$errors['message'] = __('File extension not allowed');

return;
}

// Check if MIME type matches any of the expected types for this extension
if (!in_array($fileMimeType, $extensionMimeMap[$fileExtension])) {
$errors['message'] = __('The file extension does not match the actual file content');
}
}

/**
* Validate file extension against allowed extensions
*
* @param UploadedFile $file
* @param array $errors
* @return void
*/
private function validateFileExtension(UploadedFile $file, &$errors)
{
$allowedExtensions = config('files.allowed_extensions');
$fileExtension = strtolower($file->getClientOriginalExtension());

if (!in_array($fileExtension, $allowedExtensions)) {
$errors['message'] = __('File extension not allowed');
}
}

private function validatePDFFile(UploadedFile $file, &$errors)
{
$text = $file->get();

$jsKeywords = ['/JavaScript', '<< /S /JavaScript'];

foreach ($jsKeywords as $keyword) {
if (strpos($text, $keyword) !== false) {
$errors[] = __('Dangerous PDF file content');
break;
}
}

return $errors;
}
}
136 changes: 136 additions & 0 deletions ProcessMaker/Traits/ValidatesFileTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
<?php

namespace ProcessMaker\Traits;

use Illuminate\Http\UploadedFile;

trait ValidatesFileTrait
{
/**
* Validate uploaded file for security and type restrictions
*
* @param UploadedFile $file
* @param array $errors
* @return array
*/
private function validateFile(UploadedFile $file, &$errors)
{
// Explicitly reject archive files for security
if (config('files.enable_dangerous_validation')) {
Copy link

@cursor cursor bot Oct 23, 2025

Choose a reason for hiding this comment

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

Bug: Default Validation Setting Insecure

The enable_dangerous_validation check in validateFile lacks a default true value, unlike other validation configurations. This results in dangerous file validation, like rejecting archive files, being disabled by default, which creates a security vulnerability.

Fix in Cursor Fix in Web

$this->rejectArchiveFiles($file, $errors);
}

// Validate file extension if enabled
if (config('files.enable_extension_validation', true)) {
$this->validateFileExtension($file, $errors);
}

// Validate MIME type vs extension if enabled
if (config('files.enable_mime_validation', true)) {
$this->validateExtensionMimeTypeMatch($file, $errors);
}
Copy link

@cursor cursor bot Oct 23, 2025

Choose a reason for hiding this comment

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

Bug: File Validation Defaults Cause Security Concerns

The file validation trait has inconsistent default config values. enable_dangerous_validation is disabled by default when not configured, but enable_extension_validation and enable_mime_validation default to true. This changes the previous opt-in behavior, potentially enabling validations unexpectedly and creating a security risk by silently skipping critical dangerous file checks.

Fix in Cursor Fix in Web


// Validate specific file types (e.g., PDF for JavaScript content)
if (strtolower($file->getClientOriginalExtension()) === 'pdf') {
$this->validatePDFFile($file, $errors);
}

return $errors;
}

/**
* Explicitly reject archive files for security reasons
*
* @param UploadedFile $file
* @param array $errors
* @return void
*/
private function rejectArchiveFiles(UploadedFile $file, &$errors)
{
$dangerousExtensions = config('files.dangerous_extensions');

$fileExtension = strtolower($file->getClientOriginalExtension());

if (in_array($fileExtension, $dangerousExtensions)) {
$errors['message'] = __('Uploaded file type is not allowed');

return;
}

// Also check MIME types for archive files
$dangerousMimeTypes = config('files.dangerous_mime_types');

$fileMimeType = $file->getMimeType();

if (in_array($fileMimeType, $dangerousMimeTypes)) {
$errors['message'] = __('Uploaded mime file type is not allowed');
}
}

/**
* Validate that file extension matches the MIME type
*
* @param UploadedFile $file
* @param array $errors
* @return void
*/
private function validateExtensionMimeTypeMatch(UploadedFile $file, &$errors)
{
$fileExtension = strtolower($file->getClientOriginalExtension());
$fileMimeType = $file->getMimeType();

// Get extension to MIME type mapping from configuration
$extensionMimeMap = config('files.extension_mime_map');

// Check if extension exists in our map
if (!isset($extensionMimeMap[$fileExtension])) {
$errors['message'] = __('File extension not allowed');

return;
}

// Check if MIME type matches any of the expected types for this extension
if (!in_array($fileMimeType, $extensionMimeMap[$fileExtension])) {
$errors['message'] = __('The file extension does not match the actual file content');
}
}

/**
* Validate file extension against allowed extensions
*
* @param UploadedFile $file
* @param array $errors
* @return void
*/
private function validateFileExtension(UploadedFile $file, &$errors)
{
$allowedExtensions = config('files.allowed_extensions');

$fileExtension = strtolower($file->getClientOriginalExtension());

if (!in_array($fileExtension, $allowedExtensions)) {
$errors['message'] = __('File extension not allowed');
}
}

/**
* Validate PDF files for dangerous content
*
* @param UploadedFile $file
* @param array $errors
* @return void
*/
private function validatePDFFile(UploadedFile $file, &$errors)
{
$text = $file->get();

$jsKeywords = ['/JavaScript', '<< /S /JavaScript'];

foreach ($jsKeywords as $keyword) {
if (strpos($text, $keyword) !== false) {
$errors[] = __('Dangerous PDF file content');
break;
}
}
}
Copy link

@cursor cursor bot Oct 23, 2025

Choose a reason for hiding this comment

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

Bug: Inconsistent Error Handling in Validation Methods

The validatePDFFile method adds errors using an indexed array append ($errors[]), which differs from other validation methods in this trait that use an associative key ($errors['message']). This inconsistency can lead to unexpected error array structures, potentially breaking downstream error handling that expects a consistent 'message' key.

Fix in Cursor Fix in Web

}
Loading