🔒 [security fix] Fix Path Traversal in Document Provider (Updated)#215
🔒 [security fix] Fix Path Traversal in Document Provider (Updated)#215
Conversation
- Wrapped test logic in try...finally block for robust cleanup of temporary files. - Resubmitted security fix with updated unit tests. Co-authored-by: Asutorufa <16442314+Asutorufa@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @Asutorufa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a critical security enhancement by fixing a path traversal vulnerability in the Document Provider. The core of the solution involves implementing a new, secure path validation mechanism that utilizes canonical paths to prevent unauthorized access or deletion of files outside designated directories. This ensures the application's file system interactions are confined and safe, backed by improved unit test coverage to validate the new logic. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a path traversal vulnerability by introducing a robust isChild method that validates file paths using their canonical forms and integrating it into the DocumentProvider. While the fix correctly prevents unauthorized file access, it introduces minor information leaks where an attacker could probe for the existence of files or their relationships outside the authorized directory. These can be mitigated by reordering checks and validating that all provided document IDs are within the authorized base directory. The addition of the PathTraversalTest.kt unit test provides good coverage for the new validation logic. One improvement is suggested for the test file to make temporary resource management more robust using JUnit's TemporaryFolder rule, preventing potential resource leaks.
| override fun isChildDocument(parentDocumentId: String?, documentId: String?): Boolean { | ||
| if (documentId != null) { | ||
| return parentDocumentId?.let { documentId.startsWith(it) } ?: false | ||
| if (parentDocumentId == null || documentId == null) { | ||
| return false | ||
| } | ||
| return isChild(File(parentDocumentId), File(documentId)) | ||
| } |
There was a problem hiding this comment.
The isChildDocument method verifies the parent-child relationship between any two paths on the filesystem without ensuring that they belong to the provider's authorized directory (baseDir). This allows an attacker to probe the filesystem structure and verify the existence of files and directories outside the provider's scope by checking their relationships. To fix this, ensure that the parentDocumentId is validated to be within baseDir before performing the relationship check.
override fun isChildDocument(parentDocumentId: String?, documentId: String?): Boolean {
if (parentDocumentId == null || documentId == null) {
return false
}
val parent = File(parentDocumentId)
val child = File(documentId)
return isChild(baseDir, parent) && isChild(parent, child)
}| if (!f.exists()) throw FileNotFoundException(f.absolutePath + " not found") | ||
| if (!isChild(baseDir, f)) throw FileNotFoundException("Invalid document ID: $docId") |
There was a problem hiding this comment.
The getFileForDocId method checks for the existence of a file before verifying if it resides within the authorized baseDir. This allows an attacker to distinguish between files that exist outside the authorized directory and those that do not exist at all, by observing the different exception messages returned ("not found" vs "Invalid document ID"). This information leak can be mitigated by swapping the order of the checks.
if (!isChild(baseDir, f)) throw FileNotFoundException("Invalid document ID: $docId")
if (!f.exists()) throw FileNotFoundException(f.absolutePath + " not found")| @Test | ||
| fun testIsChild() { | ||
| val provider = YuhaiinDocumentProvider() | ||
| val tempDir = Files.createTempDirectory("yuhaiin_test").toFile().canonicalFile | ||
| val otherDir = Files.createTempDirectory("yuhaiin_other").toFile().canonicalFile | ||
| val partialMatchDir = File(tempDir.parentFile, tempDir.name + "_extra") | ||
| try { | ||
| val subDir = File(tempDir, "subdir") | ||
| subDir.mkdir() | ||
| val fileInSubDir = File(subDir, "file.txt") | ||
| fileInSubDir.createNewFile() | ||
|
|
||
| val otherFile = File(otherDir, "other.txt") | ||
| otherFile.createNewFile() | ||
|
|
||
| // Construct a path that looks like it's inside tempDir but isn't after canonicalization | ||
| val traversalFile = File(tempDir, "../" + otherDir.name + "/other.txt") | ||
|
|
||
| assertTrue("Should be child of itself", provider.isChild(tempDir, tempDir)) | ||
| assertTrue("Should be child of tempDir", provider.isChild(tempDir, subDir)) | ||
| assertTrue("Should be child of tempDir", provider.isChild(tempDir, fileInSubDir)) | ||
|
|
||
| assertFalse("Should not be child of otherDir", provider.isChild(tempDir, otherDir)) | ||
| assertFalse("Should not be child of otherDir", provider.isChild(tempDir, otherFile)) | ||
| assertFalse("Traversal should be blocked: ${traversalFile.path}", provider.isChild(tempDir, traversalFile)) | ||
|
|
||
| partialMatchDir.mkdir() | ||
| assertFalse("Partial name match should be blocked: ${partialMatchDir.path}", provider.isChild(tempDir, partialMatchDir)) | ||
| } finally { | ||
| tempDir.deleteRecursively() | ||
| otherDir.deleteRecursively() | ||
| partialMatchDir.deleteRecursively() | ||
| } | ||
| } |
There was a problem hiding this comment.
The current test implementation for resource cleanup has a potential resource leak. If the creation of otherDir at line 15 fails, the tempDir created at line 14 will not be cleaned up. A more robust and idiomatic way to handle temporary files in JUnit 4 is to use the TemporaryFolder rule. This rule automatically manages the lifecycle of temporary files and directories, preventing leaks and simplifying the test code by removing the need for manual try...finally blocks for cleanup.
Please add the following imports at the top of the file:
import org.junit.Rule
import org.junit.rules.TemporaryFolder @get:Rule
val tempFolder = TemporaryFolder()
@Test
fun testIsChild() {
val provider = YuhaiinDocumentProvider()
val tempDir = tempFolder.newFolder("yuhaiin_test").canonicalFile
val otherDir = tempFolder.newFolder("yuhaiin_other").canonicalFile
val partialMatchDir = File(tempDir.parentFile, tempDir.name + "_extra")
val subDir = File(tempDir, "subdir")
subDir.mkdir()
val fileInSubDir = File(subDir, "file.txt")
fileInSubDir.createNewFile()
val otherFile = File(otherDir, "other.txt")
otherFile.createNewFile()
// Construct a path that looks like it's inside tempDir but isn't after canonicalization
val traversalFile = File(tempDir, "../" + otherDir.name + "/other.txt")
assertTrue("Should be child of itself", provider.isChild(tempDir, tempDir))
assertTrue("Should be child of tempDir", provider.isChild(tempDir, subDir))
assertTrue("Should be child of tempDir", provider.isChild(tempDir, fileInSubDir))
assertFalse("Should not be child of otherDir", provider.isChild(tempDir, otherDir))
assertFalse("Should not be child of otherDir", provider.isChild(tempDir, otherFile))
assertFalse("Traversal should be blocked: ${traversalFile.path}", provider.isChild(tempDir, traversalFile))
partialMatchDir.mkdir()
assertFalse("Partial name match should be blocked: ${partialMatchDir.path}", provider.isChild(tempDir, partialMatchDir))
}1 participant
This PR resubmits the path traversal fix with an improved unit test that ensures proper cleanup of temporary directories.
🎯 What: The vulnerability fixed is a path traversal in the Document Provider.
⚠️ Risk: Potential unauthorized access or deletion of files outside the designated directory.
🛡️ Solution: Implemented secure path validation using canonical paths and trailing separator checks.
The unit test
PathTraversalTest.kthas been updated to use atry...finallyblock for guaranteed cleanup.PR created automatically by Jules for task 16491326315817938800 started by @Asutorufa