Skip to content

Fix symlink traversal in task uploads#1103

Merged
Wendong-Fan merged 3 commits into
mainfrom
chore/fix-symlink-traversal
Feb 1, 2026
Merged

Fix symlink traversal in task uploads#1103
Wendong-Fan merged 3 commits into
mainfrom
chore/fix-symlink-traversal

Conversation

@LuoPengcheng12138

Copy link
Copy Markdown
Contributor

Description

This PR closes a symlink-based escape in task file collection and upload:

  • Task file listing now skips symlinks and validates real paths to ensure they stay inside the working directory.
  • Electron’s file enumeration uses lstat + realpath to prevent recursive traversal through external links.
  • sanitize_command() to validate ln -s will implement in Camel repo

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Comment thread electron/main/fileReader.ts Outdated
if (this.hiddenFolders.includes(file)) continue;

const filePath = path.join(dirPath, file);
const stats = fs.statSync(filePath);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How about add a try-catch here to handle potential errors(e.g file doesn't exist, permission denied)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, but there's already a try-catch block wrapping the entire function, maybe adding another one here seems a bit redundant?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right~ we should skip it and continue with the rest.

@a7m-1st a7m-1st left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 Wendong-Fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Wendong-Fan Wendong-Fan merged commit 0cab8c7 into main Feb 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Task Upload Directory Recurses Through Symbolic Links, Causing Full File Upload

4 participants