Active Storage: don't crash if mini_mime doesn't recognize content_type #42225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghiculescu
force-pushed
the
as-content-type-crash
branch
from
b13812d
to
364d881
Compare
|
Awesome! |
ghiculescu
force-pushed
the
as-content-type-crash
branch
from
364d881
to
5859e67
Compare
ghiculescu
force-pushed
the
as-content-type-crash
branch
from
5859e67
to
fbcac36
Compare
|
@ghiculescu thanks! 👍🏻 |
pixeltrix
added a commit
that referenced
this pull request
May 15, 2021
Active Storage: don't crash if mini_mime doesn't recognize content_type
pixeltrix
added a commit
that referenced
this pull request
May 15, 2021
The change on main didn't need one since it was a regression that hadn't shipped.
|
@ghiculescu I assume this is a regression on 5-2-stable and 6-0-stable as well? |
|
@pixeltrix ahhh yep it would be |
|
@ghiculescu on checking 6-0-stable it seems to be fine. The affected code is for storing variants in the database which was added in 6.1. |
|
Oops yep, I assumed too quickly. The crashing code was added in #40226 which was 6.1 only. |
ghiculescu
added a commit
to ghiculescu/rails
that referenced
this pull request
Sep 22, 2021
rails#42225 identified that some of the content types used as defaults by Active Storage aren't recognized by `mini_mime`. This means that in practice code like [this](https://github.com/rails/rails/pull/42225/files#diff-7a3ec24c556b138abdbd67066ab5125b73528e45891d83142e417d3944194128R116) will crash or not function correctly. In [this](https://github.com/rails/rails/pull/42225/files#diff-c2010824d2d2e8d841ff4fc058c264c12d870e893025b153e6de571fba6b6c6cR194) example, a file with content_type `image/jpg` is treated as a PNG by the representer, since `image/jpg` isn't a valid content type according to `mini_mime`. I don't think the default content_types should include formats that have never actually worked, so I'm proposing we remove them from the defaults.
jyoun-godaddy
pushed a commit
to jyoun-godaddy/activestorage
that referenced
this pull request
Jul 5, 2022
rails/rails#42225 identified that some of the content types used as defaults by Active Storage aren't recognized by `mini_mime`. This means that in practice code like [this](https://github.com/rails/rails/pull/42225/files#diff-7a3ec24c556b138abdbd67066ab5125b73528e45891d83142e417d3944194128R116) will crash or not function correctly. In [this](https://github.com/rails/rails/pull/42225/files#diff-c2010824d2d2e8d841ff4fc058c264c12d870e893025b153e6de571fba6b6c6cR194) example, a file with content_type `image/jpg` is treated as a PNG by the representer, since `image/jpg` isn't a valid content type according to `mini_mime`. I don't think the default content_types should include formats that have never actually worked, so I'm proposing we remove them from the defaults.
jonathanhefner
added a commit
to jonathanhefner/rails
that referenced
this pull request
Oct 26, 2022
This fixes the following warning when running Active Storage tests: ``` DEPRECATION WARNING: image/jpg is not a valid content type, it should not be used when creating a blob, and support for it will be removed in Rails 7.1. If you want to keep supporting this content type past Rails 7.1, add it to `config.active_storage.variable_content_types`. Dismiss this warning by setting `config.active_storage.silence_invalid_content_types_warning = true`. ``` Note that this test should eventually be removed. It was written to test the fix from rails#42225; however, after the deprecation from rails#42227 is complete and invalid content types have been removed from `config.active_storage.variable_content_types`, calling `variant` when the content type is invalid will raise `ActiveStorage::InvariableError` (instead of the `NoMethodError` mentioned in rails#42225 / rails#41777). And that behavior is already tested by the "variation of invariable blob" test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #41777
Since there is no format recognised for the invalid content_type, we fall back to treating it as a
png. This was also the behavior with mimemagic.Note this is a regression in 6.1.3.1. Would be good to backport to https://github.com/rails/rails/tree/6-1-stable