Enhanced Customer Data Validation to Mitigate Code Injection Risks

[in-session]

This Pull Request introduces several significant improvements to the validation process in Magento:

Centralized Validation for Customer Fields:

The validation logic for customer-related fields (such as names, phone numbers, and addresses) has been moved to global validators. This change ensures that consistent validation rules are applied across the entire application, improving both maintainability and reliability.

Enhanced Field Validation Tests:

The validation tests for customer fields have been significantly extended. This includes more comprehensive checks for various character sets, ensuring that fields such as city names, street addresses, and customer names are validated against a wide range of acceptable characters while preventing invalid inputs.

Implementation of Global Forbidden Patterns:

A new GlobalForbiddenPatterns validator has been introduced to mitigate the risk of code injection. This validator applies a set of global regex patterns designed to detect and block potentially malicious input, thereby enhancing the security of the application.

to fix the issus:

• Fixes Checkout address forms allow random code in the name fields #39002
• Extend Fix - Customer address form allows random code in the name fields #38331 #38345

The pull is aimed at all public areas such as Checkout, Register Newsletter and Review, I hope I haven't forgotten anyone

Before:
Checkout:
There was no server-side validation of the fields, which enabled code injection:

Review:

Contact:

After

The patterns from merge #38345 were made globally available and integrated into the quote, this already minimises code injection:

However, there are many other fields that do not run through the pattern, so the GlobalForbiddenPatterns was created, which can be activated via the admin:

This checks the data again to prevent code injection, for example the company field:

Review:

Create Account:

[m2-assistant]

Hi @in-session. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ 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.

[in-session]

@magento run all tests

[in-session]

@magento run all tests

[in-session]

@magento run all tests

[in-session]

@magento run all tests

[in-session]

@magento run all tests

[in-session]

@magento run all tests

[in-session]

@magento run all tests

[in-session]

@magento run all tests

[in-session]

@engcom-Bravo @engcom-Delta @ihor-sviziev after a few grey hairs
Due to a time pass, I won't be able to continue for another 2-3 weeks.
Basically, I believe that the global validations are the right approach to better protect the system.
But I have to make sure that the validation is done via validation.xml and AbstractValidator, I still have to think about it.

But in any case, the merge should be done on hold first: #38345
The patterns are too restrictive and would lead to problems with most international shops.

You should do it more along the lines of the current pull:

private const PATTERN_CITY = '/^[\p{L}\p{M}\s\-\.\'\&\[\]\(\):]{1,100}$/u';
private const PATTERN_NAME = '/(?:[\p{L}\p{M}\,\-\_\.\'’`&\s\d]){1,255}+/u';
private const PATTERN_TELEPHONE = '/(?:[\d\s\+\-\()\/]{1,30})/u';
private const PATTERN_STREET = "/^[\p{L}\p{M}\,\-\.\'’`&\s\d\[\]\(\)]{1,255}$/u";

That's why I extended the test accordingly here.
https://github.com/magento/magento2/blob/b8dfb94879768b63d5a09b6cf453431d27e2a4bb/app/code/Magento/Customer/Test/Unit/Model/Validator/CityTest.php
https://github.com/magento/magento2/blob/b8dfb94879768b63d5a09b6cf453431d27e2a4bb/app/code/Magento/Customer/Test/Unit/Model/Validator/NameTest.php
https://github.com/magento/magento2/blob/b8dfb94879768b63d5a09b6cf453431d27e2a4bb/app/code/Magento/Customer/Test/Unit/Model/Validator/StreetTest.php
https://github.com/magento/magento2/blob/b8dfb94879768b63d5a09b6cf453431d27e2a4bb/app/code/Magento/Customer/Test/Unit/Model/Validator/TelephoneTest.php

In a test I had tried if you could possibly set the pattern via admin that would be better in the long term, unfortunately this is technically not possible. And if we put the pattern directly into the framework, they are also available globally.

The whole test and also the delveopler environment time that per current pull would already go.
I may then open a new pull to better and shortener overview :-)

And it seems that the WAF via the cloud does not work in the case of code injection either.

[engcom-Hotel]

Hello @in-session,

It seems you are still working on this PR, hence moving it to On Hold. Please let us know once you are ready with this, we will pick this for further processing.

Thanks

[in-session]

@engcom-Hotel
I close the PR because another PR is better #39131