http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6874 --- Comment #19 from Frère Sébastien Marie <semarie-koha@latrappe.fr> --- Hi, I only do a "visual review" of the code, but just some suggestions... In "C4/UploadedFiles.pm" - line 147:
if( -f $file_path ) { warn "Id $id not in database, but present in filesystem, do nothing"; return; }
Use "-e" instead of "-f": the difference is when $file_path is an existing link. -f say "not a file", but -e "a node exists" (may be a directory, a plain file, a socket, a pipe, a special block device, a symbolic link...) - line 153:
unless(open $out_fh, '>', $file_path) { warn "Failed to open file '$file_path': $!"; return; }
a race condition is possible between the check of file existence and the open of file. You could use sysopen function instead of file-check + open.
use IO::Handle; unless( sysopen($out_fh, $path, O_WRONLY|O_CREAT|O_EXCL) ) { warn "Failed to open file '$file_path': $!"; return; }
note: the file is created only if don't exist, due to O_CREAT|O_EXCL. There is not time between check and open because all is done by system kernel. In ".../cataloguing/value_builder/upload.tt" (and others templates): please use "html" filter. It is (generally) always to use it, a prevent XSS (here only "uploaded_file" seems to be build with user-input)
The file [% uploaded_file | html %] has been successfully uploaded.
In the syspref message, perhaps it should be added that the uploadPath MUST NOT be a public directory, accessible from the webserver (because else, the server could be exposed to arbitrary code execution by evil uploader). Else, the patch seems promising. Thanks -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.