PR comments: Refactor GetDecodedHash(...) and `GetBase64DecodedHash…

…(...)`

- remove those methods
- move `try` blocks into receiver-specific filters
- add `CreateBadBase64EncodingResult(...)` and `CreateBadHexEncodingResult(...)`

- remove `FromBase64(...)` and use `Base64UrlTextEncoder` instead

nits:
- add receiver name to messages about bad encodings
- fix a long line

src/Microsoft.AspNetCore.WebHooks.Receivers.Dropbox/Filters/DropboxVerifySignatureFilter.cs

@@ -78,10 +78,17 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res
                     return;
                 }
 
-                var expectedHash = GetDecodedHash(header, DropboxConstants.SignatureHeaderName, out errorResult);
-                if (errorResult != null)
+                byte[] expectedHash;
+                try
                 {
-                    context.Result = errorResult;
+                    expectedHash = FromHex(header);
+                }
+                catch (Exception exception)
+                {
+                    context.Result = CreateBadHexEncodingResult(
+                        ReceiverName,
+                        DropboxConstants.SignatureHeaderName,
+                        exception);
                     return;
                 }
 

src/Microsoft.AspNetCore.WebHooks.Receivers.GitHub/Filters/GitHubVerifySignatureFilter.cs

@@ -116,10 +116,18 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res
 
                 enumerator.MoveNext();
                 var headerValue = enumerator.Current.Value;
-                var expectedHash = GetDecodedHash(headerValue, GitHubConstants.SignatureHeaderName, out errorResult);
-                if (errorResult != null)
+
+                byte[] expectedHash;
+                try
                 {
-                    context.Result = errorResult;
+                    expectedHash = FromHex(headerValue);
+                }
+                catch (Exception exception)
+                {
+                    context.Result = CreateBadHexEncodingResult(
+                        ReceiverName,
+                        GitHubConstants.SignatureHeaderKey,
+                        exception);
                     return;
                 }
 

src/Microsoft.AspNetCore.WebHooks.Receivers.Pusher/Filters/PusherVerifySignatureFilter.cs

@@ -80,10 +80,17 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res
                     return;
                 }
 
-                var expectedHash = GetDecodedHash(header, PusherConstants.SignatureHeaderName, out errorResult);
-                if (errorResult != null)
+                byte[] expectedHash;
+                try
                 {
-                    context.Result = errorResult;
+                    expectedHash = FromHex(header);
+                }
+                catch (Exception exception)
+                {
+                    context.Result = CreateBadHexEncodingResult(
+                        ReceiverName,
+                        PusherConstants.SignatureHeaderName,
+                        exception);
                     return;
                 }
 
@@ -95,7 +102,10 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res
                     return;
                 }
 
-                var applicationKey = GetRequestHeader(request, PusherConstants.SignatureKeyHeaderName, out errorResult);
+                var applicationKey = GetRequestHeader(
+                    request,
+                    PusherConstants.SignatureKeyHeaderName,
+                    out errorResult);
                 if (errorResult != null)
                 {
                     context.Result = errorResult;

src/Microsoft.AspNetCore.WebHooks.Receivers.Stripe/Filters/StripeVerifySignatureFilter.cs

@@ -125,13 +125,17 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res
             {
                 // While this looks repetitious compared to hex-encoding actualHash (once), a single v1 entry in the
                 // header is the normal case. Expect multiple signatures only when rolling secret keys.
-                var expectedHash = GetDecodedHash(
-                    signature.Value,
-                    StripeConstants.SignatureHeaderName,
-                    out errorResult);
-                if (errorResult != null)
+                byte[] expectedHash;
+                try
+                {
+                    expectedHash = FromHex(signature.Value);
+                }
+                catch (Exception exception)
                 {
-                    context.Result = errorResult;
+                    context.Result = CreateBadHexEncodingResult(
+                        ReceiverName,
+                        StripeConstants.SignatureHeaderName,
+                        exception);
                     return;
                 }
 

src/Microsoft.AspNetCore.WebHooks.Receivers.Trello/Filters/TrelloVerifySignatureFilter.cs

@@ -10,6 +10,7 @@
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.AspNetCore.Mvc.Filters;
 using Microsoft.AspNetCore.Routing;
+using Microsoft.AspNetCore.WebUtilities;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.Logging;
 
@@ -79,10 +80,17 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res
                     return;
                 }
 
-                var expectedHash = GetBase64DecodedHash(header, TrelloConstants.SignatureHeaderName, out errorResult);
-                if (errorResult != null)
+                byte[] expectedHash;
+                try
                 {
-                    context.Result = errorResult;
+                    expectedHash = Base64UrlTextEncoder.Decode(header);
+                }
+                catch (Exception exception)
+                {
+                    context.Result = CreateBadBase64EncodingResult(
+                        ReceiverName,
+                        TrelloConstants.SignatureHeaderName,
+                        exception);
                     return;
                 }
 

src/Microsoft.AspNetCore.WebHooks.Receivers/Filters/WebHookVerifySignatureFilter.cs

@@ -60,41 +60,6 @@ public bool IsApplicable(string receiverName)
             return string.Equals(ReceiverName, receiverName, StringComparison.OrdinalIgnoreCase);
         }
 
-        /// <summary>
-        /// Converts a base64-encoded string to a <see cref="T:byte[]"/> (with or without URI-safe mode encoding).
-        /// </summary>
-        /// <param name="content">The Base64-encoded string to convert.</param>
-        /// <returns>The converted <see cref="T:byte[]"/>.</returns>
-        public static byte[] FromBase64(string content)
-        {
-            if (string.IsNullOrEmpty(content))
-            {
-                return new byte[0];
-            }
-
-            // Reverse URI-safe encoding.
-            var base64 = content
-                .Replace('_', '/')
-                .Replace('-', '+');
-
-            // Append padding if missing in content.
-            switch (content.Length % 4)
-            {
-                case 2:
-                    base64 += "==";
-                    break;
-
-                case 3:
-                    base64 += "=";
-                    break;
-            }
-
-            // May throw a FormatException if base64 is not valid.
-            var data = Convert.FromBase64String(base64);
-
-            return data;
-        }
-
         /// <summary>
         /// Converts a hex-encoded string to a <see cref="T:byte[]"/>.
         /// </summary>
@@ -440,98 +405,101 @@ protected virtual async Task<byte[]> ComputeRequestBodySha256HashAsync(
         }
 
         /// <summary>
-        /// Decode the given <paramref name="hexEncodedValue"/>.
+        /// Returns a new <see cref="IActionResult"/> that when executed produces a response indicating the request
+        /// had a signature header containing an invalid (non-Base64-encoded) hash value. Also logs about the problem.
         /// </summary>
-        /// <param name="hexEncodedValue">The hex-encoded <see cref="string"/>.</param>
-        /// <param name="signatureHeaderName">
-        /// The name of the HTTP header containing the <paramref name="hexEncodedValue"/>.
-        /// </param>
-        /// <param name="errorResult">
-        /// Set to <see langword="null"/> if decoding is successful. Otherwise, an <see cref="IActionResult"/> that
-        /// when executed will produce a response containing details about the problem.
-        /// </param>
+        /// <param name="receiverName">The name of an available <see cref="IWebHookReceiver"/>.</param>
+        /// <param name="signatureHeaderName">The name of the HTTP header with invalid contents.</param>
+        /// <param name="exception">The <see cref="Exception"/> encountered when decoding the hash value.</param>
         /// <returns>
-        /// If successful, the <see cref="byte"/> array containing the decoded hash. <see langword="null"/> if any
-        /// issues occur.
+        /// An <see cref="IActionResult"/> that when executed will produce a response with status code 400 "Bad
+        /// Request" and containing details about the problem.
         /// </returns>
-        protected virtual byte[] GetDecodedHash(
-            string hexEncodedValue,
+        protected virtual IActionResult CreateBadBase64EncodingResult(
+            string receiverName,
             string signatureHeaderName,
-            out IActionResult errorResult)
+            Exception exception)
         {
-            try
+            if (receiverName == null)
             {
-                var decodedHash = FromHex(hexEncodedValue);
-                errorResult = null;
-
-                return decodedHash;
+                throw new ArgumentNullException(nameof(receiverName));
+            }
+            if (signatureHeaderName == null)
+            {
+                throw new ArgumentNullException(nameof(signatureHeaderName));
             }
-            catch (Exception ex)
+            if (exception == null)
             {
-                Logger.LogError(
-                    401,
-                    ex,
-                    "The '{HeaderName}' header value is invalid. It must be a valid hex-encoded string.",
-                    signatureHeaderName);
+                throw new ArgumentNullException(nameof(exception));
             }
 
+            Logger.LogError(
+                403,
+                exception,
+                "The '{HeaderName}' header value is invalid. The '{ReceiverName}' receiver requires a valid " +
+                "Base64-encoded string.",
+                signatureHeaderName,
+                receiverName);
+
             var message = string.Format(
                 CultureInfo.CurrentCulture,
-                Resources.VerifySignature_BadHeaderEncoding,
-                signatureHeaderName);
-            errorResult = new BadRequestObjectResult(message);
+                Resources.VerifySignature_BadBase64Encoding,
+                signatureHeaderName,
+                receiverName);
 
