Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#19282 closed defect (bug) (fixed)

wp_get_attachment_link() does not allow HTML in link text

Reported by: SergeyBiryukov Owned by: ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 2.8
Component: General Keywords: has-patch commit
Focuses: Cc:


Background: #18156

Escaping $link_text was introduced in [10495] along with the $text parameter itself and changed to esc_attr() in [11204]. Looks like it shouldn't really be there:

  1. $link_text is not an attribute, it's literally a link text.
  2. adjacent_post_link() doesn't escape link text.

Attachments (2)

19282.patch (462 bytes) - added by SergeyBiryukov 4 years ago.
19282.diff (473 bytes) - added by griffinjt 4 years ago.

Download all attachments as: .zip

Change History (7)

#1 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 3.4

#2 @griffinjt
4 years ago

Letting straight HTML come through doesn't seem like the safest way to go. Why not just filter using wp_kses_post()? Not escaping allows for <script> tags to pass through, so if we want to add HTML, let's at least filter what type of HTML tags can come through. I've attached an updated diff for it.

4 years ago

#3 @SergeyBiryukov
4 years ago

I don't see a reason to escape the text here. If someone calls wp_get_attachment_link() with <script> tags, they could as well insert them into the template file directly.

That would be inconsistent with other *_link() functions which don't escape anchor text:

  • the_feed_link()
  • post_comments_feed_link()
  • edit_term_link()
  • edit_post_link()
  • edit_comment_link()
  • edit_bookmark_link()
  • adjacent_post_link()
  • get_next_posts_link()
  • get_previous_posts_link()
  • get_next_comments_link()
  • get_previous_comments_link()

#4 @nacin
4 years ago

  • Keywords commit added

We try to avoid kses on the frontend for performance reasons.

Removing the escaping seems appropriate here.

#5 @ryan
4 years ago

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

In [20654]:

Don't escape anchor text as an attributein wp_get_attachment_link(). Props SergeyBiryukov. fixes #19282

Note: See TracTickets for help on using tickets.