1110 strip directory traversal instructions from volume and file paths #1334
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ip-directory-traversal-instructions-from-volume-and-file-paths
mzur
reviewed
Jan 5, 2026
| return false; | ||
| } | ||
|
|
||
| if (preg_match('/(\/|\\\\)*(\.\.)+(\/|\\\\)*(.)*/', urldecode($filename)) !== 0) { |
Member
There was a problem hiding this comment.
Please convince me why the regex /\.\.(\/|$)/ is not enough for our use case. Yours:
- Matches Windows-style backslashes which we can ignore because BIIGLE runs in Docker containers.
- Would also match something like
my-file..jpgormy..dir/file.jpgwhich should be fine. - Matches any trailing characters (
(.)*) which are irrelevant here.
Comment on lines
+197
to
+203
| protected function pathIsValid($path) | ||
| { | ||
| if (preg_match('/(\/|\\\\)*(\.\.)+(\/|\\\\)*(.)*/', urldecode($path)) !== 0) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
Member
There was a problem hiding this comment.
Maybe set this as public static function so it can be reused in the VolumFiles rule?
Comment on lines
+191
to
+197
| * Determine if the given path is valid | ||
| * | ||
| * @param string $path | ||
| * | ||
| * @return boolean | ||
| */ | ||
| protected function pathIsValid($path) |
Member
There was a problem hiding this comment.
A more descriptive name could be pathHasDirectoryTraversal.
| } | ||
|
|
||
| if (preg_match('/(\/|\\\\)*(\.\.)+(\/|\\\\)*(.)*/', urldecode($filename)) !== 0) { | ||
| $this->message = 'Traversing directories is not allowed. Insert a valid path within the volume.'; |
Member
There was a problem hiding this comment.
Suggested change
| $this->message = 'Traversing directories is not allowed. Insert a valid path within the volume.'; | |
| $this->message = 'Filenames with path traversal instructions are not allowed..'; |
| } | ||
|
|
||
| if (!$this->pathIsValid($value)) { | ||
| $this->message = 'The URL inserted is not valid. Please do not use path traversals.'; |
Member
There was a problem hiding this comment.
Suggested change
| $this->message = 'The URL inserted is not valid. Please do not use path traversals.'; | |
| $this->message = 'Volume URLs with path traversal instructions are not allowed.'; |
| $path = $url[1]; | ||
|
|
||
| if (!$this->pathIsValid($path)) { | ||
| $this->message = "The path inserted is not valid. Please do not use path traversals"; |
Member
There was a problem hiding this comment.
Suggested change
| $this->message = "The path inserted is not valid. Please do not use path traversals"; | |
| $this->message = "Volume URLs with path traversal instructions are not allowed."; |
Member
There was a problem hiding this comment.
Instead of adding this twice, you could also prepend it once in passes().
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #1110