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

Allow dots in bin names #7811

Merged
merged 6 commits into from
Jan 22, 2020
Merged

Conversation

valerybugakov
Copy link
Contributor

Summary

Bin regex is too restrictive at the moment. See comments here.

Test plan

Use dangerous bin name tests.

@valerybugakov
Copy link
Contributor Author

@Just-Zach regex has i flag, so it should ignore letter casing?

@BYK
Copy link
Member

BYK commented Jan 12, 2020

Colons are not allowed in file names on Windows, so not sure if this is a good idea.

@valerybugakov
Copy link
Contributor Author

@Just-Zach @BYK from the docs it seems that it should be able to create a symlink using name provided a bin key and colons are not allowed in symlinks too. So it won't validate correctly on Windows.

@arcanis
Copy link
Member

arcanis commented Jan 13, 2020

Yep colons aren't safe - can you also check against special cases (. and ..)? It'll probably be hard to exploit but eh 🤷‍♀️

@BYK
Copy link
Member

BYK commented Jan 13, 2020

As far as I have seen the key is not used as a path - The value would be used as a filepath but it is validated using the isValidBin(...) function. The key is used to reference the value, but isn't used when writing files to disk.

The key is definitely used on the filesystem as that's the name of the created bin/symlink/whatever that is used under nodel_modules/.bin. Now with PnP, maybe we can make that happen in-memory but not sure if that's a worthy investment at this point.

@valerybugakov
Copy link
Contributor Author

@arcanis multiple dots on windows are allowed
@BYK I'll remove colon from the regex

@saveman71
Copy link

Worth noting for future people coming here that yarn cache clean is required to get the missing binaries again (i.e. upgrading and installing/removing nodes_modules isn't enough)

VincentBailly pushed a commit to VincentBailly/yarn that referenced this pull request Jun 10, 2020
* Fixed bin regex

* Removed unsupported by Windows colon from bin regex

* Updated normalize-manifest tests

* Update fix.js

* Update actual.json

* Update normalize-manifest.js.snap

Co-authored-by: Maël Nison <nison.mael@gmail.com>
VincentBailly pushed a commit to VincentBailly/yarn that referenced this pull request Jun 10, 2020
* Fixed bin regex

* Removed unsupported by Windows colon from bin regex

* Updated normalize-manifest tests

* Update fix.js

* Update actual.json

* Update normalize-manifest.js.snap

Co-authored-by: Maël Nison <nison.mael@gmail.com>
rivy added a commit to rivy/scoop-bucket that referenced this pull request Oct 31, 2020
- using 1.19.2 as primary version; 1.22.5 has "invalid 'bin'" issues
  - ref: yarnpkg/yarn#7755
  - ref: yarnpkg/yarn#7811
  - but... *still* a problem even after PR#7811
rivy added a commit to rivy/scoop-bucket that referenced this pull request Nov 8, 2020
- using 1.19.2 as primary version; 1.22.5 has "invalid 'bin'" issues
  - ref: yarnpkg/yarn#7755
  - ref: yarnpkg/yarn#7811
  - but... *still* a problem even after PR#7811
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