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 dependencies #125

Merged
merged 9 commits into from
Jun 11, 2019
Merged

Conversation

CodyGramlich
Copy link
Collaborator

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: sass/node-sass#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
Copy link
Member

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

@CodyGramlich
Copy link
Collaborator Author

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
Copy link
Member

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.

@j-rewerts j-rewerts self-requested a review June 5, 2019 20:59
@CodyGramlich
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Member

LGTM. I'll merge this on the weekend.

@j-rewerts j-rewerts merged commit 39136af into yeg-relief:master Jun 11, 2019
@j-rewerts j-rewerts mentioned this pull request Jun 12, 2019
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.

2 participants