Sanitize user input

A malicious user may be able to forge new log entries or may access unexpected resources.
GeoWerkstatt · May 3, 2024 · 2d44e45 · 2d44e45
1 parent d9072d1
commit 2d44e45
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 12 deletions.
diff --git a/src/Geopilot.Api/Controllers/MandateController.cs b/src/Geopilot.Api/Controllers/MandateController.cs
@@ -42,9 +42,9 @@ public MandateController(ILogger<MandateController> logger, Context context, IVa
     [SwaggerResponse(StatusCodes.Status200OK, "Returns list of mandates associated to the current user matching the optional filter criteria.", typeof(IEnumerable<DeliveryMandate>), new[] { "application/json" })]
     public async Task<IActionResult> Get(
         [FromQuery, SwaggerParameter("Filter mandates matching validation job file extension.")]
-        string jobId = "")
+        Guid jobId = default)
     {
-        logger.LogInformation("Getting mandates for job with id <{JobId}>.", jobId.ReplaceLineEndings(string.Empty));
+        logger.LogInformation("Getting mandates for job with id <{JobId}>.", jobId);
 
         var user = await context.GetUserByPrincipalAsync(User);
         if (user == null)
@@ -53,24 +53,24 @@ public async Task<IActionResult> Get(
         var mandates = context.DeliveryMandates
             .Where(m => m.Organisations.SelectMany(o => o.Users).Any(u => u.Id == user.Id));
 
-        if (Guid.TryParse(jobId, out var guid))
+        if (jobId != default)
         {
-            var job = validationService.GetJob(guid);
+            var job = validationService.GetJob(jobId);
             if (job is null)
             {
-                logger.LogTrace("Validation job with id <{JobId}> was not found.", guid.ToString());
+                logger.LogTrace("Validation job with id <{JobId}> was not found.", jobId);
                 return Ok(Array.Empty<DeliveryMandate>());
             }
 
-            logger.LogTrace("Filtering mandates for job with id <{JobId}>", guid.ToString());
+            logger.LogTrace("Filtering mandates for job with id <{JobId}>", jobId);
             var extension = Path.GetExtension(job.OriginalFileName);
             mandates = mandates
                 .Where(m => m.FileTypes.Contains(".*") || m.FileTypes.Contains(extension));
         }
 
         var result = await mandates.ToListAsync();
 
-        logger.LogInformation("Getting mandates with for job with id <{JobId}> resulted in <{MatchingMandatesCount}> matching mandates.", guid.ToString(), result.Count);
+        logger.LogInformation("Getting mandates with for job with id <{JobId}> resulted in <{MatchingMandatesCount}> matching mandates.", jobId, result.Count);
         return Ok(mandates);
     }
 }
