Skip to content

Commit 1bf0033

Browse files
CopilotMalcolmnixon
Copilot
and
Malcolmnixon
authored
Fix Path.Combine security warnings and MSTest assertion code smells (#3)
* Initial plan * Add PathHelpers with SafePathCombine and update code to use it Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Update test assertions to use Assert.Contains and Assert.DoesNotContain Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Improve PathHelpers validation with GetFullPath check Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
1 parent ebd1139 commit 1bf0033

File tree

7 files changed

+194
-28
lines changed

7 files changed

+194
-28
lines changed

‎.github/codeql-config.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,8 @@ query-filters:
1111
paths:
1212
- src/DemaConsulting.TemplateDotNetTool/Context.cs
1313
- src/DemaConsulting.TemplateDotNetTool/Validation.cs
14+
# Exclude path-combine warnings in PathHelpers (validated safe usage)
15+
- exclude:
16+
id: cs/path-combine
17+
paths:
18+
- src/DemaConsulting.TemplateDotNetTool/PathHelpers.cs

‎src/DemaConsulting.TemplateDotNetTool/PathHelpers.cs‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// Copyright (c) DEMA Consulting
2+
//
3+
// Permission is hereby granted, free of charge, to any person obtaining a copy
4+
// of this software and associated documentation files (the "Software"), to deal
5+
// in the Software without restriction, including without limitation the rights
6+
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
7+
// copies of the Software, and to permit persons to whom the Software is
8+
// furnished to do so, subject to the following conditions:
9+
//
10+
// The above copyright notice and this permission notice shall be included in all
11+
// copies or substantial portions of the Software.
12+
//
13+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
14+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
15+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
16+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
17+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
18+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
19+
// SOFTWARE.
20+
21+
namespace DemaConsulting.TemplateDotNetTool;
22+
23+
/// <summary>
24+
/// Helper utilities for safe path operations.
25+
/// </summary>
26+
internal static class PathHelpers
27+
{
28+
/// <summary>
29+
/// Safely combines two paths, ensuring the second path doesn't contain path traversal sequences.
30+
/// </summary>
31+
/// <param name="basePath">The base path.</param>
32+
/// <param name="relativePath">The relative path to combine.</param>
33+
/// <returns>The combined path.</returns>
34+
/// <exception cref="ArgumentException">Thrown when relativePath contains invalid characters or path traversal sequences.</exception>
35+
internal static string SafePathCombine(string basePath, string relativePath)
36+
{
37+
// Ensure the relative path doesn't contain path traversal sequences
38+
if (relativePath.Contains("..") || Path.IsPathRooted(relativePath))
39+
{
40+
throw new ArgumentException($"Invalid path component: {relativePath}", nameof(relativePath));
41+
}
42+
43+
// This call to Path.Combine is safe because we've validated that:
44+
// 1. relativePath doesn't contain ".." (path traversal)
45+
// 2. relativePath is not an absolute path (IsPathRooted check)
46+
// This ensures the combined path will always be under basePath
47+
var combinedPath = Path.Combine(basePath, relativePath);
48+
49+
// Additional validation: ensure the combined path is still under the base path
50+
var fullBasePath = Path.GetFullPath(basePath);
51+
var fullCombinedPath = Path.GetFullPath(combinedPath);
52+
53+
if (!fullCombinedPath.StartsWith(fullBasePath, StringComparison.OrdinalIgnoreCase))
54+
{
55+
throw new ArgumentException($"Invalid path component: {relativePath}", nameof(relativePath));
56+
}
57+
58+
return combinedPath;
59+
}
60+
}

‎src/DemaConsulting.TemplateDotNetTool/Validation.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ private static void RunVersionTest(Context context, DemaConsulting.TestResults.T
103103
try
104104
{
105105
using var tempDir = new TemporaryDirectory();
106-
var logFile = Path.Combine(tempDir.DirectoryPath, "version-test.log");
106+
var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "version-test.log");
107107

108108
// Build command line arguments
109109
var args = new List<string>
@@ -171,7 +171,7 @@ private static void RunHelpTest(Context context, DemaConsulting.TestResults.Test
171171
try
172172
{
173173
using var tempDir = new TemporaryDirectory();
174-
var logFile = Path.Combine(tempDir.DirectoryPath, "help-test.log");
174+
var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "help-test.log");
175175

176176
// Build command line arguments
177177
var args = new List<string>
@@ -329,7 +329,7 @@ private sealed class TemporaryDirectory : IDisposable
329329
/// </summary>
330330
public TemporaryDirectory()
331331
{
332-
DirectoryPath = Path.Combine(Path.GetTempPath(), $"templatetool_validation_{Guid.NewGuid()}");
332+
DirectoryPath = PathHelpers.SafePathCombine(Path.GetTempPath(), $"templatetool_validation_{Guid.NewGuid()}");
333333

334334
try
335335
{

‎test/DemaConsulting.TemplateDotNetTool.Tests/ContextTests.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ public void Context_Create_LogFlag_OpensLogFile()
160160
// Verify log file was written
161161
Assert.IsTrue(File.Exists(logFile));
162162
var logContent = File.ReadAllText(logFile);
163-
Assert.IsTrue(logContent.Contains("Test message"));
163+
Assert.Contains("Test message", logContent);
164164
}
165165
finally
166166
{
@@ -178,7 +178,7 @@ public void Context_Create_LogFlag_OpensLogFile()
178178
public void Context_Create_UnknownArgument_ThrowsArgumentException()
179179
{
180180
var exception = Assert.Throws<ArgumentException>(() => Context.Create(["--unknown"]));
181-
Assert.IsTrue(exception.Message.Contains("Unsupported argument"));
181+
Assert.Contains("Unsupported argument", exception.Message);
182182
}
183183

184184
/// <summary>
@@ -197,7 +197,7 @@ public void Context_WriteLine_NotSilent_WritesToConsole()
197197
context.WriteLine("Test message");
198198

199199
var output = outWriter.ToString();
200-
Assert.IsTrue(output.Contains("Test message"));
200+
Assert.Contains("Test message", output);
201201
}
202202
finally
203203
{
@@ -221,7 +221,7 @@ public void Context_WriteLine_Silent_DoesNotWriteToConsole()
221221
context.WriteLine("Test message");
222222

223223
var output = outWriter.ToString();
224-
Assert.IsFalse(output.Contains("Test message"));
224+
Assert.DoesNotContain("Test message", output);
225225
}
226226
finally
227227
{

‎test/DemaConsulting.TemplateDotNetTool.Tests/IntegrationTests.cs‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public void TestInitialize()
3737
// The DLL should be in the same directory as the test assembly
3838
// because the test project references the main project
3939
var baseDir = AppContext.BaseDirectory;
40-
_dllPath = Path.Combine(baseDir, "DemaConsulting.TemplateDotNetTool.dll");
40+
_dllPath = PathHelpers.SafePathCombine(baseDir, "DemaConsulting.TemplateDotNetTool.dll");
4141

4242
Assert.IsTrue(File.Exists(_dllPath), $"Could not find Template DotNet Tool DLL at {_dllPath}");
4343
}
@@ -60,8 +60,8 @@ public void IntegrationTest_VersionFlag_OutputsVersion()
6060

6161
// Verify version is output
6262
Assert.IsFalse(string.IsNullOrWhiteSpace(output));
63-
Assert.IsFalse(output.Contains("Error"));
64-
Assert.IsFalse(output.Contains("Copyright"));
63+
Assert.DoesNotContain("Error", output);
64+
Assert.DoesNotContain("Copyright", output);
6565
}
6666

6767
/// <summary>
@@ -81,9 +81,9 @@ public void IntegrationTest_HelpFlag_OutputsUsageInformation()
8181
Assert.AreEqual(0, exitCode);
8282

8383
// Verify usage information is output
84-
Assert.IsTrue(output.Contains("Usage:"));
85-
Assert.IsTrue(output.Contains("Options:"));
86-
Assert.IsTrue(output.Contains("--version"));
84+
Assert.Contains("Usage:", output);
85+
Assert.Contains("Options:", output);
86+
Assert.Contains("--version", output);
8787
}
8888

8989
/// <summary>
@@ -103,8 +103,8 @@ public void IntegrationTest_ValidateFlag_RunsValidation()
103103
Assert.AreEqual(0, exitCode);
104104

105105
// Verify validation output
106-
Assert.IsTrue(output.Contains("Total Tests:"));
107-
Assert.IsTrue(output.Contains("Passed:"));
106+
Assert.Contains("Total Tests:", output);
107+
Assert.Contains("Passed:", output);
108108
}
109109

110110
/// <summary>
@@ -135,8 +135,8 @@ public void IntegrationTest_ValidateWithResults_GeneratesTrxFile()
135135

136136
// Verify TRX file contains expected content
137137
var trxContent = File.ReadAllText(resultsFile);
138-
Assert.IsTrue(trxContent.Contains("<TestRun"));
139-
Assert.IsTrue(trxContent.Contains("</TestRun>"));
138+
Assert.Contains("<TestRun", trxContent);
139+
Assert.Contains("</TestRun>", trxContent);
140140
}
141141
finally
142142
{
@@ -192,7 +192,7 @@ public void IntegrationTest_LogFlag_WritesOutputToFile()
192192

193193
// Verify log file contains output
194194
var logContent = File.ReadAllText(logFile);
195-
Assert.IsTrue(logContent.Contains("Template DotNet Tool version"));
195+
Assert.Contains("Template DotNet Tool version", logContent);
196196
}
197197
finally
198198
{
@@ -218,6 +218,6 @@ public void IntegrationTest_UnknownArgument_ReturnsError()
218218

219219
// Verify error
220220
Assert.AreNotEqual(0, exitCode);
221-
Assert.IsTrue(output.Contains("Error"));
221+
Assert.Contains("Error", output);
222222
}
223223
}

‎test/DemaConsulting.TemplateDotNetTool.Tests/PathHelpersTests.cs‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
// Copyright (c) DEMA Consulting
2+
//
3+
// Permission is hereby granted, free of charge, to any person obtaining a copy
4+
// of this software and associated documentation files (the "Software"), to deal
5+
// in the Software without restriction, including without limitation the rights
6+
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
7+
// copies of the Software, and to permit persons to whom the Software is
8+
// furnished to do so, subject to the following conditions:
9+
//
10+
// The above copyright notice and this permission notice shall be included in all
11+
// copies or substantial portions of the Software.
12+
//
13+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
14+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
15+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
16+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
17+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
18+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
19+
// SOFTWARE.
20+
21+
namespace DemaConsulting.TemplateDotNetTool.Tests;
22+
23+
/// <summary>
24+
/// Tests for the PathHelpers class.
25+
/// </summary>
26+
[TestClass]
27+
public class PathHelpersTests
28+
{
29+
/// <summary>
30+
/// Test that SafePathCombine correctly combines valid paths.
31+
/// </summary>
32+
[TestMethod]
33+
public void PathHelpers_SafePathCombine_ValidPaths_CombinesCorrectly()
34+
{
35+
// Arrange
36+
var basePath = "/home/user/project";
37+
var relativePath = "subfolder/file.txt";
38+
39+
// Act
40+
var result = PathHelpers.SafePathCombine(basePath, relativePath);
41+
42+
// Assert
43+
Assert.AreEqual(Path.Combine(basePath, relativePath), result);
44+
}
45+
46+
/// <summary>
47+
/// Test that SafePathCombine throws ArgumentException for path traversal with double dots.
48+
/// </summary>
49+
[TestMethod]
50+
public void PathHelpers_SafePathCombine_PathTraversalWithDoubleDots_ThrowsArgumentException()
51+
{
52+
// Arrange
53+
var basePath = "/home/user/project";
54+
var relativePath = "../etc/passwd";
55+
56+
// Act & Assert
57+
var exception = Assert.Throws<ArgumentException>(() =>
58+
PathHelpers.SafePathCombine(basePath, relativePath));
59+
Assert.Contains("Invalid path component", exception.Message);
60+
}
61+
62+
/// <summary>
63+
/// Test that SafePathCombine throws ArgumentException for path with double dots in middle.
64+
/// </summary>
65+
[TestMethod]
66+
public void PathHelpers_SafePathCombine_DoubleDotsInMiddle_ThrowsArgumentException()
67+
{
68+
// Arrange
69+
var basePath = "/home/user/project";
70+
var relativePath = "subfolder/../../../etc/passwd";
71+
72+
// Act & Assert
73+
var exception = Assert.Throws<ArgumentException>(() =>
74+
PathHelpers.SafePathCombine(basePath, relativePath));
75+
Assert.Contains("Invalid path component", exception.Message);
76+
}
77+
78+
/// <summary>
79+
/// Test that SafePathCombine throws ArgumentException for absolute paths.
80+
/// </summary>
81+
[TestMethod]
82+
public void PathHelpers_SafePathCombine_AbsolutePath_ThrowsArgumentException()
83+
{
84+
// Test Unix absolute path
85+
var unixBasePath = "/home/user/project";
86+
var unixRelativePath = "/etc/passwd";
87+
var unixException = Assert.Throws<ArgumentException>(() =>
88+
PathHelpers.SafePathCombine(unixBasePath, unixRelativePath));
89+
Assert.Contains("Invalid path component", unixException.Message);
90+
91+
// Test Windows absolute path (only on Windows since Windows paths may not be rooted on Unix)
92+
if (OperatingSystem.IsWindows())
93+
{
94+
var windowsBasePath = "C:\\Users\\project";
95+
var windowsRelativePath = "C:\\Windows\\System32\\file.txt";
96+
var windowsException = Assert.Throws<ArgumentException>(() =>
97+
PathHelpers.SafePathCombine(windowsBasePath, windowsRelativePath));
98+
Assert.Contains("Invalid path component", windowsException.Message);
99+
}
100+
}
101+
}

