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

Allow dots in bin names #7811

merged 6 commits into from Jan 22, 2020

Conversation

@valerybugakov
Copy link
Contributor

@valerybugakov valerybugakov commented Jan 10, 2020

Summary

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

Test plan

Use dangerous bin name tests.

@valerybugakov
Copy link
Contributor Author

@valerybugakov valerybugakov commented Jan 12, 2020

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

@BYK
Copy link
Member

@BYK 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

@valerybugakov valerybugakov commented Jan 13, 2020

@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 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 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

@valerybugakov valerybugakov commented Jan 13, 2020

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

@saveman71
Copy link

@saveman71 saveman71 commented Mar 16, 2020

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 issue 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 added a commit to VincentBailly/yarn that referenced this issue 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 issue 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 issue 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants