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

Upgrade vulnerable MarkBind dependencies #788

Merged
merged 3 commits into from Mar 25, 2019
Merged

Upgrade vulnerable MarkBind dependencies #788

merged 3 commits into from Mar 25, 2019

Conversation

Xenonym
Copy link
Contributor

@Xenonym Xenonym commented Mar 21, 2019

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Other, please explain: upgrading dependencies

Part of #461.

What is the rationale for this request?
npm audit (avaliable from npm >= 6.0.0) returns 434 vulnerabilities in our dependencies. We should upgrade vulnerable packages to remove these vulnerabilities.

Full results here.

What changes did you make? (Give an overview)

  • I upgraded the following vulnerable packages, categoried by their function in MarkBind:
    • Live preview
      • chokidar: 1.6.1 → 2.1.5
      • live-server: 1.2.0 → 1.2.1
    • GitHub Pages deployment
      • gh-pages: 1.1.0 → 2.0.1
    • Building pages
      • fastmatter: 2.0.1 → 2.1.0
      • nunjucks: 3.0.0 → 3.2.0
      • markdown-it-table-of-contents: 0.3.2 → 0.4.3
    • Development tools
      • eslint: 4.16.0 → 5.15.3
      • jest: 22.4.3 → 24.5.0
      • lodash: 4.17.5 → 4.17.11
  • I also upgraded the eslint config packages, as there are rule specifications that have changed/deprecated between the eslint versions:
    • eslint-config-airbnb-base: 12.1.0 → 13.1.0
    • eslint-plugin-import: 2.8.0 → 2.16.0
    • eslint-plugin-lodash: 2.6.1 → 5.1.0
  • I updated .eslintrc.js to accommodate our current coding style and disable no-else-return and implicit-arrow-linebreak as we do not currently follow these rules.
  • I changed the import order in some JS files to match the sort-import rule (package dependencies before local dependencies).

Is there anything you'd like reviewers to focus on?

  • Is the current .eslintrc.js suitable now that we have updated our configs?

Testing instructions:

  1. Ensure that npm audit returns 0 vulnerabilities.
  2. Ensure that existing functionality under the four areas affected (live preview, GitHub Pages deployment, page building, other dev tools) function as expected.

@yamgent
Copy link
Member

yamgent commented Mar 23, 2019

Did you test it under any big websites yet like se-book or cs2103t's website to see if anything's broken?

@yamgent
Copy link
Member

yamgent commented Mar 23, 2019

Also npm audit now shows 1 vulnerability, which is a recent report: https://www.npmjs.com/advisories/788.

> npm audit
                                                                                
                       === npm audit security report ===                        
                                                                                


# Run `npm update js-yaml --depth 2` to resolve 1 vulnerability
                                                                                
  moderate        Denial of Service                                             
                                                                                
  Package         js-yaml                                                       
                                                                                
  Dependency of   fastmatter                                                    
                                                                                
  Path            fastmatter > js-yaml                                          
                                                                                
  More info       https://nodesecurity.io/advisories/788                        
                                                                                

[!] 1 vulnerability found - Packages audited: 483686 (476942 dev, 301 optional)
    Severity: 1 moderate

* chokidar: 1.2.1 -> 2.1.5
* live-server: 1.2.0 -> 1.2.1
* gh-pages: 1.1.0 -> 2.0.1
* fastmatter: 2.0.1 -> 2.1.0
* nunjucks: 3.0.0 -> 3.2.0
* markdown-it-table-of-contents: 0.3.2 -> 0.4.3
* eslint: 4.16.0 -> 5.15.3
* jest: 22.4.3 -> 24.5.0
* lodash: 4.17.5 -> 4.17.11

Required upgrades to eslint configs:
* eslint-config-airbnb-base: 12.1.0 -> 13.1.0
* eslint-plugin-import: 2.8.0 -> 2.16.0
* eslint-plugin-lodash: 2.6.1 -> 5.1.0
@Xenonym
Copy link
Contributor Author

Xenonym commented Mar 23, 2019

Did you test it under any big websites yet like se-book or cs2103t's website to see if anything's broken?

@yamgent yes, se-edu/se-book and nus-cs2103/website-base should still work.

Also npm audit now shows 1 vulnerability, which is a recent report: npmjs.com/advisories/788.

Thanks for the catch! fastmatter has been updated to 2.1.0 in this PR.

@yamgent yamgent added this to the v1.21.1 milestone Mar 24, 2019
@yamgent yamgent merged commit 60adab6 into MarkBind:master Mar 25, 2019
@Xenonym Xenonym deleted the fix/upgrade-vuln-deps branch March 25, 2019 04:06
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.

None yet

2 participants