Merge pull request #51 from rubberduck-vba/webhook

Avoid consuming body stream in auth handler

Author: retailcoder

rubberduckvba.Server/Api/Admin/WebhookController.cs

@@ -1,5 +1,6 @@
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Mvc;
+using System.Text.Json;
 
 namespace rubberduckvba.Server.Api.Admin;
 
@@ -21,9 +22,11 @@ public WebhookController(
 
     [Authorize("webhook")]
     [HttpPost("webhook/github")]
-    public IActionResult GitHub([FromBody] PushWebhookPayload payload)
+    public async Task<IActionResult> GitHub([FromBody] object body)
     {
-        var eventType = _validator.Validate(payload, Request.Headers, out var content, out var gitref);
+        var json = body.ToString();
+        var payload = JsonSerializer.Deserialize<PushWebhookPayload>(json, new JsonSerializerOptions { PropertyNameCaseInsensitive = true });
+        var eventType = _validator.Validate(payload, json, Request.Headers, out var content, out var gitref);
 
         if (eventType == WebhookPayloadType.Push)
         {

rubberduckvba.Server/Api/Admin/WebhookPayloadType.cs

@@ -2,7 +2,7 @@
 
 public enum WebhookPayloadType
 {
-    Unsupported,
+    BadRequest,
     Greeting,
     Ping,
     Push

rubberduckvba.Server/Api/Admin/WebhookPayloadValidationService.cs

@@ -1,10 +1,15 @@
 namespace rubberduckvba.Server.Api.Admin;
 
-public class WebhookPayloadValidationService(ConfigurationOptions options)
+public class WebhookPayloadValidationService(ConfigurationOptions options, WebhookSignatureValidationService signatureValidation)
 {
-    public WebhookPayloadType Validate(PushWebhookPayload payload, IHeaderDictionary headers, out string? content, out GitRef? gitref)
+    public WebhookPayloadType Validate(PushWebhookPayload payload, string body, IHeaderDictionary headers, out string? content, out GitRef? gitref)
     {
         content = default;
+        if (!signatureValidation.Validate(body, headers["X-Hub-Signature-256"].OfType<string>().ToArray()))
+        {
+            gitref = default;
+            return WebhookPayloadType.BadRequest;
+        }
 
         gitref = new GitRef(payload.Ref);
         if (headers["X-GitHub-Event"].FirstOrDefault() == "ping")
@@ -21,6 +26,6 @@ public WebhookPayloadType Validate(PushWebhookPayload payload, IHeaderDictionary
             return WebhookPayloadType.Push;
         }
 
-        return WebhookPayloadType.Unsupported;
+        return WebhookPayloadType.BadRequest;
     }
 }

rubberduckvba.Server/Program.cs

@@ -160,8 +160,9 @@ private static void ConfigureServices(IServiceCollection services)
         services.AddSingleton<ConfigurationOptions>();
         services.AddSingleton<IMarkdownFormattingService, MarkdownFormattingService>();
         services.AddSingleton<ISyntaxHighlighterService, SyntaxHighlighterService>();
-        services.AddSingleton<WebhookSignatureValidationService>();
+        services.AddSingleton<WebhookHeaderValidationService>();
         services.AddSingleton<WebhookPayloadValidationService>();
+        services.AddSingleton<WebhookSignatureValidationService>();
         services.AddSingleton<HangfireLauncherService>();
 
         services.AddSingleton<IRubberduckDbService, RubberduckDbService>();

rubberduckvba.Server/WebhookAuthenticationHandler.cs

@@ -8,10 +8,10 @@ namespace rubberduckvba.Server;
 
 public class WebhookAuthenticationHandler : AuthenticationHandler<AuthenticationSchemeOptions>
 {
-    private readonly WebhookSignatureValidationService _service;
+    private readonly WebhookHeaderValidationService _service;
 
     public WebhookAuthenticationHandler(IOptionsMonitor<AuthenticationSchemeOptions> options, ILoggerFactory logger, UrlEncoder encoder,
-        WebhookSignatureValidationService service)
+        WebhookHeaderValidationService service)
         : base(options, logger, encoder)
     {
         _service = service;
@@ -27,10 +27,7 @@ protected async override Task<AuthenticateResult> HandleAuthenticateAsync()
             var xHubSignature = Context.Request.Headers["X-Hub-Signature"].OfType<string>().ToArray();
             var xHubSignature256 = Context.Request.Headers["X-Hub-Signature-256"].OfType<string>().ToArray();
 
-            using var reader = new StreamReader(Context.Request.Body);
-            var payload = reader.ReadToEndAsync().GetAwaiter().GetResult();
-
-            if (_service.Validate(payload, userAgent, xGitHubEvent, xGitHubDelivery, xHubSignature, xHubSignature256))
+            if (_service.Validate(userAgent, xGitHubEvent, xGitHubDelivery, xHubSignature, xHubSignature256))
             {
                 var principal = CreatePrincipal();
                 var ticket = new AuthenticationTicket(principal, "webhook-signature");

rubberduckvba.Server/WebhookSignatureValidationService.cs renamed to rubberduckvba.Server/WebhookHeaderValidationService.cs

@@ -7,45 +7,8 @@ namespace rubberduckvba.Server;
 
 public class WebhookSignatureValidationService(ConfigurationOptions configuration, ILogger<WebhookSignatureValidationService> logger)
 {
-    public bool Validate(
-        string payload,
-        string? userAgent,
-        string[] xGitHubEvent,
-        string[] xGitHubDelivery,
-        string[] xHubSignature,
-        string[] xHubSignature256
-        )
+    public bool Validate(string payload, string[] xHubSignature256)
     {
-        if (!(userAgent ?? string.Empty).StartsWith("GitHub-Hookshot/"))
-        {
-            // user agent must be GitHub hookshot
-            return false;
-        }
-
-        if (!xGitHubEvent.Contains("push"))
-        {
-            if (xGitHubEvent.Contains("ping"))
-            {
-                // no harm just returning 200-OK on ping
-                return true;
-            }
-
-            // only authenticate ping and push events
-            return false;
-        }
-
-        if (!Guid.TryParse(xGitHubDelivery.SingleOrDefault(), out _))
-        {
-            // delivery should parse as a GUID
-            return false;
-        }
-
-        if (!xHubSignature.Any())
-        {
-            // SHA-1 signature header must be present
-            return false;
-        }
-
         var signature = xHubSignature256.SingleOrDefault();
         if (signature == default)
         {
@@ -56,7 +19,7 @@ string[] xHubSignature256
         if (!IsValidSignature(signature, payload))
         {
             // SHA-256 signature must match
-            //return false;
+            return false;
         }
 
         return true;
@@ -87,3 +50,42 @@ private bool IsValidSignature(string? signature, string payload)
         return (signature == check);
     }
 }
+
+
+public class WebhookHeaderValidationService(ConfigurationOptions configuration, ILogger<WebhookHeaderValidationService> logger)
+{
+    public bool Validate(
+        string? userAgent,
+        string[] xGitHubEvent,
+        string[] xGitHubDelivery,
+        string[] xHubSignature,
+        string[] xHubSignature256
+        )
+    {
+        if (!(userAgent ?? string.Empty).StartsWith("GitHub-Hookshot/"))
+        {
+            // user agent must be GitHub hookshot
+            return false;
+        }
+
+        if (!xGitHubEvent.Contains("push"))
+        {
+            if (xGitHubEvent.Contains("ping"))
+            {
+                // no harm just returning 200-OK on ping
+                return true;
+            }
+
+            // only authenticate ping and push events
+            return false;
+        }
+
+        if (!Guid.TryParse(xGitHubDelivery.SingleOrDefault(), out _))
+        {
+            // delivery should parse as a GUID
+            return false;
+        }
+
+        return true;
+    }
+}