Skip to content

magento/magento2#39720: Can't upload image for custom customer addres… #40426

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

Open
pavel77718 wants to merge 7 commits into
base: 2.4-develop
Choose a base branch
from
Open

magento/magento2#39720: Can't upload image for custom customer addres… #40426

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4320612
magento/magento2#39720: Can't upload image for custom customer addres…
pavel77718 Jan 9, 2026
35cd5ee
Merge branch '2.4-develop' into fix-for-issue-39720
pavel77718 Jan 9, 2026
4bceb81
magento/magento2#39720: Can't upload image for custom customer addres…
pavel77718 Jan 9, 2026
5bb007a
Merge remote-tracking branch 'origin/fix-for-issue-39720' into fix-fo…
pavel77718 Jan 9, 2026
9da1e39
magento/magento2#39720: Can't upload image for custom customer addres…
pavel77718 Jan 9, 2026
d4d3667
magento/magento2#39720: Can't upload image for custom customer addres…
pavel77718 Jan 10, 2026
500ceaa
magento/magento2#39720: Can't upload image for custom customer addres…
pavel77718 Jan 12, 2026
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
26 changes: 22 additions & 4 deletions app/code/Magento/Customer/Model/Metadata/Form/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,15 @@ public function extractValue(\Magento\Framework\App\RequestInterface $request)
*/
protected function _validateByRules($value)
{
$label = $value['name'];
$label = $value['name'] ?? $value['file'] ?? '';
$rules = $this->getAttribute()->getValidationRules();

// For UI component uploads, get name from file path if not provided
if (empty($value['name']) && !empty($value['file'])) {
$value['name'] = basename($value['file']);
$label = $value['name'];
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The new UI component upload functionality (handling of value['file']) lacks test coverage. While the repository has comprehensive unit tests for the File and Image classes, there are no tests added to verify that the new code paths for UI component uploads work correctly. Consider adding unit tests that cover scenarios where value['file'] is provided instead of value['tmp_name'], including edge cases like empty strings, path traversal attempts, and proper file size calculation.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

Using basename on user-provided file paths could be a security concern if the value['file'] contains path traversal sequences. While basename does provide some protection, it's better to ensure proper validation of the file path earlier in the process. Consider adding explicit validation to ensure value['file'] doesn't contain unexpected characters or path traversal attempts before using it.

Copilot uses AI. Check for mistakes.
}

$extension = $this->ioFile->getPathInfo($value['name'])['extension'];
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

When value['name'] is empty, the code gets the extension from value['name'] at line 206 without first checking if the assignment on line 202 was successful. If both value['name'] and value['file'] are empty, this could result in trying to get path info on an empty string, potentially causing an undefined index error. Consider adding validation to ensure value['name'] is not empty before trying to get the extension.

Suggested change
$extension = $this->ioFile->getPathInfo($value['name'])['extension'];
// Ensure we have a valid file name before attempting to get extension
if (empty($value['name'])) {
return [__('"%1" is not a valid file.', $label)];
}
$pathInfo = $this->ioFile->getPathInfo($value['name']);
$extension = $pathInfo['extension'] ?? '';

Copilot uses AI. Check for mistakes.
$fileExtensions = ArrayObjectSearch::getArrayElementByName(
$rules,
Expand All @@ -216,7 +223,9 @@ protected function _validateByRules($value)
return $this->_fileValidator->getMessages();
}

if (!$this->_isUploadedFile($value['tmp_name'])) {
// Determine file path for validation
$filePath = $value['tmp_name'] ?? $value['file'] ?? '';
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The fallback to an empty string for filePath could bypass validation. If both tmp_name and file are empty/null, an empty string will be passed to _isUploadedFile, which may not properly handle this case. Consider adding explicit validation to ensure a valid file path exists before proceeding with file validation, or return an error if no valid file path is found.

Suggested change
$filePath = $value['tmp_name'] ?? $value['file'] ?? '';
$filePath = $value['tmp_name'] ?? $value['file'] ?? null;
if (empty($filePath)) {
return [__('"%1" is not a valid file.', $label)];
}

Copilot uses AI. Check for mistakes.
if (!$this->_isUploadedFile($filePath)) {
return [__('"%1" is not a valid file.', $label)];
}

Expand All @@ -225,7 +234,15 @@ protected function _validateByRules($value)
'max_file_size'
);
if ($maxFileSize !== null) {
$size = $value['size'];
$size = $value['size'] ?? 0;
// For UI component uploads, get file size if not provided
if ($size === 0 && !empty($value['file'])) {
$temporaryFile = FileProcessor::TMP_DIR . '/' . basename($value['file']);
if ($this->fileProcessor->isExist($temporaryFile)) {
$stat = $this->fileProcessor->getStat($temporaryFile);
$size = $stat['size'] ?? 0;
}
}
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

There's an inconsistency in how file size is retrieved between File.php and Image.php. In File.php (line 240), the code constructs a temporary file path as "FileProcessor::TMP_DIR . '/' . basename(value['file'])", while in Image.php (line 154), it uses "mediaEntityTmpReadDirectory->getAbsolutePath(ltrim(value['file'], '/'))" to construct the full path. This inconsistency could lead to different behaviors and makes the codebase harder to maintain. Consider using a consistent approach for constructing temporary file paths across both classes, preferably by extracting this logic into a shared helper method.

Copilot uses AI. Check for mistakes.
if ($maxFileSize < $size) {
return [__('"%1" exceeds the allowed file size.', $label)];
}
Expand Down Expand Up @@ -274,7 +291,8 @@ public function validateValue($value)
$label = $attribute->getStoreLabel();

$toDelete = !empty($value['delete']) ? true : false;
$toUpload = !empty($value['tmp_name']) ? true : false;
// Check both tmp_name (traditional upload) and file (UI component upload)
$toUpload = !empty($value['tmp_name']) || (!empty($value['file']) && $value['file'] !== $this->_value);

if (!$toUpload && !$toDelete && $this->_value) {
return true;
Expand Down
27 changes: 23 additions & 4 deletions app/code/Magento/Customer/Model/Metadata/Form/Image.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,17 +136,32 @@ public function __construct(
*/
protected function _validateByRules($value)
{
$label = $value['name'];
$label = $value['name'] ?? $value['file'] ?? '';
$rules = $this->getAttribute()->getValidationRules();

// For UI component uploads, get name from file path if not provided
if (empty($value['name']) && !empty($value['file'])) {
$value['name'] = basename($value['file']);
$label = $value['name'];
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

Similar to File.php, using basename on user-provided file paths could be a security concern if value['file'] contains path traversal sequences. While basename provides some protection, consider adding explicit validation to ensure value['file'] doesn't contain unexpected characters or path traversal attempts before using it.

Copilot uses AI. Check for mistakes.
}

// Determine file path for validation
$filePath = $value['tmp_name'] ?? null;

// For UI component uploads, construct the full temporary file path
if (empty($filePath) && !empty($value['file'])) {
$tmpFileName = ltrim($value['file'], '/');
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

Using ltrim to remove leading slashes from a file path could be exploited with path traversal sequences. While the path will eventually be validated by getAbsolutePath and _isUploadedFile, it's better to validate the input earlier. Consider using a more robust path sanitization method or validating that the file path doesn't contain suspicious patterns before processing.

Suggested change
$tmpFileName = ltrim($value['file'], '/');
$tmpFileName = ltrim($value['file'], '/');
// Basic path validation to prevent traversal and suspicious paths
$hasNullByte = strpos($tmpFileName, "\0") !== false;
$hasTraversal = (bool)preg_match('#(^|/)\.\.(?:/|$)#', $tmpFileName);
$isWindowsAbsolute = (bool)preg_match('#^[a-zA-Z]:[\\\\/]#', $tmpFileName);
$startsWithBackslash = isset($tmpFileName[0]) && $tmpFileName[0] === '\\';
if ($hasNullByte || $hasTraversal || $isWindowsAbsolute || $startsWithBackslash) {
return [__('"%1" is not a valid file.', $label)];
}

Copilot uses AI. Check for mistakes.
$filePath = $this->mediaEntityTmpReadDirectory->getAbsolutePath($tmpFileName);
}
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The new UI component upload functionality (handling of value['file']) lacks test coverage. While the repository has comprehensive unit tests for the Image class, there are no tests added to verify that the new code paths for UI component uploads work correctly. Consider adding unit tests that cover scenarios where value['file'] is provided instead of value['tmp_name'], including validation of the mediaEntityTmpReadDirectory path construction and file size retrieval.

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The filePath variable could be null or an invalid path, but is passed directly to getimagesize without validation. If both tmp_name and file are empty/null, this could result in getimagesize receiving null or an empty string, potentially causing warnings or unexpected behavior. Consider adding explicit validation to ensure filePath is not null/empty before calling getimagesize, or handle the case more gracefully.

Suggested change
// Ensure we have a valid file path before attempting to read image properties
if (empty($filePath) || !is_string($filePath) || !file_exists($filePath)) {
return [__('"%1" is not a valid file.', $label)];
}

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The code doesn't handle the case where value['file'] might be empty after checking !empty, and ltrim could return an empty string if value['file'] is just '/'. This would result in getAbsolutePath being called with an empty string, which could lead to unexpected behavior. Consider adding validation to ensure tmpFileName is not empty before constructing the file path.

Suggested change
$filePath = $this->mediaEntityTmpReadDirectory->getAbsolutePath($tmpFileName);
}
if ($tmpFileName !== '') {
$filePath = $this->mediaEntityTmpReadDirectory->getAbsolutePath($tmpFileName);
}
}
if (empty($filePath)) {
return [__('"%1" is not a valid file.', $label)];
}

Copilot uses AI. Check for mistakes.
try {
// phpcs:ignore Magento2.Functions.DiscouragedFunction
$imageProp = getimagesize($value['tmp_name']);
$imageProp = getimagesize($filePath);
} catch (\Throwable $e) {
$imageProp = false;
}

if (!$this->_isUploadedFile($value['tmp_name']) || !$imageProp) {
if (!$this->_isUploadedFile($filePath) || !$imageProp) {
return [__('"%1" is not a valid file.', $label)];
}

Expand All @@ -170,7 +185,11 @@ protected function _validateByRules($value)
);
$errors = [];
if ($maxFileSize !== null) {
$size = $value['size'];
$size = $value['size'] ?? 0;
// For UI component uploads, get file size if not provided
if ($size === 0 && !empty($filePath) && file_exists($filePath)) {
$size = filesize($filePath);
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The filesize function is called directly without checking if it returned false. If the file doesn't exist or is inaccessible, filesize returns false, which would then be compared against maxFileSize. Consider checking the result of filesize before using it in the comparison or handling the false case explicitly.

Suggested change
$size = filesize($filePath);
$fileSize = filesize($filePath);
if ($fileSize !== false) {
$size = $fileSize;
}

Copilot uses AI. Check for mistakes.
}
if ($maxFileSize < $size) {
$errors[] = __('"%1" exceeds the allowed file size.', $label);
}
Expand Down