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

FIX Allow inconclusive mimetypes #582

Conversation

GuySartorelli
Copy link
Member

I don't want to bump the dependency in a patch release so we can't use a named argument for the new useInconclusiveMimeTypeFallback parameter since it isn't available in flysystem until 3.23.0.

Passing an extra argument doesn't cause errors so it's safe if the project doesn't have flysystem updated - but passing missing named arguments does cause errors.

Issue

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, if mimetype cannot be worked out from the file contents, then it will fallback to using the file extension to determine the mimetype? Isn't this introducing a bit of a security risk since it would provide a vector to upload files that don't match the file extenion?

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Dec 19, 2023

It tries to get a mimetype from the file contents - but even if it gets a mimetype and it's a valid mimetype, it will ignore that type if it's deemed to be "inconclusive".
If then checks the file name - and if it can't get any mimetype from the file name (inconclusive or otherwise), an exception is thrown.

So for .brf files for example, the file contents look like text/plain. That's considered inconclusive, so it moves on to check the file type, and there is no registered mime type for .brf in flysystem, so it throws an exception.

If it returned text/plain (which this PR will make it do), that mimetype is still validated afterwards. In the case of .brf files, the mimevalidator was updated with the correct mime type, which is one of text/plain or application/octet-stream. If the mime type detected isn't one of those, it'll still fail validation.

So long as the mimetype validator is set up correctly for a given file type (which AFAIK is required to include it as uploadable) there is no risk of vulnerabilities of that type.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

OK sounds good

@emteknetnz emteknetnz merged commit 6e3fd80 into silverstripe:2.1 Dec 19, 2023
9 checks passed
@emteknetnz emteknetnz deleted the pulls/2.1/allow-inconclusive-mimetypes branch December 19, 2023 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants