-
-
Notifications
You must be signed in to change notification settings - Fork 51
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.
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
fix: Don't follow circular symlinks #126
fix: Don't follow circular symlinks #126
Conversation
@joshhunt could we just check if the error is ELoop and bail? |
@phated Could do. However, I think that's problematic because:
It's up to you which you would prefer, but I'm skeptical of just waiting for ELOOP. I think the logic in this PR is good, but I'm struggling to come up with additional test cases to better validate it... |
@phated I think this is in a good spot - open for feedback, other approaches, or scenarios to test! |
// And it should follow a symlink to a parent directory (circular symlink) without blowing up | ||
{ | ||
cwd: dir, | ||
base: dir + '/fixtures/symlinks', | ||
path: | ||
dir + | ||
'/fixtures/symlinks/symlink-dest/hey/isaidhey/whatsgoingon/test.txt', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to check if this is a duplicate glob getting through because we wouldn't want to emit it twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joshhunt! I'm going to merge this and then do some cleanup to integrate it with my changes to avoid blowing the stack.
Fixes the
Error: ELOOP: too many symbolic links encountered
exception that happens during the walkdir when the working directory contains circular symlinks.I think this is the right logic to check this, and added some tests to try and catch the common scenarios.