diff --git a/src/DemaConsulting.SarifMark/PathHelpers.cs b/src/DemaConsulting.SarifMark/PathHelpers.cs index cdbb99c..5464c78 100644 --- a/src/DemaConsulting.SarifMark/PathHelpers.cs +++ b/src/DemaConsulting.SarifMark/PathHelpers.cs @@ -34,6 +34,10 @@ internal static class PathHelpers /// Thrown when relativePath contains invalid characters or path traversal sequences. internal static string SafePathCombine(string basePath, string relativePath) { + // Validate inputs + ArgumentNullException.ThrowIfNull(basePath); + ArgumentNullException.ThrowIfNull(relativePath); + // Ensure the relative path doesn't contain path traversal sequences if (relativePath.Contains("..") || Path.IsPathRooted(relativePath)) { @@ -44,6 +48,21 @@ internal static string SafePathCombine(string basePath, string relativePath) // 1. relativePath doesn't contain ".." (path traversal) // 2. relativePath is not an absolute path (IsPathRooted check) // This ensures the combined path will always be under basePath - return Path.Combine(basePath, relativePath); + var combinedPath = Path.Combine(basePath, relativePath); + + // Additional security validation: ensure the combined path is still under the base path. + // This defense-in-depth approach protects against edge cases that might bypass the + // initial validation, ensuring the final path stays within the intended directory. + var fullBasePath = Path.GetFullPath(basePath); + var fullCombinedPath = Path.GetFullPath(combinedPath); + + // Use GetRelativePath to verify the relationship between paths + var relativeCheck = Path.GetRelativePath(fullBasePath, fullCombinedPath); + if (relativeCheck.StartsWith("..") || Path.IsPathRooted(relativeCheck)) + { + throw new ArgumentException($"Invalid path component: {relativePath}", nameof(relativePath)); + } + + return combinedPath; } } diff --git a/test/DemaConsulting.SarifMark.Tests/PathHelpersTests.cs b/test/DemaConsulting.SarifMark.Tests/PathHelpersTests.cs index 628c305..4c56716 100644 --- a/test/DemaConsulting.SarifMark.Tests/PathHelpersTests.cs +++ b/test/DemaConsulting.SarifMark.Tests/PathHelpersTests.cs @@ -26,6 +26,30 @@ namespace DemaConsulting.SarifMark.Tests; [TestClass] public class PathHelpersTests { + /// + /// Test that SafePathCombine throws ArgumentNullException for null base path. + /// + [TestMethod] + public void PathHelpers_SafePathCombine_NullBasePath_ThrowsArgumentNullException() + { + // Act & Assert + var exception = Assert.Throws(() => + PathHelpers.SafePathCombine(null!, "file.txt")); + Assert.AreEqual("basePath", exception.ParamName); + } + + /// + /// Test that SafePathCombine throws ArgumentNullException for null relative path. + /// + [TestMethod] + public void PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullException() + { + // Act & Assert + var exception = Assert.Throws(() => + PathHelpers.SafePathCombine("/home/user", null!)); + Assert.AreEqual("relativePath", exception.ParamName); + } + /// /// Test that SafePathCombine successfully combines valid paths. ///