Make WordPress Core

Opened 10 years ago

Closed 8 years ago

#27942 closed defect (bug) (fixed)

Site Title not escaped when using HTML entities

Reported by: bandonrandon's profile BandonRandon Owned by: kovshenin's profile kovshenin
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.9
Component: Options, Meta APIs Keywords: has-patch has-unit-tests commit
Focuses: administration Cc:

Description

Today I tried to set my site title to <Brooke><Codes> and learned that the site title field escapes the < character. This is fine I thought, I'll use HTML entities. While this worked at first this also failed in the end. Is there a way to use the same escaping used on posts titles on site titles?

Here are a few screenshots to show the problem:

Title using HTML entities:
https://i.cloudup.com/ct_yH6t2er-3000x3000.png

After saving the above:
https://i.cloudup.com/k2-OCiyDA7-3000x3000.png

After saving the above:
https://i.cloudup.com/ImnHqRiPJJ-3000x3000.png

To me the real issue is that it fails silently. I know enough to know why it fails but to many of our users they may have a WTF reaction.

Attachments (6)

27942.patch (415 bytes) - added by BandonRandon 10 years ago.
27942.2.patch (417 bytes) - added by BandonRandon 10 years ago.
Add spacing around attribute
unit-tests.27942.diff (978 bytes) - added by pauldewouters 10 years ago.
unit tests for sanitize_option
27942.3.patch (398 bytes) - added by BandonRandon 8 years ago.
27942.4.patch (1.5 KB) - added by BandonRandon 8 years ago.
Patch With Unit Test
27942.5.patch (2.0 KB) - added by kovshenin 8 years ago.

Download all attachments as: .zip

Change History (19)

#1 @wonderboymusic
10 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#2 @BandonRandon
10 years ago

  • Keywords has-patch added; needs-patch removed

I dung into this some more and learned that you are able to use valid HTML in the site title. For example if you decided to use <em>Site</em> <strong> Title</strong> the title will save and output correctly.

It looks like this problem is only occurring when using invalid HTML.

https://i.cloudup.com/q6Gqt8Je-n-2000x2000.png

I have submitted a patch that switches from using wp_kses_post to htmlentities2 in formatting.php resolving this issue.

@BandonRandon
10 years ago

#3 @Clorith
10 years ago

  • Focuses administration added
  • Keywords needs-unit-tests added
  • Version changed from trunk to 2.9

Seems like a simple fix, should probably update the patch to space out the attribute to conform with the coding standards.

I did test a few versions back, and in 2.0 when sanitize_option was introduced this was not a problem, but in 2.9 it does happen, so somewhere along that road a regression occurred that was missed.

We should probably also have unit tests for sanitize_option when considering all the functions the various cases relies on.

@BandonRandon
10 years ago

Add spacing around attribute

#4 @BandonRandon
10 years ago

Should probably update the patch to space out the attribute to conform with the coding standards.

Added in 27942.2.patch

#5 @lancewillett
10 years ago

This could be a duplicate of #24328.

Last edited 10 years ago by lancewillett (previous) (diff)

This ticket was mentioned in IRC in #wordpress-dev by Clorith. View the logs.


10 years ago

#7 @SergeyBiryukov
10 years ago

#24328 was marked as a duplicate.

@pauldewouters
10 years ago

unit tests for sanitize_option

#8 @pauldewouters
10 years ago

I added some tests for 'blogname', is this a good start?

#9 @Frank Klein
9 years ago

  • Keywords needs-unit-tests removed

@BandonRandon
8 years ago

Patch With Unit Test

#10 @BandonRandon
8 years ago

  • Keywords needs-testing has-unit-tests added

Updated patch against trunk and added unit test.

#11 @markjaquith
8 years ago

  • Keywords 2nd-opinion added

Seems to work. Unit tests pass. Much better user experience.

@kovshenin
8 years ago

#12 @kovshenin
8 years ago

  • Keywords commit added; needs-testing 2nd-opinion removed
  • Milestone changed from Future Release to 4.5
  • Owner set to kovshenin
  • Status changed from new to assigned

Added unit tests for blogdescription too. Also looked into some history, goes all the way back to r5541 and looks like it's good to go.

#13 @kovshenin
8 years ago

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

In 35788:

Allow usage of angle brackets in a site title or tagline.

The whole string is escaped with esc_html() anyway, so we don't
need to wp_kses_post(). This is a better experience for users who
want to use angle brackets in their site title or description.
Does not allow any HTML, adds unit tests.

props BandonRandon, pauldewouters.
fixes #27942.

Note: See TracTickets for help on using tickets.