Bug 168871 – [GTK] Downloads attributes tests are failing

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

[Michael Catanzaro]

r212972 "Download attribute should be sanitized before being used as suggested filename" added several new layout tests, all of which are broken for GTK port. These tests always fail:

fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html

This test always times out:

fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html

Text diffs for the two failing tests:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote-actual.txt
@@ -1,5 +1,5 @@
 Download started.
-Downloading URL with suggested filename "test"abe.png"
+Downloading URL with suggested filename "test\"abe.png"
 Download completed.
 The suggested filename above should be 'test"abe.png' and the download should succeed.

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-actual.txt
@@ -1,6 +1,7 @@
 Download started.
-Downloading URL with suggested filename "test1_test2abe.png"
-Download completed.
+Downloading URL with suggested filename "test1/test2\\abe.png"
+Download failed.
+Failed: WebKitNetworkError, code=302, description=Load request cancelled
 The suggested filename above should NOT include slashes or backslashes and the download should succeed.
 
 File backed blob URL

I'll update the expectations accordingly.

[Carlos Garcia Campos]

Created attachment 302784 [details]
Patch

[Build Bot]

Comment on attachment 302784 [details]
Patch

Attachment 302784 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3195756

New failing tests:
scrollingcoordinator/ios/non-stable-viewport-scroll.html

[Build Bot]

Created attachment 302785 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

Comment on attachment 302784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302784&action=review

> LayoutTests/ChangeLog:10
> +        differently but download success.

success -> succeeds

> Source/WebCore/ChangeLog:9
> +        Use libsoup to get the suggested filename from the Content-Disposition header instead of buggy
> +        filenameFromHTTPContentDisposition().

What is buggy about it? Shouldn't we fix that bug, especially if it's affecting all ports? It sounds like it requires a adding FIXME at least?

I'd rather use libsoup to get the filename *inside* the implementation of filenameFromHTTPContentDisposition if need be. Avoiding use of filenameFromHTTPContentDisposition feels like a workaround.

[Chris Dumez]

Comment on attachment 302784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302784&action=review

> LayoutTests/platform/gtk/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-expected.txt:2
> +Downloading URL with suggested filename "test2\abe.png"

Seems unfortunate that backslash is not sanitized but I guess you guys do not have to worry about Windows?

>> Source/WebCore/ChangeLog:9
>> +        filenameFromHTTPContentDisposition().
> 
> What is buggy about it? Shouldn't we fix that bug, especially if it's affecting all ports? It sounds like it requires a adding FIXME at least?
> 
> I'd rather use libsoup to get the filename *inside* the implementation of filenameFromHTTPContentDisposition if need be. Avoiding use of filenameFromHTTPContentDisposition feels like a workaround.

Is it buggy or does it just not sanitize? For what it is worth, on mac and iOS, we also let CFNetwork deal with this. CFNetwork takes care of sanitizing for us and it has the benefit of being consistent between the 2 code paths: regular content disposition header & download attribute.

> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:99
> +    SoupMessageHeaders* soupHeaders = soup_message_headers_new(SOUP_MESSAGE_HEADERS_RESPONSE);

Not familiar with the GTK code base but why do you need to create your own soup headers? Cannot you just get the ones of the underlying soup response?

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 302784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302784&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        differently but download success.
> 
> success -> succeeds

Oops.

> > Source/WebCore/ChangeLog:9
> > +        Use libsoup to get the suggested filename from the Content-Disposition header instead of buggy
> > +        filenameFromHTTPContentDisposition().
> 
> What is buggy about it? Shouldn't we fix that bug, especially if it's
> affecting all ports? It sounds like it requires a adding FIXME at least?

It's buggy because there's already a FIXME comment explaining it:

// FIXME: This function doesn't comply with RFC 6266.                                                                                                                                         
// For example, this function doesn't handle the interaction between " and ;                                                                                                                  
// that arises from quoted-string, nor does this function properly unquote                                                                                                                    
// attribute values. Further this function appears to process parameter names                                                                                                                 
// in a case-sensitive manner. (There are likely other bugs as well.)                                                                                                                         
String filenameFromHTTPContentDisposition(const String& value)

It doesn't affect other ports, it's only used by curl backend, I think. I don't even know what other port is using curl, I guess it's WinCairo.

> I'd rather use libsoup to get the filename *inside* the implementation of
> filenameFromHTTPContentDisposition if need be. Avoiding use of
> filenameFromHTTPContentDisposition feels like a workaround.

I don't think so, I see filenameFromHTTPContentDisposition as a platform independent fallback implementation when your platform code doesn't provide a better way.

[Carlos Garcia Campos]

(In reply to comment #5)
> Comment on attachment 302784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302784&action=review
> 
> > LayoutTests/platform/gtk/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-expected.txt:2
> > +Downloading URL with suggested filename "test2\abe.png"
> 
> Seems unfortunate that backslash is not sanitized but I guess you guys do
> not have to worry about Windows?

I haven't looked at libsoup code, but I suspect it sanitizes platform separator, which is / in unix. We can fix libsoup if needed.

> >> Source/WebCore/ChangeLog:9
> >> +        filenameFromHTTPContentDisposition().
> > 
> > What is buggy about it? Shouldn't we fix that bug, especially if it's affecting all ports? It sounds like it requires a adding FIXME at least?
> > 
> > I'd rather use libsoup to get the filename *inside* the implementation of filenameFromHTTPContentDisposition if need be. Avoiding use of filenameFromHTTPContentDisposition feels like a workaround.
> 
> Is it buggy or does it just not sanitize? For what it is worth, on mac and
> iOS, we also let CFNetwork deal with this. CFNetwork takes care of
> sanitizing for us and it has the benefit of being consistent between the 2
> code paths: regular content disposition header & download attribute.

See the FIXME comment.

> > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:99
> > +    SoupMessageHeaders* soupHeaders = soup_message_headers_new(SOUP_MESSAGE_HEADERS_RESPONSE);
> 
> Not familiar with the GTK code base but why do you need to create your own
> soup headers? Cannot you just get the ones of the underlying soup response?

We don't keep the soup message in the response, we only use it to build it.

[Michael Catanzaro]

(In reply to comment #6)
> > I'd rather use libsoup to get the filename *inside* the implementation of
> > filenameFromHTTPContentDisposition if need be. Avoiding use of
> > filenameFromHTTPContentDisposition feels like a workaround.
> 
> I don't think so, I see filenameFromHTTPContentDisposition as a platform
> independent fallback implementation when your platform code doesn't provide
> a better way.

I disagree. Up to you, though....

[Carlos Garcia Campos]

Committed r213062: <http://trac.webkit.org/changeset/213062>

[youenn fablet]

(In reply to comment #8)
> (In reply to comment #6)
> > > I'd rather use libsoup to get the filename *inside* the implementation of
> > > filenameFromHTTPContentDisposition if need be. Avoiding use of
> > > filenameFromHTTPContentDisposition feels like a workaround.
> > 
> > I don't think so, I see filenameFromHTTPContentDisposition as a platform
> > independent fallback implementation when your platform code doesn't provide
> > a better way.
> 
> I disagree. Up to you, though....

Probably worth filing a bug to remove filenameFromHTTPContentDisposition.
If it is only used by Curl-based ports, Curl should probably be used to parse the header.