WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#33235 closed defect (bug) (fixed)

Drop strip_tags() for widget titles in forms

Reported by: greenshady Owned by:
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Widgets Keywords:
Focuses: Cc:

Description

I was putting together a quick plugin to allow some basic HTML in widget titles. I hit a snag when I realized that core handles the output of the title field inconsistently in widget forms.

Basically, a few widgets run the title field through strip_tags() before outputting the <input> field. All we really need is esc_attr() in this case.

Uses strip_tags() + esc_attr()

  • Archives
  • Meta
  • Calendar
  • Text

Uses esc_attr() only

  • Pages
  • Search
  • Categories
  • Recent Posts
  • Recent Comments
  • Tag Cloud
  • Nav Menu

I'm adding a patch so that these are treated consistently and simply escaped with esc_attr(). If we want to keep the strip_tags(), it should be done the same across the board.

Plugin for testing: https://github.com/justintadlock/widget-title-html

Attachments (1)

default-widgets.php.diff (1.9 KB) - added by greenshady 5 years ago.

Download all attachments as: .zip

Change History (10)

#1 @welcher
5 years ago

Related #23012.

#2 follow-up: @westonruter
5 years ago

I think strip_tags() is perhaps a legacy option where a newer more appropriate sanitizing function sanitize_text_field() is available now which strips tags in addition to doing a lot more, like trimming whitespace and ensuring valid encoding. In any case, we shouldn't be using esc_attr() for sanitizing input anyway.

#3 in reply to: ↑ 2 @greenshady
5 years ago

Replying to westonruter:

I think strip_tags() is perhaps a legacy option where a newer more appropriate sanitizing function sanitize_text_field() is available now which strips tags in addition to doing a lot more, like trimming whitespace and ensuring valid encoding. In any case, we shouldn't be using esc_attr() for sanitizing input anyway.

This is not about sanitizing input. It's about escaping output.

#4 @westonruter
5 years ago

Ah, right. I missed that your patch was for the form callback, and not the update callback.

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


5 years ago

#6 @obenland
5 years ago

  • Version trunk deleted

#7 follow-up: @welcher
5 years ago

  • Keywords dev-feedback 2nd-opinion added

I think this was addressed with 33814.

#8 in reply to: ↑ 7 @greenshady
5 years ago

  • Keywords close added; dev-feedback 2nd-opinion removed

Replying to welcher:

I think this was addressed with 33814.

Yes, it was.

#9 @DrewAPicture
5 years ago

  • Keywords close removed
  • Milestone changed from Awaiting Review to 4.4
  • Resolution set to fixed
  • Status changed from new to closed

See [33814].

Note: See TracTickets for help on using tickets.