Manually deserialise and validate model so the raw body can be used i…

…n security validation first
jamesseanwright · Nov 23, 2017 · 793fd9f · 793fd9f
1 parent b1826b4
commit 793fd9f
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 10 deletions.
diff --git a/Shared/Dependencies.cs b/Shared/Dependencies.cs
@@ -9,6 +9,7 @@ public static void Register(IServiceCollection services) {
             services.AddSingleton(typeof (IResponseFactory), typeof (ResponseFactory));
             services.AddSingleton(typeof (IModelStateConverter), typeof (ModelStateConverter));
             services.AddSingleton(typeof (IJsonSerialiser), typeof (JsonSerialiser));
+            services.AddSingleton(typeof (IRequestValidator), typeof (RequestValidator));
             services.AddSingleton(typeof (IEnvironment), typeof (Environment));
             services.AddSingleton(typeof (IHttpClient), typeof (HttpClient));
         }

diff --git a/Shared/Environment.cs b/Shared/Environment.cs
@@ -2,5 +2,6 @@ namespace GitHubAutoresponder.Shared {
     public class Environment : IEnvironment
     {
         public string EncodededCredentials => System.Environment.GetEnvironmentVariable("GHAR_CREDENTIALS");
+        public string Secret => System.Environment.GetEnvironmentVariable("GHAR_SECRET");
     }
 }
diff --git a/Shared/IEnvironment.cs b/Shared/IEnvironment.cs
@@ -1,5 +1,6 @@
 namespace GitHubAutoresponder.Shared {
     public interface IEnvironment {
         string EncodededCredentials { get; }
+        string Secret { get; }
     }
 }
diff --git a/Responder/IJsonSerialiser.cs → Shared/IJsonSerialiser.cs b/Responder/IJsonSerialiser.cs → Shared/IJsonSerialiser.cs
@@ -1,7 +1,8 @@
 using System.IO;
 
