:lock: fix: CVE-2023-26115

[aashutoshrathi]

Fixes #32

Issue with existing approach

CVE-2023-26115 TLDR; ReDoS, the issue is due to Regex matching algo.

Approach used here

Removed the use of String.prototype.replace(Regex), instead wrote function to trim tabs and space from the end of line.

Benchmarks

Code snippet used

const wrap = require("word-wrap");

const trimTabAndSpaces = (str) => {
    const lines = str.split('\n');
    const trimmedLines = lines.map(line => {
      let i = line.length - 1;
      while (i >= 0 && (line[i] === ' ' || line[i] === '\t')) {
        i--;
      }
      return line.substring(0, i + 1);
    });
    return trimmedLines.join('\n'); 
}

for (let i = 0; i <= 10; i++) {
    const attack = "a" + "t".repeat(i * 10_00000);
    const start = performance.now();
    attack.replace(/[ \t]*$/gm, ''); // -> since this is happening inside code.
    // wrap(attack, { trim: true });
    console.log(`wrap.trim: ${attack.length} characters: ${performance.now() - start}ms`);
}

for (i = 0; i <= 10; i++) {
    const attack = "a" + "t".repeat(i * 10_00000);
    const start2 = performance.now();
    trimTabAndSpaces(attack);
    console.log(`trimTabAndSpaces: ${attack.length} characters: ${performance.now() - start2}ms`);
}

Performance Benchmarks

node test.js
wrap.trim: 1 characters: 0.015165984630584717ms
wrap.trim: 1000001 characters: 6.47087499499321ms
wrap.trim: 2000001 characters: 13.000333994626999ms
wrap.trim: 3000001 characters: 19.574499994516373ms
wrap.trim: 4000001 characters: 26.035459011793137ms
wrap.trim: 5000001 characters: 31.902458995580673ms
wrap.trim: 6000001 characters: 38.429375022649765ms
wrap.trim: 7000001 characters: 44.44049999117851ms
wrap.trim: 8000001 characters: 51.241291999816895ms
wrap.trim: 9000001 characters: 57.14437499642372ms
wrap.trim: 10000001 characters: 63.937458992004395ms
----------
trimTabAndSpaces: 1 characters: 0.3221670091152191ms
trimTabAndSpaces: 1000001 characters: 0.2049579918384552ms
trimTabAndSpaces: 2000001 characters: 0.3135409951210022ms
trimTabAndSpaces: 3000001 characters: 0.3718339800834656ms
trimTabAndSpaces: 4000001 characters: 0.7508749961853027ms
trimTabAndSpaces: 5000001 characters: 0.6123340129852295ms
trimTabAndSpaces: 6000001 characters: 0.7714170217514038ms
trimTabAndSpaces: 7000001 characters: 1.168584018945694ms
trimTabAndSpaces: 8000001 characters: 0.9755829870700836ms
trimTabAndSpaces: 9000001 characters: 1.406792014837265ms
trimTabAndSpaces: 10000001 characters: 1.5417499840259552ms

[lepetitpatron]

Do you have an update when this PR will be merged? Thanks!

[jainanuj0812]

Any update?

[sergeyt]

I suggest going with /\s+$/gm faster regexp first. Previous one is slower since it uses * quantifier which includes empty replacements as well which is redundant ops

[aashutoshrathi]

@jonschlinkert is this repo active?

[MateuszKikmunter]

Any update on this @jonschlinkert ? We're blocked by this issue not being fixed.

[xavisegura]

Any update on this @jonschlinkert ? Is this repo active?

[sergeyt]

Any update on this @jonschlinkert ? Is this repo active?

in meantime someone brave enough can publish a version of the package that can be a replacement in package.json. for example, like we can do for class-validator:

"class-transformer": "npm:@nestjs/class-transformer@0.4.0",
"class-validator": "npm:@nestjs/class-validator@0.13.4",

[aashutoshrathi]

Any update on this @jonschlinkert ? Is this repo active?

in meantime someone brave enough can publish a version of the package that can be a replacement in package.json. for example, like we can do for class-validator:

"class-transformer": "npm:@nestjs/class-transformer@0.4.0",
"class-validator": "npm:@nestjs/class-validator@0.13.4",

I think we can try, what do you think @sergeyt ?

[sergeyt]

Any update on this @jonschlinkert ? Is this repo active?

in meantime someone brave enough can publish a version of the package that can be a replacement in package.json. for example, like we can do for class-validator:

"class-transformer": "npm:@nestjs/class-transformer@0.4.0",
"class-validator": "npm:@nestjs/class-validator@0.13.4",

I think we can try, what do you think @sergeyt ?

Sure, why not

[aashutoshrathi]

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap In package.json, the below should do it.

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },

[jainanuj0812]

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap In package.json, the below should do it.

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },

and if you don't use yarn and using npm itself use 'overrides'

[aashutoshrathi]

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap In package.json, the below should do it.

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },

Please use the newer version the older one causes some issues

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.5"
  },

[andreidiaconescu]

If we use npm not yarn, how would we use overrides to change a package with another ? i do not find it clear from the official docs: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#overrides Thank you.

[sfc-gh-dszmolka]

If we use npm not yarn, how would we use overrides to change a package with another ? i do not find it clear from the official docs: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#overrides Thank you.

for example, using something like this in your package.json:

"overrides": {
    "word-wrap" : "npm:@aashutoshrathi/word-wrap@1.2.5"
  },

[DiegoTavelli]

Okay here's a temporary package: https://www.npmjs.com/package/@aashutoshrathi/word-wrap In package.json, the below should do it.

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.4"
  },

Please use the newer version the older one causes some issues

"resolutions": {
    "word-wrap": "npm:@aashutoshrathi/word-wrap@1.2.5"
  },

I will try with this one, I have a question. Becouse that dependency is involved into eslint and jest dependecies, I have to specify from the example parent dependency. 'jest/jest-config.../word-wrap' or just specify as you say ?

[aashutoshrathi]

You can use the same, without specifying the path.

[velsonjr]

Please have this merged to the main branch

[jainanuj0812]

Thanks @aashutoshrathi ,

But still, this needs to be merged..Can anyone from us reach out to @jonschlinkert through LinkedIn/Twitter please :-P

[velsonjr]

@aashutoshrathi , The same is still reappearing for the following path, react-scripts@5.0.1 › jest@27.5.1 › @jest/core@27.5.1 › jest-config@27.5.1 › jest-environment-jsdom@27.5.1 › jsdom@16.7.0 › escodegen@2.0.0 › optionator@0.8.3 › word-wrap@1.2.5

[aashutoshrathi]

@aashutoshrathi , The same is still reappearing for the following path, react-scripts@5.0.1 › jest@27.5.1 › @jest/core@27.5.1 › jest-config@27.5.1 › jest-environment-jsdom@27.5.1 › jsdom@16.7.0 › escodegen@2.0.0 › optionator@0.8.3 › word-wrap@1.2.5

Hey! if you use resolution override in the main package.json, it shouldn't happen

[velsonjr]

Hey @aashutoshrathi, In case do another npm i, it throws me this npm ERR! Invalid Version: npm:@aashutoshrathi/word-wrap@1.2.5 . I have the added the same inside resolutions. Also when I upgrade the react, node and npm versions, the scan still shows the vulnerability ( In my case its react 18.2, node 16.20 and npm 8.19)

[xx745]

Hi all, why is this fix not merged yet?

[bgswilde]

@xx745, I don't think @jonschlinkert has any desire to continue managing this repo. I've reached out to him several times on twitter, where he's active, but no response. I would love to be wrong though! I would be great for him to come in and merge this or for there to be a way for somebody else to be an admin here, then he doesn't have to keep up with it.

[xx745]

@bgswilde it's a shame that we can't get update for package downloaded 29877879 times every week, but business as usual in JS world :-/

@jonschlinkert / @brandscale - please help us with merging this PR.

[aashutoshrathi]

@jonschlinkert updated to a kind of safer regex to support lower node versions. cc: @sergeyt finally using your idea. 😄

[blue-rigel]

Hi @jonschlinkert, kindly assist to review merge, thanks

[jainanuj0812]

@jonschlinkert can you please review and merge?

[wellwelwel]

I think that the word-wrap owner no longer supports this project.

The best way forward is to replace it by a supported project. For now, @aashutoshrathi's fork is a good alternative.

---

To replace projects that already uses word-wrap by @aashutoshrathi's fork:

• npm

package.json

"overrides": {
  "word-wrap" : "npm:@aashutoshrathi/word-wrap"
}

---

Using @aashutoshrathi's fork in new projects:

npm i @aashutoshrathi/word-wrap

[jpogorzelski]

For those using Yarn 3, here is proper resolutions format:

"resolutions": {
  "word-wrap": "aashutoshrathi/word-wrap"
}

[sbrickner]

For those using Yarn 3, here is proper resolutions format:

"resolutions": {
  "word-wrap": "aashutoshrathi/word-wrap"
}

Thank you @jpogorzelski this is working for pnpm as well.

[aashutoshrathi]

Hey! Just an update, I recently published v1.2.6 -> https://github.com/aashutoshrathi/word-wrap/pull/2 These were also vulnerability fixes for Mocha, so let me know if we want this here too.

[CarlosBercero]

I think that @jonschlinkert no longer supports this project.

The best way forward is to replace it by a supported project. For now, @aashutoshrathi's fork is a good alternative.

To replace projects that already uses word-wrap by @aashutoshrathi's fork:

• npm

package.json

"overrides": {
  "word-wrap" : "npm:@aashutoshrathi/word-wrap"
}

Using @aashutoshrathi's fork in new projects:

npm i @aashutoshrathi/word-wrap

Hi,

I get this error:

npm ERR! code EOVERRIDE npm ERR! Override for word-wrap@^1.2.3 conflicts with direct dependency

How can I solve that?

Thank you.

[PaulSumner-Sage]

