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

Refactor #10

Merged
merged 48 commits into from
Oct 20, 2016
Merged

Refactor #10

merged 48 commits into from
Oct 20, 2016

Conversation

jonschlinkert
Copy link
Member

@jonschlinkert jonschlinkert commented Oct 8, 2016

Complete overhaul, with parser and compiler, source map support, more accurate matching (passes all Bash 4.3 brace expansion tests, all minimatch braces tests, all brace-expansion tests, and even passes tests that bash fails... and it's fast https://github.com/jonschlinkert/braces/tree/refactor#latest-results.

Still working on readme/docs.

@doowb, @hemanth, @es128, @eush77 any feedback or review would be appreciated. I'll squash this beast when it's time to merge (@phated you are of course welcome to leave feedback as well)

closes #8
closes #9

@doowb
Copy link
Member

doowb commented Oct 9, 2016

I meant to comment earlier and got caught up with messing around with the tests...

I like the changes a lot. I like how the things like parsers and compilers are split into smaller pieces that makes it easier to reason about and make changes when necessary.

updating travis-ci configuration to install the latest bash based on the specific environment (osx or linux)
adding osx to travis-ci matrix
Copy link
Collaborator

@hemanth hemanth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

"never"
]
"wrap-iife": [2, "any"],
"yoda": [2, "never"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

heh heh

I added this while I was explaining symbols to someone and forgot to remove it!!!
to ensure they pass on windows
since the bash util was remove from tests
also:
- expose parse/compile methods
- add unit tests, including tests from 1.8.5
@phated
Copy link
Member

phated commented Oct 20, 2016

@jonschlinkert I just read the README and this looks so awesome!

@jonschlinkert
Copy link
Member Author

jonschlinkert commented Oct 20, 2016

I just read the README and this looks so awesome!

thanks! that's great, I really appreciate it! I'm glad it's working out, I spent a ton of time on this refactor trying to get it right. @doowb helped a lot too. It was fun, we actually wrote out the compiler steps character by character for a bunch of brace patterns before we even wrote a single line of code in the actual compiler. Not something I get to do often lol

I'll try to write this quickly since my connection keeps going out (we've had thunderstorms for a couple of hours). the performance section captures the essence of what I meant when I said "as well as a couple of other reasons".

The first point is that there are many, many scenarios where brace patterns grow geometrically. If you define two brace patterns for example, they're multiplicative if not exponential. So even though those patterns in the readme are contrived, it's not uncommon for users to define brace patterns that result in similarly huge strings.

The second point is that there is potential for exploiting this effect in minimatch and the brace-expansion to cause a DoS (similar to the ReDoS bug minimatch had recently https://medium.com/node-security/minimatch-redos-vulnerability-590da24e6d3c#.jew0b6mpc).

Basically what I'm getting at is that, for the reasons mentioned on glob-parent alone there is enough benefit in doing the brace expansion first, before passing patterns to minimatch or node-glob. Then if we add the two reasons I just listed it seems like a no-brainer. IMHO, minimatch should remove brace expansion before it gets exploited, or someone should do a pr to the brace-expansion lib to add to-regex-range or something similar so that glob patterns can use it to avoid creating exponential patterns.

(lol quickly... well I tried)

edit: I just removed a partial sentence about bash/mac that didn't render because the code example wasn't escaped correctly. it didn't add to point anyway

@jonschlinkert
Copy link
Member Author

K I'm going to bump this to 2.0 and publish so I can push up the micromatch refactor :)

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.

Dot character not working as it should (escape + expansion) todo
4 participants