Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#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 2 years ago.
56855.1.diff (1.4 KB) - added by pbiron 2 years ago.
56855.2.diff (939 bytes) - added by SergeyBiryukov 2 years ago.
56855-docs.diff (751 bytes) - added by peterwilsoncc 2 years ago.

Download all attachments as: .zip

Change History (53)

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


2 years ago

#2 @audrasjb
2 years 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
2 years ago

  • Milestone changed from 6.0.4 to 6.1

Moving to 6.1 as this might come first :)

@pbiron
2 years ago

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

Last edited 2 years ago by pbiron (previous) (diff)

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


2 years ago

@pbiron
2 years ago

#6 @pbiron
2 years ago

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

#7 @pbiron
2 years ago

  • Keywords has-patch needs-testing added

#8 follow-up: @kebbet
2 years ago

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

Last edited 2 years ago by kebbet (previous) (diff)

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

#19 @pbiron
2 years ago

#56881 was marked as a duplicate.

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

#21 @SergeyBiryukov
2 years 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
2 years ago

#56888 was marked as a duplicate.

#23 @seriouslysenpai
2 years ago

#56890 was marked as a duplicate.

#24 @davidbaumwald
2 years ago

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

#25 @davidbaumwald
2 years 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
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 @pbiron
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 @davidbaumwald
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?

#29 @davidbaumwald
2 years 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
2 years ago

  • Keywords dev-reviewed commit removed

Resetting the keywords after the backport.

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 56855-docs.diff.

#35 @SergeyBiryukov
2 years 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
2 years 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.


2 years ago

#38 @davidbaumwald
2 years ago

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

Looks good!

#39 @desrosj
2 years 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
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.

#41 @kebbet
2 years ago

#56925 was marked as a duplicate.

#42 @SergeyBiryukov
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.

#43 @SergeyBiryukov
2 years 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.


2 years ago

#45 @peterwilsoncc
2 years 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
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 @TobiasBg
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

Last edited 2 years ago by TobiasBg (previous) (diff)

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

Note: See TracTickets for help on using tickets.