search

magento-engcom / import-export-improvements

Open Software License 3.0
31 stars 29 forks source link

Import uploader does not check Content-Disposition header #78

Open piotrekkaminski opened 6 years ago

piotrekkaminski commented 6 years ago

From @EliasZ on November 27, 2017 15:22

Preconditions

Magento 2.2.1 (probably previous versions too, cannot imagine this functionality being removed on purpose)

Steps to reproduce

  1. Create a product import CSV with an image URL (which does not have a proper image extension) leading to an image being force downloaded by HTTP headers (for example: https://gist.github.com/brasofilo/2863355 (example gist))

  2. Import it

Expected result

  1. Magento properly checks the headers, downloads the file to the filename given in the headers and then imports it

Actual result

  1. Magento does not check the headers and downloads the file (for example http://example.com/downloadsomefile becomes something like /pub/media/import/httpexamplecomdownloadsomefile)
  2. The filename does not have a valid file extension and validation fails resulting in the file not being properly imported

Problem

Magento\CatalogImportExport\Model\Import\Uploader::move() sets $fileName to a stripped version of the URL. Here it should do a Magento\Framework\Filesystem\File\ReadInterface::stat() on the URL to check if the Content-Disposition header is set and a filename is provided.

Copied from original issue: magento/magento2#12455

piotrekkaminski commented 6 years ago

From @magento-engcom-team on November 28, 2017 14:30

@EliasZ, thank you for your report. We've created internal ticket(s) MAGETWO-84645 to track progress on the issue.

piotrekkaminski commented 6 years ago

From @PieterCappelle on December 24, 2017 12:33

Should be fixed. If accepted I'll create backport to 2.2 & 2.1.

piotrekkaminski commented 6 years ago

From @magento-team on January 3, 2018 11:45

Hi @EliasZ. Thank you for your report. The issue has been fixed in magento/magento2#12872 by @PieterCappelle in 2.3-develop branch Related commit(s):

The fix will be available with the upcoming patch release.

piotrekkaminski commented 6 years ago

From @EliasZ on January 3, 2018 14:1

@PieterCappelle Did you verify this works with URLs sending a header detailing the filename? parse_url doesn't seem to check that.

dmanners commented 6 years ago

@PieterCappelle could you update this ticket for me. I see you have changed some stuff related to image processing. Is this already in 2.3-develop? Does it cover the issue raised in this ticket? Should we consider a backport here?

Feel free to change the labels if we should backport and also add to phase 2 or close if we no longer need this ticket.

Thank you.