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

npm audit critical dependencies #1187

Closed
brianpmccullough opened this issue Apr 14, 2022 · 3 comments · Fixed by #1205
Closed

npm audit critical dependencies #1187

brianpmccullough opened this issue Apr 14, 2022 · 3 comments · Fixed by #1205
Labels
status:fixed-next-drop Issue will be fixed in upcoming release.
Milestone

Comments

@brianpmccullough
Copy link
Contributor

brianpmccullough commented Apr 14, 2022

Thank you for reporting an issue, suggesting an enhancement, or asking a question. We appreciate your feedback - to help the team understand your
needs please complete the below template to ensure we have the details to help. Thanks!

Please check out the documentation to see if your question is already addressed there. This will help us ensure our documentation is up to date.

Category

[X] Enhancement

[X] Bug

[ ] Question

Version

Please specify what version of the library you are using: [ 3.7.2 ]

Expected / Desired Behavior / Question

When performing npm audit on our SPFx projects with dependency on this library and in this repo, we now get 4 critical vulnerabilities (consider the Critical vulnerabilities from a security concious enterprise). They appear to stem from the use of spfx-uifabric-themes > node-sass > meow > minimist and are a result of the Prototype Pollution in minimist vulnerability - which as I see is pretty new.

Given that spfx-uifabric-themes appears to be pre-release given it's version, and also that it hasn't received an update in about 2 years, is it appropriate to re-evaluate the use of this dependency in this project?

When I remove the dependency from this solution and perform a build, it looks like it is used in only a few spots (listed below).

Is it a good time to review the use of spfx-uifabric-themes and see if there is another spfx provided way of achieving what this library had been providing? Maybe there is better support for this now natively in SPFx with the recent changes for Theme support? Or perhaps check in with Stefan to see if there is anything that can be done within his project to prevent the vulnerabilities - such as re-evaluating the use of node-sass which also now appears to be deprecated in favor of "Dart Sass"? I am open to discussing in most appropriate repo (Stefan or here) and willing to help in whatever way I can.

I am aware we can npm audit fix on our end, and see what sort of results we get as a result. If this is a good approach, I am open, but that feels like it's only possibly fixing it for my particular situation, rather than for the masses. Also sounds like that's a band-aid approach to something that should be addressed in the library - though I am new(ish) to how Open Source works in these situations?

[16:25:31] Error - [tsc] src/controls/listItemComments/common/IAppContext.ts(1,23): error TS2307: Cannot find module 'spfx-uifabric-themes' or its corresponding type declarations.
[16:25:31] Error - [tsc] src/controls/listItemComments/ListItemComments.tsx(5,23): error TS2307: Cannot find module 'spfx-uifabric-themes' or its corresponding type declarations.
[16:25:31] Error - [tsc] src/controls/listItemComments/ListItemComments.tsx(17,22): error TS2339: Property 'themeState' does not exist on type 'Window & typeof globalThis'.
[16:25:31] Error - [tsc] src/controls/MyTeams/MyTeamsStyles.ts(9,29): error TS2339: Property 'themeState' does not exist on type 'Window & typeof globalThis'.
[16:25:31] Error - [tsc] src/controls/TeamChannelPicker/TeamChannelPicker.tsx(25,22): error TS2339: Property 'themeState' does not exist on type 'Window & typeof globalThis'.
[16:25:31] Error - [tsc] src/controls/TeamChannelPicker/TeamChannelPickerStyles.ts(3,23): error TS2307: Cannot find module 'spfx-uifabric-themes' or its corresponding type declarations.
[16:25:31] Error - [tsc] src/controls/TeamChannelPicker/TeamChannelPickerStyles.ts(12,22): error TS2339: Property 'themeState' does not exist on type 'Window & typeof globalThis'.
[16:25:31] Error - [tsc] src/controls/TeamPicker/TeamPickerStyles.ts(9,23): error TS2307: Cannot find module 'spfx-uifabric-themes' or its corresponding type declarations.
[16:25:31] Error - [tsc] src/controls/TeamPicker/TeamPickerStyles.ts(12,22): error TS2339: Property 'themeState' does not exist on type 'Window & typeof globalThis'.

Observed Behavior

npm audit --audit-level=critical --production

 Run  npm update minimist --depth 7  to resolve 4 vulnerabilities

  Critical        Prototype Pollution in minimist                               

  Package         minimist                                                      

  Dependency of   spfx-uifabric-themes                                          

  Path            spfx-uifabric-themes > node-sass > meow > minimist            

  More info       https://github.com/advisories/GHSA-xvch-5gv4-984h             


  Critical        Prototype Pollution in minimist                               

  Package         minimist                                                      

  Dependency of   spfx-uifabric-themes                                          

  Path            spfx-uifabric-themes > node-sass > node-gyp > mkdirp >        
                  minimist                                                      

  More info       https://github.com/advisories/GHSA-xvch-5gv4-984h             


  Critical        Prototype Pollution in minimist                               

  Package         minimist                                                      

  Dependency of   spfx-uifabric-themes                                          

  Path            spfx-uifabric-themes > node-sass > node-gyp > fstream >       
                  mkdirp > minimist                                             

  More info       https://github.com/advisories/GHSA-xvch-5gv4-984h             


  Critical        Prototype Pollution in minimist                               

  Package         minimist                                                      

  Dependency of   spfx-uifabric-themes                                          

  Path            spfx-uifabric-themes > node-sass > node-gyp > tar > fstream   
                  > mkdirp > minimist                                           

  More info       https://github.com/advisories/GHSA-xvch-5gv4-984h           

Steps to Reproduce

cd sp-dev-fx-controls-react
npm audit --audit-level=critical --production
@ghost
Copy link

ghost commented Apr 14, 2022

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

@ghost ghost added the Needs: Triage 🔍 label Apr 14, 2022
@github-actions
Copy link

Thank you for submitting your first issue to this project.

brianpmccullough added a commit to brianpmccullough/sp-dev-fx-controls-react that referenced this issue Apr 15, 2022
Fix pnp#1187 by bumping "spfx-uifabric-themes" to "^0.9.0".  "spfx-uifabric-themes" @ "^0.9.0" removes dependencies that are not needed and that were causing npm audit with critical failure for GHSA-xvch-5gv4-984h.
AJIXuMuK added a commit that referenced this issue Apr 19, 2022
"spfx-uifabric-themes" to "^0.9.0" to fix #1187
@AJIXuMuK AJIXuMuK added status:fixed-next-drop Issue will be fixed in upcoming release. and removed Needs: Triage 🔍 labels Apr 19, 2022
@AJIXuMuK AJIXuMuK added this to the 3.8.0 milestone Apr 19, 2022
@AJIXuMuK
Copy link
Collaborator

Thank you @brianpmccullough for reporting and fixing the issue.
The PR has been merged and will be included in the next release.
You can also test it in beta version right away.

AJIXuMuK pushed a commit that referenced this issue Apr 19, 2022
Fix #1187 by bumping "spfx-uifabric-themes" to "^0.9.0".  "spfx-uifabric-themes" @ "^0.9.0" removes dependencies that are not needed and that were causing npm audit with critical failure for GHSA-xvch-5gv4-984h.
@AJIXuMuK AJIXuMuK mentioned this issue May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:fixed-next-drop Issue will be fixed in upcoming release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants