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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Version: 7.20 » 7.x-dev

That 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?

alfaguru’s picture

In 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.

pwolanin’s picture

trying to reproduce this with core imagefield, and I don't see the triple slash:

230 |   1 | 06-search-result-biased.png | public://06-search-result-biased.png       | image/png |    81450 |      1 | 1361461760

Can you please supply steps to reproduce?

alfaguru’s picture

Yes:

  1. Installed drupal 7.20 from scratch, standard profile.
  2. Installed ctools, media, devel, devel_generate.
  3. added an image field to user, choosing the media widget but everything else default.
  4. used devel generate to generate a few test users.
  5. checked that the uris of the image field have the extra slash in the db.
  6. changed the display of the image field to be a derivative.
  7. visited a user page and saw a broken image.

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.

pwolanin’s picture

note 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.

pwolanin’s picture

Status: Active » Needs review
FileSize
906 bytes

Actually that function is a bit buggy for non-URIs, and may be changed, so this inlines the relevant code.

David_Rothstein’s picture

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

David_Rothstein’s picture

MGParisi’s picture

I'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:(

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Normal » Critical
Issue tags: +Needs backport to D7
mitron’s picture

Reroll/Backport for D7. Duplicated problem following steps in #4. Applied patch. Problem resolved. Verified that all image tests passed with patch applied.

Status: Needs review » Needs work

The last submitted patch, drupal-broken_file_uris-1923554-11-D7.patch, failed testing.

David_Rothstein’s picture

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

I 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...

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
806 bytes

For 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.

David_Rothstein’s picture

Something like this should work for the tests?

mitron’s picture

The 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.

pwolanin’s picture

Status: Needs review » Needs work

@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.

mitron’s picture

It 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?

David_Rothstein’s picture

Status: Needs work » Needs review

Right, 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 :)

ytsurk’s picture

#15 helped me in my local os-x setup.
so I could remove $conf['image_allow_insecure_derivatives'] = TRUE;

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

I RTBCed the related issue to. This is good to go.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

David_Rothstein’s picture

Fixing tags since Drupal 7.21 only contained a fix to deal with (more serious) fallout from the Drupal 7.20 security release.

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