From 44e6691e6dd29771b167e298b3b36579f79418d7 Mon Sep 17 00:00:00 2001
From: Carl Dybdahl <carldybdahl@microsoft.com>
Date: Tue, 25 Feb 2025 10:34:32 +0100
Subject: [PATCH 1/7] Add implementation and tests

---
 csharp/ql/src/Bad Practices/PathCombine.qhelp    | 16 ++++++++++++++++
 csharp/ql/src/Bad Practices/PathCombine.ql       |  7 +++++++
 .../Bad Practices/Path Combine/PathCombine.cs    | 14 ++++++++++++++
 .../Path Combine/PathCombine.expected            |  1 +
 .../Bad Practices/Path Combine/PathCombine.qlref |  1 +
 .../Bad Practices/Path Combine/options           |  2 ++
 6 files changed, 41 insertions(+)
 create mode 100644 csharp/ql/src/Bad Practices/PathCombine.qhelp
 create mode 100644 csharp/ql/src/Bad Practices/PathCombine.ql
 create mode 100644 csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs
 create mode 100644 csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected
 create mode 100644 csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.qlref
 create mode 100644 csharp/ql/test/query-tests/Bad Practices/Path Combine/options

diff --git a/csharp/ql/src/Bad Practices/PathCombine.qhelp b/csharp/ql/src/Bad Practices/PathCombine.qhelp
new file mode 100644
index 000000000000..2191ffc46761
--- /dev/null
+++ b/csharp/ql/src/Bad Practices/PathCombine.qhelp	
@@ -0,0 +1,16 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p><code>Path.Combine</code> may silently drop its earlier arguments if its later arguments are absolute paths. E.g. <code>Path.Combine("C:\\Users\\Me\\Documents", "C:\\Program Files\\") == "C:\\Program Files"</code>.</p>
+
+</overview>
+<recommendation>
+<p>Use <code>Path.Join</code> instead.</p>
+</recommendation>
+<references>
+
+
+</references>
+</qhelp>
diff --git a/csharp/ql/src/Bad Practices/PathCombine.ql b/csharp/ql/src/Bad Practices/PathCombine.ql
new file mode 100644
index 000000000000..dae7a7954846
--- /dev/null
+++ b/csharp/ql/src/Bad Practices/PathCombine.ql	
@@ -0,0 +1,7 @@
+
+import csharp
+import semmle.code.csharp.frameworks.System
+
+from MethodCall call
+where call.getTarget().hasFullyQualifiedName("System.IO", "Path", "Combine")
+select call, "Path.Combine may silently discard its initial arguments if the latter are absolute paths. Use Path.Join to consistently join them."
\ No newline at end of file
diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs
new file mode 100644
index 000000000000..5470837c9851
--- /dev/null
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs	
@@ -0,0 +1,14 @@
+using System.IO;
+
+class EmptyCatchBlock
+{
+    void bad()
+    {
+        Path.Combine(@"C:\Users", @"C:\Program Files");
+    }
+
+    void good()
+    {
+        Path.Join(@"C:\Users", @"C:\Program Files");
+    }
+}
diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected
new file mode 100644
index 000000000000..fb5a6149d507
--- /dev/null
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected	
@@ -0,0 +1 @@
+| PathCombine.cs:7:9:7:55 | catch (...) {...} | Path.Combine may silently discard its initial arguments if the latter are absolute paths. Use Path.Join to consistently join them. |
diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.qlref b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.qlref
new file mode 100644
index 000000000000..eaf41d047402
--- /dev/null
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.qlref	
@@ -0,0 +1 @@
+Bad Practices/PathCombine.ql
\ No newline at end of file
diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/options b/csharp/ql/test/query-tests/Bad Practices/Path Combine/options
new file mode 100644
index 000000000000..75c39b4541ba
--- /dev/null
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/options	
@@ -0,0 +1,2 @@
+semmle-extractor-options: /nostdlib /noconfig
+semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj

From 35fd4d226f5f3fbb287850dbe1c976ca521c6efb Mon Sep 17 00:00:00 2001
From: Carl Dybdahl <carldybdahl@microsoft.com>
Date: Wed, 26 Feb 2025 10:22:12 +0100
Subject: [PATCH 2/7] Oops

---
 .../test/query-tests/Bad Practices/Path Combine/PathCombine.cs  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs
index 5470837c9851..bf9b19c4a5c7 100644
--- a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs	
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs	
@@ -1,6 +1,6 @@
 using System.IO;
 
-class EmptyCatchBlock
+class PathCombine
 {
     void bad()
     {

From aa6779f19fc69d384951102ff55055af8c47f8b8 Mon Sep 17 00:00:00 2001
From: Carl Dybdahl <carldybdahl@microsoft.com>
Date: Wed, 26 Feb 2025 10:51:46 +0100
Subject: [PATCH 3/7] Add changelog

---
 csharp/ql/src/change-notes/2025-02-26-path-combine.md | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 csharp/ql/src/change-notes/2025-02-26-path-combine.md

diff --git a/csharp/ql/src/change-notes/2025-02-26-path-combine.md b/csharp/ql/src/change-notes/2025-02-26-path-combine.md
new file mode 100644
index 000000000000..81610502b229
--- /dev/null
+++ b/csharp/ql/src/change-notes/2025-02-26-path-combine.md
@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `csharp/path-combine`, to recommend against the `Path.Combine` method due to it silently discarding its earlier parameters if later parameters are rooted.
\ No newline at end of file

From d82295c54af868e90b76c6bfebdb1ca9fdb5a1eb Mon Sep 17 00:00:00 2001
From: Carl Dybdahl <carldybdahl@microsoft.com>
Date: Wed, 26 Feb 2025 13:51:13 +0100
Subject: [PATCH 4/7] Add QLDoc

---
 csharp/ql/src/Bad Practices/PathCombine.ql | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/csharp/ql/src/Bad Practices/PathCombine.ql b/csharp/ql/src/Bad Practices/PathCombine.ql
index dae7a7954846..7de1ad271e39 100644
--- a/csharp/ql/src/Bad Practices/PathCombine.ql	
+++ b/csharp/ql/src/Bad Practices/PathCombine.ql	
@@ -1,3 +1,13 @@
+/**
+ * @name Call to System.IO.Path.Combine
+ * @description Finds calls to System.IO.Path's Combine method
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision very-high
+ * @id cs/path-combine
+ * @tags reliability
+ *       readability
+ */
 
 import csharp
 import semmle.code.csharp.frameworks.System

From d371723fe4b944bb92f5a6ff2ac9802ef3bc3d4e Mon Sep 17 00:00:00 2001
From: Carl Dybdahl <carldybdahl@microsoft.com>
Date: Wed, 26 Feb 2025 13:56:13 +0100
Subject: [PATCH 5/7] Fix test

---
 .../query-tests/Bad Practices/Path Combine/PathCombine.expected | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected
index fb5a6149d507..07ae158d60c1 100644
--- a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected	
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected	
@@ -1 +1 @@
-| PathCombine.cs:7:9:7:55 | catch (...) {...} | Path.Combine may silently discard its initial arguments if the latter are absolute paths. Use Path.Join to consistently join them. |
+| PathCombine.cs:7:9:7:54 | call to method Combine | Path.Combine may silently discard its initial arguments if the latter are absolute paths. Use Path.Join to consistently join them. |

From b3447959fa866625c4e6c95f67fcc6635e71597d Mon Sep 17 00:00:00 2001
From: Carl Dybdahl <carldybdahl@microsoft.com>
Date: Wed, 26 Feb 2025 14:27:36 +0100
Subject: [PATCH 6/7] Match autoformatting, add QLDoc references

---
 csharp/ql/src/Bad Practices/PathCombine.qhelp                   | 2 ++
 csharp/ql/src/Bad Practices/PathCombine.ql                      | 2 +-
 .../query-tests/Bad Practices/Path Combine/PathCombine.expected | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/csharp/ql/src/Bad Practices/PathCombine.qhelp b/csharp/ql/src/Bad Practices/PathCombine.qhelp
index 2191ffc46761..2562a615c87d 100644
--- a/csharp/ql/src/Bad Practices/PathCombine.qhelp	
+++ b/csharp/ql/src/Bad Practices/PathCombine.qhelp	
@@ -11,6 +11,8 @@
 </recommendation>
 <references>
 
+  <li>Microsoft Learn, .NET API browser, <a href="https://learn.microsoft.com/en-us/dotnet/api/system.io.path.combine?view=net-9.0">Path.Combine</a>.</li>
+  <li>Microsoft Learn, .NET API browser, <a href="https://learn.microsoft.com/en-us/dotnet/api/system.io.path.join?view=net-9.0">Path.Join</a>.</li>
 
 </references>
 </qhelp>
diff --git a/csharp/ql/src/Bad Practices/PathCombine.ql b/csharp/ql/src/Bad Practices/PathCombine.ql
index 7de1ad271e39..7363999727fd 100644
--- a/csharp/ql/src/Bad Practices/PathCombine.ql	
+++ b/csharp/ql/src/Bad Practices/PathCombine.ql	
@@ -14,4 +14,4 @@ import semmle.code.csharp.frameworks.System
 
 from MethodCall call
 where call.getTarget().hasFullyQualifiedName("System.IO", "Path", "Combine")
-select call, "Path.Combine may silently discard its initial arguments if the latter are absolute paths. Use Path.Join to consistently join them."
\ No newline at end of file
+select call, "Call to System.IO.Path.Combine."
diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected
index 07ae158d60c1..ae6b252e8733 100644
--- a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected	
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected	
@@ -1 +1 @@
-| PathCombine.cs:7:9:7:54 | call to method Combine | Path.Combine may silently discard its initial arguments if the latter are absolute paths. Use Path.Join to consistently join them. |
+| PathCombine.cs:7:9:7:54 | call to method Combine | Call to System.IO.Path.Combine. |

From 2f7cdf1bfa927d7df33e75f28957996a9fbd5a31 Mon Sep 17 00:00:00 2001
From: Carl Dybdahl <carldybdahl@microsoft.com>
Date: Fri, 28 Feb 2025 12:45:27 +0100
Subject: [PATCH 7/7] Improvements

---
 csharp/ql/src/Bad Practices/PathCombine.ql                     | 3 +--
 .../Bad Practices/Path Combine/PathCombine.expected            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/csharp/ql/src/Bad Practices/PathCombine.ql b/csharp/ql/src/Bad Practices/PathCombine.ql
index 7363999727fd..aa841486bdff 100644
--- a/csharp/ql/src/Bad Practices/PathCombine.ql	
+++ b/csharp/ql/src/Bad Practices/PathCombine.ql	
@@ -6,7 +6,6 @@
  * @precision very-high
  * @id cs/path-combine
  * @tags reliability
- *       readability
  */
 
 import csharp
@@ -14,4 +13,4 @@ import semmle.code.csharp.frameworks.System
 
 from MethodCall call
 where call.getTarget().hasFullyQualifiedName("System.IO", "Path", "Combine")
-select call, "Call to System.IO.Path.Combine."
+select call, "Call to 'System.IO.Path.Combine'."
diff --git a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected
index ae6b252e8733..c0f9e405516b 100644
--- a/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected	
+++ b/csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.expected	
@@ -1 +1 @@
-| PathCombine.cs:7:9:7:54 | call to method Combine | Call to System.IO.Path.Combine. |
+| PathCombine.cs:7:9:7:54 | call to method Combine | Call to 'System.IO.Path.Combine'. |