-
Notifications
You must be signed in to change notification settings - Fork 140
Allow background image URLs for file types the web server doesn't know about, e.g. when AVIF is sent as application/octet-stream
#1956
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
Conversation
application/octet-stream Content-Type
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1956 +/- ##
=======================================
Coverage 72.25% 72.26%
=======================================
Files 85 85
Lines 6968 6958 -10
=======================================
- Hits 5035 5028 -7
+ Misses 1933 1930 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
plugins/image-prioritizer/helper.php
Outdated
| $file_extension = pathinfo( (string) wp_parse_url( $url, PHP_URL_PATH ), PATHINFO_EXTENSION ); | ||
| $valid_image_file_extensions = array( 'jpg', 'jpeg', 'png', 'gif', 'webp', 'avif', 'heic', 'heif', 'svg' ); | ||
| if ( '' !== $file_extension && in_array( strtolower( $file_extension ), $valid_image_file_extensions, true ) ) { | ||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right because then the $content_length check below is not being accounted for.
plugins/image-prioritizer/helper.php
Outdated
| $content_type = (array) wp_remote_retrieve_header( $r, 'content-type' ); | ||
| if ( ! is_string( $content_type[0] ) || ! str_starts_with( $content_type[0], 'image/' ) ) { | ||
| // This is a special case for images that are not served with the correct Content-Type. | ||
| if ( is_string( $content_type[0] ) && 'application/octet-stream' === $content_type[0] ) { | ||
| $file_extension = pathinfo( (string) wp_parse_url( $url, PHP_URL_PATH ), PATHINFO_EXTENSION ); | ||
| $valid_image_file_extensions = array( 'jpg', 'jpeg', 'png', 'gif', 'webp', 'avif', 'heic', 'heif', 'svg' ); | ||
| if ( '' !== $file_extension && in_array( strtolower( $file_extension ), $valid_image_file_extensions, true ) ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this instead?
| $content_type = (array) wp_remote_retrieve_header( $r, 'content-type' ); | |
| if ( ! is_string( $content_type[0] ) || ! str_starts_with( $content_type[0], 'image/' ) ) { | |
| // This is a special case for images that are not served with the correct Content-Type. | |
| if ( is_string( $content_type[0] ) && 'application/octet-stream' === $content_type[0] ) { | |
| $file_extension = pathinfo( (string) wp_parse_url( $url, PHP_URL_PATH ), PATHINFO_EXTENSION ); | |
| $valid_image_file_extensions = array( 'jpg', 'jpeg', 'png', 'gif', 'webp', 'avif', 'heic', 'heif', 'svg' ); | |
| if ( '' !== $file_extension && in_array( strtolower( $file_extension ), $valid_image_file_extensions, true ) ) { | |
| return true; | |
| } | |
| } | |
| $content_type = (array) wp_remote_retrieve_header( $r, 'content-type' ); | |
| if ( 'application/octet-stream' === $content_type[0] ) { | |
| // This is a special case for images that are not served with the correct Content-Type. | |
| $file_extension = pathinfo( (string) wp_parse_url( $url, PHP_URL_PATH ), PATHINFO_EXTENSION ); | |
| $valid_image_file_extensions = array( 'jpg', 'jpeg', 'png', 'gif', 'webp', 'avif', 'heic', 'heif', 'svg' ); | |
| $is_valid_content_type = in_array( strtolower( $file_extension ), $valid_image_file_extensions, true ); | |
| } else { | |
| $is_valid_content_type = is_string( $content_type[0] ) && str_starts_with( $content_type[0], 'image/' ); | |
| } | |
| if ( ! $is_valid_content_type ) { |
Co-authored-by: Weston Ruter <[email protected]>
westonruter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
application/octet-stream
Summary
Fixes #1952
Relevant technical choices
This PR addresses an issue where the Image Prioritizer plugin fails to validate background image URLs when the server returns a Content-Type of
application/octet-streaminstead of an expectedimage/*Content-Type.