Fix symlink traversal in task uploads#1103
Conversation
| if (this.hiddenFolders.includes(file)) continue; | ||
|
|
||
| const filePath = path.join(dirPath, file); | ||
| const stats = fs.statSync(filePath); |
There was a problem hiding this comment.
How about add a try-catch here to handle potential errors(e.g file doesn't exist, permission denied)
There was a problem hiding this comment.
Okay, but there's already a try-catch block wrapping the entire function, maybe adding another one here seems a bit redundant?
There was a problem hiding this comment.
This really depends on the intended behavior:
Current approach (fail fast): Any file error stops the entire traversal
With inner try-catch (best effort): Skip problematic files and continue
What's your preference here? Should one bad file fail the entire upload, or should we skip it and continue with the rest?
There was a problem hiding this comment.
You are right~ we should skip it and continue with the rest.
a7m-1st
left a comment
There was a problem hiding this comment.
Thanks @LuoPengcheng12138 , can you add test cases for this? I have already added a comprehensive test suite for electron file mngmnt. LGTM though 👍
Wendong-Fan
left a comment
There was a problem hiding this comment.
thanks @LuoPengcheng12138 and @4pmtong !
Description
This PR closes a symlink-based escape in task file collection and upload:
What is the purpose of this pull request?