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.
Comment | File | Size | Author |
---|---|---|---|
#10 | system-update-7061-1404050-10.patch | 1.24 KB | David_Rothstein |
#4 | system_update_7061_trim_slash-1404050-4.patch | 964 bytes | JamesOakley |
#1 | system_update_7061_trim_slash-1404050-1.patch | 765 bytes | JamesOakley |
Comments
Comment #1
JamesOakleyI think this is the answer:
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedfile_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.
Comment #3
JamesOakleyI 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.
Comment #4
JamesOakleyI'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.
Comment #5
JamesOakleyComment #6
tim.plunkettComment #7
David_Rothstein CreditAttribution: David_Rothstein commentedJust 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?
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, assuming this affects system_update_7061() only, there's nothing to do here for Drupal 8.
Comment #9
catchBumping 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.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedI 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).
Comment #11
JamesOakleyI 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
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedThanks!
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 :)
Comment #13
JamesOakleySadly, 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.
Comment #14
catchDo 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.
Comment #15
YesCT CreditAttribution: YesCT commented@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?
Comment #16
YesCT CreditAttribution: YesCT commented#10: system-update-7061-1404050-10.patch queued for re-testing.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, 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.)
Comment #18
catchI'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.
Comment #19
alfaguru CreditAttribution: alfaguru commentedThis 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").
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedJust 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.
Comment #21
jaredsmith CreditAttribution: jaredsmith commentedThis issue could use an updated issue summary based on the templates at http://drupal.org/issue-summaries.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedAdding tag so this shows up on the list at #2030501: [meta] Ensure that Drupal 6 sites have a functional upgrade path to either Drupal 7 or 8 before Drupal 6 loses security support.
Comment #23
Fabianx CreditAttribution: Fabianx commentedRTBC, very straightforward, not sure its still relevant, but definitely not good to leave just hanging around.
Comment #25
Fabianx CreditAttribution: Fabianx commentedComment #27
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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!