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

[CVE-2022-25758] Use dart-sass instead of node-sass #2054

Merged
merged 4 commits into from
Aug 12, 2022
Merged

[CVE-2022-25758] Use dart-sass instead of node-sass #2054

merged 4 commits into from
Aug 12, 2022

Conversation

Flyingliuhub
Copy link
Member

@Flyingliuhub Flyingliuhub commented Aug 2, 2022

Signed-off-by: Tao liu liutaoaz@amazon.com

Description

This PR fixes the Regular expression denial of service in scss-tokenizer, use dart-sass instead of node-sass.

The node-sass are deprecated, the detail here.

The suggested solution (#535) is that use dart-sass instead of node-sass

The scan detail as following and link here

yarn why scss-tokenizer
yarn why v1.22.18
[1/4] Why do we have the module "scss-tokenizer"...?
[2/4] Initialising dependency graph...
warning Resolution field "typescript@4.0.2" is incompatible with requested version "typescript@~4.5.2"
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "scss-tokenizer@0.2.3"
info Reasons this module exists
   - "_project_#@osd#ui-framework#node-sass#sass-graph" depends on it
   - Hoisted from "_project_#@osd#ui-framework#node-sass#sass-graph#scss-tokenizer"
info Disk size without dependencies: "268KB"
info Disk size with unique dependencies: "1.11MB"
info Disk size with transitive dependencies: "1.11MB"
info Number of shared dependencies: 2
Done in 1.01s.

Issues Resolved

#1842 , #535

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Signed-off-by: Tao liu <liutaoaz@amazon.com>
@Flyingliuhub Flyingliuhub requested a review from a team as a code owner August 2, 2022 20:47
@Flyingliuhub Flyingliuhub changed the title [Security] Use dart-sass instead of node-sass [CVE] Use dart-sass instead of node-sass Aug 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2022

Codecov Report

Merging #2054 (cfd6aa4) into main (76e0f20) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2054   +/-   ##
=======================================
  Coverage   67.50%   67.51%           
=======================================
  Files        3077     3077           
  Lines       59184    59188    +4     
  Branches     9003     9003           
=======================================
+ Hits        39955    39958    +3     
- Misses      17044    17045    +1     
  Partials     2185     2185           
Impacted Files Coverage Δ
...ic/application/models/sense_editor/sense_editor.ts 64.00% <0.00%> (-0.89%) ⬇️
...plugins/maps_legacy/public/map/service_settings.js 1.60% <0.00%> (-0.03%) ⬇️
.../maps_legacy/public/map/base_maps_visualization.js 1.94% <0.00%> (-0.02%) ⬇️
src/plugins/vis_type_vega/public/services.ts 100.00% <0.00%> (ø)
packages/osd-optimizer/src/node/cache.ts 52.77% <0.00%> (+2.77%) ⬆️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 87.75% <0.00%> (+4.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kavilla
Copy link
Member

kavilla commented Aug 2, 2022

looks like a snapshot failure, could update them?

@kavilla kavilla added the cve Security vulnerabilities detected by Dependabot or Mend label Aug 2, 2022
@kavilla
Copy link
Member

kavilla commented Aug 2, 2022

Do we see the issue called out here: #535 with EUI complaining?

I know we bumped EUI so it might have unblocked #535.

Signed-off-by: Tao liu <liutaoaz@amazon.com>
@Flyingliuhub
Copy link
Member Author

EUI complaining?

According the error in the #535, it sounds like that happened on during the build compile, I didn't see the build compile error and UI error.

@Flyingliuhub
Copy link
Member Author

looks like a snapshot failure, could update them?

Thanks. I updated the test, it should fixes the tests.

seraphjiang
seraphjiang previously approved these changes Aug 3, 2022
yarn.lock Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@Flyingliuhub Flyingliuhub changed the title [CVE] Use dart-sass instead of node-sass [CVE-2022-25758] Use dart-sass instead of node-sass Aug 3, 2022
yarn.lock Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@kavilla
Copy link
Member

kavilla commented Aug 3, 2022

Awesome! Do you know if there is any performance updates with the rendering? Also, will OUI be impacted by this @AMoo-Miki ? Finally, I haven't checked by do we use the sass render or renderSync?

Signed-off-by: Tao liu <liutaoaz@amazon.com>
Signed-off-by: Tao liu <liutaoaz@amazon.com>
@Flyingliuhub
Copy link
Member Author

@AMoo-Miki , Is this changes looking good from your side? if you have time, please take a look this changes, thanks

@kavilla
Copy link
Member

kavilla commented Aug 4, 2022

Hello @Flyingliuhub, are we able to eyeball any performance impact?

@Flyingliuhub
Copy link
Member Author

Hello @Flyingliuhub, are we able to eyeball any performance impact?

I didn't see the page load (dashboard/discover/Visualizations) different in my local.

It take about 12 minutes for build locally with following command

yarn build-platform linux --skip-os-packages --skip-archives --release

image

@kavilla
Copy link
Member

kavilla commented Aug 5, 2022

What are possible risk and impact to get this merged? Could we list and discuss here.

Is it a check list we could follow to self check besides automation test

Biggest worry was OUI, but Miki is good with that. Downstream might be impacted so it will be a breaking change for 3.0. The only other thing is if there is any performance degradation. But we don't have any client side perf tests so looks good to me. However, will hold off from merging until at least tomorrow just in case in becomes a blocker for OUI.

@seraphjiang
Copy link
Member

Any update team ~
@Flyingliuhub @kavilla @AMoo-Miki

@Flyingliuhub
Copy link
Member Author

Any update team ~ @Flyingliuhub @kavilla @AMoo-Miki

@kavilla , Is there any update from retro? if everything looks good from retro meeting, could you please click "Merge"? Thanks

@kavilla kavilla merged commit bb67c71 into opensearch-project:main Aug 12, 2022
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Bump node.js to 18 and fix errors

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 15, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 15, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 15, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 21, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Fix async unit test timeout issue

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Nodejs 18] fix lmdb and plugins discovery unit tests

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Fix windows path

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Increase memory limit for unit test and fix memory leak

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Apr 18, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

add unhandle-rejections

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

add worker

add mock lmdb to integration test

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

modify test start opensearch

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

only one integration test

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

update test

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

increase time

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 18, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cve Security vulnerabilities detected by Dependabot or Mend v3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CVE-2022-25758 (High) detected in scss-tokenizer-0.2.3.tgz - autoclosed Migrate from Node Sass to Dart Sass
6 participants