WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#41215 closed defect (bug) (fixed)

Escaping the value of the srcset attribute

Reported by: henry.wright Owned by: SergeyBiryukov
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch
Focuses: Cc:

Description

Should we be using esc_url() instead of esc_attr() to escape the value of the srcset attribute in get_avatar()?

The value is a URL, so to me it would make sense to use esc_url(). That said, there might be a valid reason why esc_attr() is used.

Attachments (2)

41215.diff (671 bytes) - added by joemcgill 4 years ago.
41215.2.diff (671 bytes) - added by henry.wright 4 years ago.
Could even do this?

Download all attachments as: .zip

Change History (9)

@joemcgill
4 years ago

#1 @joemcgill
4 years ago

  • Keywords has-patch reporter-feedback added

Hi @henry.wright,

Most likely, esc_attr() was used because the srcset attribute is a string like:

http://1.gravatar.com/avatar/{some_img_id}?s=64&d=mm&r=g 2x

Where the x descriptor at the end of the string means that it's not really a URL. Since this is a fairly simple srcset implementation, we could probably use esc_url() on just the URL part of the image source. 41215.diff shows how that would work.

Last edited 4 years ago by joemcgill (previous) (diff)

#2 @henry.wright
4 years ago

I had an idea the "2x" would be the reason. 41215.diff looks like a good solution.

#3 @henry.wright
4 years ago

  • Keywords reporter-feedback removed

@henry.wright
4 years ago

Could even do this?

#4 @joemcgill
4 years ago

You certainly could, but seems a bit overkill to me to run the extra escaping function on a text string that isn't variable. I thought about doing srcset="%s 2x" and then use esc_url for the image source URL only, but handling the whole attribute value as a string replacement means we only have to change one line if we decide to format the srcset differently in the future.

#5 @henry.wright
4 years ago

@joemcgill I guess if "2x" will remain a literal string and isn't likely to become a variable at a later date then I agree running esc_attr() will be a waste. Happy to go with 41215.diff :)

#6 @SergeyBiryukov
4 years ago

  • Component changed from General to Users
  • Milestone changed from Awaiting Review to 4.9

#7 @SergeyBiryukov
4 years ago

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

In 41156:

Users: Use esc_url() instead of esc_attr() to escape the value of the srcset attribute in get_avatar().

Props joemcgill, henry.wright.
Fixes #41215.

Note: See TracTickets for help on using tickets.