Just applied Drupal 7.20 this morning and immediately noticed an issue. Some of our derived images are getting Access Denied. I walked through what's happening and in the generation of the token the uri is taken from the file_managed table, where these images have uris such as "public:///imagefield_fS5HL8.png" while in the request the uri is reverse engineered to be "public://imagefield_fS5HL8.png" without the extra slash. Thus the token values do not match.
These images are uploaded to the root of the public files directory, while others that work OK are in subdirectories. So somewhere in the file module I think the uri is being generated incorrectly.
I am applying the workaround flag for now, and will also ensure that all file fields are configured with a subdirectory for better manageability, but i can't be the only person to be bitten by this one.
Comment | File | Size | Author |
---|---|---|---|
#15 | file-uri-1923554-15-TESTS-ONLY.patch | 1.69 KB | David_Rothstein |
#15 | file-uri-1923554-15.patch | 2.47 KB | David_Rothstein |
#14 | file-uri-1923554-14.patch | 806 bytes | David_Rothstein |
#11 | drupal-broken_file_uris-1923554-11-D7.patch | 906 bytes | mitron |
#6 | 1923554-6.patch | 906 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedThat inconsistency does look like a bug that will break the new protection scheme.
Do you have any custom or contrib modules generating the URIs with the three slashes?
Comment #2
alfaguru CreditAttribution: alfaguru commentedIn my current working setup these are all file fields using the media module, but populated for test purposes using devel_generate. I haven't had time yet to play around with different options.
Comment #3
pwolanin CreditAttribution: pwolanin commentedtrying to reproduce this with core imagefield, and I don't see the triple slash:
Can you please supply steps to reproduce?
Comment #4
alfaguru CreditAttribution: alfaguru commentedYes:
Tried a manual upload and that's OK, so looks like the problem is in the devel_generate routine for file uploads, which is a relief though still potentially a pain.
Comment #5
pwolanin CreditAttribution: pwolanin commentednote that this core function trims leading or trailing slashes:
file_uri_target()
So, the core patch could be improved by calling:
file_stream_wrapper_uri_normalize()
before passing the uri into the token function.
Comment #6
pwolanin CreditAttribution: pwolanin commentedActually that function is a bit buggy for non-URIs, and may be changed, so this inlines the relevant code.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedI was able to reproduce this, so I added a note about this problem to the Drupal 7.20 release notes and release announcement (in the "known issues" section).
Hopefully it's only Devel Generate that is affected, since at least that won't be likely to break production sites...
Adding that code to image_style_url() to force the URLs to be consistent probably does make sense (and the patch fixes the issue when I tested it).
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedAlso see the following issues:
#1923814: Existing hardcoded images can break after updating from Drupal 7 earlier than 7.20 if image styles have been re-saved
#1923834: New anti-DoS measure breaks displaying images in WYSIWYG content
Though they appear to be a bit different and I wasn't able to reproduce them.
Comment #9
MGParisi CreditAttribution: MGParisi commentedI'm having an issue with images and 7.20... I thought it was due to the use of imagemagicks so I filed this ticket #1922914: Incompatible with Drupal 7.21, I cant apply the new version of drupal to my site:(
Comment #10
catchComment #11
mitron CreditAttribution: mitron commentedReroll/Backport for D7. Duplicated problem following steps in #4. Applied patch. Problem resolved. Verified that all image tests passed with patch applied.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think this makes sense to apply to Drupal 8 until #1922814: Denial-of-service vulnerabiility in Image module is fixed there first (or at least, even if we could apply it, we couldn't write a test for it). I'll cross-link them, though.
Also not sure this is critical if only Devel Generate is affected in practice, but in any case it would be great to get this into Drupal 7 before the next core release so people can upgrade to it to fix any issues...
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedFor starters, we should just be able to directly call file_stream_wrapper_uri_normalize().
The only difference between that and the code in the above patches is an else statement which is a complete no-op (see #1049050: file_stream_wrapper_uri_normalize(): remove dead code) so there should be literally no difference between the two.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedSomething like this should work for the tests?
Comment #16
mitron CreditAttribution: mitron commentedThe test and patch look good.
Manually tested. Duplicated problem following steps in #4. Applied patch in #15. Problem resolved.
On the possibility that other unknown and unexpected issues existed, I tried a few extra options which all passed.
/**
* Test image_style_url() with a file URL that has an extra slash in it.
*/
function testImageStyleUrlExtraSlash() {
$this->_testImageStyleUrlAndPath('public', TRUE, TRUE);
$this->_testImageStyleUrlAndPath('public', FALSE, TRUE);
$this->_testImageStyleUrlAndPath('private', TRUE, TRUE);
$this->_testImageStyleUrlAndPath('private', FALSE, TRUE);
}
Not proposing to add these extra calls options to the test.
Comment #17
pwolanin CreditAttribution: pwolanin commented@David - no, do not call file_stream_wrapper_uri_normalize(): directly since it's not clear how the linked bug will be resolved. The original intended behavior appends file:// which would totally break here.
I think my patch in #6 was still preferable to calling a broken API function and relying on the brokenness.
Comment #18
mitron CreditAttribution: mitron commentedIt looks like there is a consensus in #1049050: file_stream_wrapper_uri_normalize(): remove dead code to remove the troublesome code.
Is not the intended functionality of file_stream_wrapper_uri_normalize() exactly the functionality that we need here? Is it not preferable to call a central function so that any future changes are in one place instead of inlined here and there?
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedRight, that issue is just about removing already dead code, not changing the behavior of the function. (Regardless of whether the original author of that function was trying to do something different, it currently behaves sensibly.) And even if people change their mind on that for Drupal 8 for some reason, it still won't change for Drupal 7 - this function is already heavily used throughout core and contrib.
I was going to upload a patch here which demonstrated that if the file:// change were ever made, the tests added in this issue would fail... However, it doesn't matter because it turns out if you make that change it's not even possible to install Drupal anymore :)
Comment #20
ytsurk#15 helped me in my local os-x setup.
so I could remove
$conf['image_allow_insecure_derivatives'] = TRUE;
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedI RTBCed the related issue to. This is good to go.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/40d178b
#1922814: Denial-of-service vulnerabiility in Image module already included the same fix for Drupal 8, so we're good to go here.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedFixing tags since Drupal 7.21 only contained a fix to deal with (more serious) fallout from the Drupal 7.20 security release.