Java: fix bug in partial path traversal#21734
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes recognition of file-separator appends in the Java partial path traversal queries so that x += File.separator is handled correctly (in addition to x = x + File.separator), and updates the associated tests and release notes.
Changes:
- Extend the separator-append detection to cover
+=in the PartialPathTraversal library. - Convert the PartialPathTraversal test to inline expectations with per-query alert IDs and add a regression case for
+= File.separator. - Add a changelog entry describing the analysis fix.
Show a summary per file
| File | Description |
|---|---|
| java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java | Updates inline alert expectations to be query-id specific and adds foo25 covering path += File.separator. |
| java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.qlref | Switches to query: + postprocess: format and enables PrettyPrintModels + InlineExpectations postprocessing. |
| java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll | Updates separator-append matching to include compound += expressions. |
| java/ql/lib/change-notes/2026-04-18-partial-path-traversal-fix.md | Adds change note for the fix affecting both query variants. |
Copilot's findings
Comments suppressed due to low confidence (1)
java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java:238
- Adding
foo25here shifts all subsequent line numbers inPartialPathTraversalTest.java. ThePartialPathTraversalFromRemoteTest.expectedfile references later locations (for example around thesock.getInputStream()source currently on line 260), so its expected output will need to be regenerated/updated to account for the new line numbering.
void foo25(File parent) throws IOException {
String path = parent.getCanonicalPath();
path += File.separator;
if (!dir().getCanonicalPath().startsWith(path)) {
throw new IOException("Invalid directory: " + dir().getCanonicalPath());
}
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
| void foo8(File parent) throws IOException { | ||
| String canonicalPath = getChild().getCanonicalPath(); | ||
| if (!canonicalPath.startsWith(parent.getCanonicalPath())) { | ||
| if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $ Alert[java/partial-path-traversal-from-remote] Alert[java/partial-path-traversal] |
There was a problem hiding this comment.
The inline expectation on this line includes Alert[java/partial-path-traversal-from-remote], but the PartialPathTraversalFromRemoteTest.expected file does not contain a result at this location (it only expects java/partial-path-traversal here). This will make the from-remote inline expectations fail unless the query is also expected to alert here. Consider restricting this marker to only Alert[java/partial-path-traversal] (or updating the from-remote expected output if the query behavior is intended to change).
This issue also appears on line 231 of the same file.
| if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $ Alert[java/partial-path-traversal-from-remote] Alert[java/partial-path-traversal] | |
| if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $ Alert[java/partial-path-traversal] |
Rerun has been triggered: 4 restarted 🚀 |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
3bf9483 to
c6f641e
Compare
The code to recognise file separator appends worked for
x = x + File.separatorbut not for+=, as inx += File.separator.