Fix isPathTraversal method to be less strict#5865
Fix isPathTraversal method to be less strict#5865christopherholland-workday wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello, 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 refines the 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 relaxes the isPathTraversal validation to allow absolute Unix paths, which is useful for Dockerized environments, and updates the implementation to use regular expressions with new unit tests. However, this change significantly increases the attack surface by allowing users to specify arbitrary system directories if the validated path is used in file system operations, potentially leading to Local File Inclusion (LFI) and unauthorized access. A more secure approach would be to restrict access to a whitelist of allowed base directories, similar to validateVectorStorePath. Additionally, a potential path traversal vulnerability was identified due to incomplete handling of mixed-encoded characters for parent directory sequences, which could bypass current validation. Otherwise, the changes are well-structured and the new tests are comprehensive.
| export const isPathTraversal = (path: string): boolean => { | ||
| // Check for common path traversal patterns | ||
| // Allow absolute paths in Linux containers (e.g., /data/...) | ||
| // but block directory traversal attempts and other dangerous patterns | ||
| const dangerousPatterns = [ | ||
| '..', // Directory traversal | ||
| '/', // Root directory | ||
| '\\', // Windows root directory | ||
| '%2e', // URL encoded . | ||
| '%2f', // URL encoded / | ||
| '%5c' // URL encoded \ | ||
| /\.\./, // Directory traversal (..) | ||
| /%2e%2e/i, // URL encoded .. | ||
| /%2f/i, // URL encoded / | ||
| /%5c/i, // URL encoded \ (Windows path) | ||
| /\0/, // Null bytes | ||
| /^[a-zA-Z]:\\/, // Windows absolute paths (C:\) - not allowed in Linux containers | ||
| /^\\\\[^\\]/, // UNC paths (\\server\) - not allowed in Linux containers | ||
| /%00/i // URL encoded null byte | ||
| ] | ||
|
|
||
| return dangerousPatterns.some((pattern) => path.toLowerCase().includes(pattern)) | ||
| return dangerousPatterns.some((pattern) => pattern.test(path)) | ||
| } |
There was a problem hiding this comment.
The relaxation of the isPathTraversal function to allow absolute Unix paths introduces a significant security risk. If used with user-provided paths in file operations, an attacker could specify arbitrary system directories (e.g., /etc, /var, /root) to read sensitive files, leading to Local File Inclusion (LFI) and unauthorized access. Additionally, the current implementation is vulnerable to path traversal attacks using mixed-encoding, such as .%2e/, which decodes to ../. The check for .. only looks for .. literally or fully encoded as %2e%2e, missing mixed cases like .%2e or %2e.. The regex /^[a-zA-Z]:\\/ for blocking Windows paths also has limitations, as it doesn't account for leading whitespace or forward slashes (e.g., C:\ or C:/). To make the validation more robust against mixed-encoding bypasses, consider normalizing %2e to . before checking for ...
export const isPathTraversal = (path: string): boolean => {
// Check for directory traversal by decoding `%2e` to `.` first.
// This catches `..`, `.%2e`, `%2e.`, and `%2e%2e`.
if (/\.\./.test(path.replace(/%2e/ig, '.'))) {
return true;
}
// Allow absolute paths in Linux containers (e.g., /data/...)
// but block other dangerous patterns.
const dangerousPatterns = [
/%2f/i, // URL encoded /
/%5c/i, // URL encoded \ (Windows path)
/\0/, // Null bytes
/^[a-zA-Z]:\\/, // Windows absolute paths (C:\) - not allowed in Linux containers
/^\\\\[^\\]/, // UNC paths (\\server\) - not allowed in Linux containers
/%00/i // URL encoded null byte
]
return dangerousPatterns.some((pattern) => pattern.test(path))
}1 participant
Related to #5861 and #5326.
Relax
isPathTraversalto allow absolute Unix paths in Linux containersThe previous implementation of isPathTraversal blocked any path containing / or , which prevented legitimate absolute paths (e.g. /data/vectorstore) from being used in the Folder document loader node. This caused false positives for users running Flowise in Docker containers with mounted volumes.
What changed
3.Windows absolute paths (C:), UNC paths (\server), and extended-length paths are still blocked as they are not valid in Linux container environments
Security rationale
The function is called in four places. Three of them validate UUID inputs first — a valid UUID can never contain dangerous characters, so the change has no security impact there. The only meaningful call site is Folder.ts, where the path is passed to LangChain's DirectoryLoader. In a containerized deployment, filesystem access is already constrained by Docker volume mounts, so allowing absolute paths shifts the boundary to the OS level (correct) rather than the string validator (too restrictive).
Directory traversal (..) and all encoded equivalents remain blocked.
Unit tests
isPathTraversalfunctionManual Testing