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().

CommentFileSizeAuthor
#31 image-itok-relative-path-1955378-31-test-only.patch1.28 KBclaudiu.cristea
#31 image-itok-relative-path-1955378-31.patch2.36 KBclaudiu.cristea
#29 image-itok-relative-path-1955378-29.patch2.39 KBDavid_Rothstein
#27 image-itok-relative-path-1955378-27-fail.patch1.44 KBclaudiu.cristea
#27 image-itok-relative-path-1955378-27.patch2.89 KBclaudiu.cristea
#19 image-itok-relative-path-D7-1955378-17-do-not-test.patch2.73 KBdarren oh
#18 image-itok-relative-path-1955378-17.patch2.88 KBdarren oh
#16 image-itok-relative-path-1955378-14.patch2.8 KBdarren oh
#9 image-itok-relative-path-1955378-9-D7-do-not-test.patch2.36 KBDavid_Rothstein
#9 image-itok-relative-path-1955378-9-TESTS-ONLY.patch1.52 KBDavid_Rothstein
#9 image-itok-relative-path-1955378-9.patch2.62 KBDavid_Rothstein
#8 image-itok_generation_issue-1955378-8.patch1.44 KBskek
#3 image-itok_generation_issue-1955378.patch1019 bytesskek
#1 image-itok_generation_issue-1955378.patch979 bytesskek
patch.txt711 bytesskek

Comments

skek’s picture

skek commented
StatusFileSize
new image-itok_generation_issue-1955378.patch979 bytes

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

skek commented
Status: Needs work » Needs review
StatusFileSize
new image-itok_generation_issue-1955378.patch1019 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

skek commented
Status: Needs work » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein commented
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

skek commented
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

skek commented
StatusFileSize
new image-itok_generation_issue-1955378-8.patch1.44 KB

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

David_Rothstein commented
StatusFileSize
new image-itok-relative-path-1955378-9.patch2.62 KB
new image-itok-relative-path-1955378-9-TESTS-ONLY.patch1.52 KB
new image-itok-relative-path-1955378-9-D7-do-not-test.patch2.36 KB

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

David_Rothstein commented
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

skek commented

Thanks!

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

#9: image-itok-relative-path-1955378-9.patch queued for re-testing.

dcam’s picture

dcam commented
Issue tags: +Needs issue summary update

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

aloyr commented

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

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Status: Needs work » Needs review
StatusFileSize
new image-itok-relative-path-1955378-14.patch2.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

darren oh
Primary language English
Location Lakeland, Florida
commented
Status: Needs work » Needs review
StatusFileSize
new image-itok-relative-path-1955378-17.patch2.88 KB

Fixed patch.

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new image-itok-relative-path-D7-1955378-17-do-not-test.patch2.73 KB

Drupal 7 version.

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
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

darren oh
Primary language English
Location Lakeland, Florida
commented
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

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue tags: +Needs tests

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

David_Rothstein’s picture

David_Rothstein commented
Issue tags: -Needs tests

The patch already has tests.

David_Rothstein’s picture

David_Rothstein commented

#18: image-itok-relative-path-1955378-17.patch queued for re-testing.

David_Rothstein’s picture

David_Rothstein commented

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

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
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
StatusFileSize
new image-itok-relative-path-1955378-27.patch2.89 KB
new image-itok-relative-path-1955378-27-fail.patch1.44 KB

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

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Any takers for review? :)

David_Rothstein’s picture

David_Rothstein commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new image-itok-relative-path-1955378-29.patch2.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

catch
he/him
Primary language English
commented
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

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new image-itok-relative-path-1955378-31.patch2.36 KB
new image-itok-relative-path-1955378-31-test-only.patch1.28 KB

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

zyxware’s picture

zyxware commented
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

zyxware commented
Issue summary: View changes

Updated summary.

David_Rothstein’s picture

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

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/18d4eaa

Status: Fixed » Closed (fixed)

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