-            return null;
+            return new BadRequestObjectResult(message);
         }
 
         /// <summary>
-        /// Decode the given <paramref name="base64EncodedValue"/>.
+        /// Returns a new <see cref="IActionResult"/> that when executed produces a response indicating the request
+        /// had a signature header containing an invalid (non-hex-encoded) hash value. Also logs about the problem.
         /// </summary>
-        /// <param name="base64EncodedValue">The Base64-encoded <see cref="string"/>.</param>
-        /// <param name="signatureHeaderName">
-        /// The name of the HTTP header containing the <paramref name="base64EncodedValue"/>.
-        /// </param>
-        /// <param name="errorResult">
-        /// Set to <see langword="null"/> if decoding is successful. Otherwise, an <see cref="IActionResult"/> that
-        /// when executed will produce a response containing details about the problem.
-        /// </param>
+        /// <param name="receiverName">The name of an available <see cref="IWebHookReceiver"/>.</param>
+        /// <param name="signatureHeaderName">The name of the HTTP header with invalid contents.</param>
+        /// <param name="exception">The <see cref="Exception"/> encountered when decoding the hash value.</param>
         /// <returns>
-        /// If successful, the <see cref="byte"/> array containing the decoded hash. <see langword="null"/> if any
-        /// issues occur.
+        /// An <see cref="IActionResult"/> that when executed will produce a response with status code 400 "Bad
+        /// Request" and containing details about the problem.
         /// </returns>
-        protected virtual byte[] GetBase64DecodedHash(
-            string base64EncodedValue,
+        protected virtual IActionResult CreateBadHexEncodingResult(
+            string receiverName,
             string signatureHeaderName,
-            out IActionResult errorResult)
+            Exception exception)
         {
-            try
+            if (receiverName == null)
             {
-                var decodedHash = FromBase64(base64EncodedValue);
-                errorResult = null;
-
-                return decodedHash;
+                throw new ArgumentNullException(nameof(receiverName));
+            }
+            if (signatureHeaderName == null)
+            {
+                throw new ArgumentNullException(nameof(signatureHeaderName));
             }
-            catch (Exception ex)
+            if (exception == null)
             {
-                Logger.LogError(
-                    401,
-                    ex,
-                    "The '{HeaderName}' header value is invalid. It must be a valid Base64-encoded string.",
-                    signatureHeaderName);
+                throw new ArgumentNullException(nameof(exception));
             }
 
+            Logger.LogError(
+                401,
+                exception,
+                "The '{HeaderName}' header value is invalid. The '{ReceiverName}' receiver requires a valid " +
+                "hex-encoded string.",
+                signatureHeaderName,
+                receiverName);
+
             var message = string.Format(
                 CultureInfo.CurrentCulture,
-                Resources.VerifySignature_BadBase64HeaderEncoding,
-                signatureHeaderName);
-            errorResult = new BadRequestObjectResult(message);
+                Resources.VerifySignature_BadHexEncoding,
+                signatureHeaderName,
+                receiverName);
 
-            return null;
+            return new BadRequestObjectResult(message);
         }
 
         /// <summary>
-        /// Returns a new <see cref="IActionResult"/> that when executed produces a response indicating that a
-        /// request had an invalid signature and as a result could not be processed.
+        /// Returns a new <see cref="IActionResult"/> that when executed produces a response indicating the request
+        /// invalid signature (an unexpected hash value) and as a result could not be processed. Also logs about the
+        /// problem.
         /// </summary>
         /// <param name="receiverName">The name of an available <see cref="IWebHookReceiver"/>.</param>
         /// <param name="signatureHeaderName">The name of the HTTP header with invalid contents.</param>
@@ -541,6 +509,15 @@ protected virtual byte[] GetBase64DecodedHash(
         /// </returns>
         protected virtual IActionResult CreateBadSignatureResult(string receiverName, string signatureHeaderName)
         {
+            if (receiverName == null)
+            {
+                throw new ArgumentNullException(nameof(receiverName));
+            }
+            if (signatureHeaderName == null)
+            {
+                throw new ArgumentNullException(nameof(signatureHeaderName));
+            }
+
             Logger.LogError(
                 402,
                 "The WebHook signature provided by the '{HeaderName}' header field does not match the value " +
@@ -553,9 +530,8 @@ protected virtual IActionResult CreateBadSignatureResult(string receiverName, st
                 Resources.VerifySignature_BadSignature,
                 signatureHeaderName,
                 receiverName);
-            var badSignature = new BadRequestObjectResult(message);
 
-            return badSignature;
+            return new BadRequestObjectResult(message);
         }
     }
 }