Harden legacy file paths and Twilio webhook validation

src/PowderCoating.Application/Services/FileService.cs

@@ -14,6 +14,7 @@ namespace PowderCoating.Application.Services;
 /// </summary>
 public class FileService : IFileService
 {
+    private const string UploadsRootFolder = "uploads";
     private readonly IWebHostEnvironment _environment;
     private readonly ILogger<FileService> _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 <see cref="Path.GetFileName"/> 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 <c>wwwroot</c>) suitable for storing in the database.
+    /// slashes. The target subfolder is resolved and confined under <c>wwwroot/uploads/</c> before
+    /// any file system access occurs. Returns a relative path (from <c>wwwroot</c>) suitable for
+    /// storing in the database.
     /// </summary>
     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
     /// <summary>
     /// Deletes a file given its relative path from <c>wwwroot</c>.
     /// 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
+    /// existence before calling. The relative path is normalized and must remain under
-    /// <see cref="Path.Combine"/> rather than string concatenation to prevent directory traversal.
+    /// <c>wwwroot/uploads/</c>; paths outside that root are rejected.
     /// </summary>
     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
 
     /// <summary>
     /// 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 <c>wwwroot</c> (or otherwise not directly
+    /// Intended for serving files that are stored under the legacy <c>wwwroot/uploads/</c> path but
-    /// accessible via the static-files middleware) so controllers can stream them as file responses.
+    /// are otherwise not directly exposed through the static-files middleware.
     /// </summary>
     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<byte>(), 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<byte>(), string.Empty, pathError);
+            }
 
             if (!File.Exists(fullPath))
             {
@@ -175,7 +188,7 @@ public class FileService : IFileService
     }
 
     /// <summary>
-    /// Checks whether a file exists at the given <c>wwwroot</c>-relative path without reading it.
+    /// Checks whether a file exists at the given <c>wwwroot/uploads/</c>-relative path without reading it.
     /// Used by views and controllers to conditionally show download links only when the file is present.
     /// </summary>
     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);
+    }
 }

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<WebhooksController> _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<WebhooksController> logger)
     {
         _context = context;
         _configuration = configuration;
+        _environment = environment;
         _logger = logger;
     }
 
@@ -269,16 +272,22 @@ public class WebhooksController : ControllerBase
         };
 
     /// <summary>
-    /// Validates the incoming Twilio webhook request using HMAC-SHA1. Skips validation when the
+    /// Validates the incoming Twilio webhook request using HMAC-SHA1. Local development may skip
-    /// auth token is unconfigured so the endpoint works in local development without real Twilio credentials.
+    /// validation when the auth token is unconfigured, but shared environments fail closed.
     /// </summary>
     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");
+            if (_environment.IsDevelopment())
-            return true;
+            {
+                _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;

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<IWebHostEnvironment>(x => x.WebRootPath == WebRootPath);
+            Service = new FileService(environment, Mock.Of<ILogger<FileService>>());
+        }
+
+        public string WebRootPath { get; }
+        public FileService Service { get; }
+
+        public void Dispose()
+        {
+            if (Directory.Exists(WebRootPath))
+            {
+                Directory.Delete(WebRootPath, recursive: true);
+            }
+        }
+    }
+}

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<KeyValuePair<string, string?>>(), "Production");
+        controller.ControllerContext = new ControllerContext
+        {
+            HttpContext = CreateHttpContext()
+        };
+
+        var result = await controller.TwilioSms(new TwilioSmsPayload { From = "+15551234567", Body = "STOP" });
+
+        Assert.IsType<ForbidResult>(result);
+    }
+
+    [Fact]
+    public async Task TwilioSms_AllowsMissingAuthToken_InDevelopment()
+    {
+        await using var dbContext = CreateDbContext();
+        var controller = CreateController(dbContext, Array.Empty<KeyValuePair<string, string?>>(), "Development");
+        controller.ControllerContext = new ControllerContext
+        {
+            HttpContext = CreateHttpContext()
+        };
+
+        var result = await controller.TwilioSms(new TwilioSmsPayload());
+
+        var content = Assert.IsType<ContentResult>(result);
+        Assert.Equal("application/xml", content.ContentType);
+        Assert.Equal("<Response/>", content.Content);
+    }
+
+    private static WebhooksController CreateController(
+        ApplicationDbContext dbContext,
+        IEnumerable<KeyValuePair<string, string?>> configValues,
+        string environmentName)
+    {
+        var configuration = new ConfigurationBuilder()
+            .AddInMemoryCollection(configValues)
+            .Build();
+
+        var environment = Mock.Of<IWebHostEnvironment>(x => x.EnvironmentName == environmentName);
+
+        return new WebhooksController(
+            dbContext,
+            configuration,
+            environment,
+            Mock.Of<ILogger<WebhooksController>>());
+    }
+
+    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<IFormFeature>(new FormFeature(new FormCollection(new Dictionary<string, Microsoft.Extensions.Primitives.StringValues>
+        {
+            ["From"] = "+15551234567",
+            ["Body"] = "STOP"
+        })));
+        return context;
+    }
+
+    private static ApplicationDbContext CreateDbContext()
+    {
+        var options = new DbContextOptionsBuilder<ApplicationDbContext>()
+            .UseInMemoryDatabase(Guid.NewGuid().ToString())
+            .Options;
+
+        return new ApplicationDbContext(options);
+    }
+}