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

fix(security): Use latest graphql-voyager package to remove legacy node-fetch dep #19255

Conversation

adamdmharvey
Copy link
Member

@adamdmharvey adamdmharvey commented Aug 7, 2023

Hey, I just made a Pull Request!

Note: Dependency update requires Material UI v5, and without it when patching this, all core-components fail to compile.

The repo identifies it is exposed to a node-fetch high vulnerability due to a very old version. This is through a transitive dependency in the GraphQL Voyager plugin, and the vulnerability which was previously patched was reintroduced when that plugin was added.

The plugin uses an npm package of graphql-voyager which was in an early release candidate mode. So I removed that identification in the yarn lock, and let Yarn pick up a newer version of that dep which includes some bumped transitive deps to patch this issue again.

It seems to remove very old version of MaterialUI Core v3, as well.

Removed -->

-"graphql-voyager@npm:^1.0.0-rc.31":
-  version: 1.0.0-rc.31
-  resolution: "graphql-voyager@npm:1.0.0-rc.31"
-  dependencies:
-    "@f/animate": ^1.0.1
-    "@material-ui/core": ^3.9.3
-    classnames: ^2.2.6
-    clipboard: ^2.0.4
-    commonmark: ^0.29.0
-    lodash: ^4.17.10
-    prop-types: ^15.7.2
-    svg-pan-zoom: ^3.6.0
-    viz.js: 2.1.2
-  peerDependencies:
-    graphql: ">=14.0.0"
-    react: ">=15.4.2"
-    react-dom: ">=15.4.2"
-  checksum: 80e826eeb42d286540447723ea35a463a3a155287914a45e24b69f31617d8ca74c0c43bdaa64123bf4ed046e68f7e31feca7661c06288551040f3e5b96c48fed
-  languageName: node
-  linkType: hard

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

…de-fetch deps

Signed-off-by: Adam Harvey <aharvey00@gmail.com>
@adamdmharvey adamdmharvey added the security Pull requests that address a security vulnerability label Aug 7, 2023
@adamdmharvey
Copy link
Member Author

i'm getting a TSC compilation failure locally for a bunch of stuff in core-components, but can't figure out why. Wonder if this dep was holding on to Material UI Core v3 somehow, and we're using it elsewhere without knowing it? oof..... gonna keep this open to investigate or welcome an eye.

yarn tsc
packages/core-components/src/components/Avatar/Avatar.tsx:27:3 - error TS2345: Argument of type '(theme: Theme) => { avatar: { width: string; height: string; color: string; }; avatarText: { fontWeight: Property.FontWeight | undefined; letterSpacing: string; textTransform: "uppercase"; }; }' is not assignable to parameter of type 'Styles<Theme, {}, "avatar" | "avatarText">'.
  Type '(theme: Theme) => { avatar: { width: string; height: string; color: string; }; avatarText: { fontWeight: Property.FontWeight | undefined; letterSpacing: string; textTransform: "uppercase"; }; }' is not assignable to type 'StyleRulesCallback<Theme, {}, "avatar" | "avatarText">'.
    Call signature return types '{ avatar: { width: string; height: string; color: string; }; avatarText: { fontWeight: FontWeight | undefined; letterSpacing: string; textTransform: "uppercase"; }; }' and 'StyleRules<{}, "avatar" | "avatarText">' are incompatible.
      The types of 'avatarText' are incompatible between these types.
        Type '{ fontWeight: Property.FontWeight | undefined; letterSpacing: string; textTransform: "uppercase"; }' is not assignable to type 'CSSProperties | CreateCSSProperties<{}> | PropsFunc<{}, CreateCSSProperties<{}>>'.
          Type '{ fontWeight: Property.FontWeight | undefined; letterSpacing: string; textTransform: "uppercase"; }' is not assignable to type 'CreateCSSProperties<{}>'.
            Types of property 'fontWeight' are incompatible.
              Type 'FontWeight | undefined' is not assignable to type 'FontWeightProperty | PropsFunc<{}, FontWeightProperty | undefined> | undefined'.
                Type 'string & {}' is not assignable to type 'FontWeightProperty | PropsFunc<{}, FontWeightProperty | undefined> | undefined'.

 27   (theme: Theme) => ({
      ~~~~~~~~~~~~~~~~~~~~
 28     avatar: {
    ~~~~~~~~~~~~~
...
 37     },
    ~~~~~~
 38   }),
    ~~~~

@adamdmharvey
Copy link
Member Author

Oh it also bumps from using MUI3 to MUI5.... but we're on v5 aren't we? hmm

@adamdmharvey
Copy link
Member Author

ah never mind; not fully :)

When the core components are on MUI v5, we can likely then repeat this bump to resolve this package dependency for graphql-voyager which is holding on to this security vulnerability!

Reference: #7094

Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant