feat(io/image): allow restricting mime types #2999
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2999 +/- ##
===========================================
- Coverage 62.89% 52.10% -10.80%
===========================================
Files 146 114 -32
Lines 11822 11052 -770
===========================================
- Hits 7436 5759 -1677
- Misses 4386 5293 +907
|
|
tests are failing. Probably you have also to add unit test and fix the e2e once the gRPC PR got merged. |
sauyon
force-pushed
the
image-mime
branch
2 times, most recently
from
c25e3ee
to
18ba195
Compare
| for mtype in chain(self._allowed_mimes, [self._mime_type]): | ||
| if mtype not in MIME_EXT_MAPPING: # pragma: no cover | ||
| raise InvalidArgument( | ||
| f"Invalid Image mime_type '{mtype}'; supported mime types are {', '.join(PIL.Image.MIME.values())} " | ||
| ) |
There was a problem hiding this comment.
Should we have separate messages to indicate where the invalid mime type came from, mime_type or allowed_mime_types.
| if ( | ||
| val_content_type in MIME_EXT_MAPPING | ||
| or val_content_type.startswith("image/") | ||
| ): | ||
| bytes_ = await val.read() | ||
| break |
There was a problem hiding this comment.
Shouldn't this be a reject case with BadInput?
There was a problem hiding this comment.
This is our old behavior if allowed_mime_types is not specified.
| if self._allowed_mimes is None: | ||
| raise BadInput( | ||
| f"no multipart image file (with mime type in {MIME_EXT_MAPPING.keys()} or 'image/*'), got request with content type {mime_type}" | ||
| ) | ||
| else: | ||
| raise BadInput( | ||
| f"no multipart image file (with mime type in {self._allowed_mimes}), got a request with content type {mime_type}" | ||
| ) |
There was a problem hiding this comment.
Are they still "multipart" image files under the else condition?
There was a problem hiding this comment.
Oops, I failed to update this copy-paste.
| def __init__( | ||
| self, | ||
| pilmode: _Mode | None = DEFAULT_PIL_MODE, | ||
| mime_type: str = "image/jpeg", | ||
| *, | ||
| allowed_mime_types: t.Iterable[str] | None = None, |
There was a problem hiding this comment.
@aarnphm changed this to have a separate parameter for allowed mimes.
|
|
||
| if isinstance(bytes_, str): | ||
| bytes_ = bytes(bytes_, "UTF-8") | ||
|
|
||
| try: | ||
| return PIL.Image.open(io.BytesIO(bytes_)) |
There was a problem hiding this comment.
after we open the image, should we also run verify ?
There was a problem hiding this comment.
I think we should leave this lazy and with as little overhead as we can; users can do that if they want. At some point maybe we can turn on some sort of option to enable automatic verification.
|
maybe we need test for this :) |
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
|
I will fix the broken CI on #2984 since we have a lot of changes wrt e2e tests there. |
This PR enables support for passing a list of mime types to the
ImageIO descriptor, which, alongside passingaccept_all_images=False, will allow users to whitelist allowed image formats.