Make WordPress Core

Opened 16 months ago

Last modified 9 months ago

#57979 new defect (bug)

Can't upload images to WordPress Comments

Reported by: sbb's profile sbb Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 6.0.3
Component: Comments Keywords: has-patch 2nd-opinion dev-feedback needs-testing changes-requested early
Focuses: administration Cc:

Description

As the admin, I am unable to upload images from my image library to a WordPress comment posted by a user. Please Note: I can upload images to my own comments, but not a user-generated comment. On the admin page, I edit a user comment, click IMG button, add the image URL, and the correct code is added to the comment. When I click UPDATE, the image code disappears. Please note that all existing images in Comments display properly. This is a new problem. Theme is Genesis Magazine Pro. I tried: deactivating all plugins, multiple browsers, multiple operating systems (PC and Mac), and multiple computers. Also contacted my web host, WP-Engine, who has had other reports of this problem and believes it is a WordPress issue. Site is buildingadvisor.com. Thank you!

Attachments (4)

57979.diff (553 bytes) - added by khokansardar 16 months ago.
Capture d’écran 2023-04-20 à 11.48.02.png (103.9 KB) - added by lphoumpakka 15 months ago.
Result after testing the patch 57979.diff
57979.1.diff (576 bytes) - added by khokansardar 10 months ago.
57979.2.diff (553 bytes) - added by khokansardar 9 months ago.

Download all attachments as: .zip

Change History (30)

#1 @azaozz
16 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Version changed from 6.1.1 to 6.0.3

@sbb Welcome to Trac and thanks for the bug report.

Seems this is a regression introduced in [54527]. Comments edited by users with unfiltered_html (admins or editors) should not be run through KSES.

Looking at the changeset, it checks if the comment author has unfiltered_html, but doesn't check cases where an admin may be editing the comment. This prevents admins and editors from using their capabilities there.

Last edited 16 months ago by azaozz (previous) (diff)

#2 @sbb
16 months ago

Thanks. So does that make this a feature or a bug that should be fixed? In the meantime, can you suggest a workaround so the admin can add images to user-posted Comments? Many thanks, Steve

This ticket was mentioned in PR #4265 on WordPress/wordpress-develop by @khokansardar.


16 months ago
#3

  • Keywords has-patch added

Trac ticket: 57979

@khokansardar
16 months ago

#4 @khokansardar
16 months ago

@azaozz I have added a patch by adding

current_user_can( 'unfiltered_html' )

Please have a look.

Last edited 16 months ago by khokansardar (previous) (diff)

#5 follow-up: @sbb
15 months ago

Hello and many thanks for the patches. I have tried adding the patch in 57979.diff and also the latest patch as "Additional CSS" in the WordPress theme customizer. However, it did not solve the problem. Still unable to load images to Comments posted by users. Any other suggestions? Should the patch be added to functions.php instead. Sorry, but I am not a programmer...

#6 in reply to: ↑ 5 @khokansardar
15 months ago

Replying to sbb:

Hello and many thanks for the patches. I have tried adding the patch in 57979.diff and also the latest patch as "Additional CSS" in the WordPress theme customizer. However, it did not solve the problem. Still unable to load images to Comments posted by users. Any other suggestions? Should the patch be added to functions.php instead. Sorry, but I am not a programmer...

This is not the process to apply the patches. All these are core tickets, so you have to apply respective patch to respective files. For this one you have to change the line of this file path -

/wp-includes/comment.php src/wp-includes/comment.php

as mentioned in .diff file of this ticket and then you can do the check.

#7 @lphoumpakka
15 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/57979/57979.diff

Environment

  • OS: macOS 13.2.1
  • Web Server: Nginx
  • PHP: 7.4.27
  • WordPress: 6.3-alpha-55505-src
  • Browser: Chrome 112.0.5615.137
  • Theme: twentytwentythree
  • Active Plugins:

Actual Results

  • ✅ Issue resolved with patch. A administrator can upload image in another user's comment.

@lphoumpakka
15 months ago

Result after testing the patch 57979.diff

#8 @azaozz
15 months ago

  • Keywords 2nd-opinion added

Not sure if that's the best patch here. This was a security fix, need to make sure the initial bug is not reintroduced.

Last edited 15 months ago by azaozz (previous) (diff)

#9 @oglekler
13 months ago

  • Milestone changed from 6.3 to 6.4

Because there are doubts about security and tickets had no activities in 2 months, I am moving this into the 6.4 milestone.

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


10 months ago

#11 @oglekler
10 months ago

  • Keywords dev-feedback added

This ticket was discussed during a bug scrub, and it looks like the patch is solving the issue, but this restriction with checking user ID with commenter user ID was also there for a reason, so, let's have dev feedback about this.

Add props to @mukesh27

#12 @audrasjb
10 months ago

  • Keywords changes-requested added

Hello and thanks for the patch.

The proposed patch probably reintroduces the security issue fixed in [54527]. It would be better to add a conditional to check whether the user is an admin or not.

#13 @khokansardar
10 months ago

  • Keywords needs-testing added; changes-requested removed

@audrasjb I have updated the patch here - 57979.1.diff please check.

#14 @devmuhib
10 months ago

Test Report

Tested the latest patch 57979.1.diff​ and it solves the issue. When I first tried, I could not add any image in user comments. After applying the patch, I added the image in user's comment.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/57979/57979.1.diff

Environment

