util.lisp

(defun make-upload-filename ()
  (etypecase *upload-filename-generator* 
    ;; the old behaviour.
    (null t)
    ;; the new behaviour.
    ((or symbol function) 
     (lambda (&rest args)
       (let ((filename (apply *upload-filename-generator* args)))
         (when *file-upload-hook*
           (funcall *file-upload-hook* filename))
         filename)))
    (directory-pathname
     (lambda (&rest args &key file-name)
       (let ((filename (make-pathname :name file-name
                                      :directory *upload-filename-generator*)))
         (when *file-upload-hook*
           (funcall *file-upload-hook* filename))
         filename)))))

(deftype directory-pathname ()
  '(and pathname (satisfies cl-fad:directory-pathname-p)))

You asked for review: The clauses for a function designator and a directory pathname are almost identical. Please refactor so that no code duplication occurs. Also, note that all functionality that you add needs documentation.

Maybe you can come up with a better name for upload-filename-generator, now that it is can be a directory pathname.

The invocation of the upload-filename-generator function should have :allow-other-keys t - That actually makes sense.

Finally, not having the rest of the source before me, why can make-upload-filename return T? Could the NIL case for upload-filename-generator be handled at the call site?

-Hans

this is the call site. https://github.com/fighting-spirit/hunchentoot/blob/master/request.lisp#L127 the T case is the old default value of :write-content-to-file (as to not disturb old users)

…

On Mon, May 21, 2012 at 11:19 PM, Hans Hübner ***@***.*** wrote: You asked for review:  The clauses for a function designator and a directory pathname are almost identical.  Please refactor so that no code duplication occurs.  Also, note that all functionality that you add needs documentation. Maybe you can come up with a better name for _upload-filename-generator_, now that it is can be a directory pathname. The invocation of the _upload-filename-generator_ function should have :allow-other-keys t - That actually makes sense. Finally, not having the rest of the source before me, why can make-upload-filename return T?  Could the NIL case for _upload-filename-generator_ be handled at the call site? -Hans --- Reply to this email directly or view it on GitHub: https://gist.github.com/2763794

I'll look into other comments and include them in the fork.

Thanks for your time.

On Mon, May 21, 2012 at 9:29 PM, Ala'a Mohammad ***@***.*** wrote: this is the call site. https://github.com/fighting-spirit/hunchentoot/blob/master/request.lisp#L127 the T case is the old default value of :write-content-to-file (as to not disturb old users)

I find this rather confusing, really. The name make-upload-filename gives me the impression that the function would return a file name, but it really returns a filename generator, right? If that is the case, Then it should be called make-upload-filename-generator. The special case when _upload-filename-generator_ is NIL should be dealt with at the call site, as that really is something that makes sense only in the context of the rfc2388 API. Then, make-upload-filename-generator would always return a function that generates a filename when called. I am not quite sure whether I'm in favor of the directory-pathname approach yet, but that is also because I have not reviewed the call site thoroughly. The problem that I see is that the filename as specified in the MIME part is used to construct the pathname without further validation. Is this safe against filenames in the MIME part that contain relative or absolute directory specifications? While the same problem exists with a function designator (i.e. a naive user could easily make the same mistake), if the functionality is provided by Hunchentoot, it should be reasonably secured against straightforward attacks and not contain potential security leaks that cannot be fixed (i.e. if the directory-pathname mechanism is in there, it must be safe against malicious file names in the MIME part or be left out).

…

-Hans

Hi, I'd changed the name of that function to make-upload-filename-generator I'd also removed the directory-pathname clause, and will consider it in future. I do understand the call-site proximity about RFC2388 API. This new function 'make-upload-filename-generator', could be written directly at call site, but since this function has to know what to handle to the RFC2388 API, then my purpose of this function was abstracting - or more of refactoring of the code at call site. We can do one thing which is to move the function definition near the call-site, but I prefer it near the make-tmp-file-name function in util.lisp Regards, Ala'a On Mon, May 21, 2012 at 11:51 PM, Hans Hübner reply@reply.github.com wrote:

…

On Mon, May 21, 2012 at 9:29 PM, Ala'a Mohammad ***@***.*** wrote: > this is the call site. > > https://github.com/fighting-spirit/hunchentoot/blob/master/request.lisp#L127 > > the T case is the old default value of :write-content-to-file (as to > not disturb old users) I find this rather confusing, really.  The name make-upload-filename gives me the impression that the function would return a file name, but it really returns a filename generator, right?  If that is the case, Then it should be called make-upload-filename-generator.  The special case when _upload-filename-generator_ is NIL should be dealt with at the call site, as that really is something that makes sense only in the context of the rfc2388 API.  Then, make-upload-filename-generator would always return a function that generates a filename when called. I am not quite sure whether I'm in favor of the directory-pathname approach yet, but that is also because I have not reviewed the call site thoroughly.  The problem that I see is that the filename as specified in the MIME part is used to construct the pathname without further validation.  Is this safe against filenames in the MIME part that contain relative or absolute directory specifications? While the same problem exists with a function designator (i.e. a naive user could easily make the same mistake), if the functionality is provided by Hunchentoot, it should be reasonably secured against straightforward attacks and not contain potential security leaks that cannot be fixed (i.e. if the directory-pathname mechanism is in there, it must be safe against malicious file names in the MIME part or be left out). -Hans --- Reply to this email directly or view it on GitHub: https://gist.github.com/2763794

(defun make-upload-filename-generator ()
  (etypecase *upload-filename-generator* 
    ;; the old behaviour.
    (null (lambda (&rest args)
            (declare (ignore args))
            (funcall #'make-tmp-file-name)))
    ;; the new behaviour.
    ((or symbol function) 
     (lambda (&rest args)
       (let ((filename (apply *upload-filename-generator* args :allow-other-keys t)))
         (when *file-upload-hook*
           (funcall *file-upload-hook* filename))
         filename)))))

This looks good - One more thing - The file-upload-hook invocation should be moved out of make-tmp-file-name and called from the function that is returned from make-upload-filename-generator, something like;

(let ((make-filename (ecase ...)))
(lambda (&rest args)
(let ((filename (funcall make-filename args)))
(when file-upload-hook
(funcall ..))
filename)))

i.e. don't spread the hacky file-upload-hook throughout the code if it can be localized to one place.

Thanks,
Hans