‎test/DemaConsulting.TemplateDotNetTool.Tests/ProgramTests.cs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ public void Program_Run_WithVersionFlag_DisplaysVersionOnly()
4242
Program.Run(context);
4343

4444
var output = outWriter.ToString();
45-
Assert.IsFalse(output.Contains("Copyright"));
46-
Assert.IsFalse(output.Contains("Template DotNet Tool version"));
45+
Assert.DoesNotContain("Copyright", output);
46+
Assert.DoesNotContain("Template DotNet Tool version", output);
4747
}
4848
finally
4949
{
@@ -67,10 +67,10 @@ public void Program_Run_WithHelpFlag_DisplaysUsageInformation()
6767
Program.Run(context);
6868

6969
var output = outWriter.ToString();
70-
Assert.IsTrue(output.Contains("Usage:"));
71-
Assert.IsTrue(output.Contains("Options:"));
72-
Assert.IsTrue(output.Contains("--version"));
73-
Assert.IsTrue(output.Contains("--help"));
70+
Assert.Contains("Usage:", output);
71+
Assert.Contains("Options:", output);
72+
Assert.Contains("--version", output);
73+
Assert.Contains("--help", output);
7474
}
7575
finally
7676
{
@@ -94,7 +94,7 @@ public void Program_Run_WithValidateFlag_RunsValidation()
9494
Program.Run(context);
9595

9696
var output = outWriter.ToString();
97-
Assert.IsTrue(output.Contains("Total Tests:"));
97+
Assert.Contains("Total Tests:", output);
9898
}
9999
finally
100100
{
@@ -118,8 +118,8 @@ public void Program_Run_NoArguments_DisplaysDefaultBehavior()
118118
Program.Run(context);
119119

120120
var output = outWriter.ToString();
121-
Assert.IsTrue(output.Contains("Template DotNet Tool version"));
122-
Assert.IsTrue(output.Contains("Copyright"));
121+
Assert.Contains("Template DotNet Tool version", output);
122+
Assert.Contains("Copyright", output);
123123
}
124124
finally
125125
{

0 commit comments

Comments
 (0)