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().
Comments
Comment #1
skek CreditAttribution: skek commentedThe 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.
Comment #3
skek CreditAttribution: skek commentedAdding a new patch with the path to the module and file.
Comment #5
skek CreditAttribution: skek commentedThis is actually fixed in 7.22 in better way.
Closing this one.
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedI 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...
Comment #7
skek CreditAttribution: skek commented@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.
Comment #8
skek CreditAttribution: skek commentedAdding 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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedThanks!
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.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedI will also mention this issue in the Drupal 7.20 release notes (http://drupal.org/drupal-7.20-release-notes).
Comment #11
skek CreditAttribution: skek commentedThanks!
Comment #12
benjifisher#9: image-itok-relative-path-1955378-9.patch queued for re-testing.
Comment #13
dcam CreditAttribution: dcam commentedhttp://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.
Comment #15
aloyr CreditAttribution: aloyr commentedi am going to try to reroll the patch, but realized patch is already re-rolled.
Comment #16
Darren OhUpdated patch.
Comment #18
Darren OhFixed patch.
Comment #19
Darren OhDrupal 7 version.
Comment #20
Darren OhThis 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.
Comment #21
Darren OhMaking image_style_url() work for files shipped with modules or themes would require a major redesign, so my last comment is wrong.
Comment #22
claudiu.cristeaWe need to prove this bug with an automated test first.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedThe patch already has tests.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commented#18: image-itok-relative-path-1955378-17.patch queued for re-testing.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedBut I'm wondering if they still pass...
Comment #27
claudiu.cristeaReworked and rerolled against image style D8 conversion. Attached, a failure and a full patch.
Comment #28
claudiu.cristeaAny takers for review? :)
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedNot 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.
Comment #30
catchCommitted/pushed to 8.x, thanks!
Moving to 7.x for backport.
Comment #31
claudiu.cristeaRetested patch from #9 and provided a patch-only test.
Comment #32
zyxware CreditAttribution: zyxware commentedTested 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.
Comment #32.0
zyxware CreditAttribution: zyxware commentedUpdated summary.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/18d4eaa