Skip to content
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

Cannot Replace a File With The Same Extension But Different MIME Type #1783

Closed
neildaniels opened this issue May 16, 2019 · 12 comments
Closed

Cannot Replace a File With The Same Extension But Different MIME Type #1783

neildaniels opened this issue May 16, 2019 · 12 comments

Comments

@neildaniels
Copy link
Contributor

@neildaniels neildaniels commented May 16, 2019

Describe the bug
A "JPEG 2000" image was accidentally uploaded to the Panel, but it the original file had the extension .jpg instead of the more appropriate .jp2. However, once it was uploaded, the Panel would only accept uploads that had the MIME type of image/jp2. The file picker wouldn't allow a replacement .jpg file, even though the original file had a .jpg extension.

I think the Panel should be a bit more generous in files that it allows to be uploaded: it should allow the same MIME type and/or the original extension.

To Reproduce
Steps to reproduce the behavior:

  1. Export out a JPEG 2000 image, rename the extension from .jp2 to .jpg
  2. Upload the .jpg to the Panel
  3. Attempt to replace the file with a totally normal .jpg file

Expected behavior
The file picker should allow either .jp2 or .jpg files.

Kirby Version
3.1.4-rc.1

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented May 17, 2019

If I remember correctly, the reason for that check was to avoid that the extension needs to change: Let’s say you uploaded a JPG first and then try to replace it with PNG. Because the filename would change, it could break templates that relied on the filename.

So unless there was a specific reason for a MIME check, I think we should actually only compare the extension itself, not the MIME type.

@neildaniels

This comment has been minimized.

Copy link
Contributor Author

@neildaniels neildaniels commented May 17, 2019

Yes, I totally agree that the filename should never change by a replacement uploading (including the filename).

I'm saying in the scenario you have a example.md upload, I would fully expect that I would be able to replace it with replacement.txt file. Although the extensions differ, they are both text/plain files. Effectively, the uploaded file would have its extension changed.

In a similar way, I would expect to be able to replace .jpg file with a .jpeg file. Although the extensions differ, the MIME type is the same. Once again, the original filename .jpg would remain.

I would not expect to replace .jpg with a .png though. Neither the extensions nor the MIME types match.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented May 17, 2019

That's a good point. So basically the only issue is that the Panel does not allow files with the same extension but a different MIME type.

If I understood you correctly, the behavior should be like this:

  • If MIME type matches for a file of any name or extension (like at the moment), allow replacement.
  • If extension matches (but not MIME type), also allow replacement.
  • If neither matches, deny replacement.
@neildaniels

This comment has been minimized.

Copy link
Contributor Author

@neildaniels neildaniels commented May 17, 2019

Yep. Exactly.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Aug 8, 2019

I'm not a 100% sure if I understood all the different scenarios. Could one of you please test if it is achieved what you discussed by removing these lines:
https://github.com/getkirby/kirby/blob/master/src/Cms/FileRules.php#L88-L93

Thank you.

@neildaniels

This comment has been minimized.

Copy link
Contributor Author

@neildaniels neildaniels commented Aug 27, 2019

@distantnative Commenting out that block of code does not fully fix the problem.

For the following, I'm using my original scenario. Summary: I renamed a JPEG-2000 image to have the extension .jpg and uploaded it to the panel.

When trying to replace files, the browser's file picker is still being restricted to .jp2 files (because of the MIME type).

However, if I renamed a regular JPEG image to have the extension .jp2, that was able to replace the original file. The MIME type displayed in the panel changes from image/jp2 to image/jpeg. This what your identified code block was previously preventing.

The final part of this puzzle should be:

  • Let the browser file picker accept .jpg and .jp2 files (it currently accepts only .jp2)
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Dec 6, 2019

I'm trying to pick this up. As far as I can tell the backend part is not really difficult. But on the frontend we would somehow get a lot smarter with the mime type restrictions. I guess we need some kind of smart mime map that includes all possible mime types for a specific type of file. I.e.

jpg => image/jpeg, image/pjpeg, image/jp2

Definitely not trivial.

@neildaniels

This comment has been minimized.

Copy link
Contributor Author

@neildaniels neildaniels commented Dec 6, 2019

@bastianallgeier

In addition to removing the lines that @distantnative suggested, I suspect the "only" additional thing that needs to be done is add the existing extension to the accept parameter of the <input>.

There shouldn't be a need to create an exhaustive list of MIME types, as the <input type="file" accept="xxxxx"> accepts mime types and file extensions. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#attr-accept

@bastianallgeier bastianallgeier removed this from the 3.3.2 milestone Dec 9, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Dec 10, 2019

@neildaniels oh, wow. I didn't know that to be honest. That definitely makes it a lot easier.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Dec 15, 2019

I have a solution ready where

  • the upload input only allows you to pick a file with the same extension OR with the same mime type
  • the backend only throws an exception when extension and mime are different

That should fulfill all cases discuss, right?

@distantnative distantnative self-assigned this Dec 15, 2019
distantnative added a commit that referenced this issue Dec 15, 2019
@neildaniels

This comment has been minimized.

Copy link
Contributor Author

@neildaniels neildaniels commented Dec 15, 2019

@distantnative That sounds like everything to me.

bastianallgeier added a commit that referenced this issue Jan 9, 2020
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Jan 9, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.