fix default request headers are added twice on Android using Mono (Fa…

…ntasticFiasco#29)

The behavior on Mono is different from the behavior on .NET Framework or .NET Core, where a default request header that already exists on the request message is ignored.

Closes FantasticFiasco#28

CHANGELOG.md

@@ -6,6 +6,10 @@ This project adheres to [Semantic Versioning](http://semver.org/) and is followi
 
 ## Unreleased
 
+### :syringe: Fixed
+
+- [#28](https://github.com/FantasticFiasco/aws-signature-version-4/issues/28) Default request headers are added twice on Android using Mono. The behavior on Mono is different from the behavior on .NET Framework or .NET Core, where a default request header that already exists on the request message is ignored. (contribution by [@Daniel-NP](https://github.com/Daniel-NP))
+
 ## [1.0.1] - 2019-05-28
 
 ### :syringe: Fixed

Directory.Build.props

@@ -1,7 +1,7 @@
 <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
 
   <PropertyGroup>
-    <VersionPrefix>1.0.1</VersionPrefix>
+    <VersionPrefix>1.0.2</VersionPrefix>
     <!--
       Only increment the assembly version on major version changes to help users reduce binding
       redirects, and how often they're updated. For more information, please read

src/Private/CanonicalRequest.cs

@@ -18,6 +18,11 @@ namespace AwsSignatureVersion4.Private
     /// </summary>
     public static class CanonicalRequest
     {
+        /// <summary>
+        /// Gets or sets an instance capable of probing the environment.
+        /// </summary>
+        public static EnvironmentProbe EnvironmentProbe { get; set; } = new EnvironmentProbe();
+
         /// <summary>
         /// Gets or sets the header value separator. The default value is ", " and it is defined in
         /// <see href="https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderParser.cs">
@@ -30,7 +35,9 @@ public static class CanonicalRequest
         /// <returns>
         /// The first value is the canonical request, the second value is the signed headers.
         /// </returns>
-        public static async Task<(string, string)> BuildAsync(HttpRequestMessage request)
+        public static async Task<(string, string)> BuildAsync(
+            HttpRequestMessage request,
+            HttpRequestHeaders defaultHeaders)
         {
             var builder = new StringBuilder();
 
@@ -102,7 +109,7 @@ public static class CanonicalRequest
             //   PLEASE NOTE: Microsoft has chosen to separate the header values with ", ", not ","
             //   as defined by the Canonical Request algorithm.
             // - Append a new line ('\n').
-            var sortedHeaders = SortHeaders(request.Headers);
+            var sortedHeaders = SortHeaders(request.Headers, defaultHeaders);
 
             foreach (var header in sortedHeaders)
             {
@@ -167,15 +174,21 @@ public static SortedList<string, List<string>> SortQueryParameters(string query)
 
             return sortedQueryParameters;
         }
-
-        public static SortedDictionary<string, List<string>> SortHeaders(HttpRequestHeaders headers)
+
+        public static SortedDictionary<string, List<string>> SortHeaders(
+            HttpRequestHeaders headers,
+            HttpRequestHeaders defaultHeaders)
         {
             var sortedHeaders = new SortedDictionary<string, List<string>>(StringComparer.Ordinal);
 
-            foreach (var header in headers)
+            string FormatHeaderName(string headerName)
+            {
+                return headerName.ToLowerInvariant();
+            }
+
+            void AddHeader(KeyValuePair<string, IEnumerable<string>> header)
             {
-                // Convert header name to lowercase
-                var headerName = header.Key.ToLowerInvariant();
+                var headerName = FormatHeaderName(header.Key);
 
                 // Create header if it doesn't already exist
                 if (!sortedHeaders.TryGetValue(headerName, out var headerValues))
@@ -189,6 +202,49 @@ public static SortedDictionary<string, List<string>> SortHeaders(HttpRequestHead
                 headerValues.AddRange(header.Value.Select(headerValue => headerValue.Trim().NormalizeWhiteSpace()));
             }
 
+            void AddDefaultDotnetHeaders()
+            {
+                foreach (var defaultHeader in defaultHeaders)
+                {
+                    // On .NET Framework or .NET Core we only add header values if they're not
+                    // already added on the message. Note that we don't merge collections: If both
+                    // the default headers and the message have set some values for a certain
+                    // header, then we don't try to merge the values.
+                    if (!sortedHeaders.ContainsKey(FormatHeaderName(defaultHeader.Key)))
+                    {
+                        AddHeader(defaultHeader);
+                    }
+                }
+            }
+
+            void AddDefaultMonoHeaders()
+            {
+                foreach (var defaultHeader in defaultHeaders)
+                {
+                    // On Mono we add header values indifferent of whether the header already exists
+                    AddHeader(defaultHeader);
+                }
+            }
+
+            // Add headers
+            foreach (var header in headers)
+            {
+                AddHeader(header);
+            }
+
+            // Add default headers
+            if (defaultHeaders != null)
+            {
+                if (EnvironmentProbe.IsMono)
+                {
+                    AddDefaultMonoHeaders();
+                }
+                else
+                {
+                    AddDefaultDotnetHeaders();
+                }
+            }
+
             return sortedHeaders;
         }
     }

src/Private/EnvironmentProbe.cs

@@ -0,0 +1,16 @@
+using System;
+
+namespace AwsSignatureVersion4.Private
+{
+    public class EnvironmentProbe
+    {
+        private readonly Lazy<bool> isMono;
+
+        public EnvironmentProbe()
+        {
+           isMono = new Lazy<bool>(() => Type.GetType("Mono.Runtime") != null);
+        }
+
+        public virtual bool IsMono => isMono.Value;
+    }
+}

src/Private/Signer.cs

@@ -19,15 +19,14 @@ public static async Task<Result> SignAsync(
             if (httpClient == null) throw new ArgumentNullException(nameof(httpClient));
             if (request == null) throw new ArgumentNullException(nameof(request));
             if (request.Headers.Contains(HeaderKeys.XAmzDateHeader)) throw new ArgumentException(ErrorMessages.XAmzDateHeaderExists, nameof(request));
-            if (request.Headers.Authorization != null)  throw new ArgumentException(ErrorMessages.AuthorizationHeaderExists, nameof(request));
+            if (request.Headers.Authorization != null) throw new ArgumentException(ErrorMessages.AuthorizationHeaderExists, nameof(request));
             if (request.Headers.Contains(HeaderKeys.AuthorizationHeader)) throw new ArgumentException(ErrorMessages.AuthorizationHeaderExists, nameof(request));
             if (regionName == null) throw new ArgumentNullException(nameof(regionName));
             if (serviceName == null) throw new ArgumentNullException(nameof(serviceName));
             if (serviceName == "s3") throw new NotSupportedException(ErrorMessages.S3NotSupported);
             if (credentials == null) throw new ArgumentNullException(nameof(credentials));
 
             UpdateRequestUri(httpClient, request);
-            AddDefaultHeaders(httpClient, request);
 
             // Add required headers
             request.AddHeader(HeaderKeys.XAmzDateHeader, now.ToIso8601BasicDateTime());
@@ -37,7 +36,7 @@ public static async Task<Result> SignAsync(
             request.AddHeaderIf(!request.Headers.Contains(HeaderKeys.HostHeader), HeaderKeys.HostHeader, request.RequestUri.Host);
 
             // Build the canonical request
-            var (canonicalRequest, signedHeaders) = await CanonicalRequest.BuildAsync(request);
+            var (canonicalRequest, signedHeaders) = await CanonicalRequest.BuildAsync(request, httpClient.DefaultRequestHeaders);
 
             // Build the string to sign
             var (stringToSign, credentialScope) = StringToSign.Build(
@@ -90,21 +89,5 @@ private static void UpdateRequestUri(HttpClient httpClient, HttpRequestMessage r
                 request.RequestUri = requestUri;
             }
         }
-
-        private static void AddDefaultHeaders(HttpClient httpClient, HttpRequestMessage request)
-        {
-            if (httpClient.DefaultRequestHeaders == null) return;
-
-            foreach (var header in httpClient.DefaultRequestHeaders)
-            {
-                // Only add header values if they're not already set on the message. Note that
-                // we don't merge collections: If both the default headers and the message have
-                // set some values for a certain header, then we don't try to merge the values.
-                if (!request.Headers.Contains(header.Key))
-                {
-                    request.AddHeaders(header.Key, header.Value);
-                }
-            }
-        }
     }
 }

test/Unit/Private/CanonicalRequestGivenDotnetShould.cs

@@ -0,0 +1,73 @@
+using System;
+using System.Net.Http;
+using AwsSignatureVersion4.Private;
+using AwsSignatureVersion4.TestSuite;
+using Shouldly;
+using Xunit;
+
+namespace AwsSignatureVersion4.Unit.Private
+{
+    public class CanonicalRequestGivenDotnetShould : IClassFixture<TestSuiteContext>, IDisposable
+    {
+        private readonly TestSuiteContext context;
+        private readonly EnvironmentProbe defaultEnvironmentProbe;
+
+        public CanonicalRequestGivenDotnetShould(TestSuiteContext context)
+        {
+            this.context = context;
+
+            context.AdjustHeaderValueSeparator();
+
+            // Mock .NET environment
+            defaultEnvironmentProbe = CanonicalRequest.EnvironmentProbe;
+            CanonicalRequest.EnvironmentProbe = new DotnetEnvironmentProbe();
+        }
+
+        [Fact]
+        public void RespectDefaultHeader()
+        {
+            // Arrange
+            var headers = new HttpRequestMessage().Headers;
+
+            var defaultHeaders = new HttpRequestMessage().Headers;
+            defaultHeaders.Add("some-header-name", "some-header-value");
+
+            // Act
+            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+
+            // Assert
+            actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
+        }
+
+        [Fact]
+        public void IgnoreDuplicateDefaultHeader()
+        {
+            // Arrange
+            var headers = new HttpRequestMessage().Headers;
+            headers.Add("some-header-name", "some-header-value");
+
+            var defaultHeaders = new HttpRequestMessage().Headers;
+            defaultHeaders.Add("some-header-name", "some-ignored-header-value");
+
+            // Act
+            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+
+            // Assert
+            actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
+        }
+
+        public void Dispose()
+        {
+            // Reset header value separator
+            context.ResetHeaderValueSeparator();
+
+            // Reset environment probe
+            CanonicalRequest.EnvironmentProbe = defaultEnvironmentProbe;
+        }
+
+        private class DotnetEnvironmentProbe : EnvironmentProbe
+        {
+            public override bool IsMono { get; } = false;
+        }
+    }
+}

test/Unit/Private/CanonicalRequestGivenMonoShould.cs

@@ -0,0 +1,73 @@
+using System;
+using System.Net.Http;
+using AwsSignatureVersion4.Private;
+using AwsSignatureVersion4.TestSuite;
+using Shouldly;
+using Xunit;
+
+namespace AwsSignatureVersion4.Unit.Private
+{
+    public class CanonicalRequestGivenMonoShould : IClassFixture<TestSuiteContext>, IDisposable
+    {
+        private readonly TestSuiteContext context;
+        private readonly EnvironmentProbe defaultEnvironmentProbe;
+
+        public CanonicalRequestGivenMonoShould(TestSuiteContext context)
+        {
+            this.context = context;
+
+            context.AdjustHeaderValueSeparator();
+
+            // Mock Mono environment
+            defaultEnvironmentProbe = CanonicalRequest.EnvironmentProbe;
+            CanonicalRequest.EnvironmentProbe = new MonoEnvironmentProbe();
+        }
+
+        [Fact]
+        public void RespectDefaultHeader()
+        {
+            // Arrange
+            var headers = new HttpRequestMessage().Headers;
+
+            var defaultHeaders = new HttpRequestMessage().Headers;
+            defaultHeaders.Add("some-header-name", "some-header-value");
+
+            // Act
+            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+
+            // Assert
+            actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
+        }
+
+        [Fact]
+        public void RespectDuplicateDefaultHeader()
+        {
+            // Arrange
+            var headers = new HttpRequestMessage().Headers;
+            headers.Add("some-header-name", "some-header-value");
+
+            var defaultHeaders = new HttpRequestMessage().Headers;
+            defaultHeaders.Add("some-header-name", "some-other-header-value");
+
+            // Act
+            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+
+            // Assert
+            actual["some-header-name"].ShouldBe(new[] { "some-header-value", "some-other-header-value" });
+        }
+
+        public void Dispose()
+        {
+            // Reset header value separator
+            context.ResetHeaderValueSeparator();
+
+            // Reset environment probe
+            CanonicalRequest.EnvironmentProbe = defaultEnvironmentProbe;
+        }
+
+        private class MonoEnvironmentProbe : EnvironmentProbe
+        {
+            public override bool IsMono { get; } = true;
+        }
+    }
+}

test/Unit/Private/CanonicalRequestShould.cs

@@ -61,7 +61,7 @@ public async Task PassTestSuite(params string[] scenarioName)
             scenario.Request.AddHeader(HeaderKeys.XAmzDateHeader, context.UtcNow.ToIso8601BasicDateTime());
 
             // Act
-            var (canonicalRequest, signedHeaders) = await CanonicalRequest.BuildAsync(scenario.Request);
+            var (canonicalRequest, signedHeaders) = await CanonicalRequest.BuildAsync(scenario.Request, null);
 
             // Assert
             canonicalRequest.ShouldBe(scenario.ExpectedCanonicalRequest);
@@ -79,7 +79,7 @@ public void LowerCaseHeaderNames(string headerName, string expected)
             headers.Add(headerName, "some header value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers);
+            var actual = CanonicalRequest.SortHeaders(headers, null);
 
             // Assert
             actual.Keys.ShouldBe(new[] { expected });
@@ -103,7 +103,7 @@ public void SortHeaderNames(string[] headerNames, string[] expected)
             }
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers);
+            var actual = CanonicalRequest.SortHeaders(headers, null);
 
             // Assert
             actual.Keys.ShouldBe(expected);
@@ -126,7 +126,7 @@ public void TrimHeaderValue(string headerValue, string expected)
             headers.Add("some-header-name", headerValue);
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers);
+            var actual = CanonicalRequest.SortHeaders(headers, null);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { expected });
@@ -146,7 +146,7 @@ public void RemoveSequentialSpacesInHeaderValue(string headerValue, string expec
             headers.Add("some-header-name", headerValue);
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers);
+            var actual = CanonicalRequest.SortHeaders(headers, null);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { expected });