[BUG] BlobSasBuilder.ToString() doesn't apply URL encoding to ContentDisposition value.

[shuk]

Description
BlobSasBuilder.ToString() doesn't apply URL encoding to ContentDisposition value.

Context Details

• OS: Windows 10
• IDE : Visual Studio 2017
• Platform: .Net Core 2.2
• Library version: https://www.nuget.org/packages/Azure.Storage.Blobs/12.1.0

Code Snippet
The following unit test completely describes this bug:

    public class ToString
    {
        [Test]
        public void Applies_Url_Encoding()
        {
            // When
            var attachmentName = "test video.mp4";
            var attachmentContentDisposition = $"attachment;filename=\"{attachmentName}\"";

            var sasBuilder = new BlobSasBuilder
            {
                BlobContainerName = "test container name",
                BlobName = "test blob name.mp4",
                Resource = "b",
                ExpiresOn = DateTimeOffset.UtcNow.AddMinutes(30),
                ContentDisposition = attachmentContentDisposition
            };

            var storageSharedKeyCredential = new StorageSharedKeyCredential("AccountName", "AccountKey");

            sasBuilder.SetPermissions(BlobSasPermissions.Write);
            var result= sasBuilder
                .ToSasQueryParameters(storageSharedKeyCredential)
                .ToString();

            // Actual result: "sv=2019-02-02&se=2019-12-31T07%3A29%3A44Z&sr=b&sp=w&rscd=attachment;filename="test video.mp4"&sig=F6Udz5wKnhj%2BAeDcmQ7RsZzMas%2FLqRYoXcfkVan6xT0%3D"

            // Expected result: "sv=2019-02-02&se=2019-12-31T07%3A29%3A44Z&sr=b&sp=w&rscd=attachment;filename="test%20video.mp4"&sig=F6Udz5wKnhj%2BAeDcmQ7RsZzMas%2FLqRYoXcfkVan6xT0%3D"

            // Then
            result.Should().NotContain(" ");
        }
    }

Expected behavior
BlobSasBuilder.ToString() method should return string where ContentDisposition is URL encoded string as other parts of the result string.

[JoshLove-msft]

/cc @alzimmermsft @rakshith91 @jeremymeng
in case this is an issue in other languages as well.

[shuk]

@seanmcc-msft , merged code uses System.Net.WebUtility.UrlEncode hence all spaces in ContentDisposition will be replaced by "+" sign.

Probably we need to apply System.Web.HttpUtility.UrlPathEncode method and replace all spaces by "%20" instead of "+". The contend-disposition encoding was discussed here: https://stackoverflow.com/questions/93551/how-to-encode-the-filename-parameter-of-content-disposition-header-in-http

Does it make sense?

[seanmcc-msft]

I think + instead of %20 is fine, because the SAS is in the query of the URL. Also, in the unit test I added, the service is returning the non-url-encoded ContentDisposition header for blob.GetPropertiesAsync().

[jeremymeng]

JS doesn't have this problem

[hamish-omny]

The guidance from dotnet/runtime#45328 (comment) suggests not using WebUtility.UrlEncode

We recently upgraded from the now deprecated version of the storage SDK to this version and have hit an issue with spaces being encoded as + in the content disposition. Our situation is likely a little unique, but our issue is that + isn't always treated as a character that needs encoding by other systems. We have CDN that sits in front of our storage account, it encodes the query params of a request before writing them to the access logs, we then process these access logs and unencode the query string to get back to the "original" request. This no longer works correctly as + isn't double encoded and so it goes back to incorrectly being a space when we try and remove the encoding that occurred when the CDN wrote the logs. The older version of the storage SDK used Uri.EscapeDataString which encoded spaces to '%20` which would get double encoded correctly so this worked for us.

Would you consider an option that allowed us to opt into using Uri.EscapeDataString when it came to url encoding?