Make WordPress Core

#56855 closed defect (bug) (fixed)

Featured Image bug in 6.0.3

Reported by: raduiason's profile raduiason Owned by: davidbaumwald's profile davidbaumwald
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)

56855.diff (721 bytes) - added by pbiron 18 months ago.
56855.1.diff (1.4 KB) - added by pbiron 18 months ago.
56855.2.diff (939 bytes) - added by SergeyBiryukov 18 months ago.
56855-docs.diff (751 bytes) - added by peterwilsoncc 18 months ago.

Download all attachments as: .zip

Change History (53)

This ticket was mentioned in Slack in #core by pbiron. View the logs.


18 months ago

#2 @audrasjb
18 months ago

  • Milestone changed from Awaiting Review to 6.0.4

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.

#3 @audrasjb
18 months ago

  • Milestone changed from 6.0.4 to 6.1

Moving to 6.1 as this might come first :)

@pbiron
18 months ago

#4 follow-up: @pbiron
18 months 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.

Last edited 18 months ago by pbiron (previous) (diff)

This ticket was mentioned in Slack in #core by pbiron. View the logs.


18 months ago

@pbiron
18 months ago

#6 @pbiron
18 months ago

56855.1.diff adds a test case for object-fit to the dataProvider for the test_safecss_filter_attr() unit test.

#7 @pbiron
18 months ago

  • Keywords has-patch needs-testing added

#8 follow-up: @kebbet
18 months ago

I can not reproduce this issue in trunk, following instructions in ticket description.

Last edited 18 months ago by kebbet (previous) (diff)

#9 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
18 months 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: @Bernhard Reiter
18 months 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: @Bernhard Reiter
18 months 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: @Bernhard Reiter
18 months 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 @pbiron
18 months 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.


18 months ago

This ticket was mentioned in Slack in #core by bernhardreiter. View the logs.


18 months ago

This ticket was mentioned in Slack in #core-test by bernhardreiter. View the logs.


18 months ago

#17 @ironprogrammer
18 months ago

  • Keywords has-testing-info has-unit-tests added

Test Report

Steps to Reproduce

  1. SETUP: Apply setup patch from comment:12. This is required to apply the safecss_filter_attr() fix from 6.0.3.
  2. Create a new post.
  3. Insert a Post Featured Image block, add an image to the block (should be taller than 100px).
  4. Set the height to 100px and scale to "Cover".
  5. Publish post and view the new post.
  6. 🐞 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

https://cldup.com/l1HVtxKGjy.png
Figure 1: Reproducing error.

https://cldup.com/ACg2o2cs-o.png
Figure 2: After patch.

#18 in reply to: ↑ 11 @xknown
18 months 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.

#19 @pbiron
18 months ago

#56881 was marked as a duplicate.

#20 @audrasjb
18 months 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.

#21 @SergeyBiryukov
18 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to commit and backport.

56855.2.diff is a refresh after [54100], [54102], [54117], and [54667].

#22 @ivanjeronimo
18 months ago

#56888 was marked as a duplicate.

#23 @seriouslysenpai
18 months ago

#56890 was marked as a duplicate.

#24 @davidbaumwald
18 months ago

  • Keywords commit added
  • Owner set to davidbaumwald
  • Status changed from new to assigned

#25 @davidbaumwald
18 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 54675:

Media: Add object-fit to the allowed list of CSS properties.

This resolves a bug in Featured Image blocks where object-fit was being removed during the render_callback.

Props raduiason, pbiron, kebbet, SergeyBiryukov, bernhard-reiter, ironprogrammer, xknown, audrasjb, ckanderson22, ivanjeronimo, seriouslysenpai.
Fixes #56855.

#26 follow-up: @davidbaumwald
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to the 6.1 branch.

#27 in reply to: ↑ 26 @pbiron
18 months 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 @davidbaumwald
18 months 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?

#29 @davidbaumwald
18 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 54676:

Media: Add object-fit to the allowed list of CSS properties.

This resolves a bug in Featured Image blocks where object-fit was being removed during the render_callback.

Props raduiason, pbiron, kebbet, SergeyBiryukov, bernhard-reiter, ironprogrammer, xknown, audrasjb, ckanderson22, ivanjeronimo, seriouslysenpai.
Reviewed by SergeyBiryukov.
Merges [54675] to the 6.1 branch.
Fixes #56855.

#30 @davidbaumwald
18 months ago

  • Keywords dev-reviewed commit removed

Resetting the keywords after the backport.

#31 follow-up: @desrosj
18 months 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 @SergeyBiryukov
18 months 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 @peterwilsoncc
18 months ago

In 56855-docs.diff I've noted the introduction of object-fit in the 6.1.0 section of the docblock.

#34 @SergeyBiryukov
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 56855-docs.diff.

#35 @SergeyBiryukov
18 months ago

In 54698:

Docs: Add a @since note for object-fit support in safecss_filter_attr().

Follow-up to [54675].

Props peterwilsoncc.
See #56855.

#36 @SergeyBiryukov
18 months ago

  • Keywords dev-feedback added

Marking [54698] for backporting to the 6.1 branch.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


18 months ago

#38 @davidbaumwald
18 months ago

  • Keywords commit dev-reviewed added; dev-feedback removed

Looks good!

#39 @desrosj
18 months ago

In 54700:

Docs: Add a @since note for object-fit support in safecss_filter_attr().

Follow-up to [54675].

Props peterwilsoncc, SergeyBiryukov, davidbaumwald.
Merges [54698] to the 6.1 branch.
See #56855.

#40 @desrosj
18 months ago

  • Keywords fixed-major added; commit dev-reviewed removed

Resetting action-based keywords, and adding fixed-major to indicate this needs further backporting.

#41 @kebbet
18 months ago

#56925 was marked as a duplicate.

#42 @SergeyBiryukov
18 months ago

  • Milestone changed from 6.1 to 6.2

Moving to 6.2 for further backports, as the 6.1 milestone is closed.

#43 @SergeyBiryukov
18 months ago

  • Milestone changed from 6.2 to 6.1.1

Or perhaps 6.1.1 is more appropriate here.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


18 months ago

#45 @peterwilsoncc
18 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 54763:

Media: Add object-fit to the allowed list of CSS properties.

This resolves a bug in Featured Image blocks where object-fit was being removed during the render_callback.

Props raduiason, pbiron, kebbet, SergeyBiryukov, bernhard-reiter, ironprogrammer, xknown, audrasjb, ckanderson22, ivanjeronimo, seriouslysenpai, davidbaumwald.
Merges [54675] to the 6.0 branch.
Fixes #56855.

#46 @peterwilsoncc
18 months 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 @TobiasBg
18 months 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

Last edited 18 months ago by TobiasBg (previous) (diff)

#48 follow-up: @SergeyBiryukov
18 months 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 @desrosj
18 months 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.

Note: See TracTickets for help on using tickets.