From 5fa11eef169d1ee9999e889451f6e609b05ce033 Mon Sep 17 00:00:00 2001 From: steven-rothwell <45801433+steven-rothwell@users.noreply.github.com> Date: Sun, 12 Nov 2023 10:13:35 -0500 Subject: [PATCH] Version 1.0.1 * Sanitized typeName prior to using it. (#30) * Added release notes and updated NuGet packages. (#31) --- Crud.Api/Constants/Default.cs | 7 ++ Crud.Api/Controllers/CrudController.cs | 100 +++++++++++++----- Crud.Api/Crud.Api.csproj | 4 +- Crud.Api/Program.cs | 1 + Crud.Api/Services/ISanitizerService.cs | 7 ++ Crud.Api/Services/SanitizerService.cs | 21 ++++ .../Controllers/CrudControllerTests.cs | 8 +- .../Crud.Api.Tests/Crud.Api.Tests.csproj | 6 +- .../Services/SanitizerServiceTests.cs | 55 ++++++++++ docs/release-notes/RELEASE-1.0.1.md | 33 ++++++ 10 files changed, 209 insertions(+), 33 deletions(-) create mode 100644 Crud.Api/Constants/Default.cs create mode 100644 Crud.Api/Services/ISanitizerService.cs create mode 100644 Crud.Api/Services/SanitizerService.cs create mode 100644 Crud.Tests/Crud.Api.Tests/Services/SanitizerServiceTests.cs create mode 100644 docs/release-notes/RELEASE-1.0.1.md diff --git a/Crud.Api/Constants/Default.cs b/Crud.Api/Constants/Default.cs new file mode 100644 index 0000000..683cb94 --- /dev/null +++ b/Crud.Api/Constants/Default.cs @@ -0,0 +1,7 @@ +namespace Crud.Api.Constants +{ + public static class Default + { + public const String TypeName = "DefaultTypeName"; + } +} diff --git a/Crud.Api/Controllers/CrudController.cs b/Crud.Api/Controllers/CrudController.cs index 397fc00..6f625d3 100644 --- a/Crud.Api/Controllers/CrudController.cs +++ b/Crud.Api/Controllers/CrudController.cs @@ -28,9 +28,10 @@ public class CrudController : BaseApiController private readonly IQueryCollectionService _queryCollectionService; private readonly IPreprocessingService _preprocessingService; private readonly IPostprocessingService _postprocessingService; + private readonly ISanitizerService _sanitizerService; public CrudController(IOptions applicationOptions, ILogger logger, IValidator validator, IPreserver preserver, IStreamService streamService, ITypeService typeService, IQueryCollectionService queryCollectionService, - IPreprocessingService preprocessingService, IPostprocessingService postprocessingService) + IPreprocessingService preprocessingService, IPostprocessingService postprocessingService, ISanitizerService sanitizerService) : base(applicationOptions) { _logger = logger; @@ -41,14 +42,19 @@ public CrudController(IOptions applicationOptions, ILogger CreateAsync(String typeName) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -80,7 +86,7 @@ public async Task CreateAsync(String typeName) } catch (Exception ex) { - _logger.LogError(ex, $"Error creating with typeName: {typeName}."); + _logger.LogError(ex, $"Error creating with typeName: {sanitizedTypeName}."); return InternalServerError(ex); } } @@ -88,9 +94,13 @@ public async Task CreateAsync(String typeName) [Route("{typeName}/{id:guid}"), HttpGet] public async Task ReadAsync(String typeName, Guid id) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -108,7 +118,7 @@ public async Task ReadAsync(String typeName, Guid id) model = await (dynamic)readAsync.Invoke(_preserver, new object[] { id }); if (model is null) - return NotFound(String.Format(ErrorMessage.NotFoundRead, typeName)); + return NotFound(String.Format(ErrorMessage.NotFoundRead, sanitizedTypeName)); var postprocessingMessageResult = (MessageResult)await _postprocessingService.PostprocessReadAsync(model, id); if (!postprocessingMessageResult.IsSuccessful) @@ -118,7 +128,7 @@ public async Task ReadAsync(String typeName, Guid id) } catch (Exception ex) { - _logger.LogError(ex, $"Error reading with typeName: {typeName}, id: {id}."); + _logger.LogError(ex, $"Error reading with typeName: {sanitizedTypeName}, id: {id}."); return InternalServerError(ex); } } @@ -126,9 +136,13 @@ public async Task ReadAsync(String typeName, Guid id) [Route("{typeName}"), HttpGet] public async Task ReadAsync(String typeName) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -159,7 +173,7 @@ public async Task ReadAsync(String typeName) } catch (Exception ex) { - _logger.LogError(ex, $"Error reading with typeName: {typeName}."); + _logger.LogError(ex, $"Error reading with typeName: {sanitizedTypeName}."); return InternalServerError(ex); } } @@ -167,9 +181,13 @@ public async Task ReadAsync(String typeName) [Route("query/{typeName}"), HttpPost] public async Task QueryReadAsync(String typeName) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -222,7 +240,7 @@ public async Task QueryReadAsync(String typeName) } catch (Exception ex) { - _logger.LogError(ex, $"Error query reading with typeName: {typeName}."); + _logger.LogError(ex, $"Error query reading with typeName: {sanitizedTypeName}."); return InternalServerError(ex); } } @@ -230,9 +248,13 @@ public async Task QueryReadAsync(String typeName) [Route("query/{typeName}/count"), HttpPost] public async Task QueryReadCountAsync(String typeName) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -277,7 +299,7 @@ public async Task QueryReadCountAsync(String typeName) } catch (Exception ex) { - _logger.LogError(ex, $"Error query reading count with typeName: {typeName}."); + _logger.LogError(ex, $"Error query reading count with typeName: {sanitizedTypeName}."); return InternalServerError(ex); } } @@ -285,9 +307,13 @@ public async Task QueryReadCountAsync(String typeName) [Route("{typeName}/{id:guid}"), HttpPut] public async Task UpdateAsync(String typeName, Guid id) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -312,7 +338,7 @@ public async Task UpdateAsync(String typeName, Guid id) var updatedModel = await _preserver.UpdateAsync(model, id); if (updatedModel is null) - return NotFound(String.Format(ErrorMessage.NotFoundUpdate, typeName)); + return NotFound(String.Format(ErrorMessage.NotFoundUpdate, sanitizedTypeName)); var postprocessingMessageResult = (MessageResult)await _postprocessingService.PostprocessUpdateAsync(updatedModel, id); if (!postprocessingMessageResult.IsSuccessful) @@ -322,7 +348,7 @@ public async Task UpdateAsync(String typeName, Guid id) } catch (Exception ex) { - _logger.LogError(ex, $"Error updating with typeName: {typeName}, id: {id}."); + _logger.LogError(ex, $"Error updating with typeName: {sanitizedTypeName}, id: {id}."); return InternalServerError(ex); } } @@ -330,9 +356,13 @@ public async Task UpdateAsync(String typeName, Guid id) [Route("{typeName}/{id:guid}"), HttpPatch] public async Task PartialUpdateAsync(String typeName, Guid id) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -359,7 +389,7 @@ public async Task PartialUpdateAsync(String typeName, Guid id) var updatedModel = await (dynamic)partialUpdateAsync.Invoke(_preserver, new object[] { id, propertyValues }); if (updatedModel is null) - return NotFound(String.Format(ErrorMessage.NotFoundUpdate, typeName)); + return NotFound(String.Format(ErrorMessage.NotFoundUpdate, sanitizedTypeName)); var postprocessingMessageResult = (MessageResult)await _postprocessingService.PostprocessPartialUpdateAsync(updatedModel, id, propertyValues); if (!postprocessingMessageResult.IsSuccessful) @@ -369,7 +399,7 @@ public async Task PartialUpdateAsync(String typeName, Guid id) } catch (Exception ex) { - _logger.LogError(ex, $"Error partially updating with typeName: {typeName}, id {id}."); + _logger.LogError(ex, $"Error partially updating with typeName: {sanitizedTypeName}, id {id}."); return InternalServerError(ex); } } @@ -377,9 +407,13 @@ public async Task PartialUpdateAsync(String typeName, Guid id) [Route("{typeName}"), HttpPatch] public async Task PartialUpdateAsync(String typeName) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -415,7 +449,7 @@ public async Task PartialUpdateAsync(String typeName) } catch (Exception ex) { - _logger.LogError(ex, $"Error partially updating with typeName: {typeName}."); + _logger.LogError(ex, $"Error partially updating with typeName: {sanitizedTypeName}."); return InternalServerError(ex); } } @@ -423,9 +457,13 @@ public async Task PartialUpdateAsync(String typeName) [Route("{typeName}/{id:guid}"), HttpDelete] public async Task DeleteAsync(String typeName, Guid id) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -443,7 +481,7 @@ public async Task DeleteAsync(String typeName, Guid id) var deletedCount = await (dynamic)deleteAsync.Invoke(_preserver, new object[] { id }); if (deletedCount == 0) - return NotFound(String.Format(ErrorMessage.NotFoundDelete, typeName)); + return NotFound(String.Format(ErrorMessage.NotFoundDelete, sanitizedTypeName)); var postprocessingMessageResult = (MessageResult)await _postprocessingService.PostprocessDeleteAsync(model, id, deletedCount); if (!postprocessingMessageResult.IsSuccessful) @@ -453,7 +491,7 @@ public async Task DeleteAsync(String typeName, Guid id) } catch (Exception ex) { - _logger.LogError(ex, $"Error deleting with typeName: {typeName}, id: {id}."); + _logger.LogError(ex, $"Error deleting with typeName: {sanitizedTypeName}, id: {id}."); return InternalServerError(ex); } } @@ -461,9 +499,13 @@ public async Task DeleteAsync(String typeName, Guid id) [Route("{typeName}"), HttpDelete] public async Task DeleteAsync(String typeName) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -494,7 +536,7 @@ public async Task DeleteAsync(String typeName) } catch (Exception ex) { - _logger.LogError(ex, $"Error deleting with typeName: {typeName}."); + _logger.LogError(ex, $"Error deleting with typeName: {sanitizedTypeName}."); return InternalServerError(ex); } } @@ -502,9 +544,13 @@ public async Task DeleteAsync(String typeName) [Route("query/{typeName}"), HttpDelete] public async Task QueryDeleteAsync(String typeName) { + var sanitizedTypeName = Default.TypeName; + try { - var type = _typeService.GetModelType(typeName); + sanitizedTypeName = _sanitizerService.SanitizeTypeName(typeName); + + var type = _typeService.GetModelType(sanitizedTypeName); if (type is null) return BadRequest(ErrorMessage.BadRequestModelType); @@ -549,7 +595,7 @@ public async Task QueryDeleteAsync(String typeName) } catch (Exception ex) { - _logger.LogError(ex, $"Error deleting with typeName: {typeName}."); + _logger.LogError(ex, $"Error deleting with typeName: {sanitizedTypeName}."); return InternalServerError(ex); } } diff --git a/Crud.Api/Crud.Api.csproj b/Crud.Api/Crud.Api.csproj index 3ae791c..ebdc995 100644 --- a/Crud.Api/Crud.Api.csproj +++ b/Crud.Api/Crud.Api.csproj @@ -7,8 +7,8 @@ - - + + \ No newline at end of file diff --git a/Crud.Api/Program.cs b/Crud.Api/Program.cs index 5b8d544..748d46f 100644 --- a/Crud.Api/Program.cs +++ b/Crud.Api/Program.cs @@ -19,6 +19,7 @@ builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); +builder.Services.AddScoped(); var conventionPack = new ConventionPack { diff --git a/Crud.Api/Services/ISanitizerService.cs b/Crud.Api/Services/ISanitizerService.cs new file mode 100644 index 0000000..bdea1e0 --- /dev/null +++ b/Crud.Api/Services/ISanitizerService.cs @@ -0,0 +1,7 @@ +namespace Crud.Api.Services +{ + public interface ISanitizerService + { + String SanitizeTypeName(String? className); + } +} diff --git a/Crud.Api/Services/SanitizerService.cs b/Crud.Api/Services/SanitizerService.cs new file mode 100644 index 0000000..e1092e6 --- /dev/null +++ b/Crud.Api/Services/SanitizerService.cs @@ -0,0 +1,21 @@ +using System.Text.RegularExpressions; +using Crud.Api.Constants; + +namespace Crud.Api.Services +{ + public class SanitizerService : ISanitizerService + { + public String SanitizeTypeName(String? typeName) + { + if (String.IsNullOrWhiteSpace(typeName)) + return Default.TypeName; + + string sanitizedTypeName = Regex.Replace(typeName, @"[^@\w]", String.Empty); + + if (sanitizedTypeName.Length == 0) + return Default.TypeName; + + return sanitizedTypeName; + } + } +} diff --git a/Crud.Tests/Crud.Api.Tests/Controllers/CrudControllerTests.cs b/Crud.Tests/Crud.Api.Tests/Controllers/CrudControllerTests.cs index 0d07aea..090c99d 100644 --- a/Crud.Tests/Crud.Api.Tests/Controllers/CrudControllerTests.cs +++ b/Crud.Tests/Crud.Api.Tests/Controllers/CrudControllerTests.cs @@ -32,6 +32,7 @@ public class CrudControllerTests : IDisposable private Mock _queryCollectionService; private Mock _preprocessingService; private Mock _postprocessingService; + private Mock _sanitizerService; private CrudController _controller; private Stream _stream; @@ -46,11 +47,12 @@ public CrudControllerTests() _queryCollectionService = new Mock(); _preprocessingService = new Mock(); _postprocessingService = new Mock(); + _sanitizerService = new Mock(); _stream = new MemoryStream(Encoding.UTF8.GetBytes("this-does-not-matter")); var httpContext = new DefaultHttpContext() { Request = { Body = _stream, ContentLength = _stream.Length } }; var controllerContext = new ControllerContext { HttpContext = httpContext }; - _controller = new CrudController(_applicationOptions, _logger.Object, _validator.Object, _preserver.Object, _streamService.Object, _typeService.Object, _queryCollectionService.Object, _preprocessingService.Object, _postprocessingService.Object) { ControllerContext = controllerContext }; + _controller = new CrudController(_applicationOptions, _logger.Object, _validator.Object, _preserver.Object, _streamService.Object, _typeService.Object, _queryCollectionService.Object, _preprocessingService.Object, _postprocessingService.Object, _sanitizerService.Object) { ControllerContext = controllerContext }; } public void Dispose() @@ -273,6 +275,7 @@ public async Task ReadAsync_WithStringGuid_ModelIsNull_ReturnsNotFound() Model? model = null; var preprocessingMessageResult = new MessageResult(true); + _sanitizerService.Setup(m => m.SanitizeTypeName(It.IsAny())).Returns(typeName); _typeService.Setup(m => m.GetModelType(It.IsAny())).Returns(type); _preprocessingService.Setup(m => m.PreprocessReadAsync(It.IsAny(), It.IsAny())).ReturnsAsync(preprocessingMessageResult); _preserver.Setup(m => m.ReadAsync(It.IsAny())).ReturnsAsync(model); @@ -1093,6 +1096,7 @@ public async Task UpdateAsync_UpdatedModelIsNull_ReturnsNotFound() Model? updatedModel = null; var preprocessingMessageResult = new MessageResult(true); + _sanitizerService.Setup(m => m.SanitizeTypeName(It.IsAny())).Returns(typeName); _typeService.Setup(m => m.GetModelType(It.IsAny())).Returns(type); _streamService.Setup(m => m.ReadToEndThenDisposeAsync(It.IsAny(), It.IsAny())).ReturnsAsync(json); _validator.Setup(m => m.ValidateUpdateAsync(It.IsAny(), It.IsAny())).ReturnsAsync(validationResult); @@ -1287,6 +1291,7 @@ public async Task PartialUpdateAsync_WithStringGuid_UpdatedModelIsNull_ReturnsNo Model? updatedModel = null; var preprocessingMessageResult = new MessageResult(true); + _sanitizerService.Setup(m => m.SanitizeTypeName(It.IsAny())).Returns(typeName); _typeService.Setup(m => m.GetModelType(It.IsAny())).Returns(type); _streamService.Setup(m => m.ReadToEndThenDisposeAsync(It.IsAny(), It.IsAny())).ReturnsAsync(json); _validator.Setup(m => m.ValidatePartialUpdateAsync(It.IsAny(), It.IsAny(), It.IsAny>())).ReturnsAsync(validationResult); @@ -1588,6 +1593,7 @@ public async Task DeleteAsync_WithStringGuid_DeletedCountIsZero_ReturnsNotFound( var deletedCount = 0; var preprocessingMessageResult = new MessageResult(true); + _sanitizerService.Setup(m => m.SanitizeTypeName(It.IsAny())).Returns(typeName); _typeService.Setup(m => m.GetModelType(It.IsAny())).Returns(type); _preprocessingService.Setup(m => m.PreprocessDeleteAsync(It.IsAny(), It.IsAny())).ReturnsAsync(preprocessingMessageResult); _preserver.Setup(m => m.DeleteAsync(It.IsAny())).ReturnsAsync(deletedCount); diff --git a/Crud.Tests/Crud.Api.Tests/Crud.Api.Tests.csproj b/Crud.Tests/Crud.Api.Tests/Crud.Api.Tests.csproj index fbfe329..42a772f 100644 --- a/Crud.Tests/Crud.Api.Tests/Crud.Api.Tests.csproj +++ b/Crud.Tests/Crud.Api.Tests/Crud.Api.Tests.csproj @@ -10,11 +10,11 @@ - + - - + + runtime; build; native; contentfiles; analyzers; buildtransitive all diff --git a/Crud.Tests/Crud.Api.Tests/Services/SanitizerServiceTests.cs b/Crud.Tests/Crud.Api.Tests/Services/SanitizerServiceTests.cs new file mode 100644 index 0000000..be952aa --- /dev/null +++ b/Crud.Tests/Crud.Api.Tests/Services/SanitizerServiceTests.cs @@ -0,0 +1,55 @@ +using Crud.Api.Constants; +using Crud.Api.Services; + +namespace Crud.Api.Tests.Services +{ + public class SanitizerServiceTests + { + private SanitizerService _sanitizerService; + + public SanitizerServiceTests() + { + _sanitizerService = new SanitizerService(); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void SanitizeTypeName_TypeNameIsNullOrWhitespace_ReturnsDefaultTypeName(String? typeName) + { + var result = _sanitizerService.SanitizeTypeName(typeName); + + Assert.Equal(Default.TypeName, result); + } + + [Theory] + [InlineData("!#$%^")] + [InlineData("(>*-*)>")] + public void SanitizeTypeName_SanitizedTypeNameEmpty_ReturnsDefaultTypeName(String? typeName) + { + var result = _sanitizerService.SanitizeTypeName(typeName); + + Assert.Equal(Default.TypeName, result); + } + + [Theory] + [ClassData(typeof(CleanTypeNames))] + public void SanitizeTypeName_TypeNameIsAlreadyClean_ReturnsUnchangedTypeName(String? typeName) + { + var result = _sanitizerService.SanitizeTypeName(typeName); + + Assert.Equal(typeName, result); + } + + private class CleanTypeNames : TheoryData + { + public CleanTypeNames() + { + Add("ThisIsAlreadyValid"); + Add("@if"); + Add("_1ClassName"); + } + } + } +} diff --git a/docs/release-notes/RELEASE-1.0.1.md b/docs/release-notes/RELEASE-1.0.1.md new file mode 100644 index 0000000..517702c --- /dev/null +++ b/docs/release-notes/RELEASE-1.0.1.md @@ -0,0 +1,33 @@ +# Release v1.0.0 + +## Notes + +Fix security issues with type name and updated NuGet packages. + +### Breaking Changes + +- None + +### New Features + +- None + +### Maintenance + +- Sanitized `typeName` prior to use. +- Updated NuGet packages. + - Crud.Api + - MongoDB.Driver + - MongoDB.Bson + - Crud.Api.Tests + - Microsoft.NET.Test.Sdk + - xunit + - xunit.runner.visualstudio + +## Available Preservers + +- MongoDB + +## Framework + +- .NET 7