wp_get_attachment_url() is not url encoding

[danorton]

A fairly fundamental flaw, the function ​wp_get_attachment_url() doesn't return a valid URL if the filename contains unescaped URL characters.

I'm not sure, but this might be a security issue, as the current version can generate URLs that don't match the filename, but instead passes query parameters back to the server.

The attached patch for Version 3.0 file fixes this in wp-includes/post.php

[nacin]

Can you post an example URL and what would get returned with and without the patch?

[danorton]

Replying to nacin:

Can you post an example URL and what would get returned with and without the patch?

For a file named "X%X.txt"

Currently the URL returned is:
.../wp-content/uploads/2010/06/X%X.txt

This patch corrects it to return:
.../wp-content/uploads/2010/06/X%25X.txt

[kawauso]

Compact patch based on danorton's but adding corrections for thumbnails and intermediate sizes. Tidy up comments a bit too.

[kawauso]

Had to force image_downsize() to use encoded filenames when replacing which may break things. Any other ideas?

[ryan]

This is too risky for 3.1 and will require thorough testing.

[chriscct7]

Tagging for re-review

[Mte90]

refresh patch

[Mte90]

The code in this years it's changed so the patch for post.php I changed approach with an encoding after the generation of the url itself.

[nevis2us]

Replying to Mte90:

The code in this years it's changed so the patch for post.php I changed approach with an encoding after the generation of the url itself.

IMHO this is the right approach but php urlencode can't be used to encode the whole url.
An equivalent of javascript encodeURI is needed here.

​https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI.

eg

function encodeURI ($uri)	{

	$revert = array (

		// reserved characters
		"%3B" => ";", "%2C" => ",", "%2F" => "/", "%3F" => "?", "%3A" => ":",
		"%40" => "@", "%26" => "&", "%3D" => "=", "%2B" => "+", "%24" => "$",

		// unescaped characters
		"%2D" => "-", "%5F" => "_", "%2E" => ".", "%21" => "!", "%7E" => "~",
		"%2A" => "*", "%27" => "'", "%28" => "(", "%29" => ")",

		// number sign
		"%23" => "#"
	);

	return strtr (rawurlencode ($uri), $revert);
}

[afercia]

14148.2.diff​ needs some coding standards.

[andizer]

Fix codestyle issue

[mukkoo]

I've checked this issue and related patches but I can't reproduce a bugged context.
Uploading a media that needs encoding WordPress remove all special chars making this scenario not reproducible.
I've used tests/phpunit/tests/media.php as test base using

     $filename = DIR_TESTDATA . '/images/test%%%%image-large.png';

uploading this media WordPress remove all special chars leaving testimage-large.png

Can someone suggest any possible way to replicate bug?

[nevis2us]

Replying to mukkoo:

Can someone suggest any possible way to replicate bug?

Upload a file with a valid non-ASCII name on a system which supports UTF-8.

[nevis2us]

This patch assumes only base names may contain characters which need to be encoded. What about directory names? This may not be the case with wordpress attachments but why reinvent the wheel? There's a (client-side) standard function which can be safely applied to the whole URI and spare the trouble of handling URI components separately.

BTW there's a number of similar issues in functions like image_get_intermediate_size and image_downsize.

[nevis2us]

One more note. Patching the core on this level may break existing code. I'm currently handling this problem in a custom plugin which applies the above encodeURI function to the whole URL in wp_get_attachment_url filter. The filter can be easily turned off in the UI in case the patches make it to the core.

[Mte90]

Cna you share the code of your custom plugin?
So we can understand better how to handle it.
Cna you share also an image already existing with the issues on the name?

[nevis2us]

Replying to Mte90:

Cna you share the code of your custom plugin?
So we can understand better how to handle it.

Sure, I can share the relevant parts of the code:

add_filter("wp_get_attachment_url", "<namespace>_wp_get_attachment_url", 90, 2);

function <namespace>_wp_get_attachment_url($url, $post_id)	{
	return encodeURI($url);
}

The code for encodeURI() can be found in comment:9 above.

This is what a plugin dev can use if he needs this bug fixed ASAP before the relevant patches make it to the core. But fixing this and similar issues in the core or in a filter may break existing 3rd party code. I don't use 3rd party plugins on my sites so it works for me.

Cna you share also an image already existing with the issues on the name?

If I got you right:

The function wp_get_attachment_url() doesn't return a valid URL if the filename contains UTF-8 characters.

Example:

attachment filename	.../wp-content/uploads/2015/10/Оля-в-Инстаграме.BW_.jpg
wp_get_attachment_url() returns	.../wp-content/uploads/2015/10/Оля-в-Инстаграме.BW_.jpg
valid URL	.../wp-content/uploads/2015/10/%D0%9E%D0%BB%D1%8F-%D0%B2-%D0%98%D0%BD%D1%81%D1%82%D0%B0%D0%B3%D1%80%D0%B0%D0%BC%D0%B5.BW_.jpg

[nevis2us]

Replying to Mte90:

Cna you share the code of your custom plugin?
So we can understand better how to handle it.

Complete plugin with the fix mentioned above can be downloaded from ​http://ipros24.ru/wordpress/#ipros24-fixpack. It also includes fixes for similar encoding problems in image_get_intermediate_size() and image_downsize() core functions. After install individual fixes must be enabled in Settings -> Fixpack.

This plugin may break 3rd party code e.g. Jetpack, which also uses image_downsize filter to override the core function.

[davidbaumwald]

With 5.4 Beta 3 approaching, and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Trac ticket: https://core.trac.wordpress.org/ticket/14148

The unit test is incomplete, also we need to think about rawurlencode or urlencodeas option.

[Mte90]

I did a PR ​https://github.com/WordPress/wordpress-develop/pull/2377 that include a unit test but just use àìòù and I am using rawurlencode.

Probably can be improved and added more tests but is now open for feedbacks.

[Mte90]

@SergeyBiryukov any updates for this?

[johnbillion]

I'm unable to reproduce this problem. I tried the following filenames:

• test-image-àèòìù.jpeg (resulted in example.com/test-image-aeoiu)
• X%X.jpeg (resulted in example.com/xx)

Can someone provide specific steps to reproduce if this is still a problem please?

[Mte90]

I can confirm that the bugfix is not needed anymore.
The filenames are already sanitized somewhere, not like my patch so probably something more up in the flow.

[johnbillion]

Grazie Daniele