Bug 25649

Summary: [webkit-gtk] Wrong handling of file upload if no file selected
Product: WebKit Reporter: Stephan Haller <nomad>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, jmalonzo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
URL: http://forum.gmc-clan.de
Attachments:
Description Flags
PHP script to test file upload in form
none
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form
none
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form
none
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form
gns: review-
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form gns: review+

Description Stephan Haller 2009-05-08 12:07:29 PDT
I faced a problem on our forum when using Midori, a WebKit-GTK browser. I tried to reply to a post but as soon as I clicked on submit I got the error message that my file upload was broken (empty). I was so irritated by this error message that I wrote a short html file containing a form for file upload (see attachment).

If I load this page and press submit without choosing a file I'll get this $_FILES array in a php script:

Array
(
    [file_test] => Array
        (
            [name] => .
            [type] => application/octet-stream
            [tmp_name] => /tmp/phpgo127Y
            [error] => 0
            [size] => 0
        )
)

WebKit-GTK tried to upload a file called "." without a file selected for upload. I have never clicked on the button to choose any file. I found the problem in WebCore/platform/gtk/FileSystemGtk.cpp near line 180 in the function pathGetFileName(...). As the documentation of glib for function "g_path_get_basename" says it will return the file name "." (dot) if the string is empty and WebKit wants to send an empty file name. So write a patch (see attachment again) returning an empty string if the given file name and path is empty also. Now I can post again in my forum and I get this result in my php script:

rray
(
    [file_test] => Array
        (
            [name] => 
            [type] => 
            [tmp_name] => 
            [error] => 4
            [size] => 0
        )
)

Regards,
Stephan Haller
Comment 1 Stephan Haller 2009-05-08 12:08:46 PDT
Created attachment 30134 [details]
PHP script to test file upload in form
Comment 2 Stephan Haller 2009-05-08 12:09:57 PDT
Created attachment 30135 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form
Comment 3 Jan Alonzo 2009-05-12 08:56:31 PDT
(In reply to comment #2)
> Created an attachment (id=30135) [review]
> Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form
> 

Hi Stephen. Thanks for the patch. This patch requires a ChangeLog (see http://webkit.org/coding/contributing.html for more info). I'm also downgrading the priority to P2 since this defect is not immediate (i.e., not a high-priority crasher).
Comment 4 Stephan Haller 2009-05-12 09:57:40 PDT
Created attachment 30239 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

Corrected patch file - added Changelog
Comment 5 Mark Rowe (bdash) 2009-05-12 11:00:44 PDT
You should set the review flag to ? to get a patch reviewed.  I see a few issues with the patch that should be addressed:
1) It would be clearer to be structured to have an early-return
2) It uses tabs for indentation instead of spaces
3) A return statements has redundant parentheses.
4) There should be a space after "if" and before the parenthesis.
Comment 6 Stephan Haller 2009-05-12 11:28:36 PDT
Created attachment 30244 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

Restructured to have an early return, tried to follow code guidelines ;)
Comment 7 Gustavo Noronha (kov) 2009-05-12 11:34:35 PDT
Comment on attachment 30244 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

> +    if (pathName.isEmpty())
> +         return path;
> +

This is wrong. There's no 'path' in this context, it seems.
Comment 8 Stephan Haller 2009-05-12 19:45:28 PDT
Created attachment 30259 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

Oops, sorry. Yes, it wasn't. Corrected and recompiled successfully. I hope I haven't made more mistakes in just these few lines :(
Comment 9 Gustavo Noronha (kov) 2009-05-13 07:31:45 PDT
Comment on attachment 30259 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

Looks good now =).
Comment 10 Gustavo Noronha (kov) 2009-05-13 12:06:55 PDT
(In reply to comment #9)
> (From update of attachment 30259 [details] [review])
> Looks good now =).
> 

Except that you patched the wrong ChangeLog file, and had trailing spaces at the end of some sentences. Use ./WebKitTools/Scripts/prepare-ChangeLog next time =).
Comment 11 Gustavo Noronha (kov) 2009-05-13 12:13:22 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 30259 [details] [review] [review])
> > Looks good now =).
> > 
> 
> Except that you patched the wrong ChangeLog file, and had trailing spaces at
> the end of some sentences. Use ./WebKitTools/Scripts/prepare-ChangeLog next
> time =).

Also, you had an additional space before the return. The indentation is 4 spaces, not 5 =). I fixed those and landed the patch as r43649. Thanks.