Bug 25649 – [webkit-gtk] Wrong handling of file upload if no file selected

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Stephan Haller]

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

[Stephan Haller]

Created attachment 30134 [details]
PHP script to test file upload in form

[Stephan Haller]

Created attachment 30135 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

[Jan Alonzo]

(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).

[Stephan Haller]

Created attachment 30239 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

Corrected patch file - added Changelog

[Mark Rowe (bdash)]

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.

[Stephan Haller]

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 ;)

[Gustavo Noronha (kov)]

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.

[Stephan Haller]

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 :(

[Gustavo Noronha (kov)]

Comment on attachment 30259 [details]
Patch against WebKit-GTK 1.1.6 for correct handling of file uploads in form

Looks good now =).

[Gustavo Noronha (kov)]

(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 =).

[Gustavo Noronha (kov)]

(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.