FIX: Can't validate uploaded file for tmp folder mismatching #27352

baranada · 2020-03-19T08:17:41Z

Description (*)

when upload file, php $_FILE variable returns destination path of symlinked tmp folder.
but sys_get_temp_dir() function only return target path of symlinked folder.
this mismatch made it can't pass the validation. this commit validate on both target path and destination path if symlink found.
I couldn't find php $_FILE source reference, so I put both path on validation.

Related Pull Requests

Fixed Issues (if relevant)

Fixes Invalid parameter given. A valid $fileId[tmp_name] is expected #25835: Invalid parameter given. A valid $fileId[tmp_name] is expected

Manual testing scenarios (*)

set default magento php environment sys_temp_dir ini value to any folder you made symlink to other folder.
Go to Stores - Settings - Configuration.
Open tab Catalog - Catalog. Fieldset "Product Image Placeholders".
Select the placeholder image for field "Base"
Click to button "Save"
You can try another setting with type "file"
check if validation passes or below message will pop.
"Invalid parameter given. A valid $fileId[tmp_name] is expected"

Questions or comments

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)
All automated tests passed successfully (all builds are green)

…r tmp folder mismatching magento#25835 when upload file, php returns destination path of symlinked tmp folder. but sys_get_temp_dir() function only return target path of symlinked folder. this mismatch made it can't pass the validation. this commit validate on both target path and destination path if symlink found. I couldn't find php $_FILE source reference, so I put both pathes on validation.

m2-assistant · 2020-03-19T08:17:44Z

Hi @baranada. Thank you for your contribution
Here is some useful tips 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

For more details, please, review the Magento Contributor Guide documentation.

lenaorobei · 2020-03-30T15:42:21Z

Closing this PR since the issue can be resolved by configuring sys_temp_dir PHP setting.

m2-assistant · 2020-03-30T15:42:23Z

Hi @baranada, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

hostep · 2020-04-02T15:34:10Z

@baranada: changes look good at first sight, but don't work in all cases.

For example, on macOS, when executing is_link('/var/tmp/') it will not work properly, because on macOS, the symlink is on the /var/ directory and not on /var/tmp/.

Here are suggested changes to make it work in all cases I think:

$sysTmpFolder = sys_get_temp_dir();
$sysTmpFolderRealPath = realpath($sysTmpFolder);

$allowedFolders = [
    $sysTmpFolder,
    $sysTmpFolderRealPath,
    $this->directoryList->getPath(DirectoryList::MEDIA),
    $this->directoryList->getPath(DirectoryList::VAR_DIR),
    $this->directoryList->getPath(DirectoryList::TMP),
    $this->directoryList->getPath(DirectoryList::UPLOAD),
];

I'm reopening this PR based on the fact that I believe there is an actual improvement we can make here.

baranada · 2020-04-04T12:25:30Z

@baranada: changes look good at first sight, but don't work in all cases.

For example, on macOS, when executing is_link('/var/tmp/') it will not work properly, because on macOS, the symlink is on the /var/ directory and not on /var/tmp/.

Here are suggested changes to make it work in all cases I think:
$sysTmpFolder = sys_get_temp_dir();
$sysTmpFolderRealPath = realpath($sysTmpFolder);

$allowedFolders = [
    $sysTmpFolder,
    $sysTmpFolderRealPath,
    $this->directoryList->getPath(DirectoryList::MEDIA),
    $this->directoryList->getPath(DirectoryList::VAR_DIR),
    $this->directoryList->getPath(DirectoryList::TMP),
    $this->directoryList->getPath(DirectoryList::UPLOAD),
];
I'm reopening this PR based on the fact that I believe there is an actual improvement we can make here.

@hostep, thank you for your reply.
I found that if you use realpath function, we don't have to put both paths in the validation folder.
we just pass sys_get_temp_dir results to realpath, then it will pass the validation, symlink or not.
I will apply this changes and make unit test.

hostep · 2020-04-10T14:12:30Z

@baranada: to be safe I would check on both paths, I'm not familiar enough with how the path in the $_FILES global are being set in all kinds of different environments.

I think we are probably most safe when checking for both the symlinked and the realpath directories.

hostep · 2020-04-24T06:09:20Z

Hi @baranada: are you still able/willing to put some extra time into this? Thanks!

baranada · 2020-04-24T13:48:47Z

I couldn't make test code for this.
I pushed the code with your suggestion. it was wrote back on 21 days ago. so github timeline is tangled.
if it's necessary to write test code, then I hope someone to help on this.
Thank you for following up.

hostep · 2020-05-12T08:10:48Z

@lenaorobei: can you or somebody else please review this, that would be appreciated, thanks! 🙂

lenaorobei · 2020-09-04T17:10:21Z

@magento run all tests

lenaorobei

The fix looks good. @baranada, could you please write simple integration test to cover this case?

baranada · 2020-09-18T13:40:02Z

The fix looks good. @baranada, could you please write simple integration test to cover this case?

Since I am not familiar with magento2 test code, I will try and come back to you next week.

…o MC-29272

ihor-sviziev

Moving this PR to corresponding status as requested in #27352 (review)

engcom-Golf · 2020-09-30T08:26:47Z

@magento run all tests

engcom-Golf · 2020-09-30T10:54:38Z

Hi, @baranada!
Please review test failures and fix them for the further processing of PR.
Thank you!

m2-assistant · 2020-09-30T15:21:57Z

Hi @baranada, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

m2-assistant · 2020-09-30T15:22:02Z

Hi @baranada. Thank you for your contribution
Here is some useful tips 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:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

baranada · 2020-11-06T08:18:46Z

@magento run all tests

ghost added the Progress: pending review label Mar 19, 2020

magento-engcom-team added Component: File Release Line: 2.4 labels Mar 19, 2020

lenaorobei closed this Mar 30, 2020

hostep reopened this Apr 2, 2020

hostep mentioned this pull request Apr 2, 2020

Invalid parameter given. A valid $fileId[tmp_name] is expected #25835

Open

FIX: symlink check failure on macOS - thanks to @hostep

18ca553

sidolov added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Aug 17, 2020

Merge branch '2.4-develop' into MC-29272

22bd4b9

lenaorobei self-assigned this Sep 4, 2020

lenaorobei reviewed Sep 17, 2020

View reviewed changes

Merge branch '2.4-develop' of https://github.com/magento/magento2 int…

f750761

…o MC-29272

ihor-sviziev requested changes Sep 21, 2020

View reviewed changes

ghost assigned ihor-sviziev Sep 21, 2020

ghost added Progress: needs update and removed Progress: pending review labels Sep 21, 2020

ihor-sviziev removed their assignment Sep 21, 2020

baranada and others added 2 commits September 23, 2020 13:29

Merge branch '2.4-develop' into MC-29272

5c27bf9

ADD: test code for validating uploaded file

6297bb4

engcom-Golf self-assigned this Sep 30, 2020

baranada closed this Sep 30, 2020

baranada reopened this Sep 30, 2020

ghost added Progress: pending review and removed Progress: needs update labels Sep 30, 2020

ghost unassigned lenaorobei and engcom-Golf Sep 30, 2020

baranada added 2 commits October 9, 2020 15:37

Merge branch '2.4-develop' into MC-29272

c37639a

FIX: validation failure on upload. fix for unittest.

afcb8cb

FIX: Can't validate uploaded file for tmp folder mismatching #27352

Are you sure you want to change the base?

FIX: Can't validate uploaded file for tmp folder mismatching #27352

Uh oh!

Conversation

baranada commented Mar 19, 2020 • edited by ihor-sviziev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Mar 19, 2020

Uh oh!

lenaorobei commented Mar 30, 2020

Uh oh!

m2-assistant bot commented Mar 30, 2020

Uh oh!

hostep commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baranada commented Apr 4, 2020

Uh oh!

hostep commented Apr 10, 2020

Uh oh!

hostep commented Apr 24, 2020

Uh oh!

baranada commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hostep commented May 12, 2020

Uh oh!

lenaorobei commented Sep 4, 2020

Uh oh!

lenaorobei left a comment

Choose a reason for hiding this comment

Uh oh!

baranada commented Sep 18, 2020

Uh oh!

ihor-sviziev left a comment

Choose a reason for hiding this comment

Uh oh!

engcom-Golf commented Sep 30, 2020

Uh oh!

engcom-Golf commented Sep 30, 2020

Uh oh!

m2-assistant bot commented Sep 30, 2020

Uh oh!

m2-assistant bot commented Sep 30, 2020

Uh oh!

baranada commented Nov 6, 2020

Uh oh!

Uh oh!

baranada commented Mar 19, 2020 •

edited by ihor-sviziev

Loading

hostep commented Apr 2, 2020 •

edited

Loading

baranada commented Apr 24, 2020 •

edited

Loading