Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Use forked trim library to avoid regex DOS #72

Merged
merged 1 commit into from
Jan 14, 2021
Merged

Conversation

davidcui1225
Copy link
Contributor

@davidcui1225 davidcui1225 commented Jan 14, 2021

Issue #, if available:
N/A
Description of changes:
component/trim library that is used by nteract/outputs causes a RegEx DOS error. Forked trim and changed implementation to address the RegEx DOS (needs confirmation from whitesourcing still)

Makes sure package.json references forked version of trim.

Forked trim

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@joshuali925
Copy link
Contributor

I'm not very sure if we can use dependencies from unpublished fork. Also could you give a bit more information/reference on this (the dependabot regex DOS alert is for highlight.js, why are we patching trim instead of upgrading highlight.js?

@davidcui1225
Copy link
Contributor Author

I'm not very sure if we can use dependencies from unpublished fork. Also could you give a bit more information/reference on this (the dependabot regex DOS alert is for highlight.js, why are we patching trim instead of upgrading highlight.js?

We have a regex DOS alert from WhiteSource scanning for trim which is what is causing the CVE. If needed, I can address the highlight.js alert separately. Also, using an unpublished fork should not be an issue, it's not an uncommon practice and was also signed off by @seraphjiang

@joshuali925
Copy link
Contributor

Got it, you might have to put it into resolutions, because now your fork is only added as a dependency, I think nteract/outputs will still use the original trim

trim@0.0.1:
version "0.0.1"
resolved "https://registry.yarnpkg.com/trim/-/trim-0.0.1.tgz#5858547f6b290757ee95cccc666fb50084c460dd"
integrity sha1-WFhUf2spB1fulczMZm+1AITEYN0=
"trim@https://github.com/davidcui-amzn/trim.git":
version "0.0.1"
resolved "https://github.com/davidcui-amzn/trim.git#279ee1d64575b95f929373232115b43914747d2d"

@davidcui1225
Copy link
Contributor Author

Got it, you might have to put it into resolutions, because now your fork is only added as a dependency, I think nteract/outputs will still use the original trim

trim@0.0.1:
version "0.0.1"
resolved "https://registry.yarnpkg.com/trim/-/trim-0.0.1.tgz#5858547f6b290757ee95cccc666fb50084c460dd"
integrity sha1-WFhUf2spB1fulczMZm+1AITEYN0=
"trim@https://github.com/davidcui-amzn/trim.git":
version "0.0.1"
resolved "https://github.com/davidcui-amzn/trim.git#279ee1d64575b95f929373232115b43914747d2d"

I don't think it does- I tested with yarn kbn bootstrap and within node_modules the only trim package that's there is my forked version

Copy link
Contributor

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

Ok, approved, thanks

@joshuali925
Copy link
Contributor

One second, could you check if you have remark-parse/node_modules/trim in your node_modules? it seems to be the old one and used by nteract

@davidcui1225 davidcui1225 merged commit 8bc323d into dev Jan 14, 2021
@davidcui1225 davidcui1225 deleted the fix-trim-cve branch January 14, 2021 17:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants