Skip to content

fix adminhtml file attribute edit form #11267

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.

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

Merged
merged 3 commits into from
Dec 13, 2017
Merged

Conversation

fsw
Copy link
Member

@fsw fsw commented Oct 6, 2017

fixes #11252

Description

not sure why this line was introduced as the only place this function is used seems to be here:

$result = $fileProcessor->saveTemporaryFile($this->scope . '[' . $this->getAttributeCode() . ']');

and it uses 'path' key right after call.
hence no tests are included.

Fixed Issues (if relevant)

  1. Custom attribute - File not allowing uploads #11252: Custom attribute - File not allowing uploads

Manual testing scenarios

  1. Create customer attribute with type file
  2. Got to Customer edit form Account Information tab
  3. Upload works instead of showing error

fixes magento#11252

not sure why this line was introduced as the only place this function is used seems to be here:
https://github.com/magento/magento2/blob/55e9a5cc9d88ef785f39217e96da02b1cdf3e247/app/code/Magento/Customer/Model/FileUploader.php#L110
and it uses 'path' key right after call.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Oct 6, 2017

CLA assistant check
All committers have signed the CLA.

@@ -181,7 +181,6 @@ public function saveTemporaryFile($fileId)
);

$result = $uploader->save($path);
unset($result['path']);
Copy link
Contributor

Choose a reason for hiding this comment

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

It was added for security reasons: https://github.com/magento/magento2/blame/2.2-develop/app/code/Magento/Customer/Model/FileProcessor.php#L184

Please figure out another way to fix bug you faced with.

@fsw
Copy link
Member Author

fsw commented Oct 10, 2017

Hi @orlangur,
Thanks for your reply.

Maybe I am missing something but only place in CE and EE I am seeing that is using this function is this place:

$result = $fileProcessor->saveTemporaryFile($this->scope . '[' . $this->getAttributeCode() . ']');

And it is using 'path' right after and If I see correctly it is using it to create file path to validate it later. So it needs this data.

Path can be unset after that (in FileUploader::upload()) but without knowing what the security issue was I can only guess, Is MAGETWO-70580 an internal ticket number? Can we know exactly what issue this change fixed?
Otherwise I have no other ideas.

cheers.

@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:32
@orlangur
Copy link
Contributor

orlangur commented Dec 4, 2017

So, basically, initial security fix needs to be revised which was reverted by changes in this PR and the correct way of fix needs to be understood.

Unassigning from myself for now, will check it again when I have some spare time.

@orlangur orlangur removed their assignment Dec 4, 2017
@omiroshnichenko omiroshnichenko self-assigned this Dec 5, 2017
@omiroshnichenko
Copy link
Contributor

Hi, @fsw. After debugging I found that path used only here. In comment says that tmp_name required for attribute validation, but validation performs before this assignment. So, after I removed usage of $result['path'], in line that I mentioned before, it works correct. Could you please revert your changes and remove usage of $result['path']?

@fsw
Copy link
Member Author

fsw commented Dec 7, 2017

Hi @omiroshnichenko, by "remove usage of $result['path']" do you mean to remove pointed line at all or to change it to something like this:

    $result['tmp_name'] = ltrim($result['file'], '/');

If I recall correctly when I did this and uploaded file via admin form it indeed broke saying tmp_name is undefined or something like that but I can't recall exactly. So I think it is indeed used for some validation.

Anyway I added you as collaborator so please feel free to do whatever you like with this PR or create a new one.

cheers.

@magento-team magento-team merged commit f63d74c into magento:2.3-develop Dec 13, 2017
magento-team pushed a commit that referenced this pull request Dec 13, 2017
[EngCom] Public Pull Requests - develop
 - MAGETWO-85584: Add --no-update option to sampledata:deploy and remove commands #12663
 - MAGETWO-85539: fix adminhtml file attribute edit form #11267
@magento-engcom-team magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom attribute - File not allowing uploads
7 participants