There is an issue with image token generation in the image module. The function image_style_url() allows you to use either a path or a URI. For example, we can use either logo_1.gif or public://logo_1.gif. However if we use logo_1.gif, the token is generated for the path, but the validation always tries the URI. This causes itok validation to fail and you will get an "Access denied" page.

The latest patch converts path to URI in image_style_url().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

skek’s picture

The above patch was not correct. The needed functionality here is if we receive a path e.g. 'logo_1.gif' we should normalize it to the default schema e.g. 'public://logo_1.gif'.
Here you go a working solution patch for the previous described problem.

Status: Needs review » Needs work

The last submitted patch, image-itok_generation_issue-1955378.patch, failed testing.

skek’s picture

Status: Needs work » Needs review
FileSize
1019 bytes

Adding a new patch with the path to the module and file.

Status: Needs review » Needs work

The last submitted patch, image-itok_generation_issue-1955378.patch, failed testing.

skek’s picture

Status: Needs work » Closed (fixed)

This is actually fixed in 7.22 in better way.
Closing this one.

David_Rothstein’s picture

Title: Image derivative token generation issue » Image derivative tokens don't work when image_style_url() is called on a path (rather than a URI)
Version: 7.21 » 8.x-dev
Status: Closed (fixed) » Needs work
Issue tags: +Needs backport to D7

I can reproduce this in the latest 7.x code, actually (and presumably 8.x too). Although most people don't call image_style_url() this way, it seems pretty bad.

Is the fix you're referring to the one in #1923554: New anti-DoS measure breaks for some file URIs? It doesn't look to me like that helped here...

skek’s picture

Version: 8.x-dev » 7.x-dev

@David_Rothstein,

Actually the issue you are referring to is different from the one I've reported, so closing this bug was actually not a good idea :).
$token_query = array(IMAGE_DERIVATIVE_TOKEN => image_style_path_token($style_name, file_stream_wrapper_uri_normalize($path)));
The above code is fixing the issue you are referring to and it accused me to close this one but actually this doesn't fix the problem I'm referring to.
The problem here is when you add an image using a path like "logo.jpg", not an URI. I agree that calling the image_style_url() this way is not a good idea but actually the function allows you to do that, so the code should handle both ways I think, cause a non experienced developer can easily use the bad practice.

skek’s picture

Adding not git patch, sorry about this but I don't have the time to do it.
Please somebody to make re-work it and make a correct patch, sorry about that.

David_Rothstein’s picture

Thanks!

Here is a version for Drupal 8 (also backported to Drupal 7), with tests. I think we can use file_build_uri() here, so I went ahead and tried that.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review

I will also mention this issue in the Drupal 7.20 release notes (http://drupal.org/drupal-7.20-release-notes).

skek’s picture

Thanks!

benjifisher’s picture

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Status: Needs review » Needs work

The last submitted patch, image-itok-relative-path-1955378-9.patch, failed testing.

aloyr’s picture

i am going to try to reroll the patch, but realized patch is already re-rolled.

Darren Oh’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

Updated patch.

Status: Needs review » Needs work

The last submitted patch, image-itok-relative-path-1955378-14.patch, failed testing.

Darren Oh’s picture

Status: Needs work » Needs review
FileSize
2.88 KB

Fixed patch.

Darren Oh’s picture

Darren Oh’s picture

Status: Needs review » Needs work

This patch assumes that the path is within the default files directory. The purpose of passing in a path instead of a URL with a schema is to be able to generate derivatives of images which are not in the files directory. That still doesn’t work, and this patch doesn’t fix that.

Darren Oh’s picture

Status: Needs work » Needs review

Making image_style_url() work for files shipped with modules or themes would require a major redesign, so my last comment is wrong.

claudiu.cristea’s picture

Issue tags: +Needs tests

We need to prove this bug with an automated test first.

David_Rothstein’s picture

Issue tags: -Needs tests

The patch already has tests.

David_Rothstein’s picture

David_Rothstein’s picture

But I'm wondering if they still pass...

Status: Needs review » Needs work

The last submitted patch, image-itok-relative-path-1955378-17.patch, failed testing.

claudiu.cristea’s picture

Title: Image derivative tokens don't work when image_style_url() is called on a path (rather than a URI) » Return same derivative token with path or URI
Assigned: Unassigned » claudiu.cristea
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
2.89 KB
1.44 KB

Reworked and rerolled against image style D8 conversion. Attached, a failure and a full patch.

claudiu.cristea’s picture

Any takers for review? :)

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.39 KB

Not sure if I should RTBC this since I wrote most of the patch, but the recent changes are all pretty simple so I'm going to go ahead and do that.

Just rerolling it to remove the extra whitespace that the previous patch added to the test file.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

claudiu.cristea’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.36 KB
1.28 KB

Retested patch from #9 and provided a patch-only test.

zyxware’s picture

Status: Needs review » Reviewed & tested by the community

Tested and verified patch in comment 31 - http://drupal.org/files/image-itok-relative-path-1955378-31.patch

Set up latest drupal 7 dev

Created URLs with

image_style_url('thumbnail', 'field/image/test.JPG')
image_style_url('thumbnail', 'public://field/image/test.JPG')

Tested both URLS one by one after deleting the cached file created at sites/default/files/styles/thumbnail/public/field/image/tets.JPG

Replicated access denied error with URL generated from the file system path. The token generated was different for the two URLs.

Applied patch

Tested both URLS one by one after deleting the cached file created at sites/default/files/styles/thumbnail/public/field/image/tets.JPG

Was able to access both URLs without access denied error. Verified that the token generated was the same for both urls.

zyxware’s picture

Issue summary: View changes

Updated summary.

David_Rothstein’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: +7.25 release notes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.