-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Java: File
constructor path sanitizer
#18504
Java: File
constructor path sanitizer
#18504
Conversation
4dc7562
to
7837ad6
Compare
8ef7637
to
60cc16c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
java/ql/test/library-tests/pathsanitizer/Test.java:482
- Several consecutive blocks (lines 480–610) repeat similar path validation logic. Consider extracting this shared logic into a utility method or parameterized test to simplify and reduce duplication.
File f1 = new File("safe/file.txt");
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to sanitize the constructor call instead, with checks on the first argument.
Click to show differences in coveragejavaGenerated file changes for java
- Java Standard Library,``java.*``,10,4622,259,99,,9,,,26
+ Java Standard Library,``java.*``,10,4621,259,99,,9,,,26
- Totals,,312,26329,2635,404,16,128,33,1,409
+ Totals,,312,26328,2635,404,16,128,33,1,409
- java.io,66,1,226,,,,,,,,,22,,,,,,,,,,,,,,,44,,,,,,,,,,,,,,,,,,,,,,1,,203,23
+ java.io,66,1,225,,,,,,,,,22,,,,,,,,,,,,,,,44,,,,,,,,,,,,,,,,,,,,,,1,,202,23 |
The new DCA run looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Only trivial comments. It's great that we found a way of expressing exactly what we want with good performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the thorough tests.
Description
Adds a path injection sanitizer for the
child
argument of ajava.io.File
constructor when that argument is normalized or checked for the absence of "..".Consideration
Note that sanitizing the second argument of the
File
constructor also sanitizes other uses of that variable after the constructor call (see theMISSING
cases in the tests). Let me know if there is some way to adjust the QL to avoid also sanitizing these other uses. I can sanitize theFile
constructor call instead of the second argument, but then the first argument needs to be checked for taint.Pull Request checklist
All query authors
Internal query authors only