New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle query strings on image import to prevent security error #35

Merged
merged 3 commits into from Aug 16, 2017

Conversation

3 participants
@ethanclevenger91
Contributor

ethanclevenger91 commented Aug 10, 2017

If you're trying to import an image with a query string (such as from Placeholder.com using custom copy), WordPress says Sorry, this file type is not permitted for security reasons. because the query string makes it all the way to file name, so the file fails validation.

This PR strips the query string when passing it along, and only does the operation if the file is remote.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 10, 2017

Member

Thanks for the pull request @ethanclevenger91.

This PR strips the query string when passing it along, and only does the operation if the file is remote.

I don't think this is quite the right approach. These two images are technically different images:

The correct approach would be to use the full URL with query strings. Is this possible?

Member

danielbachhuber commented Aug 10, 2017

Thanks for the pull request @ethanclevenger91.

This PR strips the query string when passing it along, and only does the operation if the file is remote.

I don't think this is quite the right approach. These two images are technically different images:

The correct approach would be to use the full URL with query strings. Is this possible?

@ethanclevenger91

This comment has been minimized.

Show comment
Hide comment
@ethanclevenger91

ethanclevenger91 Aug 10, 2017

Contributor

@danielbachhuber So the file that is ultimately copied is pulled with the query string intact, but yeah, if you ran both of those, they would have the same name in the WP media library (well, with -1 attached).

You could move the query string to before the file extension? Even if you could work around the validation, I worry having a query string at the end of the filename WordPress is importing may cause problems elsewhere (though nothing jumps to mind).

Contributor

ethanclevenger91 commented Aug 10, 2017

@danielbachhuber So the file that is ultimately copied is pulled with the query string intact, but yeah, if you ran both of those, they would have the same name in the WP media library (well, with -1 attached).

You could move the query string to before the file extension? Even if you could work around the validation, I worry having a query string at the end of the filename WordPress is importing may cause problems elsewhere (though nothing jumps to mind).

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 10, 2017

Member

So the file that is ultimately copied is pulled with the query string intact, but yeah, if you ran both of those, they would have the same name in the WP media library (well, with -1 attached).

Oh. The query string remains intact, and the image is imported at two different resolutions?

Member

danielbachhuber commented Aug 10, 2017

So the file that is ultimately copied is pulled with the query string intact, but yeah, if you ran both of those, they would have the same name in the WP media library (well, with -1 attached).

Oh. The query string remains intact, and the image is imported at two different resolutions?

@ethanclevenger91

This comment has been minimized.

Show comment
Hide comment
@ethanclevenger91

ethanclevenger91 Aug 10, 2017

Contributor

Exactly. So if I hit Placeholder.com for two images and pull, for example:

http://via.placeholder.com/350x150.jpg?text=Foo
http://via.placeholder.com/350x150.jpg?text=Bar

My media library gets two different images, one that says "Foo" and one that says "Bar". Their names reflected in the library, however, are:

350x150.jpg
350x150-1.jpg
Contributor

ethanclevenger91 commented Aug 10, 2017

Exactly. So if I hit Placeholder.com for two images and pull, for example:

http://via.placeholder.com/350x150.jpg?text=Foo
http://via.placeholder.com/350x150.jpg?text=Bar

My media library gets two different images, one that says "Foo" and one that says "Bar". Their names reflected in the library, however, are:

350x150.jpg
350x150-1.jpg
@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 10, 2017

Member

@ethanclevenger91 Makes sense. This pull request is fine then.

Can you add some tests, please? We can use the placeholder service you mentioned for sample data.

Member

danielbachhuber commented Aug 10, 2017

@ethanclevenger91 Makes sense. This pull request is fine then.

Can you add some tests, please? We can use the placeholder service you mentioned for sample data.

@danielbachhuber

Needs tests reflecting the changes.

@ethanclevenger91

This comment has been minimized.

Show comment
Hide comment
@ethanclevenger91

ethanclevenger91 Aug 14, 2017

Contributor
Contributor

ethanclevenger91 commented Aug 14, 2017

@ethanclevenger91

This comment has been minimized.

Show comment
Hide comment
@ethanclevenger91

ethanclevenger91 Aug 15, 2017

Contributor

Wait, where are existing tests?

Contributor

ethanclevenger91 commented Aug 15, 2017

Wait, where are existing tests?

@schlessera

This comment has been minimized.

Show comment
Hide comment
Member

schlessera commented Aug 15, 2017

@danielbachhuber danielbachhuber added this to the 1.0.5 milestone Aug 15, 2017

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 16, 2017

Member

Thanks for your work on this @ethanclevenger91 !

Member

danielbachhuber commented Aug 16, 2017

Thanks for your work on this @ethanclevenger91 !

@danielbachhuber danielbachhuber merged commit 3b3b817 into wp-cli:master Aug 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ethanclevenger91 ethanclevenger91 deleted the ethanclevenger91:handle-query-string branch Aug 16, 2017

@danielbachhuber danielbachhuber changed the title from Handle query strings on URL import. to Properly handle query strings on image import to prevent security error Oct 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment