Having just migrated a site from Drupal 6 to Drupal 7.10, I noticed that none of my uploaded files were accessible. I use the private file system, but if I loaded example.com/system/files/foo.jpg, I got "access denied".

I then created a new node, and uploaded a file. It was accessible. So I compared the entries in the file_managed table for the migrated file that did not work, and the new file that did. I found the old file appeared in the table with private:///foo.jpg. The new one appeared as private://bar.jpg. I removed the extra forward slash from the migrated file and cleared the cache - the file displayed fine.

I think the problem is in update 7061, which strips the basename out of the old file (so that the full path of the files directory is not included). It does this with a replace function, replacing the old basename with an empty string. But the old basename being matched ends in "files". An actual file would be called /home/path_to_files/files/foo.jpg, so stripping out the basename leaves "/foo.jpg". Then prepend the $scheme, and you get private:///foo.jpg.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JamesOakley’s picture

Status: Active » Needs review
FileSize
765 bytes

I think this is the answer:

David_Rothstein’s picture

Component: install system » database update system

file_stream_wrapper_uri_normalize() runs shortly after that code and claims, at least in the documentation, to take care of that triple slash problem...

So you're saying that when it's called from that update function, it doesn't actually do that? Seems like we should figure out why.

JamesOakley’s picture

I can certainly see what you mean - it shouldn't do what it appears to be doing.

I'll insert some debug code into that update function and run it again, and see if that sheds any more light.

JamesOakley’s picture

I've done some more testing, and I'm now very confused...

I put some debug code around those lines, and dumped the results into the error_log file. What I got out showed that file_stream_wrapper_uri_normalize() was doing its job correctly. What's more, the file_managed table only had file://, not the erroneous file:///.

Before coming here to mark as "can't reproduce", I thought I'd better run the update again. I removed the debug code; this time I was back to file:///.

So, somehow, the debug code itself was making the update perform correctly. Without it, it was misbehaving.

I have reduced the number of lines of debug code to the minimum to "make it work". It turns out that you need to read the variable $file['uri'] before and after the normalise call (just doing one or the other is not sufficient). What's more, it's not enough to simply read the variable before and after - I've found that some other operation is needed. In my case, appending one to the other (much as I was doing when I was preparing a string to output to a debug log) does the trick.

Now, I am NOT claiming that the attached patch should be committed. I think this needs more understanding first as to why a change as simple as this should fix something that should work anyway. (Changing status to "needs work"). I'm running a pretty standard PHP 5.3 setup, but it may be that this is only an issue under certain system configurations. It certainly needs work, though, as something isn't right as it is.

JamesOakley’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs tests, +Needs backport to D7
David_Rothstein’s picture

Just wanted to point out there have been a few patches committed recently related to system_update_7061(), e.g.:

#966210: DB Case Sensitivity: system_update_7061() fails on inserting files with same name but different case
#1542854: system_update_7061() converts filepaths too aggressively

I have no idea, but I wonder if they affect the issue here?

David_Rothstein’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -Needs backport to D7

Also, assuming this affects system_update_7061() only, there's nothing to do here for Drupal 8.

catch’s picture

Priority: Major » Critical

Bumping this to critical.

If we don't fix this before 8.x is released, we'll need to add a corrective upgrade path to 8.x at some point (for sites that go 6.x to 7.x to 8.x before this is fixed in 7.x), I really don't want to do that, so lets make it an 8.x release blocker instead.

David_Rothstein’s picture

Title: system_update_7061 leaves one too many forward slashes in protocol of migrated URIs » system_update_7061 breaks private files by leaving one too many forward slashes in protocol of migrated URIs
Status: Needs work » Needs review
FileSize
1.24 KB

I ran into this today while testing some things for the D6/D7 security release.

Like JamesOakley above, I hit some odd issues where I couldn't reproduce it always and it seemed like my debugging code was having an effect, but ultimately I couldn't reproduce anything consistently with that, and I think it may have just been a coincidence due to slightly different upgrade methods used each time I was testing. Because ultimately, this bug seems to be due to the fact that during the Drupal 6 to 7 upgrade, the private:// file scheme isn't recognized as a valid one.... And recognizing it as valid relies on file_get_stream_wrappers(), which has static caching and relies on hook invocations. I'm not 100% sure exactly what causes it to not be recognized, but we shouldn't invoke hooks during the upgrade if we can help it anyway.

The attached patch avoids calling that function and seems to work for me. I have no idea how to write automated tests for this, though (and not really sure it's worth the effort).

JamesOakley’s picture

Status: Needs review » Reviewed & tested by the community

I can't comment on whether this needs automated tests.

Here's what I've tested:

1. Install vanilla Drupal 6.
2. Set file system to private, enable Upload module, create one node with an attachment. Verify that the attachment can be viewed.
3. Make a copy of the database so that I can be sure I'm testing the same site.
4. Download vanilla Drupal 7. Tweak default settings.php to point to the database created in step 1.
5. drush updatedb; drush cc all.
6. Load /node/1 and view the upload - it failed. Check file_managed table - private:///{snip}
7. Replace database with the backup copy of my D6 site.
8. Patch my D7 site with the patch from #10
9. drush updatedb; drush cc all
10. Load /node/1 and view the upload - success. Check file_managed table - private://{snip}

Thanks David

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Thanks!

Since I wrote the patch and am also likely to be the person who commits it, I think it would be good to get one more review before committing (ideally if someone has a real database with a whole bunch of actual files to test this on).

I will say that my first version of this (which I caught just before uploading it) almost broke things badly, because I called the variable $scheme rather than $file_uri_scheme without noticing that it was overwriting an existing variable. So if you tested it with one node and one attachment, it actually worked fine, but as soon as you had a second attachment and it went through the loop a second time, things exploded. And that would have caused a nice big regression in the D6-to-D7 upgrade path for sites with any kinds of files (including public files)... So now that made me paranoid about this patch :)

JamesOakley’s picture

Sadly, I no longer have the old site that I was trying to upgrade when I opened the issue - I upgraded by hand with a SQL query, and haven't had cause to go back.

I do have one more site that isn't yet upgraded from D6, but I haven't got time to look at that today. If nobody else has tested this soon, I'll see if I can set that up for another test.

That said, as you say, another tester (not just another test from the same person) would be reassuring.

catch’s picture

Do we also need a corrective upgrade path for previously upgraded sites that have the extra forward slash? If we do we could probably open a new issue for that though.

YesCT’s picture

@David_Rothstein
what do you think about catch's question?

Also, what kind of review are you looking for?
... manual testing (manual testing doc for people curious: http://drupal.org/node/1489010)
and/or code review (code review doc: http://drupal.org/node/1488992)

In addition to the review you are looking for, does this still need tests?

YesCT’s picture

David_Rothstein’s picture

Sorry, I missed this earlier.

Manual testing and code reviews would both be useful. (I think the patch is relatively straightforward but I don't think anyone has said above that they actually reviewed the code.)

I also think we could commit this without tests and without an additional update to fix previously upgraded sites, although either of these are certainly good ideas. (For the latter, the site would have been quite broken after the initial upgrade, so perhaps no one out there even kept an upgraded site around in that state.)

catch’s picture

I'm aware of at least one person who upgraded their site with no comment bodies and didn't keep the backup, so messed up filenames seems quite minor compared to that case.

alfaguru’s picture

This bug, or something like it, appears to be still in effect. I just found that the new anti-DoS code in Drupal 7.20 breaks for those of my images which were uploaded to the public root, because for those the uri in the file_managed table is of the form public:///... and the one generated for comparison when checking a token is public://... Those in sub-directories are OK, and the site is still in dev, so there's a workround (apart from setting "image_allow_insecure_derivatives").

David_Rothstein’s picture

Just linking to the other issue you filed: #1923554: New anti-DoS measure breaks for some file URIs

In the end it looks like it's unrelated to this one. (This issue only occurs for private files which get migrated into file fields, but that one is for image fields - and affects public files too.)

Sites which had private files and upgraded from Drupal 6 will get the extra slash too, but most likely there is no overlap with the image derivative issue.

jaredsmith’s picture

This issue could use an updated issue summary based on the templates at http://drupal.org/issue-summaries.

David_Rothstein’s picture

Fabianx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

RTBC, very straightforward, not sure its still relevant, but definitely not good to leave just hanging around.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: system-update-7061-1404050-10.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.36 release notes

Thanks for the final review. OK, let's do this...

If someone wants to follow up with tests or with an additional update function to fix the problem on sites that already did a D6-to-D7 upgrade, I think they could file a new issue for that and link to it here.

Committed to 7.x - thanks!

  • David_Rothstein committed 9dafd62 on 7.x
    Issue #1404050 by JamesOakley, David_Rothstein: system_update_7061...

Status: Fixed » Closed (fixed)

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