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

Upgrade string-width #2029

Closed
wants to merge 1 commit into from
Closed

Upgrade string-width #2029

wants to merge 1 commit into from

Conversation

adam2k
Copy link

@adam2k adam2k commented Sep 13, 2021

@adam2k
Copy link
Author

adam2k commented Sep 13, 2021

@bcoe or @substack can I get maintainer approval to run workflows? I haven't contributed to this repo yet, so let me know if there is some other way I should request access. Thanks!

@ljharb
Copy link
Contributor

ljharb commented Sep 13, 2021

Noting: string-width v5 requires node 12 while v4 requires node 8, luckily this isn't a breaking change for yargs since it already requires node 12.

However, string-width v5 is ESM-only, which means it can't be required, which means yargs likely can't use it.

I'd suggest finding an alternative dependency that isn't ESM-only and also lacks the CVE.

@bcoe
Copy link
Member

bcoe commented Sep 13, 2021

@ljharb @adam2k the ESM version of yargs already dropped using string-width, ironically because at the time string-width wasn't ESM.

The alternate implementation is here.

I suggest we just switch to this approach for both ESM and CJS, the downside is that string width will be slightly off for some unicode characters I believe (am I correct in my understanding that some unicode characters are two ascii characters wide?).

Either way, I think this is a minor regression, and worth dropping a dep.

@ljharb
Copy link
Contributor

ljharb commented Sep 14, 2021

@bcoe that alternate implementation is quite wrong for many things - the spread operator only splits to code points, but many emojis are multiple code points - '🏳️‍🌈' for example, has a length of 6 and contains 4 code points.

A proper replacement would likely require using Intl.Segmenter and splitting by grapheme clusters, and then checking the resulting array length.

@woehr
Copy link

woehr commented Sep 22, 2021

The path of least resistance here would probably be to see whether strip-ansi will backport a dependency bump to address the CVE and then to get string-width 4.2.2 to update the strip-ansi dependency. I've asked whether strip-ansi can do a hotfix here: chalk/strip-ansi#40.

@bcoe
Copy link
Member

bcoe commented Sep 23, 2021

@Qix- kindly back-ported a fix to ansi-regex@5.0.1, so we should be good now 👍

chalk/ansi-regex#38

@bcoe bcoe closed this Sep 23, 2021
@adam2k adam2k deleted the fix/cwe-400 branch September 30, 2021 22:38
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.

None yet

5 participants