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

Update micromatch #2888

Closed
wants to merge 4 commits into from
Closed

Update micromatch #2888

wants to merge 4 commits into from

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented Sep 14, 2017

Previously #2864

See the detailed issues in the previous PRs: #2691 #2753, #2597 and #2808

@jeddy3
Copy link
Member

jeddy3 commented Sep 14, 2017

@stylelint/core This dependency update has been floating around for a while. If anyone has time to throw some fresh eyes on it, that'd be great!

@modosc
Copy link
Member

modosc commented Oct 30, 2017

i think the build errors would be fixed by #2936 - any objections to rebasing this off of master?

@jeddy3
Copy link
Member

jeddy3 commented Oct 30, 2017

any objections to rebasing this off of master?

Yep, rebase.

i think the build errors

Appveyor is still likely to fail due to an incompatibility with how we're using micromatch and the windows platform.

@modosc modosc force-pushed the greenkeeper/micromatch-3.1.0 branch from 0a4db73 to de8ab9b Compare October 30, 2017 19:39
@modosc
Copy link
Member

modosc commented Oct 30, 2017

Appveyor is still likely to fail due to an incompatibility with how we're using micromatch and the windows platform.

ah, i see - i don't have a windows machine so i'm not going to be able to do much troubleshooting here.

@jeddy3 jeddy3 changed the title fix(package): update micromatch to version 3.1.0 Update micromatch Mar 15, 2018
@prettyv
Copy link

prettyv commented May 14, 2018

FWIW, versions of micromatch<3 still depend on a pre-v2 version of braces, which transitively depends on an older version of randomatic, for which npm audit spits out a (low severity) security warning.

stylelint > micromatch > braces > expand-range > fill-range > randomatic
(https://nodesecurity.io/advisories/157)

This is probably not too important in the context of stylelint but is at least a minor annoyance because npm now outputs vulnerability information after install/update/uninstall operations by default.
Sadly, I don't have a Windows machine to help push this along assuming that the above comments are still valid.

@ntwb
Copy link
Member Author

ntwb commented May 27, 2018

Thanks @prettyv, I'll try to dig into this again soon 👍

@Berkmann18
Copy link
Member

Berkmann18 commented Jul 26, 2018

As of v9.4.0, stylelint has a "Cryptographically Weak PRNG" vulnerability disclosed here: https://nodesecurity.io/advisories/157;

Here's the npm audit output:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Cryptographically Weak PRNG                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ randomatic                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ stylelint [dev]                                              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ stylelint > micromatch > braces > expand-range > fill-range  │
│               │ > randomatic                                                 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/157                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

Has anyone here informed the devs behind micromatch and so on?
I myself disclosed that vulnerability to randomatic's repo owners here: randomatic#15 since it was affecting other packages.

@jonschlinkert
Copy link

jonschlinkert commented Jul 26, 2018

Hey guys, I created both randomatic and micromatch. Unless you're using micromatch to generate passwords or API tokens, I think it's safe to say that this is unlikely to actually be a concern.

Randomatic optionally generates random strings, but the main use of the library has always been to generate strings that follow a certain pattern. For example, it was used in micromatch for simplifying how patterns are expanded in brace patterns. You could also use it to generate a pseudo-random string for mocking out unit tests for things like order numbers that follow a pattern, like ORDER-001-AAAB, then randomatic can generate some strings that follow that pattern. Today, we mention passwords, prior to that, it wasn't used by anyone to do that. The language changed on the readme. As it relates to the usage in micromatch, it's specifically called when a brace pattern is expanded. IMHO, it's pretty unlikely that anyone would use a glob pattern for a password.

That said, I do see the following string a lot in password examples: **************** ;). My recommendation is that if you can avoid using micromatch for cryptography, you'll be in good shape.

@hudochenkov
Copy link
Member

@Berkmann18 I installed stylelint@9.4.0 and didn't get any vulnerability warnings. Most likely one of the other packages you're using require vulnerable sub-dependency, which is in line with our sub-dependencies, and npm use vulnerable one.

@hudochenkov
Copy link
Member

@jonschlinkert thank you for an explanation!

@stylelint/contributors I tried to take a look at this update when I get chance to do it on Windows machine. Surprisingly I couldn't run tests on master without errors (probably because of path differences). I have no idea why Appveyor didn't fail.

We have to take a look at this issue, as micromatch update is hanging for a very long time already.

@gucong3000
Copy link
Member

Related issues: micromatch/micromatch#132

@gucong3000
Copy link
Member

Glob is converted to absolute path:

if (path.isAbsolute(glob.replace(/^!/, ""))) return glob;
return globjoin(configDir, glob);

But micromatch@3 does not support the absolute path of Windows style

@jonschlinkert
Copy link

jonschlinkert commented Jul 30, 2018

thanks @gucong3000.

But micromatch@3 does not support the absolute path of Windows style

To clarify, micromatch no longer converts backslashes in glob patterns to forward slashes. Micromatch 3.0 does handle and match Windows paths just fine.

  • What changed?: In 3.0 if you join foo\\bar\\ to *.js, you are escaping the * with the backslashes, since the resulting pattern will be foo\\bar\\*.js.
  • Why?: We chose end-user safety and predictability over implementor convenience. Micromatch has no way of knowing which backslashes are path separators and which are intended to be used for escaping special characters. For example, an end user might want to match a literal ? or (...) or [..] in a file path. Since a number of characters are valid as both regex characters and path characters, the only way for a user to reliably match those characters in paths is to escape them with backslashes, like \\? or \\(...\\) and \\[..\\]. This is actually pretty common, and if we convert \\ to / - as in previous versions of micromatch and minimatch, we are mutating the user's pattern, which at best will simply fail to match whatever the user is trying to match. At worst we will inadvertantly be unecaping a special character that causes the glob pattern to match things it shouldn't match, or match agressively or potentially catastrophically.
  • Solution: If you want to make a glob pattern absolute, the best solution is to just use forward slashes to join a file path to the glob pattern. Also make sure to convert backslashes in the file path to forward slashes before joining.

Hope this helps.

@jeddy3
Copy link
Member

jeddy3 commented Jul 31, 2018

Solution: If you want to make a glob pattern absolute, the best solution is to just use forward slashes to join a file path to the glob pattern. Also make sure to convert backslashes in the file path to forward slashes before joining.

@jonschlinkert Many thanks for sharing this!

Copy link

@bitjoo bitjoo left a comment

Choose a reason for hiding this comment

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

lgtm

@hudochenkov hudochenkov mentioned this pull request Nov 1, 2018
@hudochenkov hudochenkov closed this Nov 1, 2018
@hudochenkov hudochenkov deleted the greenkeeper/micromatch-3.1.0 branch November 1, 2018 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants