Error with file api and open_basedir restriction

This seems to be an interesting alternate fix to my dump patch for locale.module, which is the very same issue... Moving the file out of the upload folder right away to the Drupal temp folder seems to be a good idea to me. The only caveat is that files in the upload folder get removed automatically, while one needs to remove the file from the Drupal temp folder, once it is not needed anymore!

Please look at this stuff. We need to solve open_basedir problems at least in core before the next point release. There are quite a few users trying to use Drupal on shared hosting here, where open_basedir is in effect.

This functionality gets a +1 from me, the user. We solved this problem by changing upload_tmp_dir in the server config, but a fix in Drupal would be more elegant.

Doesn't apply anymore.

I gave your patch a try. It applies fine but doesn't address several issues, particularily with the image module.

The attached patch makes attachments work, along with image (from cvs). I haven't seen any ill effects yet, but I believe that file_delete must be used to avoid getting a build-up of dead files in the local temporary directory (set to getcwd().'/tmp'). This isn't a problem with my install, but it might be a problem with other modules.

In order for the patched code to work, $drupal/tmp must exist and be writable by the web server.

+1 for Jose's patch

(I've attached an updated version, which I've tested successfully. The new version uses $file->filepath rather than $file->path and cleans up the code a bit.)

While the larger patch suggested may have merit, I don't fully follow it (not knowing what the image module issues are). I'd like to see the identified problem fixed first, and quickly. Later cleanup and further patches can follow.

This is a significant issue, meaning many in hosted environments can't use Drupal's file uploading. This has come up at least twice in the help forums. Jose's solution is simple. Let's apply it!

This same changes work for 4.6.

I'll describe the issue that I found with file.inc concerning image.module.

From what I've seen, image.module calls file_copy() directly from _image_insert(). file_copy() doesn't call file_check_upload(), so the open_basedir problem remains because file_copy() attempts to access the file while it's still in the restricted directory. I'm sure that there's a cleaner way to do it, but the patch that I submitted does the trick for my purposes by addressing problems with both file_copy() and file_check_upload().

I'll have a go at making a smaller patch that only addresses the issue with file_copy(), since Nedjo's patch neatly fixes file_check_upload().