I think that @jonschlinkert no longer supports this project. The best way forward is to replace it by a supported project. For now, @aashutoshrathi's fork is a good alternative.

To replace projects that already uses word-wrap by @aashutoshrathi's fork:

• npm

package.json

"overrides": {
  "word-wrap" : "npm:@aashutoshrathi/word-wrap"
}

Using @aashutoshrathi's fork in new projects:

npm i @aashutoshrathi/word-wrap

Hi,

I get this error:

npm ERR! code EOVERRIDE npm ERR! Override for word-wrap@^1.2.3 conflicts with direct dependency

How can I solve that?

Thank you.

You presumably have word-wrap directly listed as one of your dependencies in the package.json file? the npm docs have a line about that:

You may not set an override for a package that you directly depend on unless both the dependency and the override itself share the exact same spec. To make this limitation easier to deal with, overrides may also be defined as a reference to a spec for a direct dependency by prefixing the name of the package you wish the version to match with a $.

Found here: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#overrides

[zdods]

👀 I take it we don't want to .trimEnd, to remove the Regex all together for that line? (I was getting a ReDoS warning from CodeQL for that line that's being added here)

[aashutoshrathi]

👀 I take it we don't want to .trimEnd, to remove the Regex all together for that line? (I was getting a ReDoS warning from CodeQL for that line that's being added here)

What do you mean exactly?

[zdods]

👀 I take it we don't want to .trimEnd, to remove the Regex all together for that line? (I was getting a ReDoS warning from CodeQL for that line that's being added here)

What do you mean exactly?

If the goal is to remove the trailing whitespace, you can use the string prototype method to remove it without regex.

const foo = '   some string with whitespace on both ends  ';

foo.trimEnd() === '  some string with whitespace on both ends'; // true

[wellwelwel]

👀 I take it we don't want to .trimEnd, to remove the Regex all together for that line? (I was getting a ReDoS warning from CodeQL for that line that's being added here)

What do you mean exactly?

If the goal is to remove the trailing whitespace, you can use the string prototype method to remove it without regex.

const foo = '   some string with whitespace on both ends  ';

foo.trimEnd() === '  some string with whitespace on both ends'; // true

---

@zdods, it would cause a break change since trimEnd is available starting from NodeJS 10.3.0.

Please, see: https://node.green/#ES2019-features-string-trimming-String-prototype-trimEnd

[zdods]

👀 I take it we don't want to .trimEnd, to remove the Regex all together for that line? (I was getting a ReDoS warning from CodeQL for that line that's being added here)

What do you mean exactly?

If the goal is to remove the trailing whitespace, you can use the string prototype method to remove it without regex.

const foo = '   some string with whitespace on both ends  ';

foo.trimEnd() === '  some string with whitespace on both ends'; // true

---

@zdods, it would cause a break change since trimEnd is available starting from NodeJS 10.3.0.

Please, see:

https://node.green/#ES2019-features-string-trimming-String-prototype-trimEnd

Fair 🤷‍♂️ I guess in my use case I'm running v16+ so will likely fork and do that. (Rather than still be vulnerable with a different pattern)

[aashutoshrathi]

trimEnd

Another suggestion I have is that we can add trimEnd polyfill

[sergeyt]

trimEnd

Another suggestion I have is that we can add trimEnd polyfill

we can have a package with major version increment and bump engine version where trimEnd is natively supported

[lorand-horvath]

@jonschlinkert Could you please merge this? It's starting to get out of hand...

[nocive]

Another widely used and abandoned third party library? An all too familiar scenario in the nodejs world sadly :yawning_face:

[lorand-horvath]

Another widely used and abandoned third party library? An all too familiar scenario in the nodejs world sadly yawning_face

@nocive As if your beloved php would be any better...

[nhasan7]

@jonschlinkert Could you please merge this?

[seahindeniz]

Lets wait for solving the redos problem

[aaleksandrov]

@jonschlinkert Would be really great to merge this one. This repo is widely used

[nicholas-quirk-mass-gov]

@doowb This PR didn't resolve the issue. Please review this thread: https://github.com/jonschlinkert/word-wrap/pull/33/files#r1245633651

[doowb]

Thanks @nicholas-quirk-mass-gov , I was just coming here to comment.

We choose to go with #41 since it's using the inline trimEnd implementation for backwards compatibility. That PR was branched off of this PR, so GitHub automatically merged in the changes here when I merged in the other PR.

I'm publishing word-wrap@1.2.4 in a few minutes with the fix from #41.

[wellwelwel]

@aashutoshrathi, since this PR has been merged and this Issue was resolved with PR #41, I'm backing to the original project now.

Thank you for bringing a solution until this problem was actually solved here, I believe your fork has helped many people and keep helping (the numbers themselves prove this) 🚀

[aashutoshrathi]

@aashutoshrathi, since this PR has been merged and this Issue was resolved with PR #41, I'm backing to the original project now.

Thank you for bringing a solution until this problem was actually solved here, I believe your fork has helped many people and keep helping (the numbers themselves prove this) 🚀

No problemo anytime for OSS folks! 🖖🏻