From 752852ba8a2155fcf3c73e9d842f21ea2aafc2a7 Mon Sep 17 00:00:00 2001 From: Mathieu Guindon Date: Wed, 5 Feb 2025 19:38:28 -0500 Subject: [PATCH] works on my machine :sweat-smile: --- .../Api/Admin/WebhookController.cs | 7 +- .../Api/Admin/WebhookPayloadType.cs | 2 +- .../Admin/WebhookPayloadValidationService.cs | 11 ++- rubberduckvba.Server/Program.cs | 3 +- .../WebhookAuthenticationHandler.cs | 9 +-- ...e.cs => WebhookHeaderValidationService.cs} | 80 ++++++++++--------- 6 files changed, 60 insertions(+), 52 deletions(-) rename rubberduckvba.Server/{WebhookSignatureValidationService.cs => WebhookHeaderValidationService.cs} (90%) diff --git a/rubberduckvba.Server/Api/Admin/WebhookController.cs b/rubberduckvba.Server/Api/Admin/WebhookController.cs index a1bb468..bd9a799 100644 --- a/rubberduckvba.Server/Api/Admin/WebhookController.cs +++ b/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 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(json, new JsonSerializerOptions { PropertyNameCaseInsensitive = true }); + var eventType = _validator.Validate(payload, json, Request.Headers, out var content, out var gitref); if (eventType == WebhookPayloadType.Push) { diff --git a/rubberduckvba.Server/Api/Admin/WebhookPayloadType.cs b/rubberduckvba.Server/Api/Admin/WebhookPayloadType.cs index 741da60..3ab8bab 100644 --- a/rubberduckvba.Server/Api/Admin/WebhookPayloadType.cs +++ b/rubberduckvba.Server/Api/Admin/WebhookPayloadType.cs @@ -2,7 +2,7 @@ public enum WebhookPayloadType { - Unsupported, + BadRequest, Greeting, Ping, Push diff --git a/rubberduckvba.Server/Api/Admin/WebhookPayloadValidationService.cs b/rubberduckvba.Server/Api/Admin/WebhookPayloadValidationService.cs index 256433d..a904273 100644 --- a/rubberduckvba.Server/Api/Admin/WebhookPayloadValidationService.cs +++ b/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().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; } } diff --git a/rubberduckvba.Server/Program.cs b/rubberduckvba.Server/Program.cs index abd4e74..0f1cd38 100644 --- a/rubberduckvba.Server/Program.cs +++ b/rubberduckvba.Server/Program.cs @@ -160,8 +160,9 @@ private static void ConfigureServices(IServiceCollection services) services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); - services.AddSingleton(); + services.AddSingleton(); services.AddSingleton(); + services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); diff --git a/rubberduckvba.Server/WebhookAuthenticationHandler.cs b/rubberduckvba.Server/WebhookAuthenticationHandler.cs index 9b6c528..f409ab0 100644 --- a/rubberduckvba.Server/WebhookAuthenticationHandler.cs +++ b/rubberduckvba.Server/WebhookAuthenticationHandler.cs @@ -8,10 +8,10 @@ namespace rubberduckvba.Server; public class WebhookAuthenticationHandler : AuthenticationHandler { - private readonly WebhookSignatureValidationService _service; + private readonly WebhookHeaderValidationService _service; public WebhookAuthenticationHandler(IOptionsMonitor options, ILoggerFactory logger, UrlEncoder encoder, - WebhookSignatureValidationService service) + WebhookHeaderValidationService service) : base(options, logger, encoder) { _service = service; @@ -27,10 +27,7 @@ protected async override Task HandleAuthenticateAsync() var xHubSignature = Context.Request.Headers["X-Hub-Signature"].OfType().ToArray(); var xHubSignature256 = Context.Request.Headers["X-Hub-Signature-256"].OfType().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"); diff --git a/rubberduckvba.Server/WebhookSignatureValidationService.cs b/rubberduckvba.Server/WebhookHeaderValidationService.cs similarity index 90% rename from rubberduckvba.Server/WebhookSignatureValidationService.cs rename to rubberduckvba.Server/WebhookHeaderValidationService.cs index b81bbe0..2d32ecb 100644 --- a/rubberduckvba.Server/WebhookSignatureValidationService.cs +++ b/rubberduckvba.Server/WebhookHeaderValidationService.cs @@ -7,45 +7,8 @@ namespace rubberduckvba.Server; public class WebhookSignatureValidationService(ConfigurationOptions configuration, ILogger 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 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; + } +}