Validate frontend input type for file uploads

[SkyMulley]

Description (*)

The customer address file upload endpoint (Customer/Controller/Address/File/Upload.php) did not validate whether the requested attribute's frontend_input type was file or image before proceeding with the upload.

This allowed an attacker to supply any custom address attribute code (e.g. a text type attribute) as the upload target. Since non-file attributes have no file_extensions validation rules configured, getAllowedExtensions() returns an empty array, causing the framework uploader to permit all file types — including executable files such as .php.

The fix adds a check immediately after fetching the attribute metadata, throwing a LocalizedException if the frontend_input is not file or image.

Manual testing scenarios (*)

1. Create a custom customer address attribute with frontend_input type of text (e.g. my_text_attribute)
2. Log in as a customer and send a POST request to /customer/address/file/upload with custom_attributes[my_text_attribute] as the file field, uploading a .php file
3. Without fix: file is accepted and saved to pub/media/customer_address/tmp/
4. With fix: request returns an error — Attribute "my_text_attribute" does not support file uploads. — and no file is written to disk
5. Verify that uploading via a legitimate file or image type attribute still works as expected

Questions or comments

This vulnerability requires an authenticated customer session to exploit. However, customer registration is typically open on Magento storefronts, making the attack surface effectively unauthenticated. The exploitability of the resulting upload also depends on whether PHP execution is blocked in the media directory, but defence in depth requires rejecting the upload regardless.

Resolved issues:

1. resolves [Issue] Validate frontend input type for file uploads #40795: Validate frontend input type for file uploads

[m2-assistant]

Hi @SkyMulley. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[ct-prd-pr-scan]

The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email psirt@adobe.com.

[rgarciar-hiberuscom]

Hi @SkyMulley

As you can see, there is already a validation in place in the code, but it is not working as expected:

$errors = $fileUploader->validate();

The method \Magento\Customer\Model\FileUploader::validate creates a form element based on the attribute, but it does not validate the input type. As a result, when \Magento\Customer\Model\Metadata\Form\AbstractData::validateValue is called, it may use a different type instead of \Magento\Customer\Model\Metadata\Form\File::validateValue, which is the one responsible for validating allowed file extensions.

A possible improvement to prevent this issue would be to validate the input type directly in FileUploader::validate:

