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

Upgrade Formik to v2.x to reduce vulnerability #236

Merged
merged 18 commits into from
Jan 26, 2021
Merged

Conversation

ftianli-amzn
Copy link

@ftianli-amzn ftianli-amzn commented Jan 21, 2021

Issue #, if available:

There is a vulnerability CVE-2020-7793 shows that the package ua-parser-js before 0.7.23 are vulnerable to Regular Expression Denial of Service (ReDoS) in multiple regexes.

In Alerting Kibana plugin, the dependency relationship of ua-parser-js is:
formik -> create-react-context -> fbjs -> ua-parser-js

Upgrading Formik is the way to reduce the vulnerability.

Description of changes:

  • Upgrade Formik from 1.1.1 to ^2.2.5 (which is the same as Anomaly Detection Kibana plugin commit, thus the two plugins can be built together in same directory)
  • Change all rejected promise of errors caused by throw to resolved promise of errors with return in validation functions for Monitor and Destination, according to Formik v2 migration guide, or there will be an error like below.
    v1 to v2 validate promise error
  • In ManageEmailGroups.js and ManageSender.js, initialize initialValues variable that used for Formik with a empty object, because initialValues is required in Formik v2 and unit test fails with the variable initialization.
  • Fix unit test failure due to the upgrade:
  1. In MonitorIndex.test.js and AnomalyDetector.test.js, change the assertion step according to the advice here, because Enzyme's wrapper.instance() returns null so that the Formik values in state can't be obtained.
  2. In MonitorIndex.test.js and AnomalyDetector.test.js, modify the logic of the test to wait for asynchronous Formik's handlers, because Enzyme's change event is synchronous. I used the same way as tests related to Email Destination: commit
  3. For the test calls onSearchChange when changing input value in MonitorIndex.test.js, cancel waiting for asynchronous actions as the assertion result is not affected with or without it.
  4. Add an async mark of a test in WhereExpression.test.js to eliminate the following console error:
    Snipaste_2021-01-20_23-52-55 console error
  5. Update jest snapshot files.

Note:
There will be plenty of console.warn in both unit tests and browser according to Formik v2 migration guide.
I will address it in another PR as lots of files are involved.
Snipaste_2021-01-21_11-39-23 console warn render

Test results:
Unit test:

Test Suites: 6 skipped, 83 passed, 83 of 89 total
Tests:       20 skipped, 385 passed, 405 total
Snapshots:   4 updated, 131 passed, 135 total
Time:        45.271 s
Ran all test suites.
✨  Done in 46.40s.

End-to-End test:

       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  alert_spec.js                            02:46        5        5        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  destination_spec.js                      00:33        4        4        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  monitor_spec.js                          00:47        6        6        -        -        - │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✔  All specs passed!                        04:07       15       15        -        -        -  

✨  Done in 286.14s.

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

@ftianli-amzn ftianli-amzn marked this pull request as ready for review January 21, 2021 19:45
@ftianli-amzn ftianli-amzn added dependencies Pull requests that update a dependency file maintenance improves code quality, but not the product labels Jan 22, 2021
Copy link

@yizheliu-amazon yizheliu-amazon left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@dbbaughe dbbaughe left a comment

Choose a reason for hiding this comment

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

👍

@ftianli-amzn ftianli-amzn merged commit 2260715 into opendistro-for-elasticsearch:master Jan 26, 2021
@ftianli-amzn ftianli-amzn deleted the formik2 branch January 26, 2021 05:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file maintenance improves code quality, but not the product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants