From 49d37c517d2221a05e047524e8f56d088011476d Mon Sep 17 00:00:00 2001
From: Jami Cogswell <jcogs33@Jamis-MacBook-Pro.local>
Date: Mon, 17 Mar 2025 16:02:13 -0400
Subject: [PATCH] Java: fix replacement char check and add tests

---
 .../code/java/security/PathSanitizer.qll      |  4 ++-
 .../library-tests/pathsanitizer/Test.java     | 28 +++++++++++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
index 35a22662725..a120cb4489f 100644
--- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
+++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
@@ -419,7 +419,8 @@ private predicate replacesDirectoryCharactersWithSingleReplaceAll(
 ) {
   exists(CompileTimeConstantExpr target, string targetValue |
     isReplaceAllTarget(replaceAllCall, target) and
-    target.getStringValue() = targetValue
+    target.getStringValue() = targetValue and
+    replaceAllCall.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar()
   |
     not targetValue.matches("%[^%]%") and
     targetValue.matches("[%.%]") and
@@ -460,6 +461,7 @@ private predicate replacesDirectoryCharactersWithDoubleReplaceOrReplaceAll(
     rc2.getQualifier() = rc1 and
     target1.getStringValue() = targetValue1 and
     target2.getStringValue() = targetValue2 and
+    rc1.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
     rc2.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
     // make sure the calls replace different characters
     targetValue2 != targetValue1 and
diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java
index 4fc4a80119e..db00508c56a 100644
--- a/java/ql/test/library-tests/pathsanitizer/Test.java
+++ b/java/ql/test/library-tests/pathsanitizer/Test.java
@@ -716,14 +716,20 @@ public class Test {
         }
         {
             String source = (String) source();
-            source = source.replaceAll("\\.|[/\\\\]", "");
+            source = source.replaceAll("\\.|[/\\\\]", "-");
             sink(source); // Safe
         }
         {
             String source = (String) source();
-            source = source.replaceAll("[.][.]|[/\\\\]", "");
+            source = source.replaceAll("[.][.]|[/\\\\]", "_");
             sink(source); // Safe
         }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character
+            source = source.replaceAll("[.][.]|[/\\\\]", "/");
+            sink(source); // $ hasTaintFlow
+        }
         {
             String source = (String) source();
             source = source.replaceAll(".|[/\\\\]", "");
@@ -761,6 +767,24 @@ public class Test {
             source = source.replaceAll("\\.", "").replaceAll("/", "");
             sink(source); // Safe
         }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character in each call
+            source = source.replaceAll("\\.", "/").replaceAll("/", ".");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character in first call
+            source = source.replaceAll("\\.", "/").replaceAll("/", "-");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character in second call
+            source = source.replaceAll("\\.", "_").replaceAll("/", ".");
+            sink(source); // $ hasTaintFlow
+        }
         {
             String source = (String) source();
             source = source.replaceAll("\\.", "").replaceAll("/", "").replaceAll("\\\\", "");