WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16611 closed defect (bug) (fixed)

Too few arguments to sprintf in WP_List_Table::comments_bubble()

Reported by: nacin Owned by:
Milestone: 3.1 Priority: high
Severity: normal Version: 3.1
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

Attachments (5)

16611.patch (1.3 KB) - added by SergeyBiryukov 5 years ago.
16611.diff (1.4 KB) - added by nacin 5 years ago.
16611.2.diff (1.1 KB) - added by nacin 5 years ago.
16611.2.patch (948 bytes) - added by SergeyBiryukov 5 years ago.
16611.3.patch (921 bytes) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 @SergeyBiryukov5 years ago

Can reproduce with non-English characters in siteurl. The link becomes something like this:

<a href='...%d1%82%d0%b5%d1%81%d1%82/wp-admin/edit-comments.php?p=4' title='0 pending' class='post-com-count'><span class='comment-count'>%s</span></a>

@SergeyBiryukov5 years ago

comment:2 @SergeyBiryukov5 years ago

  • Keywords has-patch added

comment:3 @scribu5 years ago

  • Keywords commit added
  • Version set to 3.1

Ah, right, because sprintf() thinks those encoded chars are placeholders. Makes sense now.

comment:4 @nacin5 years ago

  • Priority changed from normal to high

$pending_phrase also needs to be escaped.

@nacin5 years ago

comment:5 @nacin5 years ago

  • Keywords commit removed

The URL should also be escaped.

Regardless, patch leaves an issue, due to the nature of comments_number(). If comments > 1, then comments_number() will do an str_replace() on the %. This borks the URL.

comment:6 @nacin5 years ago

Proper function here is get_comments_number(), wrapped with number_format_i18n(). Anything else is extra we don't need or leverage, due to the arguments we pass comments_number().

@nacin5 years ago

comment:7 @nacin5 years ago

New patch.

comment:8 @nacin5 years ago

  • Keywords commit added

I was wondering why we were using comments_number() like this.

In 3.0, we had code that looked like the red in r15504 -- basically, massive strings going into comments_number(). We also didn't wrap the link in admin_url(), thus we had no extra percentages to screw things up. sprintf() plus admin_url(), two pretty innocuous changes, caused the regression.

The smarter structuring in r15504 in turn revealed that get_comments_number() is a better approach.

comment:9 @ryan5 years ago

Looks good, tests good. Ship it.

comment:10 @nacin5 years ago

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

(In [17475]) Use get_comments_number() in comments_bubble() method. Removes chance of sprintf arguments error due to percent encoding in URLs and kills unnecessary translations. Escape translations into attributes. esc_url on admin_url. fixes #16611 for trunk.

comment:11 @nacin5 years ago

(In [17476]) Use get_comments_number() in comments_bubble() method. Removes chance of sprintf arguments error due to percent encoding in URLs and kills unnecessary translations. Escape translations into attributes. esc_url on admin_url. fixes #16611 for the 3.1 branch.

@SergeyBiryukov5 years ago

comment:12 @SergeyBiryukov5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

16611.2.diff doesn't fix the initial issue. printf() throws the same error message, as there are still encoded chars in the URL.

Added new patch.

comment:13 @duck_5 years ago

Do we even need the printf? Why not just string concatenation?

@SergeyBiryukov5 years ago

comment:14 @SergeyBiryukov5 years ago

Added patch with string concatenation.

comment:15 @nacin5 years ago

I was going to cycle back around and just string concatenate this, but decided to leave it for 3.2. Didn't see the extra bug in my testing.

Looks good. Commit and ship.

comment:16 @nacin5 years ago

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

(In [17481]) Avoid printf entirely. props SergeyBiryukov, fixes #16611 for trunk.

comment:17 @nacin5 years ago

(In [17482]) Avoid printf entirely. props SergeyBiryukov, fixes #16611 for 3.1.

Note: See TracTickets for help on using tickets.