OS: Window 10
Web Server: nginx/1.25.2
PHP: 7.4.33
WordPress: 6.4-alpha-56267-src
Browser: Google Chrome
Theme: Twenty-Twenty-Three

Actual Results

  • ✅ Issue resolved with patch.

Supplemental Artifacts

Screenshot: https://i.imgur.com/Q8AbKVe.png

#16 @SergeyBiryukov
9 months ago

  • Keywords changes-requested added

Thanks for the patch!

Checking for current_user_can( 'administrator' ) does not seem ideal here, as this does not account for custom roles, see a note in current_user_can() documentation:

While checking against particular roles in place of a capability is supported in part, this practice is discouraged as it may produce unreliable results.

I think current_user_can( 'unfiltered_html' ) should be used instead:

if ( ! current_user_can( 'unfiltered_html' ) && ! has_filter( 'pre_comment_content', 'wp_filter_kses' ) ) {
	$filter_comment = ! user_can( isset( $comment['user_id'] ) ? $comment['user_id'] : 0, 'unfiltered_html' );
}

If I understand the issue correctly, the comment author's capabilities should only be checked if the current user does not have unfiltered_html.

#17 @khokansardar
9 months ago

  • Keywords changes-requested removed

@SergeyBiryukov I have updated PR by reverting changes with my initail commit. Initially I have done the same as you suggested. But changes those as per commented feedback. Anyway please check the updated PR - https://github.com/WordPress/wordpress-develop/pull/4265

Here is the diff. - https://core.trac.wordpress.org/attachment/ticket/57979/57979.2.diff

#18 @oglekler
9 months ago

I think the latest PR makes sense. Because we need to check the rights not of the comment user, but of the current user who is editing the comment.

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


9 months ago

#20 @priyanshii5
9 months ago

Test Report

I just tested the latest patch provided by @khokansardar.

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/57979/57979.2.diff

Environment

OS: macOS 12.5
Web Server: Docker
PHP: 8.0.28
WordPress: 6.4-alpha-56267-src
Browser: Google Chrome Version 115.0.5790.114 (Official Build) (arm64)
Theme: Twenty Nineteen

Actual Results

I saw progress after applying the patch.

Dashboard Before Applying Patch:
https://i.imgur.com/3JTvOxd.png

Dashboard After Applying Patch:
https://i.imgur.com/ts2fhFx.png

#21 @nicolefurlan
9 months ago

Wonderful! Looks like we have a passing test report. Does this seem ready for commit?

Pinging you @davidbaumwald as the component maintainer :)

This ticket was mentioned in PR #5485 on WordPress/wordpress-develop by @davidbaumwald.


9 months ago
#22

#23 follow-up: @davidbaumwald
9 months ago

@nicolefurlan Thanks for the ping! I actually don't think the latest patch is quite ready yet. The unit tests are failing on the 4265 PR and I don't think it's the right approach. Sergey's suggestion is more appropriate so that the commenter's capabilities are only checked if the current user's capes don't include unfiltered_html.

I created PR #5485 to get that approach working, but we might need to rework the tests a bit.

@SergeyBiryukov do you think that the expected result in test_update_comment_from_unprivileged_user_by_privileged_user should actually be reversed with this change? If we allow a privileged user to post unfiltered HTML as proposed here, this test would need to be adjusted to check that disallowed=attribute is included in the expected result. Thoughts?

#24 in reply to: ↑ 23 @SergeyBiryukov
9 months ago

Replying to davidbaumwald:

do you think that the expected result in test_update_comment_from_unprivileged_user_by_privileged_user should actually be reversed with this change? If we allow a privileged user to post unfiltered HTML as proposed here, this test would need to be adjusted to check that disallowed=attribute is included in the expected result.

Yes, that sounds right.

#25 @peterwilsoncc
9 months ago

  • Keywords changes-requested early added
  • Milestone changed from 6.4 to Future Release

I'm very, very reluctant to make this change.

Without going in to too many details, each of the PRs linked to the ticket will reintroduce the issue [54527] resolved. As it's been 10 months since the security issue was resolved, it's probably fine to introduce the some obvious tests illustrating the problem, but I'll need to find them.

As this issue relates to a prior security flaw, I'm going to move this off the current milestone as there's no suitable patch at this stage. I've added the early label so the security team can monitor and test any changes.


To allow images in comments, I suggest the tag be added via a filter:

<?php
add_filter(
        'wp_kses_allowed_html',
        function( $allowed_html ) {
                if ( isset( $allowed_html['img'] ) ) {
                        // Nothing to do.
                        return $allowed_html;
                }

                /* START: Only allow in admin */
                if ( ! function_exists( 'get_current_screen' ) ) {
                        return $allowed_html;
                }

                $current_screen = get_current_screen();
                if ( ! $current_screen || 'edit-comments' !== $current_screen->parent_base ) {
                        return $allowed_html;
                }
                /* END: Only allow in admin */

                $allowed_html['img'] = array(
                        'alt'      => true,
                        'align'    => true,
                        'border'   => true,
                        'height'   => true,
                        'hspace'   => true,
                        'loading'  => true,
                        'longdesc' => true,
                        'vspace'   => true,
                        'src'      => true,
                        'usemap'   => true,
                        'width'    => true,
                );

                return $allowed_html;
        }
);

#26 @nicolefurlan
9 months ago

Thank you everyone for your input on this!

Note: See TracTickets for help on using tickets.