--- a/vendor/magento/module-customer/Model/FileUploader.php
+++ b/vendor/magento/module-customer/Model/FileUploader.php
@@ -83,6 +83,12 @@ class FileUploader
      */
     public function validate()
     {
+        // Avoid file upload for non-file attributes
+        if(!\in_array($this->attributeMetadata->getFrontendInput(), ['file', 'image'])) {
+            $errors[] = __('Attribute input type is not valid for file uploading.');
+            return $errors;
+        }
+        
         $formElement = $this->elementFactory->create(
             $this->attributeMetadata,
             null,

With this change, a new error would be added to the expected $errors array, maintaining a clean and consistent structure:

$errors = $formElement->validateValue($this->getData());

[SkyMulley]

Good catch @rgarciar-hiberuscom , I've applied your suggested amendments, thanks!

[engcom-Hotel]

@magento create issue

[ct-prd-pr-scan]

The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email psirt@adobe.com.

[SamJUK]

Quick suggestion to the proposed change, can we get the allowed input types declared via di.xml for ease of future extendability?
I see potential for merchants wanting to add video/audio/document types. And some people just like todo strange and whacky stuff with Magento 😅.

Just as a note as well - the original Issue & PR (#40262) got removed for this ~6 months ago (security related).
Which is a shame, since it did not land in 2.4.8-p4 or 2.4-develop since, and we have had zero updates / way to track Adobes internal progress for it further.

The same PR got merged to MageOS though (mage-os/mageos-magento2#174), if you want a quick reference for the di.xml stuff. We've also been running the suggested fix in production across a bunch of projects since, and its been working well.

[engcom-Hotel]

@magento run all tests

[rgarciar-hiberuscom]

Maybe Tests should be fixed first as testValidate() is expecting a boolean result when this could be "array|bool"

Failed asserting that Array &0 [
    0 => Magento\Framework\Phrase Object #1466995 (
        'text' => 'Attribute input type is not valid for file uploading.',
        'arguments' => Array &1 [],
    ),
] is true.

\Magento\Customer\Test\Unit\Model\FileUploaderTest::testValidate

    public function testValidate()
    {
        $attributeCode = 'attribute_code';

        $filename = 'filename.ext1';

        $_FILES = [
            'customer' => [
                'name' => [
                    $attributeCode => $filename,
                ],
            ],
        ];

        $formElement = $this->getMockBuilder(Image::class)
            ->disableOriginalConstructor()
            ->getMock();
        $formElement->expects($this->once())
            ->method('validateValue')
            ->with(['name' => $filename])
            ->willReturn(true);

        $this->elementFactory->expects($this->once())
            ->method('create')
            ->with($this->attributeMetadata, null, CustomerMetadataInterface::ENTITY_TYPE_CUSTOMER)
            ->willReturn($formElement);

        $model = $this->getModel(CustomerMetadataInterface::ENTITY_TYPE_CUSTOMER, 'customer');
        $this->assertTrue($model->validate());
    }

Furthermore, it is not good practice to return values in this way. Maybe the best approach is returning always array (empty has no errors, so validation was succesfull)

This will never be fixed until tests work properly as they are required before merging.

The code is correct:

\Magento\Customer\Model\FileUploader::validate

    /**
     * Validate uploaded file
     *
     * @return array|bool
     */
    public function validate()
    {
        // Avoid file upload for non-file attributes
        if(!\in_array($this->attributeMetadata->getFrontendInput(), ['file', 'image'])) {
            $errors[] = __('Attribute input type is not valid for file uploading.');
            return $errors;
        }
        
        $formElement = $this->elementFactory->create(
            $this->attributeMetadata,
            null,
            $this->entityTypeCode
        );

        $errors = $formElement->validateValue($this->getData());
        return $errors;
    }

\Magento\Customer\Model\Metadata\Form\Select::validateValue

    /**
     * {@inheritdoc}
     */
    public function validateValue($value)
    {
        $errors = [];
        $attribute = $this->getAttribute();
        $label = __($attribute->getStoreLabel());

        if ($value === false) {
            // try to load original value and validate it
            $value = $this->_value;
        }

        if ($attribute->isRequired() && empty($value) && $value !== '0') {
            $errors[] = __('"%1" is a required value.', $label);
        }

        if (!$errors && !$attribute->isRequired() && empty($value)) {
            return true;
        }

        if (count($errors) == 0) {
            return true;
        }

        return $errors;
    }

Value returned: true or array containing a phrase.

[engcom-Hotel]

Hello @SkyMulley,

We are unable to reproduce the issue mentioned here. Please refer to this #40795 (comment) for more information.

Meanwhile moving this PR on hold.

Thank you

[rgarciar-hiberuscom]

@engcom-Hotel maybe progress should be updated as issue is confirmed?

[engcom-Hotel]

Sure @SkyMulley we are picking this PR for further review.

[engcom-Hotel]

@magento run all tests

[engcom-Hotel]

Hello @SkyMulley,

Thank you for the contribution!

As part of the quick review, please sign the CLA and add some automated tests for this fix.

Meanwhile moving this PR on hold.

[rgarciar-hiberuscom]

@engcom-Hotel unit tests will always fail as validate() can return array or bool and value expected on testValidate() is only bool: #40583 (comment)

[engcom-Hotel]

Hello @rgarciar-hiberuscom,

I agree with your point, but as per the quick check, we can fix it via stub getFrontendInput() to an allowed value before getModel():

$this->attributeMetadata->expects($this->once())
    ->method('getFrontendInput')
    ->willReturn('file'); // or 'image'

Place that after $_FILES and before $model = $this->getModel(...).

The rest of the test (form element mock, elementFactory->create, assertTrue) can stay as-is. Please try the fix or you can think any other best way to fix the same.

@SkyMulley The CLA is still failing. Please sign the CLA, its necessary to proceed further with this PR.

Thank you

[ct-prd-pr-scan]

The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email psirt@adobe.com.

[SkyMulley]

@engcom-Hotel CLA signed and tests amended

[engcom-Hotel]

@magento run all tests

[engcom-Dash]

Hi @SkyMulley
Thank you for your contribution!

✔️ QA Passed

Preconditions:
Install fresh Magento 2.4-develop

Before PR

After PR

Functional builds are failing, hence moving the PR for Extended Testing

[engcom-Dash]

@magento run all tests