search

naz / swagger-express-validator

Lightweight requrest/response validation middleware based on swagger/OpenAPI schema
MIT License
56 stars 24 forks source link

Dependency update to remove security vulnerabilities #15

Closed stdarg closed 6 years ago

stdarg commented 6 years ago

Hello, I'm interested in using your middleware to validate requests at Eaze. However, I noticed a number of security vulnerabilities by using npm v6.0.1 and snyk v1.80.1. All of the vulnerabilities were able to be removed by updating the dependencies. The vulnerabilities are:

Since you had tests, I updated all the dependencies and ensured the tests ran. However, after updating eslint and its dependencies, I decided to relax the rules for "prefer-destructuring" and "no-buffer-constructor" as I just wanted this commit to be about updating the dependencies. If you are interested, I would submit another PR to turn on "prefer-destructuring" and "no-buffer-constructor," fixing the code issues.

Additionally, I modified the dependencies to use "~" instead of the "^" because sometimes, people introduce breaking changes in minor version changes. Though, I added a lock file to prevent such problems, it's a good practice for people using old versions of NPM.

I hope you find this PR useful as I'd like to work with you on this.

naz commented 6 years ago

hey @stdarg! from the first glance, the changes look good. I've had a look at other repos to check the conventions for including package-lock files and it seems like it's common to not include it in version control, e.g express, myslq. but on the other hand some do e.g. npm. I know the advantages of having it, but would like to do some research around the reason not to have it in the repository. as for another comment around having separate PR to fix eslint problems (Buffer constructor and destructuring), lets keep things small per PR and please create one more just for those fixes. I'll try to take a closer look at the rest during the weekend. cheers and thanks for your contribution!

stdarg commented 6 years ago

Thanks for accepting the PR. I'm happy to provide an additional PR for the code, but consider that prefer-destructuring requires ES6 and, by extension, newer versions of Node. Have you considered what versions of Node.js you are supporting. I'm using v10.0.1, but what are your other users on? Let me know. Additionally, the option no-buffer-constructor moves us to newer versions of Node.js with Buffer.alloc(<size>). You may want to keep the new .eslintrc options for people with older versions of Node.js.

stdarg commented 6 years ago

I'm not sure why many modules are not using package-lock.json, but I suspect many aren't quick to upgrade to newer versions of Node.js and npm. The design of package-lock.json is backward-compatible as it does not interfere with older versions of npm. However, many module developers may be thinking, "I want the top-level app to have a package-lock file and it's the application author's responsibility to test, lock and deploy. If you decide to git rm package-lock.json, I won't mind.

naz commented 6 years ago

Sorry for a long time to merge this :relaxed: Life happened :man_shrugging: Thanks for the contribution!