Twenty Fourteen: Full size image link attribute escaping missing in image template

[philiparthurmoore]

The full size image link in the image template file is missing escaping. The attached patch wraps wp_get_attachment_url with esc_url.

[lancewillett]

Thanks Philip. (Fanks!)

Two quick thoughts:

1. Can you take a look at all the default themes? My guess is it's not escaped everywhere.
2. Should we submit "upstream" to the core function to be escaped by default? I think it's a better experience for theme developers not to need to escape core functions.

[obenland]

The get_permalink() call a few lines later needs escaping too.

[philiparthurmoore]

Replying to lancewillett:

Thanks Philip. (Fanks!)

Two quick thoughts:

1. Can you take a look at all the default themes? My guess is it's not escaped everywhere.
2. Should we submit "upstream" to the core function to be escaped by default? I think it's a better experience for theme developers not to need to escape core functions.

Fanks, Lance!

1. Sure thing, I can take a look first thing tomorrow.
2. This is a really good point, and it's something that I had to make sure of before I submitted this patch (searching to see if wp_get_attachment_url was already escaped by core). In general I think that if the escaping happens within the core functions then all the better.

Would it still make sense to proceed with escaping all-the-things until core has taken care of them (plus 2 versions for back compat)?

[lancewillett]

Replying to philiparthurmoore:

Would it still make sense to proceed with escaping all-the-things until core has taken care of them (plus 2 versions for back compat)?

Yes, totes.

[philiparthurmoore]

This new patch for Twenty Fourteen adds in proper escaping for the parent post link and attachment link in the image template.

[philiparthurmoore]

Add in proper attribute escaping in Twenty Ten's image attachment template part.

[philiparthurmoore]

More thorough attribute escaping in Twenty Ten's image attachment template part.

[philiparthurmoore]

It looks like only Twenty Fourteen and Twenty Ten had issues with this. Twenty Thirteen, Twenty Twelve, and Twenty Eleven all look to have proper escaping present.

[lancewillett]

In 28462:

Twenty Fourteen: correct escaping for parent post link and attachment link in image template. Props philiparthurmoore, see #28251.

[lancewillett]

In 28463:

Twenty Ten: correct attribute escaping in the attachment template. Props philiparthurmoore, see #28251.

[lancewillett]

Should we submit "upstream" to the core function to be escaped by default? I think it's a better experience for theme developers not to need to escape core functions.

Leaving this open until we patch the core file to escape by default.

[obenland]

Replying to lancewillett:

Leaving this open until we patch the core file to escape by default.

We should probably open a new ticket for that.

[lancewillett]

Replying to obenland:

Replying to lancewillett:

Leaving this open until we patch the core file to escape by default.

We should probably open a new ticket for that.

:)

[obenland]

lancewillett and I agreed that adding escaping to wp_get_attachment_url() might not be the best idea. It is used a lot internally and not only in the context of a template tag. See conversation in IRC.