Fix bug so += File.separator is recognized

owen-mc · owen-mc · commit 6d4a3974ced2 · 2026-04-19T07:18:42.000+01:00
diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll
@@ -40,8 +40,11 @@ private class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, Chara
   CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" }
 }
 
-private class FileSeparatorAppend extends AddExpr {
-  FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr }
+private class FileSeparatorAppend extends BinaryExpr {
+  FileSeparatorAppend() {
+    this.(AddExpr).getRightOperand() instanceof FileSeparatorExpr or
+    this.(AssignAddExpr).getRightOperand() instanceof FileSeparatorExpr
+  }
 }
 
 private predicate isSafe(Expr expr) {
diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected
@@ -14,4 +14,3 @@
 | PartialPathTraversalTest.java:176:14:176:65 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal. |
 | PartialPathTraversalTest.java:194:18:194:87 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal. |
 | PartialPathTraversalTest.java:212:14:212:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal. |
-| PartialPathTraversalTest.java:234:14:234:54 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal. |
diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.expected b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.expected
@@ -14,7 +14,6 @@
 | PartialPathTraversalTest.java:176:14:176:37 | getCanonicalPath(...) | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | PartialPathTraversalTest.java:176:14:176:37 | getCanonicalPath(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal from $@. | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | user-supplied data |
 | PartialPathTraversalTest.java:194:18:194:47 | getCanonicalPath(...) | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | PartialPathTraversalTest.java:194:18:194:47 | getCanonicalPath(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal from $@. | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | user-supplied data |
 | PartialPathTraversalTest.java:212:14:212:26 | canonicalPath | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | PartialPathTraversalTest.java:212:14:212:26 | canonicalPath | Partial Path Traversal Vulnerability due to insufficient guard against path traversal from $@. | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | user-supplied data |
-| PartialPathTraversalTest.java:234:14:234:37 | getCanonicalPath(...) | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | PartialPathTraversalTest.java:234:14:234:37 | getCanonicalPath(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal from $@. | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | user-supplied data |
 edges
 | PartialPathTraversalTest.java:13:14:13:18 | dir(...) : File | PartialPathTraversalTest.java:13:14:13:37 | getCanonicalPath(...) | provenance | MaD:6 |
 | PartialPathTraversalTest.java:20:10:20:14 | dir(...) : File | PartialPathTraversalTest.java:20:10:20:33 | getCanonicalPath(...) | provenance | MaD:6 |
@@ -44,7 +43,6 @@ edges
 | PartialPathTraversalTest.java:194:18:194:28 | encodedFile : File | PartialPathTraversalTest.java:194:18:194:47 | getCanonicalPath(...) | provenance | MaD:6 |
 | PartialPathTraversalTest.java:211:46:211:50 | dir(...) : File | PartialPathTraversalTest.java:211:46:211:69 | getCanonicalPath(...) : String | provenance | MaD:6 |
 | PartialPathTraversalTest.java:211:46:211:69 | getCanonicalPath(...) : String | PartialPathTraversalTest.java:212:14:212:26 | canonicalPath | provenance |  |
-| PartialPathTraversalTest.java:234:14:234:18 | dir(...) : File | PartialPathTraversalTest.java:234:14:234:37 | getCanonicalPath(...) | provenance | MaD:6 |
 | PartialPathTraversalTest.java:260:45:260:117 | new BufferedReader(...) : BufferedReader | PartialPathTraversalTest.java:261:31:261:44 | filenameReader : BufferedReader | provenance |  |
 | PartialPathTraversalTest.java:260:64:260:116 | new InputStreamReader(...) : InputStreamReader | PartialPathTraversalTest.java:260:45:260:117 | new BufferedReader(...) : BufferedReader | provenance | MaD:2 |
 | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | PartialPathTraversalTest.java:260:64:260:116 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:1 MaD:7 |
@@ -64,7 +62,6 @@ edges
 | PartialPathTraversalTest.java:262:20:262:37 | new File(...) : File | PartialPathTraversalTest.java:108:14:108:18 | dir(...) : File | provenance |  |
 | PartialPathTraversalTest.java:262:20:262:37 | new File(...) : File | PartialPathTraversalTest.java:176:14:176:18 | dir(...) : File | provenance |  |
 | PartialPathTraversalTest.java:262:20:262:37 | new File(...) : File | PartialPathTraversalTest.java:211:46:211:50 | dir(...) : File | provenance |  |
-| PartialPathTraversalTest.java:262:20:262:37 | new File(...) : File | PartialPathTraversalTest.java:234:14:234:18 | dir(...) : File | provenance |  |
 | PartialPathTraversalTest.java:262:20:262:37 | new File(...) : File | PartialPathTraversalTest.java:269:16:269:20 | dir(...) : File | provenance |  |
 | PartialPathTraversalTest.java:262:29:262:36 | filename : String | PartialPathTraversalTest.java:262:20:262:37 | new File(...) : File | provenance | MaD:4 |
 | PartialPathTraversalTest.java:269:16:269:20 | dir(...) : File | PartialPathTraversalTest.java:269:16:269:38 | getAbsolutePath(...) : String | provenance | MaD:5 |
@@ -125,8 +122,6 @@ nodes
 | PartialPathTraversalTest.java:211:46:211:50 | dir(...) : File | semmle.label | dir(...) : File |
 | PartialPathTraversalTest.java:211:46:211:69 | getCanonicalPath(...) : String | semmle.label | getCanonicalPath(...) : String |
 | PartialPathTraversalTest.java:212:14:212:26 | canonicalPath | semmle.label | canonicalPath |
-| PartialPathTraversalTest.java:234:14:234:18 | dir(...) : File | semmle.label | dir(...) : File |
-| PartialPathTraversalTest.java:234:14:234:37 | getCanonicalPath(...) | semmle.label | getCanonicalPath(...) |
 | PartialPathTraversalTest.java:260:45:260:117 | new BufferedReader(...) : BufferedReader | semmle.label | new BufferedReader(...) : BufferedReader |
 | PartialPathTraversalTest.java:260:64:260:116 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader |
 | PartialPathTraversalTest.java:260:86:260:106 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java
@@ -231,7 +231,7 @@ void foo24(File parent) throws IOException {
     void foo25(File parent) throws IOException {
         String path = parent.getCanonicalPath();
         path += File.separator;
-        if (!dir().getCanonicalPath().startsWith(path)) { // $ SPURIOUS: Alert[java/partial-path-traversal-from-remote] Alert[java/partial-path-traversal]
+        if (!dir().getCanonicalPath().startsWith(path)) {
             throw new IOException("Invalid directory: " + dir().getCanonicalPath());
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -40,8 +40,11 @@ private class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, Chara`
`40`	`40`	`CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" }`
`41`	`41`	`}`
`42`	`42`
`43`		`-private class FileSeparatorAppend extends AddExpr {`
`44`		`- FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr }`
	`43`	`+private class FileSeparatorAppend extends BinaryExpr {`
	`44`	`+ FileSeparatorAppend() {`
	`45`	`+ this.(AddExpr).getRightOperand() instanceof FileSeparatorExpr or`
	`46`	`+ this.(AssignAddExpr).getRightOperand() instanceof FileSeparatorExpr`
	`47`	`+ }`
`45`	`48`	`}`
`46`	`49`
`47`	`50`	`private predicate isSafe(Expr expr) {`
Original file line number	Diff line number	Diff line change
`@@ -231,7 +231,7 @@ void foo24(File parent) throws IOException {`
`231`	`231`	`void foo25(File parent) throws IOException {`
`232`	`232`	`String path = parent.getCanonicalPath();`
`233`	`233`	`path += File.separator;`
`234`		`- if (!dir().getCanonicalPath().startsWith(path)) { // $ SPURIOUS: Alert[java/partial-path-traversal-from-remote] Alert[java/partial-path-traversal]`
	`234`	`+ if (!dir().getCanonicalPath().startsWith(path)) {`
`235`	`235`	`throw new IOException("Invalid directory: " + dir().getCanonicalPath());`
`236`	`236`	`}`
`237`	`237`	`}`