yeg-relief / youcanbenefit

YouCanBenefit is a web application that increases social benefit program discoverability for people of lesser means and their allies.
https://youcanbenefit.edmonton.ca
MIT License
13 stars 9 forks source link

Upgrade dependencies #125

Closed CodyGramlich closed 5 years ago

CodyGramlich commented 5 years ago

Resolves issue #118.

There were 26 vulnerabilities on the frontend and 8 on the backend. I resolved these except for 1 on the frontend. npm audit on the frontend now gives

                                Manual Review
             Some vulnerabilities require your attention to resolve
          Visit https://go.npm.me/audit-guide for additional guidance
  High            Arbitrary File Overwrite
  Package         tar
  Patched in      >=4.4.2
  Dependency of   @angular-devkit/build-angular [dev]
  Path            @angular-devkit/build-angular > node-sass > node-gyp > tar
  More info       https://nodesecurity.io/advisories/803

It looks like node-sass hasn't fixed the vulnerability yet: https://github.com/sass/node-sass/issues/2625.

I didn't fix the extend vulnerability because the latest version of the parent package of it that we're using, supertest, hasn't been upgraded it yet.

`-- supertest@3.3.0
  `-- superagent@3.8.3
    `-- extend@3.0.1

The latest version of supertest still uses superagent 3.8.3. Also, this vulnerability didn't show up when I ran npm audit.

j-rewerts commented 5 years ago

We have a new one with Axios. Would you be able to get that in?

CodyGramlich commented 5 years ago

npm audit on the frontend reports

found 0 vulnerabilities
 in 42729 scanned packages

On the backend, it reports

found 0 vulnerabilities
 in 863814 scanned packages

We are still waiting on a new release from nestjs to fix the axios vulnerability.

I upgraded nestjs from 5.4.0 to 6.2.4 for now. There was a breaking change with the middleware. I removed the middleware class that we had for CORS, and instead use app.enableCors() in server.ts.

I removed cors@2.8.4 as one of our dependencies as it is a dependency of @nestjs/platform-express, which is a package used for nestjs 6. I actually couldn't install platform-express with cors as one of our dependencies. Here is the migration guide from nestjs 5 to 6 that I used: https://docs.nestjs.com/migration-guide

I removed jest-html-reporter package as it was giving a warning for requiring a peer dependency of a version of jest that we're not using. I tried updating the package to a newer version, but then running tests would output html reports in the working directory. I don't think we need the package, so I removed it.

j-rewerts commented 5 years ago

Great work!

Lets wait on merging this until the fix from Nest comes through.

The CORS thing is tricky. That's the kind of change that doesn't impact you in local dev, but does when you move through your environments. We'll need to stand up a staging site to make sure everything is all good.

CodyGramlich commented 5 years ago

The PR to update axios in nestjs got merged today and they released nestjs 6.3.0. I upgraded nestjs 6.3.0.

Backend unit tests and integration tests pass. My manual end to end testing worked with loading the current production database.

CodyGramlich commented 5 years ago

Right, the CORS thing might be different in production. When I have app.enableCors() and hit localhost:3000, there is Access-Control-Allow-Origin → * in the response header. Without that line, that header isn't there. I just looked up how to check if CORS is enabled and I found that way.

I wasn't sure how update the changes to the middleware the way that we had it, but I found this: https://docs.nestjs.com/techniques/security. Here it shows to add app.enableCors(), which seemed simpler.

j-rewerts commented 5 years ago

LGTM. I'll merge this on the weekend.