Comment # 19 from Frère Sébastien Marie

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