It seems likely that the image module issue - with using file_copy() directly - can be addressed by adding a file_check_upload() call in the file_copy() function by changing

  // Process a file upload object.
  if (is_object($source)) {
    $file = $source;

to

  // Process a file upload object.
  if (is_object($source)) {
    $file = file_check_upload($source);

I can't immediately test this. Do you want to, mpd?

This needs to get fixed for Drupal 4.7, or otherwise Drupal is not "hosted environment friendly". People cannot even start with importing a translation with open_basedir restrictions (which is quite common here in Hungary, because of security precautions).

How is it that I use this patch. There isn't very much code compared to the original file. Do i copy/paste over just a part of the text in the original file?

thanks!

can somebody explain how to apply this patch?

+1 for me too.

The only thing I would add would be to warn about files exceeding the PHP UPLOAD_ERR_INI_SIZE. Something like

else {
if ($_FILES["edit"]["error"][$source] == UPLOAD_ERR_INI_SIZE) {
print "The uploaded file exceeds the upload_max_filesize directive (".ini_get("upload_max_filesize").") in php.ini.";
}
// In case of previews return previous file object.
if (file_exists($_SESSION['file_uploads'][$source]->filepath)) {
return $_SESSION['file_uploads'][$source];
}
}

in the final clause of file_check_upload(). I don't know what the proper way of doing drupal error messages is, otherwise I would submit a patch.

Our Hungarian users are coming in crowds because they cannot upload the translation on hosted environments. This is a mission critical patch IMHO, and it needs to get into 4.7.0. See the comments from those, who already tested, and test the patch if possible.

Same problem here in Belgium, we need Dutch & French translations.

Ok, here I come. I said this is important, and since the guys who previously worked on it (Jose and Nedjo) are not following the route, I have set up a testing environment with a very restrictive open_basedir setting. I took Nedjo's latest patch and moved the code to the proper place and made some small modifications.

The problem with the current file API is that it thinks that the location where the file was uploaded is safe to work with (read from, write to). However, on most hosted environments, this is not the case, as open_basedir does not let you to tinker with folders outside of the allowed ones. The most useful function here is move_uploaded_file, which allows you to move stuff from the upload folder to a folder under the open_basedir umbrella. So before doing any tinkering with an uploaded file, if we smell open_basedir, we should move the file. The attached patch does just this.

Things to note:

• Users need to set their temp folder to a folder under the open basedir for this to work. This is inevitable, as Drupal needs a folder to work with for temp files.
• My involvement in this bug report started with the realization that you cannot import translations on hosted servers because of this issue. Now with this patch, you happily can, given that you have your temp folder properly configured in Drupal.
• Any module doing work on uploaded files should first do a file_check_upload() to ensure that the file object is properly set and the file is moved to a place where it is possible to work with. AFAIS this was the supposed way of using the file API all along.
• Since the uploaded file is moved to a temp folder, it is the developer's task to remove it, if there is no use for it anymore. It is not removed by PHP automatically (as with uploaded and untouched files).

This is a critical issue, and needs to be fixed. I tested this extensively tonight on Drupal HEAD with the locale import form on an open_basedir limited environment and it seems to be working just fine.

Additionaly to the locale import, I also tested the patch with the upload module (with AJAX file uploads and without AJAX file uploads with preview submits). It works perfectly. Before submitting the node the files are saved properly in the temp folder (with their readable names), after submitting the node, they get moved to the files folder.

I also went on to test the user picture upload, although user account editing is completely broken in todays HEAD version. For my test it was not needed however that the picture is properly saved in the datase (which did not happen). It was enough that the picture is properly saved in the file system and is handed over to the forms api handlers to save to the DB. The problem is in the forms API handlers, see the linked issue.

To make the picture uploading work however, I also needed a previous oneliner from Jose, since the picture moving was not done on the processed upload, so it would have worked improperly. The attached patch now includes this oneliner.

So I tested the patch with locale imports, upload module and picture uploads, and the files are all saved properly now even in a highly open_basedir restricted environment.

Please test, please apply. This is important.

Yes, we need this.

Hi,

i had a problem very simular to this one. When I tried to upload files I got an error ("The selected file ... could not be copied."), however open_basedir doesn't seem to be enabled on my server. I'm not sure whether it's caused by safe_mode or something else, but I've found out that the solution is the same, we have to use move_uploaded_file(). So I modified your patch a little bit to fix this case as well:

• I removed the check for open_basedir:
  if (!is_readable($file->filepath) && ini_get('open_basedir'))
  In fact, on my system, is_readable() returns true, ini_get('open_basedir') is an empty string, and yet move_uploaded_file() is neccessary. I believe the PHP manual says that that's the way uploads should be handled anyway, so it probably won't hurt even if it's not needed.
• In file_check_upload, there is a big if-clause with 3 cases, of which the second is called when the file is uploaded to the server, and the third case is reached in later calls, when the 'preview'-function is used. In this case, the location of the temporary file should be read from $_SESSION['file_uploads'][$source], but that never gets set, and file_check_upload returns an empty string instead of a file object.

  The reason is that on the first call, when the file is actually uploaded, it is checked for is_uploaded_file($_FILES["edit"]["tmp_name"][$source]) (line 144). However $_FILES["edit"]["tmp_name"][$source] is empty, the third case gets executed, and the function returns an empty string. Because of that, in file_save_upload(), line 395, the check fails, and the line where the session variable is supposed to be set is never reached. Kind of a vicious circle :-)

  My solution is to manually set $_SESSION['file_uploads'][$source] = $file; in the second case. I'm not quite sure if that's the best way to do it. Maybe it would be better to have $_FILES["edit"]["tmp_name"][$source] set in the first place.

I've attached a patch for file.inc only. It's made for 4.7.0 beta 2, not CVS, but it looks like the file hasn't changed much since then, so it should still work.

PS: Sorry if my post is confusing... It's late and I'm not used to writing English ;-)

Patches need to be merged? It could do with some extra documentation/comments.

Could someone please pick up this issue? I am seriously low on time having exams at the university, but think this should be in core for 4.7, or the prediction of Dries that Drupal is not going to cater for the small websites in 2006 immediately comes true.

Nacho, it would be nice to know what causes your problems, before doing "random" fixing. It might happen that your problem is actually easier to fix, or that it causes other problems elsewhere in Drupal.

Ok, I'll take up this one...

This patch addresses only the file.inc:file_check_upload(..) open_basedir restriction.
I dropped the special cases for if open_basedir is enabled or not, and just use move_uploaded_file to move all uploaded files to drupal's tmp dir for further processing... This is the recommended way to handle all php uploads.

The fileAPI is raw and not that intelligent. If modules are misusing it in its current state then bugs should be filed against those modules. It does not make sense that locale.module, or image.module issues be addressed in file.inc.. If all modules were affected I would say it is the proper location.

this patch was rolled against beta4 I can reroll against head if it is too far out of sync.

hrm.. meant code needs review..

I don't have open_basedir restriction on my host but I've tested file attachments with HEAD both before this patch and after.

Before:
1. Attaching a file with JS enabled fails. Script seems to time out and progress graphic does not move.
2. Attaching a file with JS disabled is successful. There is a file in 'files/tmp' called 'tmp_8PCgI7' after the edit is completed. The attached file is located in 'files'.

After:
1. Attaching a file with JS enabled fails. Same as above.
2. Attaching a file with JS disabled is successful. The file is placed in 'files' as expected but there is no leftover file in 'files/tmp'.

I also tested creating an image node with this patch applied and I get an error that I didn't get before, though it's likely a bug with image.module.

    * The selected file /home/foo/public_html/drupal-cvs/files could not be copied.
    * The selected file /home/foo/public_html/drupal-cvs/files could not be copied.
    * The selected file /home/foo/public_html/drupal-cvs/files could not be copied.
    * The selected file /home/foo/public_html/drupal-cvs/files could not be copied.
    * The selected file /home/foo/public_html/drupal-cvs/files could not be copied.
    * The selected file /home/foo/public_html/drupal-cvs/files could not be copied.

The lack of progress.gif animation is a problem in Firefox. I've double-checked the gif animation settings, deleted cache and history multiple times, and deleted and replaced progress.gif but cannot get it to animate. It animates if I load it locally (file > open) but not from that url. Anyway, can't blame Drupal for that. It doesn't animate from http://scratch.drupal.org/misc/progress.gif either. The strange thing is that it doesn't even look the same in Firefox as it does in IE. Can anyone else confirm or deny this?

The AJAX upload still doesn't work but I've taken that issue over to http://drupal.org/node/50366.

I'll try to work out the JS stuff tonight.. If anyone can correlate I would appreciate it. Image module is probably trying to grab the files from session, but they've already been moved by the new check upload functionality and it should be using the node's file object instead. I'll try to verify that as well.

I really appreciate someone finally giving me some feedback on this...

.darrel.

Guys, what if we concentrate on the open_basedir issue, which is the objective of this issue, and not progress gif and JS problems...

Goba can you test against head? Do you get the same result?

testing note:
this only applies when upload_tmp_dir is set out side of the open_basedir...
So if you do setup open_basedir, make sure php's upload_tmp_dir is not inside the tree.

Retested against HEAD.

I've tested with and without JS..

I do not experience the JS issues.
I do not have the left over file issues.

This is against head with only the single patch listed. No additional modules.

This patch changes the way file uploads are handled in core, and may break some contrib modules that handle file uploads.

The big change is, when file_check_upload is called, files are moved from php's upload_tmp_dir to drupal's tmp dir.

Thanks dopry for working on this important issue.

(Note: patch should be generated from the drupal directory, and use original file names (not file.inc.orig).)

I applied the patch and did some basic testing on a hosted server with open_basedir restriction. Prior to the patch no uploads were working.

Result of testing:

1. js file upload works as expected :)
2. Tried to upload a new logo for a theme. No error message, but no file in /files and a file left in files/tmp.

So, it seems we're partway there but not all the way.

Ok so I discovered that this is related to the fact the file_check_load is called multiple times and expects the data to be in the _files array, so I added a cache to keep track of what has already been uploaded. The issues regarding undeleted files in the drupal tmp dir is still a factor. There may be a need for some garbage collection based cron code...