-namespace GitHubAutoresponder.Responder {
+namespace GitHubAutoresponder.Shared {
     public interface IJsonSerialiser {
         string Serialise(object obj);
+        T Deserialise<T>(string json);
     }
 }
diff --git a/Responder/JsonSerialiser.cs → Shared/JsonSerialiser.cs b/Responder/JsonSerialiser.cs → Shared/JsonSerialiser.cs
@@ -1,9 +1,8 @@
 using System;
 using System.IO;
-using GitHubAutoresponder.Shared;
 using Newtonsoft.Json;
 
-namespace GitHubAutoresponder.Responder {
+namespace GitHubAutoresponder.Shared {
     public class JsonSerialiser : IJsonSerialiser {
         private JsonSerializerSettings settings;
 
@@ -16,5 +15,9 @@ public JsonSerialiser() {
         public string Serialise(object obj) {
             return JsonConvert.SerializeObject(obj, this.settings);
         }
+
+        public T Deserialise<T>(string json) {
+            return JsonConvert.DeserializeObject<T>(json, this.settings);
+        }
     }
 }
diff --git a/Webhook/Tests/WebhookControllerTests.cs b/Webhook/Tests/WebhookControllerTests.cs
@@ -10,20 +10,30 @@
 using Microsoft.AspNetCore.Mvc.Controllers;
 using System.Net;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
+using GitHubAutoresponder.Shared;
 
 namespace GitHubAutoresponder.Webhook.Tests {
     public class WebhookControllerTests : IDisposable {
         private Mock<IGitHubResponder> gitHubResponder;
         private Mock<IModelStateConverter> modelStateConverter;
+        private Mock<IJsonSerialiser> jsonSerialiser;
+        private Mock<IRequestValidator> requestValidator;
+        private Mock<IEnvironment> environment;
         private WebhookController webhookController;
 
         public WebhookControllerTests() {
             this.gitHubResponder = new Mock<IGitHubResponder>();
             this.modelStateConverter = new Mock<IModelStateConverter>();
+            this.jsonSerialiser = new Mock<IJsonSerialiser>();
+            this.requestValidator = new Mock<IRequestValidator>();
+            this.environment = new Mock<IEnvironment>();
 
             this.webhookController = new WebhookController(
                 this.gitHubResponder.Object,
-                this.modelStateConverter.Object
+                this.modelStateConverter.Object,
+                this.jsonSerialiser.Object,
+                this.requestValidator.Object,
+                this.environment.Object
             );
         }
 
@@ -39,7 +49,7 @@ public async Task ItShouldForwardThePayload() {
                 .Setup(g => g.RespondAsync(It.IsAny<Payload>()))
                 .ReturnsAsync(true);
 
-            ContentResult result = await this.webhookController.PostAsync(payload);
+            ContentResult result = await this.webhookController.PostAsync();
 
             Assert.StrictEqual<int?>((int) HttpStatusCode.OK, result.StatusCode);
             Assert.Equal("OK", result.Content);
@@ -56,7 +66,7 @@ public async Task ItShouldRespondWithBadRequestWhenPayloadIsInvalid() {
 
             this.webhookController.ModelState.AddModelError("key", "Some model error");
 
-            ContentResult result = await this.webhookController.PostAsync(payload);
+            ContentResult result = await this.webhookController.PostAsync();
 
             Assert.StrictEqual<int?>((int) HttpStatusCode.BadRequest, result.StatusCode);
             Assert.Equal("Model validation errors", result.Content);
@@ -70,7 +80,7 @@ public async Task ItShouldRespondWithBadGatewayWhenUpstreamReturnsError() {
                 .Setup(g => g.RespondAsync(It.IsAny<Payload>()))
                 .ReturnsAsync(false);
 
-            ContentResult result = await this.webhookController.PostAsync(payload);
+            ContentResult result = await this.webhookController.PostAsync();
 
             Assert.StrictEqual<int?>((int) HttpStatusCode.BadGateway, result.StatusCode);
         }

diff --git a/Webhook/WebhookController.cs b/Webhook/WebhookController.cs
@@ -8,21 +8,54 @@
 using GitHubAutoresponder.Responder;
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
+using GitHubAutoresponder.Shared;
+using System.IO;
 
 namespace GitHubAutoresponder.Webhook {
     [Route("api/[controller]")]
     public class WebhookController : Controller {
         private IGitHubResponder gitHubResponder;
         private IModelStateConverter modelStateConverter;
+        private IJsonSerialiser jsonSerialiser;
+        private IRequestValidator requestValidator;
+        private IEnvironment environment;
 
-        public WebhookController(IGitHubResponder gitHubResponder, IModelStateConverter modelStateConverter) {
+        public WebhookController(
+            IGitHubResponder gitHubResponder,
+            IModelStateConverter modelStateConverter,
+            IJsonSerialiser jsonSerialiser,
+            IRequestValidator requestValidator,
+            IEnvironment environment
+        ) {
             this.gitHubResponder = gitHubResponder;
             this.modelStateConverter = modelStateConverter;
+            this.jsonSerialiser = jsonSerialiser;
+            this.requestValidator = requestValidator;
+            this.environment = environment;
         }
 
         [HttpPost]
-        public async Task<ContentResult> PostAsync([FromBody]Payload payload) {
-            if (!ModelState.IsValid) {
+        public async Task<ContentResult> PostAsync() {
+            /* We need the raw body to valid the request
+             * by computing a HMAC hash from it based upon
+             * the secret key. We then manually deserialise
+             * it for validation and further manipulation.
+             */
+            string body = await this.GetBodyAsync();
+
+            bool isValidRequest = this.requestValidator.IsValidRequest(
+                Request.Headers["X-Hub-Signature"],
+                this.environment.Secret,
+                body
+            );
+
+            if (!isValidRequest) {
+                return this.CreateUnauthorisedResult();
+            }
+
+            Payload payload = this.jsonSerialiser.Deserialise<Payload>(body);
+
+            if (!TryValidateModel(payload)) {
                 return this.CreateValidationErrorResult();
             }
 
@@ -31,6 +64,24 @@ public async Task<ContentResult> PostAsync([FromBody]Payload payload) {
             return isSuccessful? this.CreateSuccessResult() : this.CreateUpstreamErrorResult();
         }
 
+        private async Task<string> GetBodyAsync() {
+            using (StreamReader reader = new StreamReader(Request.Body)) {
+                return await reader.ReadToEndAsync();
+            }
+        }
+
+        private ContentResult CreateUnauthorisedResult() {
+            ContentResult result = Content(
+                "This request lacks the required authorisation information",
+                MediaTypeNames.Text.Plain,
+                Encoding.UTF8
+            );
+
+            result.StatusCode = (int) HttpStatusCode.Unauthorized;
+
+            return result;
+        }
+
         private ContentResult CreateValidationErrorResult() {
             ContentResult result = Content(
                 modelStateConverter.AsString(ModelState),