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

Maintenance: Upgrades to Storybook, fetch-mock, fetch-mock-jest, config for auditjs #2758

Merged
merged 2 commits into from
Feb 4, 2020

Conversation

kevinrobinson
Copy link
Contributor

Who is this PR for?

developers

What does this PR do?

This started from a low-quality vulnerability alert on a development dependency, but ended in episode 79 of "computers are the worst" :) here's the story:

1. auditjs fails on serialize-javascript 1.4.0.

  • The yarn field "resolutions" in package.json correctly locks serialize-javascript at version 2.12. Looking manually at yarn.lock, we can see that this is what is installed and that the insecure version is never installed.
  • The audit is failing because auditjs is not actually reading the yarn.lock, but instead is parsing package.json and looking up the dependencies in npm. Since "resolutions" is only supported by yarn, this doesn't respect it, and incorrectly flags that we are using a lower vulnerable version of serialize-javascript.
  • We can explicitly tell auditjs to use yarn.lock, which resolves this issue.

2. When we change auditjs to instead audit yarn.lock, auditjs fails on safe-eval 0.4.1.

  • Following the npm advisory is misleading, since it reports versions after 0.4.1 as patched.
  • You have to create a Sonatype login to see its affected versions. This shows that all versions are marked vulnerable (,999.999). Reading through the github issues shows that safe-eval essentially can't provide a full sandbox the way you might think, and the README has been updated to indicate the package is insecure. So the aggressiveness in the Sonatype index is probably reasonable. npm advisory: https://www.npmjs.com/advisories/337 and Sonatype index: https://ossindex.sonatype.org/vuln/a4bde2fe-bf45-4835-9959-ac23b3efaf16, safe-eval README: https://github.com/hacksparrow/safe-eval
  • safe-eval is being used by Storybook, inside the @storybook#addon-actions package.
  • We could whitelist, but auditjs only allows whitelisting the vulnerability altogether, not just that it is occurring inside a specific package (eg, only in dev). This means we wouldn't detect other packages including this (eg, in a dependency actually used in production).
  • Since telejson (which depends on safe-eval) was bumped in Storybook 5.1.0-beta (we're on alpha), let's upgrade to the next stable version, 5.1.10. Doing this for both @storybook/react and storybook/addon-actions removes safe-eval and requires no migrations to Storybook.

3. Storybook 5.1.10 does not build.

  • This appears to be from a conflict with a build of core-js that Storybook is bundling, and a build of core-js that fetch-mock is bundling. See ES5 bundle conflict with core-js v3 wheresrhys/fetch-mock#419.
  • To resolve, we can try to upgrade fetch-mock, which is also tangled up with jest-fetch-mock. Start by upgrading to jest-fetch-mock@^3.0.0, which breaks jest. Then upgrade fetch-mock@^8.2.0. This leads to 7 test failures. Let's fix them.

4. Fix test failures after upgrading fetch-mock and jest-fetch-mock.

  • There's some drift in either fetch-mock or fetch-mock-jest, and some internals are now exposed when asserting against fetchMock.calls(). Added a function to remove these and updated tests to use calls() instead of fetchMock.calls() directly.
    • app/assets/javascripts/helpers/apiFetchJson.test.js
    • app/assets/javascripts/student_profile/Api.test.js
  • Something changed about the Express-style route matching. Updated the mocking to remove the query string altogether. Tests all pass now.
    • app/assets/javascripts/class_lists/ClassListsViewPage.test.js
    • app/assets/javascripts/class_lists/ClassListCreatorPage.js

5. Pop back up the stack.

  • Storybook 5.1.10 now builds.
  • auditjs passes, since safe-eval is no longer bundled.

@kevinrobinson
Copy link
Contributor Author

selfie

@kevinrobinson kevinrobinson merged commit e17daa9 into master Feb 4, 2020
@kevinrobinson kevinrobinson deleted the patch/upgrade-storybook branch February 4, 2020 19:56
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.

1 participant