Skip to content
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

chore: Upgrade minimatch to fix RegEx DoS security issue #10338

Merged
merged 3 commits into from
Mar 13, 2019

Conversation

phacks
Copy link
Contributor

@phacks phacks commented Dec 7, 2018

Fixes #10282

TODO:

  • Switch to recursive-readdir library
  • Implement async readdir behavior
  • Fix existing tests
  • Test error handling
  • Test on a gatsby-starter-blog with few examples of REPLs
  • Try and implement the Promise.all refactor

@phacks phacks changed the title Use recursive-readdir instead of recursive-readdir-synchronous [Security][#10282] Upgrade minimatch to fix RegEx DoS security issue Dec 7, 2018
@DSchau
Copy link
Contributor

DSchau commented Dec 7, 2018

@phacks applied the do not merge and WIP labels while you're still working on this and validating.

Please let us know when it's ready for another set of 👀!

@phacks
Copy link
Contributor Author

phacks commented Dec 8, 2018

@DSchau Should be ready for a review!

I created a sample repo with lots of code REPLs to test it out, it’s available here: https://github.com/phacks/gatsby-code-repl-samples

I managed to implement the Promise.all() refactor, although in my benchmark (~10 000 code REPLs), I found virtually no differences in the createPages duration. I’m not sure whether that’s normal or not 🤔

@phacks
Copy link
Contributor Author

phacks commented Dec 11, 2018

I forgot to mention that for some (as yet unknown) reason, the /redirect-to-codepen links do not work in my example repo. However, they do not work with the current gatsby-remark-code-repls plugin, so I don’t think my PR has something to do with that. I may take some time to investigate the issue after this PR!

@phacks
Copy link
Contributor Author

phacks commented Dec 17, 2018

@DSchau do you think you may have some time this week to review the PR? Thanks!

@DSchau
Copy link
Contributor

DSchau commented Dec 17, 2018

@phacks of course--will look at it today and leave any feedback! Thanks!

@LekoArts LekoArts added the status: awaiting author response Additional information has been requested from the author label Jan 29, 2019
@LekoArts
Copy link
Contributor

@phacks Hi. Do you have time to answer @DSchau question? :)

@phacks
Copy link
Contributor Author

phacks commented Jan 29, 2019

@LekoArts @DSchau Oops! This one went under my radar. I will have a look at it by this weekend (probably today though), thanks!

@LekoArts LekoArts removed the status: awaiting author response Additional information has been requested from the author label Jan 29, 2019
@phacks phacks force-pushed the fix-security-issue-minimatch branch from 68db468 to 68cb7b6 Compare January 29, 2019 17:56
@wardpeet wardpeet changed the title [Security][#10282] Upgrade minimatch to fix RegEx DoS security issue chore: Upgrade minimatch to fix RegEx DoS security issue Jan 31, 2019
@phacks phacks force-pushed the fix-security-issue-minimatch branch from 68cb7b6 to d636495 Compare March 4, 2019 20:30
@phacks phacks force-pushed the fix-security-issue-minimatch branch from d636495 to cb3902c Compare March 4, 2019 20:36
@wardpeet
Copy link
Contributor

wardpeet commented Mar 6, 2019

@phacks almost there! sorry about this!

@phacks
Copy link
Contributor Author

phacks commented Mar 6, 2019

@wardpeet No worries! It looks like the syntax you suggested isn’t available for async functions — I suggested a few alternatives (including mine) in the first comment.

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.

4 participants