Opened 2 years ago
Closed 2 years ago
#56855 closed defect (bug) (fixed)
Featured Image bug in 6.0.3
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.1.1 | Priority: | normal |
Severity: | minor | Version: | 6.0.3 |
Component: | Media | Keywords: | has-patch has-testing-info has-unit-tests fixed-major |
Focuses: | Cc: |
Description
Hi,
after the 6.0.3 update, the Featured Image block shows some bugs in respect to image sizing. regardless of your choice of a “Scale” option, it will always behave as if the selected option is “Fill”.
AFAIK this only happens in blocks inserted in the page, not default blocks, built in the themes. It also seems to happen in hooks.
This also happens on clean fresh installs.
To replicate: create a new post, insert a Featured Image in it, set the height at 100px and the scale as “Cover”. provided the image is taller than 100px, it will be stretched in the preview and the live post.
To fix: reverting to 6.0.2 seems to have fixed it. i suppose inserting some object-fit CSS should work as well, did not try it.
thanks,
R
PS i also created a forum topic before actually reading the "read this" sticky :| https://wordpress.org/support/topic/featured-image-bug-in-6-0-3/
Attachments (4)
Change History (53)
This ticket was mentioned in Slack in #core by pbiron. View the logs.
2 years ago
#4
follow-up:
↓ 11
@
2 years ago
The problem reported here seems to be that the security fixes in 6.0.3 added a call to safecss_filter_attr() in the render callback for the featured image block and that the object-fit
CSS property is stripped out by safecss_filter_attr()
.
56855.diff adds object-fit
to the list of allowed CSS properties, so that safecss_filter_attr()
no longer strips it out.
As the the addition of the call to safecss_filter_attr()
was added for security reasons, the WP security team should double check the patch to make sure it doesn't open any vulnerabilities.
This ticket was mentioned in Slack in #core by pbiron. View the logs.
2 years ago
#6
@
2 years ago
56855.1.diff adds a test case for object-fit
to the dataProvider for the test_safecss_filter_attr()
unit test.
#8
follow-up:
↓ 9
@
2 years ago
I can not reproduce this issue in trunk, following instructions in ticket description.
#9
in reply to:
↑ 8
;
follow-up:
↓ 10
@
2 years ago
Replying to kebbet:
I can not reproduce this issue in trunk, following instructions in ticket description.
It looks like the associated change for the Featured Image block has not been merged to trunk yet.
I see six blocks updated for the 6.0 branch in [54543], but only five in the PR for the Gutenberg plugin, which then was merged for 6.1 RC2 as part of other package updates in [54632].
#10
in reply to:
↑ 9
;
follow-up:
↓ 12
@
2 years ago
Replying to SergeyBiryukov:
It looks like the associated change for the Featured Image block has not been merged to trunk yet.
I see six blocks updated for the 6.0 branch in [54543], but only five in the PR for the Gutenberg plugin, which then was merged for 6.1 RC2 as part of other package updates in [54632].
FWIW, I've now filed a PR against Gutenberg to carry over that change. (We need it to escape that output; we'll then need to solve this issue here separately -- likely via the patches that folks have already provided.)
#11
in reply to:
↑ 4
;
follow-up:
↓ 18
@
2 years ago
Replying to pbiron:
As the the addition of the call to
safecss_filter_attr()
was added for security reasons, the WP security team should double check the patch to make sure it doesn't open any vulnerabilities.
@xknown Would you mind giving this a look? :)
#12
in reply to:
↑ 10
;
follow-up:
↓ 13
@
2 years ago
Replying to Bernhard Reiter:
FWIW, I've now filed a PR against Gutenberg to carry over that change. (We need it to escape that output; we'll then need to solve this issue here separately -- likely via the patches that folks have already provided.)
Note that that change will be part of WP 6.1 RC3 next Tuesday. To simulate the impact already, apply the following patch to trunk
:
diff --git a/src/wp-includes/blocks/post-featured-image.php b/src/wp-includes/blocks/post-featured-image.php index de5683b297..495c8ec534 100644 --- a/src/wp-includes/blocks/post-featured-image.php +++ b/src/wp-includes/blocks/post-featured-image.php @@ -64,7 +64,7 @@ function render_block_core_post_featured_image( $attributes, $content, $block ) if ( ! empty( $attributes['scale'] ) ) { $image_styles .= "object-fit:{$attributes['scale']};"; } - $featured_image = str_replace( 'src=', 'style="' . esc_attr( $image_styles ) . '" src=', $featured_image ); + $featured_image = str_replace( '<img ', '<img style="' . esc_attr( safecss_filter_attr( $image_styles ) ) . '" ', $featured_image ); } return "<figure {$wrapper_attributes}>{$featured_image}</figure>";
#13
in reply to:
↑ 12
@
2 years ago
Replying to Bernhard Reiter:
Note that that change will be part of WP 6.1 RC3 next Tuesday. To simulate the impact already, apply the following patch to
trunk
:
After applying that patch, you should see the behavior reported in this ticket. If you then apply 56855.1.diff, you should see the corrected behavior.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by bernhardreiter. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-test by bernhardreiter. View the logs.
2 years ago
#17
@
2 years ago
- Keywords has-testing-info has-unit-tests added
Test Report
Steps to Reproduce
- SETUP: Apply setup patch from comment:12. This is required to apply the
safecss_filter_attr()
fix from 6.0.3. - Create a new post.
- Insert a Post Featured Image block, add an image to the block (should be taller than 100px).
- Set the height to 100px and scale to "Cover".
- Publish post and view the new post.
- 🐞 Observe that the image in the block is stretched in the frontend (and preview).
Expected Results
- ✅ Block's image in frontend is set to 100x100px, instead of "object-fit: cover". (See Figure 1.)
Patch tested: attachment:56855.1.diff 👍🏻
Environment
- Hardware: MacBook Pro Apple M1 Pro
- OS: macOS 12.6
- Browser: Safari 16.0
- Server: nginx/1.23.1
- PHP: 7.4.32
- WordPress: 6.2-alpha-54642-src
- Theme: twentytwentythree v1.0
Actual Results
- ✅ Block's image in frontend is not stretched, and properly reflects "object-fit: cover". (See Figure 2.)
Supplemental Artifacts
#18
in reply to:
↑ 11
@
2 years ago
Replying to Bernhard Reiter:
Replying to pbiron:
As the the addition of the call to
safecss_filter_attr()
was added for security reasons, the WP security team should double check the patch to make sure it doesn't open any vulnerabilities.
@xknown Would you mind giving this a look? :)
The change to safecss_filter_attr
seems fine to me.
#20
@
2 years ago
- Keywords dev-feedback added; needs-testing removed
[double committer signoff needed]
I tested the patch and it works fine on my side (it adds object-fit: cover
style attribute).
Waiting for another committer review before commit
and backport to branch 6.1.
#24
@
2 years ago
- Keywords commit added
- Owner set to davidbaumwald
- Status changed from new to assigned
#26
follow-up:
↓ 27
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for backport to the 6.1 branch.
#27
in reply to:
↑ 26
@
2 years ago
Replying to davidbaumwald:
Reopening for backport to the 6.1 branch.
I think this should also be backported to 6.0 and all other previous branches that [54543] was backported to.
#28
@
2 years ago
@pbiron Agree. I think this would happen during the next minor cycle.
@SergeyBiryukov How do you think we should handle backporting this fix to the other branches affected by the addition of safecss_filter_attr()
during the most recent minor release? Should a dedicated ticket be opened for tracking that, or could we just tag this ticket for 6.1.1?
#31
follow-up:
↓ 32
@
2 years ago
I think this ticket should remain in the current milestone because there are associated changes being released with 6.1.
This scenario is often one that's an in between.
Unless there's a ticket in a milestone representing the change, it's likely it will be forgotten during a release when writing out release posts, updating About pages, etc..
Personally, I think that we could just backport this to any affected branches now (or shortly after 6.1 is released) and attach the commits to this ticket. It's possible the older branches won't be released in coordination with 6.1.1, so a new ticket in that milestone may not be correct either.
I don't think there's a hard rule, but I feel recently we've been using the "backport and just let it sit waiting for a future security release".
#32
in reply to:
↑ 31
@
2 years ago
Replying to desrosj:
Personally, I think that we could just backport this to any affected branches now (or shortly after 6.1 is released) and attach the commits to this ticket.
That would be my suggestion too, the fix will then ship with the next security release for those branches.
#33
@
2 years ago
In 56855-docs.diff I've noted the introduction of object-fit in the 6.1.0 section of the docblock.
#34
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for 56855-docs.diff.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
2 years ago
#40
@
2 years ago
- Keywords fixed-major added; commit dev-reviewed removed
Resetting action-based keywords, and adding fixed-major
to indicate this needs further backporting.
#42
@
2 years ago
- Milestone changed from 6.1 to 6.2
Moving to 6.2 for further backports, as the 6.1 milestone is closed.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
2 years ago
#46
@
2 years ago
[54675] has been merged to the 5.9 and 6.0 branches, although the 5.9 commit is not showing at the time of writing.
Although this isn't a security issue, the bug was introduced to the earlier branches as part of a security release so a fix will be included as a courtesy in the next releases for the 5.9 and 6.0 branches. The timing of these releases is not yet known.
#47
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
After [54675] (and that other commit for 5.9, I guess), the docblock needs to be adjusted (in 5.9, 6.0, 6.1, and trunk), as object-fit
has then technically been added before 6.1, compare https://core.trac.wordpress.org/browser/branches/6.1/src/wp-includes/kses.php?rev=54763#L2232
#48
follow-up:
↓ 49
@
2 years ago
Hmm, while it could probably say @since 5.9.6
, that might give an impression that object-fit
is also available in 6.0, which is not the case before 6.0.4.
@since 6.0.4
would be another option, but I think it's fine to keep @since 6.1.0
in this case. I believe @since
tags for security-related backports generally mention the latest relevant version, not the earliest.
#49
in reply to:
↑ 48
@
2 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Replying to SergeyBiryukov:
I believe
@since
tags for security-related backports generally mention the latest relevant version, not the earliest.
That is my understanding as well. It may be worth adding a note about this within the inline documentation standards.
I'm going to close this out again since it seems there's nothing left to do.
Hello, welcome to WordPress Core Trac and thank you for reporting this.
As per today's devchat, it appears your issue was reproduced at least by @pbiron.
Moving to the next minor release milestone.