Here is a patch with both the caching file_check_upload, and configurable cron driven garbage collection for drupal's file temp. There are further things that may need to done to prevent issues if drupal's tmp is the same as the PHP upload_tmp_dir. It may need a documentation upgrade that states that drupal needs its own tmp, not shared tmp directory if the ttl/file gc period in drupal is less that the file gc period for php.....

.darrel.

Here's a cumulative patch...

Did a quick test with this new patch, again with open_basedir restriction in place, and everything I tried works now.

* js file uploads
* user picture uploads
* theme logo uploads

and no more files left in files/tmp.

Haven't found time to review the code yet. Offhand I'd say we probably don't need to include the _cron function here. Useful perhaps, but not needed to solve this issue. (And if the issue is successfully solved, cleanup shouldn't be needed.)

Here is a patch with just the file.inc changes. There was a bug with the earlier patch with the cache checking coming before the is_object check. (Who would have thought that objects weren't valid array keys. ) This patch
also completes the error trap at the top so if a file object is passed and the filepath is incorrect file_check_upload will explicitly return false.

I've tested the patch in comment #40. As mentioned previously, my host is not open_basedir restricted but all functionality seems to work correctly.

I see only a few formatting nags with the patch.

1. It looks like some inappropriate whitespace changes snuck into this patch in the first chunk.
2. The watchdog line in the second chunk is indented too far and the "Upload Error" sentence seems unnecessary.

I agree that added cron cleanup logic is not, and should not be, necessary.

Now the whitespacing ugliness is removed... please test...

more style clean up after the unrelated comment assault... ;)...

This patch needs to be re-rolled. It no longer applies due to the mime-type magic being removed.

Quick review:

1. Code looks good.

2. The quality of the code comments could be better:
a. There is a lot of information captured in this issue. I think we can do a better job commenting the code.
b. We should also use proper punctuation.
c. Shouldn't we extend the PHPdoc above the function declaration?

3. The name file_check_upload() has a different meaning now. It no longer 'checks' your upload. It 'prepares' your upload. file_prepare_upload() might be a better name.

I improve the documentation. (i hope, my puctuation isn't so good.)

I turned file_check_upload into a wrapper function for file_prepare_upload, for compatability with existing contribs and core modules.

I moved the upload error checking in file_save_upload to file_prepare_upload.

I think file_save_upload could be completely rolled into file_check_upload. I don't understand the need to have these be seperate functions. I'm not sure if that is a change that should wait for 4.8.

Please remove the wrapper functionality.

The code comments are still sloppy: inconsistent use of capital letters and space.

indeed, one of the characteristics of drupal is "no backwards compatibility" "for compatability with existing contribs and core modules." equals bloat in drupalspeak.

I changed the function name back to file_check_upload. I don't feel aesthetic changes this late in the game are appropriate. I'll do the renaming when HEAD reopens. File handling is going to see a lot of work for 4.8 I believe.

I cleaned up my Capitalization and spacing in my comments.

also removed an unneccessary rename... that got in there somehow...

-- more spacing and capitalization cleanups.

Looks good. Two tiny details:

1. We write 'Drupal', not 'drupal'.
2. Maybe add a @TODO so we can rename the function later on.

We can commit this, after some final testing. Please report success/failure.

Well I'm going to leave the reviewing up to others, but here are, hopefully, the last of the comment cleanups.

Small remark: Drupal 4.7 requires 4.3.3 or higher so we can use the constants now ...

This patch, like the last version I tested, successfully fixes the open_basedir issue on my testing.

I applied the patch to today's CVS HEAD and tested on a server with open_basedir restriction.

Before the patch no file uploads worked. With the patch, all file uploads I tried worked as expected. Tested:

* js file uploads (via upload.module)
* user picture uploads
* theme logo uploads

nedjo,if that's so why you have not set the status? :)

Committed to HEAD. Marking this active as we can now use PHP's file constants. See my previous comment.

Here is the switch to using constants available in php 4.3.3.

There is one additional constant for 4.x series we are not using, but it is only available in >= 4.3.10

Setting to RTBC... I haven't had a problem locally and I've been running this patch on my test installation for wuite some time.

I uploaded a few times using this code and no ill effects. It is hard to exercise each code branch though. IMO, this is RTBC

applied