Skip to content

🔒 [security fix] Fix Path Traversal in Document Provider (Updated) #215

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.

Sign up for GitHub

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

Open
Asutorufa wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🔒 [security fix] Fix Path Traversal in Document Provider (Updated) #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions app/src/main/kotlin/io/github/asutorufa/yuhaiin/docuemntprovider/YuhaiinDocumentProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,23 @@ class YuhaiinDocumentProvider : DocumentsProvider() {
}

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))
}
Comment on lines 87 to +92
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

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)
    }


return false
internal fun isChild(parent: File, child: File): Boolean {
return try {
val canonicalParent = parent.canonicalPath
val canonicalChild = child.canonicalPath
if (canonicalChild == canonicalParent) return true
val parentPathWithSeparator =
if (canonicalParent.endsWith(File.separator)) canonicalParent else canonicalParent + File.separator
canonicalChild.startsWith(parentPathWithSeparator)
} catch (e: IOException) {
false
}
}

override fun querySearchDocuments(
Expand Down Expand Up @@ -119,12 +131,7 @@ class YuhaiinDocumentProvider : DocumentsProvider() {
val file = pending.removeAt(0)
// Avoid directories outside the $HOME directory linked with symlinks (to avoid e.g. search
// through the whole SD card).
val isInsideHome: Boolean = try {
file.canonicalPath.startsWith(baseDir.toString())
} catch (_: IOException) {
true
}
if (isInsideHome) {
if (isChild(baseDir, file)) {
if (file.isDirectory) {
file.listFiles()?.let { pending.addAll(it) }
} else {
Expand All @@ -144,6 +151,7 @@ class YuhaiinDocumentProvider : DocumentsProvider() {
private fun getFileForDocId(docId: String): File {
val f = File(docId)
if (!f.exists()) throw FileNotFoundException(f.absolutePath + " not found")
if (!isChild(baseDir, f)) throw FileNotFoundException("Invalid document ID: $docId")
Comment on lines 153 to +154
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

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")

return f
}

Expand Down
45 changes: 45 additions & 0 deletions app/src/test/kotlin/io/github/asutorufa/yuhaiin/docuemntprovider/PathTraversalTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package io.github.asutorufa.yuhaiin.docuemntprovider

import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import java.io.File
import java.nio.file.Files

class PathTraversalTest {

@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()
}
}
Comment on lines +11 to +44
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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))
    }

}