diff --git a/src/Geopilot.Api/Controllers/ValidationController.cs b/src/Geopilot.Api/Controllers/ValidationController.cs
@@ -166,9 +166,9 @@ public IActionResult GetStatus(Guid jobId)
     [SwaggerResponse(StatusCodes.Status404NotFound, "The job or log file cannot be found.", typeof(ProblemDetails), new[] { "application/json" })]
     public IActionResult Download(Guid jobId, string file)
     {
-        var sanitizedFilename = Path.GetFileName(file.Trim().ReplaceLineEndings(string.Empty));
+        var sanitizedFilename = file.SanitizeFileName();
 
-        logger.LogInformation("Download file <{File}> for job <{JobId}> requested.", sanitizedFilename, jobId.ToString());
+        logger.LogInformation("Download file <{File}> for job <{JobId}> requested.", sanitizedFilename, jobId);
         fileProvider.Initialize(jobId);
 
         var validationJob = validationService.GetJob(jobId);

diff --git a/src/Geopilot.Api/Extensions.cs b/src/Geopilot.Api/Extensions.cs
@@ -0,0 +1,30 @@
+using System.Globalization;
+
+namespace Geopilot.Api;
+
+/// <summary>
+/// GeoPilot API extensions.
+/// </summary>
+public static class Extensions
+{
+    /// <summary>
+    /// Sanitizes a file name by removing invalid characters.
+    /// </summary>
+    /// <param name="fileName">The file name to sanitize.</param>
+    /// <returns>The sanitized file name.</returns>
+    public static string SanitizeFileName(this string fileName)
+    {
+        fileName = fileName.Trim().ReplaceLineEndings(string.Empty);
+
+        // Get invalid characters for file names and add some platform-specific ones.
+        var invalidFileNameChars = Path.GetInvalidFileNameChars()
+            .Concat(new[] { '?', '$', '*', '|', '<', '>', '"', ':' }).ToArray();
+
+        foreach (var invalidFileNameChar in invalidFileNameChars)
+        {
+            fileName = fileName.Replace(invalidFileNameChar.ToString(CultureInfo.InvariantCulture), string.Empty);
+        }
+
+        return fileName;
+    }
+}
diff --git a/tests/Geopilot.Api.Test/Controllers/MandateControllerTest.cs b/tests/Geopilot.Api.Test/Controllers/MandateControllerTest.cs
@@ -70,7 +70,7 @@ public async Task GetWithJobIdIncludesMatchingMandates()
                 .Setup(m => m.GetJob(jobId))
                 .Returns(new ValidationJob(jobId, "Original.xtf", "tmp.xtf"));
 
-            var result = (await mandateController.Get(jobId.ToString())) as OkObjectResult;
+            var result = (await mandateController.Get(jobId)) as OkObjectResult;
             var mandates = (result?.Value as IEnumerable<DeliveryMandate>)?.ToList();
 
             Assert.IsNotNull(mandates);
@@ -88,7 +88,7 @@ public async Task GetWithJobIdExcludesNonMatchinMandates()
                 .Setup(m => m.GetJob(jobId))
                 .Returns(new ValidationJob(jobId, "Original.csv", "tmp.csv"));
 
-            var result = (await mandateController.Get(jobId: jobId.ToString())) as OkObjectResult;
+            var result = (await mandateController.Get(jobId)) as OkObjectResult;
             var mandates = (result?.Value as IEnumerable<DeliveryMandate>)?.ToList();
 
             Assert.IsNotNull(mandates);
@@ -106,7 +106,7 @@ public async Task GetWithInvalidJobIdReturnsEmptyArray()
                 .Setup(m => m.GetJob(jobId))
                 .Returns(() => null);
 
-            var result = (await mandateController.Get(jobId: jobId.ToString())) as OkObjectResult;
+            var result = (await mandateController.Get(jobId)) as OkObjectResult;
             var mandates = (result?.Value as IEnumerable<DeliveryMandate>)?.ToList();
 
             Assert.IsNotNull(mandates);

diff --git a/tests/Geopilot.Api.Test/Extensions.cs b/tests/Geopilot.Api.Test/Extensions.cs
@@ -0,0 +1,37 @@
+namespace Geopilot.Api.Controllers;
+
+[TestClass]
+public class Extensions
+{
+    [TestMethod]
+    public void SanitizeFileNameForValidFileNames()
+    {
+        AssertSanitizeFileName("SQUIRRELGENESIS", "SQUIRRELGENESIS");
+        AssertSanitizeFileName("JUNIORARK.xyz", "JUNIORARK.xyz");
+        AssertSanitizeFileName("PEEVEDBEAM-ANT.MESS.abc", "PEEVEDBEAM-ANT.MESS.abc");
+        AssertSanitizeFileName("WEIRD WATER.example", "WEIRD WATER.example");
+        AssertSanitizeFileName("AUTOFIRE123.doc", "AUTOFIRE123.doc");
+        AssertSanitizeFileName("SUNNY(1).doc", "SUNNY(1).doc");
+        AssertSanitizeFileName("ODD_MONKEY.doc", "ODD_MONKEY.doc");
+        AssertSanitizeFileName("SILLY,MONKEY.docx", "SILLY,MONKEY.docx");
+        AssertSanitizeFileName("CamelCase.bat", "CamelCase.bat");
+        AssertSanitizeFileName("SLICKER-CHIPMUNK.bat", "SLICKER-CHIPMUNK.bat");
+    }
+
+    [TestMethod]
+    public void SanitizeFileNameForInvalidFileNames()
+    {
+        AssertSanitizeFileName("CHIPMUNKWALK", "  CHIPMUNKWALK  ");
+        AssertSanitizeFileName("SLEEPYBOUNCE", "SLEEPYBOUNCE\n");
+        AssertSanitizeFileName("PLOWARK", "PLOWARK\r");
+        AssertSanitizeFileName("JUNIORGLEE", "JUNIORGLEE\t");
+        AssertSanitizeFileName("LATENTROUTE34", "LATENTROUTE?34");
+        AssertSanitizeFileName("TRAWLSOUFFLE", "TRAWLSOUFFLE*");
+        AssertSanitizeFileName("VIOLENTIRON", "><VIOLENTIRON\"|");
+        AssertSanitizeFileName("YELLOWBAGEL", "YELLOWBAGEL://");
+        AssertSanitizeFileName("ZANYWATER", "ZANYWATER$");
+    }
+
+    private static void AssertSanitizeFileName(string expected, string fileName)
+        => Assert.AreEqual(expected, fileName.SanitizeFileName());
+}