Add encoding to file lock names

[tony-montemuro]

Description (*)

This pull request was created to address for Magento instances using the file lock provider. If a lock name contains characters that are forbidden for file names (e.g., / on Unix-like systems), the operating system will fail to create the lock file, leading to a FileSystemException being thrown.

There are instances where the system attempts to acquire a lock using a string that can contain these forbidden characters. An example includes: app/code/Magento/Catalog/Plugin/ProductRepositorySaveOperationSynchronizer.php::aroundSave(). This plugin wraps calls to product saves in a ProductMutex, which is implemented using a lock, and the name of that lock is determined by the product SKU. The product SKU attribute has no restriction that makes the / character forbidden.

To address this limitation, I have introduced file lock name encoding. I chose rawurlencode() for a few reasons:

• It is an injective algorithm, so naming collisions are impossible.

• It is human readable, so users that wish to monitor the lock directory have a relatively unchanged experience.

• Easy to decode using rawurldecode().

• Encodes forbidden characters on Unix-like operating systems, as well as Windows.

  • Forbidden Linux/Unix Characters:
    • /
  • Forbidden Windows Characters:
    • <
    • >
    • :
    • "
    • /
    • \
    • |
    • ?
    • *

  % is valid for both types of systems.

Implementation details

1. The lock name is encoded using rawurlencode() within Magento\Framework\Lock\Backend\FileLock::getLockPath().
2. The corresponding file path generation is automatically handled in lock(), unlock(), and isLocked() methods because they all utilize the updated getLockPath() method.
3. The cleanupOldLocks() method was also updated to use rawurldecode() before checking lock status, preventing issues where double-encoded paths would prevent detection of active locks or deletion of encoded lock files.

Manual testing scenarios (*)

1. Switch to the file lock provider:

bin/magento setup:config:set --lock-provider=file --lock-file-path=var/locks
bin/magento cache:flush

2. Acquire an admin access token:

export MAGENTO_TOKEN=$(curl -k -s \
  -X POST "https://magento.test/rest/V1/integration/admin/token" \
  -H "Content-Type: application/json" \
  -d '{
    "username": "john.smith", 
    "password": "password123"
  }' | tr -d '"')
echo "Token Acquired: $MAGENTO_TOKEN"

3. Using the token acquired above, make a POST request to the products endpoint. In the request body, ensure that a / is present in the sku attribute:

curl -k \
  -X POST "https://magento.test/rest/V1/products" \
  -H "Authorization: Bearer $MAGENTO_TOKEN" \
  -H "Content-Type: application/json" \
  -d '{
      "product": {
          "sku": "ABC/XYZ",
          "name": "A",
          "attribute_set_id": 4,
          "price": 5.00,
          "status": 1,
          "visibility": 4,
          "type_id": "simple"
      }
  }'

4. Observe that an exception is returned with the following message: "Cannot acquire a lock.", returning a status of 400.
5. Apply PR, and repeat steps 1-3.
6. Observe that the expected product JSON is returned, returning a status of 200 (success).

Questions or comments

I acknowledge that rawurlencode() might not be the optimal encoding mechanism in this scenario. I would argue it is good for this scenario, but I do not claim to be an expert on methods of encoding text. If anyone has a better idea, or concerns with this encoding method, please feel free to let me know!

Per the Adobe Commerce documentation, Windows is not supported, but I am not 100% sure if this applies to new versions of Magento Open Source as well. If Windows is not supported, I could potentially devise a more simple encoding scheme that just handles /. For extra clarification, I did all development & testing on Linux.

I am a bit concerned with how backwards compatible this change might be. If the deployment occurs while a long-lasting lock is active, the new code will miss the existing lock if it's encoded name is different from it's current name.

• If Windows support can be ignored, I can simplify the encoding, making this scenario impossible.
• If Windows support is required, I might need some guidance on this point in particular.

Finally, this is my first contribution to Magento Open Source. I did my best to follow the contribution guidelines, but it's very possible I did something wrong. Please let me know so I can do better next time.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Add encoding to file lock names #40335: Add encoding to file lock names

[m2-assistant]

Hi @tony-montemuro. 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.

[tony-montemuro]

@magento run all tests

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

[tony-montemuro]

@magento run all tests

[hostep]

Your request makes sense.

However, using rawurldecode/rawurlencode feels inappropriate to me, since we're not dealing with url's but with filenames.
I couldn't find anything in the Magento framework itself (or its dependencies) to sanitise filenames and I also don't believe a PHP built-in function exists for this.
Closest I could find is Magento\Framework\File\Uploader::getCorrectFileName, but since the class name refers to uploaded files, it also feels inappropriate to use here.

So either we should go with a simple preg_replace similar to the one from that Uploader class (either directly in the FileLock class, or in the Magento File framework code or the Filesystem framework code so it can be reused in other places), or introduce a small composer dependency that can do this. For example this one: https://github.com/OndrejVrto/php-filename-sanitize

Just my 2 cents, looking forward to hearing from other their thoughts on this?

[tony-montemuro]

@hostep Thank you for looking at this request.

I agree that the URL encode / decode functions are not ideal for this situation. And your alternative suggestions are great; I was hoping that I would get some like this. When I first made this request, I was under the impression that the filename transformation had to be reversible, but after considering your suggestions, it seems that with a very minor refactor, this property is unnecessary. It is unfortunate that these suggestions lack a one-to-one mapping, meaning that you will occasionally have a false lock (two resources mapping to the same file), but I suppose this is preferable to certain locks failing to be acquired 100% of the time (and in practice, I imagine these collisions would be rare).

My personal preference would be to break Magento\Framework\File\Uploader::getCorrectFileName out of the Uploader class so that it can be reused in other places, but the composer dependency may be a more complete solution -- I haven't had a chance to look at it in more depth.

It would be great to get a third opinion on this issue.

[engcom-Hotel]

@magento run all tests

[hostep]

@engcom-Hotel: before you start running tests, we should first figure out the best way forward, see the discussions above, only after we've got a good conclusion, we can implement it and only then tests should be ran in my opinion.

[engcom-Hotel]

I think it would be best to put this on hold for now while we figure out the most suitable solution.