Bug 168839 – Download attribute should be sanitized before being used as suggested filename

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Download attribute should be sanitized before being used as suggested filename for download.

[Chris Dumez]

<rdar://problem/30683109>

[Chris Dumez]

Created attachment 302692 [details]
Patch

[Chris Dumez]

Comment on attachment 302692 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:384
> +            downloadAttribute = ResourceResponse::sanitizeSuggestedFilename(attributeWithoutSynchronization(downloadAttr));

We could also use FileSystem.h 's encodeForFileName() but it does %-encoding and it would be inconsistent with what CFNetwork does.

[Chris Dumez]

Comment on attachment 302692 [details]
Patch

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

> LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode-expected.txt:2
> +Downloading URL with suggested filename "你好.png"

This is due to bugzilla. The output does look correct locally.

[Chris Dumez]

Comment on attachment 302692 [details]
Patch

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

> Source/WebCore/platform/network/ResourceResponseBase.cpp:230
> +    ResourceResponse response(URL(ParsedURLString, "http://example.com"), String(), -1, String());

We need a HTTP URL and a non-zero status code or ResourceResponse::initNSURLResponse() will not process the headers when constructing the NSURLResponse.

[Darin Adler]

Comment on attachment 302692 [details]
Patch

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

> Source/WebCore/platform/network/ResourceResponseBase.cpp:235
> +    StringBuilder value;
> +    value.appendLiteral("attachment; filename=\"");
> +    value.append(suggestedFilename.replace("\"", "\\\""));
> +    value.appendLiteral("\"");

This could be done more efficiently without a StringBuilder:

    String value = makeString("attachment; filename=\"", suggestedFilename.replace("\"", "\\\""), ''"');

Or you could use operator+ if you like that syntax better than makeString.

I’m also surprised that we don‘t have an overload of replace that takes a char for the first argument and a const char* for the second argument. If we do, then that should be used.

> Source/WebCore/platform/network/ResourceResponseBase.h:117
> +    WEBCORE_EXPORT static String sanitizeSuggestedFilename(String);

Typically the argument would be a const String& rather than a String to avoid unnecessary reference count churn.

[Chris Dumez]

Comment on attachment 302692 [details]
Patch

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

>> Source/WebCore/platform/network/ResourceResponseBase.h:117
>> +    WEBCORE_EXPORT static String sanitizeSuggestedFilename(String);
> 
> Typically the argument would be a const String& rather than a String to avoid unnecessary reference count churn.

This issue is that String::replace() is not const. I can make the parameter a const String&, but then I'll need a local variable.

[Chris Dumez]

Created attachment 302694 [details]
Patch

[Chris Dumez]

Created attachment 302696 [details]
Patch

[Chris Dumez]

(In reply to comment #6)
> Comment on attachment 302692 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302692&action=review
> 
> > Source/WebCore/platform/network/ResourceResponseBase.cpp:235
> > +    StringBuilder value;
> > +    value.appendLiteral("attachment; filename=\"");
> > +    value.append(suggestedFilename.replace("\"", "\\\""));
> > +    value.appendLiteral("\"");
> 
> This could be done more efficiently without a StringBuilder:
> 
>     String value = makeString("attachment; filename=\"",
> suggestedFilename.replace("\"", "\\\""), ''"');
> 
> Or you could use operator+ if you like that syntax better than makeString.
> 
> I’m also surprised that we don‘t have an overload of replace that takes a
> char for the first argument and a const char* for the second argument. If we
> do, then that should be used.

We do have such overload. I am using it now.

> 
> > Source/WebCore/platform/network/ResourceResponseBase.h:117
> > +    WEBCORE_EXPORT static String sanitizeSuggestedFilename(String);
> 
> Typically the argument would be a const String& rather than a String to
> avoid unnecessary reference count churn.

[Chris Dumez]

Committed r212972: <http://trac.webkit.org/changeset/212972>

[Michael Catanzaro]

New tests are all failing for GTK, see bug #168871.