skip unsignable headers

FantasticFiasco · Nov 30, 2024 · 45f3e32 · 45f3e32
1 parent 19e6039
commit 45f3e32
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 12 deletions.
diff --git a/src/Private/CanonicalRequest.cs b/src/Private/CanonicalRequest.cs
@@ -30,6 +30,24 @@ public static class CanonicalRequest
         /// </summary>
         public static string HeaderValueSeparator { get; set; } = ", ";
 
+        // Including most headers from
+        // https://github.com/smithy-lang/smithy-typescript/blob/430021abf44f8a4d6c24de2dfa25709bf91a92c8/packages/signature-v4/src/constants.ts#L19-L35
+        private static HashSet<string> UnsignableHeaders = new(StringComparer.OrdinalIgnoreCase) {
+            "authorization",
+            "connection",
+            "expect",
+            "keep-alive",
+            "proxy-authenticate",
+            "proxy-authorization",
+            "proxy-connection",
+            "range",
+            "te",
+            "trailer",
+            "transfer-encoding",
+            "upgrade",
+            HeaderKeys.XAmznTraceIdHeader,
+        };
+
         /// <returns>
         /// The first value is the canonical request, the second value is the signed headers.
         /// </returns>
@@ -91,8 +109,10 @@ public static (string, string) Build(
             builder.Append($"{string.Join("&", parameters)}\n");
 
             // Add the canonical headers, followed by a newline character. The canonical headers
-            // consist of a list of all the HTTP headers that you are including with the signed
-            // request.
+            // consist of a list of HTTP headers that you are including with the signed request.
+            //
+            // Some headers are unsignable, i.e. signatures including them are always deemed invalid.
+            // They are excluded from the canonical headers (but will be kept in the HTTP request).
             //
             // To create the canonical headers list, convert all header names to lowercase and
             // remove leading spaces and trailing spaces. Convert sequential spaces in the header
@@ -108,7 +128,7 @@ public static (string, string) Build(
             //   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, defaultHeaders);
+            var sortedHeaders = PruneAndSortHeaders(request.Headers, defaultHeaders);
 
             foreach (var header in sortedHeaders)
             {
@@ -187,7 +207,7 @@ public static SortedList<string, List<string>> SortQueryParameters(string query)
             return sortedQueryParameters;
         }
 
-        public static SortedDictionary<string, List<string>> SortHeaders(
+        public static SortedDictionary<string, List<string>> PruneAndSortHeaders(
             HttpRequestHeaders headers,
             IEnumerable<KeyValuePair<string, IEnumerable<string>>> defaultHeaders)
         {
@@ -202,6 +222,11 @@ void AddHeader(KeyValuePair<string, IEnumerable<string>> header)
             {
                 var headerName = FormatHeaderName(header.Key);
 
+                if (UnsignableHeaders.Contains(headerName))
+                {
+                    return;
+                }
+
                 // Create header if it doesn't already exist
                 if (!sortedHeaders.TryGetValue(headerName, out var headerValues))
                 {

diff --git a/test/Unit/Private/CanonicalRequestGivenDotnetShould.cs b/test/Unit/Private/CanonicalRequestGivenDotnetShould.cs
@@ -33,7 +33,7 @@ public void RespectDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
@@ -50,7 +50,7 @@ public void IgnoreDuplicateDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-ignored-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value" });

diff --git a/test/Unit/Private/CanonicalRequestGivenMonoShould.cs b/test/Unit/Private/CanonicalRequestGivenMonoShould.cs
@@ -33,7 +33,7 @@ public void RespectDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
@@ -50,7 +50,7 @@ public void RespectDuplicateDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-other-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value", "some-other-header-value" });

diff --git a/test/Unit/Private/CanonicalRequestShould.cs b/test/Unit/Private/CanonicalRequestShould.cs
@@ -109,7 +109,7 @@ public void TransformHeaderNamesToLowercase(string headerName, string expected)
             headers.Add(headerName, "some header value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, null);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, null);
 
             // Assert
             actual.Keys.ShouldBe(new[] { expected });
@@ -133,7 +133,7 @@ public void SortHeaderNames(string[] headerNames, string[] expected)
             }
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, null);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, null);
 
             // Assert
             actual.Keys.ShouldBe(expected);
@@ -156,7 +156,7 @@ public void TrimHeaderValue(string headerValue, string expected)
             headers.Add("some-header-name", headerValue);
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, null);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, null);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { expected });
@@ -176,12 +176,32 @@ public void RemoveSequentialSpacesInHeaderValue(string headerValue, string expec
             headers.Add("some-header-name", headerValue);
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, null);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, null);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { expected });
         }
 
+        [Theory]
+        [InlineData("Connection", "keep-alive")]
+        [InlineData("Expect", "100-continue")]
+        [InlineData("Keep-Alive", "timeout=5")]
+        [InlineData("Proxy-Authenticate", "Basic")]
+        [InlineData("Range", "bytes=500-999")]
+        [InlineData("TE", "gzip")]
+        public void RemoveUnsignableHeaders(string headerName, string headerValue)
+        {
+            // Arrange
+            var headers = new HttpRequestMessage().Headers;
+            headers.Add(headerName, headerValue);
+
+            // Act
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, null);
+
+            // Assert
+            actual.ShouldNotContainKey(headerName.ToLowerInvariant());
+        }
+
         [Theory]
         [InlineData("?a=1&b=2&c=3", new[] { "a", "b", "c" })]
         [InlineData("?a=1&c=3&b=2", new[] { "a", "b", "c" })]