From 0ea192d55b47a51dfa554f931e15e8ba842be64a Mon Sep 17 00:00:00 2001 From: Scott Pouliot Date: Sun, 26 Apr 2026 18:14:16 -0400 Subject: [PATCH] Harden legacy file paths and Twilio webhook validation --- .../Services/FileService.cs | 131 ++++++++++++++++-- .../Controllers/WebhooksController.cs | 17 ++- .../FileServiceTests.cs | 95 +++++++++++++ .../WebhooksControllerTests.cs | 89 ++++++++++++ 4 files changed, 317 insertions(+), 15 deletions(-) create mode 100644 tests/PowderCoating.UnitTests/FileServiceTests.cs create mode 100644 tests/PowderCoating.UnitTests/WebhooksControllerTests.cs diff --git a/src/PowderCoating.Application/Services/FileService.cs b/src/PowderCoating.Application/Services/FileService.cs index 23c8de4..bd4686c 100644 --- a/src/PowderCoating.Application/Services/FileService.cs +++ b/src/PowderCoating.Application/Services/FileService.cs @@ -14,6 +14,7 @@ namespace PowderCoating.Application.Services; /// public class FileService : IFileService { + private const string UploadsRootFolder = "uploads"; private readonly IWebHostEnvironment _environment; private readonly ILogger _logger; @@ -31,7 +32,9 @@ public class FileService : IFileService /// Validation order: null/empty check, size limit, then extension allowlist. The original file /// name is sanitised with to strip any directory components before /// prepending the GUID prefix, preventing path traversal if the browser supplies a name with - /// slashes. Returns a relative path (from wwwroot) suitable for storing in the database. + /// slashes. The target subfolder is resolved and confined under wwwroot/uploads/ before + /// any file system access occurs. Returns a relative path (from wwwroot) suitable for + /// storing in the database. /// public async Task<(bool Success, string FilePath, string ErrorMessage)> SaveFileAsync( IFormFile file, @@ -65,7 +68,11 @@ public class FileService : IFileService // Create upload directory if it doesn't exist // NOTE: WebRootPath is read-only on Azure Linux App Service; this service is legacy // and should only be called for on-premises deployments. New uploads use Azure Blob. - var uploadPath = Path.Combine(_environment.WebRootPath, "uploads", subfolder); + if (!TryResolveUploadSubfolder(subfolder, out var uploadPath, out var relativeSubfolder, out var subfolderError)) + { + return (false, string.Empty, subfolderError); + } + if (!Directory.Exists(uploadPath)) { try @@ -93,7 +100,7 @@ public class FileService : IFileService } // Return relative path from wwwroot - var relativePath = Path.Combine("uploads", subfolder, uniqueFileName).Replace("\\", "/"); + var relativePath = Path.Combine(UploadsRootFolder, relativeSubfolder, uniqueFileName).Replace("\\", "/"); _logger.LogInformation("File saved successfully: {FilePath}", relativePath); return (true, relativePath, string.Empty); @@ -108,8 +115,8 @@ public class FileService : IFileService /// /// Deletes a file given its relative path from wwwroot. /// Returns success if the file does not exist (idempotent) so that callers do not need to check - /// existence before calling. The relative path is converted to an absolute path with - /// rather than string concatenation to prevent directory traversal. + /// existence before calling. The relative path is normalized and must remain under + /// wwwroot/uploads/; paths outside that root are rejected. /// public async Task<(bool Success, string ErrorMessage)> DeleteFileAsync(string filePath) { @@ -120,7 +127,10 @@ public class FileService : IFileService return (false, "File path is required."); } - var fullPath = Path.Combine(_environment.WebRootPath, filePath.Replace('/', Path.DirectorySeparatorChar)); + if (!TryResolveLegacyUploadPath(filePath, out var fullPath, out var pathError)) + { + return (false, pathError); + } if (!File.Exists(fullPath)) { @@ -142,8 +152,8 @@ public class FileService : IFileService /// /// Reads a file from disk and returns its raw bytes along with a derived MIME content type. - /// Intended for serving files that are stored outside wwwroot (or otherwise not directly - /// accessible via the static-files middleware) so controllers can stream them as file responses. + /// Intended for serving files that are stored under the legacy wwwroot/uploads/ path but + /// are otherwise not directly exposed through the static-files middleware. /// public async Task<(bool Success, byte[] FileContent, string ContentType, string ErrorMessage)> GetFileAsync(string filePath) { @@ -154,7 +164,10 @@ public class FileService : IFileService return (false, Array.Empty(), string.Empty, "File path is required."); } - var fullPath = Path.Combine(_environment.WebRootPath, filePath.Replace('/', Path.DirectorySeparatorChar)); + if (!TryResolveLegacyUploadPath(filePath, out var fullPath, out var pathError)) + { + return (false, Array.Empty(), string.Empty, pathError); + } if (!File.Exists(fullPath)) { @@ -175,7 +188,7 @@ public class FileService : IFileService } /// - /// Checks whether a file exists at the given wwwroot-relative path without reading it. + /// Checks whether a file exists at the given wwwroot/uploads/-relative path without reading it. /// Used by views and controllers to conditionally show download links only when the file is present. /// public bool FileExists(string filePath) @@ -185,7 +198,11 @@ public class FileService : IFileService return false; } - var fullPath = Path.Combine(_environment.WebRootPath, filePath.Replace('/', Path.DirectorySeparatorChar)); + if (!TryResolveLegacyUploadPath(filePath, out var fullPath, out _)) + { + return false; + } + return File.Exists(fullPath); } @@ -212,4 +229,96 @@ public class FileService : IFileService _ => "application/octet-stream" }; } + + private bool TryResolveUploadSubfolder( + string subfolder, + out string uploadPath, + out string relativeSubfolder, + out string errorMessage) + { + uploadPath = string.Empty; + relativeSubfolder = string.Empty; + errorMessage = string.Empty; + + if (string.IsNullOrWhiteSpace(subfolder)) + { + errorMessage = "Upload subfolder is required."; + return false; + } + + if (!TryGetUploadsRootPath(out var uploadsRoot, out errorMessage)) + { + return false; + } + + var normalizedSubfolder = subfolder.Replace('\\', '/').Trim('/'); + var resolvedPath = Path.GetFullPath( + Path.Combine(uploadsRoot, normalizedSubfolder.Replace('/', Path.DirectorySeparatorChar))); + + if (!IsWithinDirectory(resolvedPath, uploadsRoot)) + { + errorMessage = "Invalid upload subfolder."; + _logger.LogWarning("Rejected upload subfolder outside uploads root: {Subfolder}", subfolder); + return false; + } + + relativeSubfolder = Path.GetRelativePath(uploadsRoot, resolvedPath).Replace("\\", "/"); + uploadPath = resolvedPath; + return true; + } + + private bool TryResolveLegacyUploadPath(string filePath, out string fullPath, out string errorMessage) + { + fullPath = string.Empty; + errorMessage = string.Empty; + + if (!TryGetUploadsRootPath(out var uploadsRoot, out errorMessage)) + { + return false; + } + + var normalizedRelativePath = filePath.Replace('\\', '/').TrimStart('/'); + if (!normalizedRelativePath.StartsWith($"{UploadsRootFolder}/", StringComparison.OrdinalIgnoreCase)) + { + errorMessage = "Invalid file path."; + _logger.LogWarning("Rejected legacy file path outside uploads root: {FilePath}", filePath); + return false; + } + + var resolvedPath = Path.GetFullPath( + Path.Combine(_environment.WebRootPath, normalizedRelativePath.Replace('/', Path.DirectorySeparatorChar))); + + if (!IsWithinDirectory(resolvedPath, uploadsRoot)) + { + errorMessage = "Invalid file path."; + _logger.LogWarning("Rejected path traversal attempt for legacy file path: {FilePath}", filePath); + return false; + } + + fullPath = resolvedPath; + return true; + } + + private bool TryGetUploadsRootPath(out string uploadsRoot, out string errorMessage) + { + uploadsRoot = string.Empty; + errorMessage = string.Empty; + + if (string.IsNullOrWhiteSpace(_environment.WebRootPath)) + { + errorMessage = "File storage is not available in this environment."; + _logger.LogWarning("WebRootPath is not configured for the legacy file service."); + return false; + } + + uploadsRoot = Path.GetFullPath(Path.Combine(_environment.WebRootPath, UploadsRootFolder)); + return true; + } + + private static bool IsWithinDirectory(string candidatePath, string rootPath) + { + var normalizedRoot = rootPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + + Path.DirectorySeparatorChar; + return candidatePath.StartsWith(normalizedRoot, StringComparison.OrdinalIgnoreCase); + } } diff --git a/src/PowderCoating.Web/Controllers/WebhooksController.cs b/src/PowderCoating.Web/Controllers/WebhooksController.cs index 5ed4ce2..8b30d61 100644 --- a/src/PowderCoating.Web/Controllers/WebhooksController.cs +++ b/src/PowderCoating.Web/Controllers/WebhooksController.cs @@ -22,6 +22,7 @@ public class WebhooksController : ControllerBase { private readonly ApplicationDbContext _context; private readonly IConfiguration _configuration; + private readonly IWebHostEnvironment _environment; private readonly ILogger _logger; // CTIA-standard opt-out keywords (case-insensitive) @@ -39,10 +40,12 @@ public class WebhooksController : ControllerBase public WebhooksController( ApplicationDbContext context, IConfiguration configuration, + IWebHostEnvironment environment, ILogger logger) { _context = context; _configuration = configuration; + _environment = environment; _logger = logger; } @@ -269,16 +272,22 @@ public class WebhooksController : ControllerBase }; /// - /// Validates the incoming Twilio webhook request using HMAC-SHA1. Skips validation when the - /// auth token is unconfigured so the endpoint works in local development without real Twilio credentials. + /// Validates the incoming Twilio webhook request using HMAC-SHA1. Local development may skip + /// validation when the auth token is unconfigured, but shared environments fail closed. /// private bool ValidateTwilioRequest() { var authToken = _configuration["Twilio:AuthToken"]; if (string.IsNullOrWhiteSpace(authToken) || authToken.StartsWith("your-")) { - _logger.LogDebug("Twilio auth token not configured; skipping signature validation"); - return true; + if (_environment.IsDevelopment()) + { + _logger.LogDebug("Twilio auth token not configured in development; skipping signature validation"); + return true; + } + + _logger.LogError("Twilio auth token is not configured; rejecting webhook request"); + return false; } var signature = Request.Headers["X-Twilio-Signature"].FirstOrDefault() ?? string.Empty; diff --git a/tests/PowderCoating.UnitTests/FileServiceTests.cs b/tests/PowderCoating.UnitTests/FileServiceTests.cs new file mode 100644 index 0000000..b7255d6 --- /dev/null +++ b/tests/PowderCoating.UnitTests/FileServiceTests.cs @@ -0,0 +1,95 @@ +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; +using Moq; +using PowderCoating.Application.Services; + +namespace PowderCoating.UnitTests; + +public class FileServiceTests +{ + [Fact] + public async Task DeleteFileAsync_ReturnsError_WhenPathEscapesUploadsRoot() + { + using var harness = new FileServiceHarness(); + + var result = await harness.Service.DeleteFileAsync("uploads/../secrets.txt"); + + Assert.False(result.Success); + Assert.Equal("Invalid file path.", result.ErrorMessage); + } + + [Fact] + public async Task GetFileAsync_ReturnsError_WhenPathEscapesUploadsRoot() + { + using var harness = new FileServiceHarness(); + + var result = await harness.Service.GetFileAsync("uploads/../../appsettings.json"); + + Assert.False(result.Success); + Assert.Equal("Invalid file path.", result.ErrorMessage); + Assert.Empty(result.FileContent); + } + + [Fact] + public async Task SaveFileAsync_ReturnsError_WhenSubfolderEscapesUploadsRoot() + { + using var harness = new FileServiceHarness(); + + var result = await harness.Service.SaveFileAsync( + CreateFormFile("manual.pdf"), + "../outside", + new[] { ".pdf" }, + 1024 * 1024); + + Assert.False(result.Success); + Assert.Equal("Invalid upload subfolder.", result.ErrorMessage); + } + + [Fact] + public async Task GetFileAsync_ReturnsFile_WhenPathIsUnderUploadsRoot() + { + using var harness = new FileServiceHarness(); + var uploadsPath = Path.Combine(harness.WebRootPath, "uploads", "equipment-manuals"); + Directory.CreateDirectory(uploadsPath); + + var fullPath = Path.Combine(uploadsPath, "manual.pdf"); + await File.WriteAllBytesAsync(fullPath, [1, 2, 3]); + + var result = await harness.Service.GetFileAsync("uploads/equipment-manuals/manual.pdf"); + + Assert.True(result.Success); + Assert.Equal("application/pdf", result.ContentType); + Assert.Equal(new byte[] { 1, 2, 3 }, result.FileContent); + } + + private static IFormFile CreateFormFile(string fileName) + { + var bytes = new byte[] { 1, 2, 3, 4 }; + var stream = new MemoryStream(bytes); + return new FormFile(stream, 0, bytes.Length, "file", fileName); + } + + private sealed class FileServiceHarness : IDisposable + { + public FileServiceHarness() + { + WebRootPath = Path.Combine(Path.GetTempPath(), "powdercoating-fileservice-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(WebRootPath); + + var environment = Mock.Of(x => x.WebRootPath == WebRootPath); + Service = new FileService(environment, Mock.Of>()); + } + + public string WebRootPath { get; } + public FileService Service { get; } + + public void Dispose() + { + if (Directory.Exists(WebRootPath)) + { + Directory.Delete(WebRootPath, recursive: true); + } + } + } +} diff --git a/tests/PowderCoating.UnitTests/WebhooksControllerTests.cs b/tests/PowderCoating.UnitTests/WebhooksControllerTests.cs new file mode 100644 index 0000000..58989df --- /dev/null +++ b/tests/PowderCoating.UnitTests/WebhooksControllerTests.cs @@ -0,0 +1,89 @@ +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Mvc; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using Moq; +using PowderCoating.Infrastructure.Data; +using PowderCoating.Web.Controllers; + +namespace PowderCoating.UnitTests; + +public class WebhooksControllerTests +{ + [Fact] + public async Task TwilioSms_ReturnsForbid_WhenAuthTokenMissingOutsideDevelopment() + { + await using var dbContext = CreateDbContext(); + var controller = CreateController(dbContext, Array.Empty>(), "Production"); + controller.ControllerContext = new ControllerContext + { + HttpContext = CreateHttpContext() + }; + + var result = await controller.TwilioSms(new TwilioSmsPayload { From = "+15551234567", Body = "STOP" }); + + Assert.IsType(result); + } + + [Fact] + public async Task TwilioSms_AllowsMissingAuthToken_InDevelopment() + { + await using var dbContext = CreateDbContext(); + var controller = CreateController(dbContext, Array.Empty>(), "Development"); + controller.ControllerContext = new ControllerContext + { + HttpContext = CreateHttpContext() + }; + + var result = await controller.TwilioSms(new TwilioSmsPayload()); + + var content = Assert.IsType(result); + Assert.Equal("application/xml", content.ContentType); + Assert.Equal("", content.Content); + } + + private static WebhooksController CreateController( + ApplicationDbContext dbContext, + IEnumerable> configValues, + string environmentName) + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(configValues) + .Build(); + + var environment = Mock.Of(x => x.EnvironmentName == environmentName); + + return new WebhooksController( + dbContext, + configuration, + environment, + Mock.Of>()); + } + + private static DefaultHttpContext CreateHttpContext() + { + var context = new DefaultHttpContext(); + context.Request.Method = HttpMethods.Post; + context.Request.Scheme = "https"; + context.Request.Host = new HostString("example.com"); + context.Request.Path = "/Webhooks/TwilioSms"; + context.Features.Set(new FormFeature(new FormCollection(new Dictionary + { + ["From"] = "+15551234567", + ["Body"] = "STOP" + }))); + return context; + } + + private static ApplicationDbContext CreateDbContext() + { + var options = new DbContextOptionsBuilder() + .UseInMemoryDatabase(Guid.NewGuid().ToString()) + .Options; + + return new ApplicationDbContext(options); + } +}