Flurl: Content disposition file name is not escaped

[tomasr78]

There is bug in content disposition filename reading. You just read the string from content disposition string and pass it as file name to write to disc. That's would cause a lot of download url to fail. The content disposition could have a lot of invalid string which can't be used to create files, like ?\

Example to reproduce the problem

resp.Content?.Headers.ContentDisposition = {attachment; filename="Rencontre_de_fin_d?année.pptx"; filename*=UTF-8''Rencontre_de_fin_d%E2%80%99ann%C3%A9e.pptx}

Problem here

https://github.com/tmenier/Flurl/blob/1b072fce15e0bca7f064a2d265fcf49d5125025d/src/Flurl.Http/DownloadExtensions.cs#L27

and here

https://github.com/tmenier/Flurl/blob/1b072fce15e0bca7f064a2d265fcf49d5125025d/src/Flurl.Http/DownloadExtensions.cs#L32

[tomasr78]

You should wrap content disposition file name string with the method

 public static string MakeFileNameValid(string fileName)
    {
        return Path.GetInvalidFileNameChars().Aggregate(fileName, (current, c) => current.Replace(c, '_'));
    }

[tomasr78]

Another problem which I see in your file name decoding is how you read file name from URL. The string extracted from URL.Path must be uri decoded like this Uri.UnescapeDataString or System.Web.HttpUtility.UrlDecode() before sending it to file writer and as I said in my previous post also wrapped into MakeFileNameValid() method. Current DownloadFileAsync implementation would fail a lot in real life application.

        localFileName =
                    localFileName ??
                    resp.Content?.Headers.ContentDisposition?.FileName?.StripQuotes() ??
                    request.Url.Path.Split('/').Last();

[tomasr78]

Another small issue is that FileNameStar is ignored from ContentDisposition - filename reading would not work on unicode file names. Is here any reason not to use FileNameStar?

[tmenier]

Is here any reason not to use FileNameStar?

Because to be honest, I don't know how. That's new to me. How exactly would I use that? Can you provide small code sample that handles a scenario where both FileName and FileNameStar are provided? I did some googling and this is the closest thing I could come up with. Is that correct - simply prefer FileNameStar over FileName if it's present?

Your other suggestions are great. I'll try and get those done soon. #402 will go out with the next release, so at least you can fall back on providing your own filename when Flurl's heuristics for inferring one aren't perfect.

[tmenier]

All suggestions here have been implemented, including preferring filename* over filename, which is indeed correct per RFC when both are present. Thanks for the suggestions.

[tomasr78]

@tmenier Great! Sorry that I didn't answered your question, just missed GitHub notification in a lot of e-mails. All my suggestions here from my real experience while implementing Download File functionality in our file conversion rest api https://www.convertapi.com. The main problem with content-disposition that many web applications do not follow RFC and do not format content disposition correctly or even do not provide it like google drive, drop box or github.

I will test your library in next month and let post other issues if I will find one. Overall nice easy to use library, should be considered to include in next official .NET release by Microsoft :)

[tmenier]

Ha, thanks for the kind words. :)

Here's the test cases if you're interested:

https://github.com/tmenier/Flurl/blob/dev/Test/Flurl.Test/Http/RealHttpTests.cs#L51-L72

I probably could have built the name inference rules in a more unit-testable way, but this worked out. I can't believe how long it took me to get the filename* value to come through in the response object - it was surprisingly hard to find a simple example of how to format that. Good learning exercise though.