Locale module's po import is not safe mode ready. It tries to open and read the uploaded file straight from the PHP upload tmp dir, which is usually out of open_basedir on systems, where safe mode is turned on: ie. really cheap or free hosting accounts. The solution is to move_uploaded_file() the uploaded po to the Drupal temporary folder, import from there and remove the file. Since the file API is not yet move_uploaded_file() ready AFAIK, it needs to be made aware of this PHP restriction and provide a wrapper for this function, or locale module needs to use the PHP functions right away.

CommentFileSizeAuthor
#1 Drupal-save-import-upload.diff3.75 KBgábor hojtsy

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new Drupal-save-import-upload.diff3.75 KB

Here is something to show what needs to be changed. Although I think this solution is not perfect, I have tried to signify earlier that I am not much experienced in the fileapi inner workings, so I am not the perfect person to submit this patch, but noone seems to get it up.

The patch changes file_copy() to accept a move request, so that uploaded files, which are only about to be moved from the PHP upload folder (where they would get removed automatically later) can reuse the checking code in file_copy(). This makes file_move() a wrapper around file_copy(). Unsure if a $file object exclusively means that we deal with an uploaded file, I have added a new isuploaded field, so that if a file is an uploaded one, we can perform a move_uploaded_file() operation, which is the only safe_mode compatible move we can do. copy() an unlink() does not work for files outside the open_basedir.

I do think this patch needs heavy review, and I do think it should go into Drupal 4.5.1. Until this is fixed, people on systems with safe_mode are unable to upload and import a po file, thus they are unable to translate their website interface...

Steven’s picture

Steven commented

This patch means that you need to have the filesystem correctly configured to import a PO file. Not sure if this is a problem, but we should be aware of that.

I'm not sure we should keep file_move(). It's just a wrapper for file_copy(), I say get rid of it.

As far as $file->isuploaded goes, perhaps $file->uploaded would be better. We typically avoid varname prefixes like 'is' or 'not'.

About this:
+ $isuploaded = FALSE;
....
+ $isuploaded = isset($file->isuploaded) && $file->isuploaded;

Couldn't you just do away with all this? The 'isuploaded' member is only set to TRUE once, if it's unset then casting it to bool would be FALSE as well. In fact, I don't see why you need the $isuploaded variable at all. Just use if ($file->isuploaded).

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

First I am unsure that I sent a clean solution. I also thought about moving out the checking logic from file_copy(), and introducing a file_move_uploaded(), which would reuse that checking logic. That would keep file_move() intact.

Checking for if ($file->isuploaded) is only safe if this member is always set. It could be, if every $file object comes from a file upload, but then this member is redundant... Otherwise you would get an E_NOTICE if that member is not set. My impression was that Drupal now tries to have clean code, which is E_NOTICE safe. I introduced the precomputed $isupload to avoid using the expression I have used to compute it multiple times in the patch.

Anyway, we need more people to review this.

killes@www.drop.org’s picture

killes@www.drop.org commented

Does not apply anymore.

wulff’s picture

wulff commented
Version: » x.y.z

Still applies. Uploading a .po file when safe_mode is on and open_basedir is set results in the following errors:

warning: fopen(): open_basedir restriction in effect. File(C:\WINNT\TEMP\php22.tmp) is not within the allowed path(s): (d:\wwwroot) in d:\wwwroot\drupal-cvs\includes\locale.inc on line 271.

warning: fopen(C:\WINNT\TEMP\php22.tmp): failed to open stream: Operation not permitted in d:\wwwroot\drupal-cvs\includes\locale.inc on line 271.

warning: Cannot modify header information - headers already sent by (output started at d:\wwwroot\drupal-cvs\includes\common.inc:409) in d:\wwwroot\drupal-cvs\includes\common.inc on line 216.
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Closed (duplicate)

Continued in the issue opened by Jose (long before this one).