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

Add note about why installing via npm is not recommended #291

Merged
merged 3 commits into from
Dec 4, 2016

Conversation

Daniel15
Copy link
Member

I tried to be as neutral as possible while writing this, let me know if it doesn't make sense.

are not signed and npm does not perform any integrity checks, which is a
security risk when installing system-wide apps. Additionally, in an environment
with a system-wide package manager such as most Linux distributions, apps
installed via npm are not updated when you update the rest of your system.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only really convincing bit here is the security check, I would simplify it to just that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true? Fairly certain NPM DOES do shasum validation - https://github.com/npm/npm/blob/latest/lib/cache/add-remote-tarball.js#L86

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hash verification is not the same. It doesn't verify who uploaded the
package or that it's actually a legitimate release, it's just a hash of the
contents. Additionally, npm uses SHA1, which is not considered secure.

@Daniel15
Copy link
Member Author

I should probably also mention the fact that dependency versions are locked down with all other installation methods, so things like yarnpkg/yarn#2072 are avoided.

@wjordan
Copy link

wjordan commented Dec 3, 2016

I should probably also mention the fact that dependency versions are locked down with all other installation methods, so things like yarnpkg/yarn#2072 are avoided.

@Daniel15 floating dependency versions are not required in package.json, it's just an npm install default. You really should lock dependency versions in package.json, if its important to keep yarn's correct behavior strictly guaranteed across all installation methods. To lock down dependency versions, just update the package.json to specify exact versions of your project's dependencies (e.g., "node-emoji": "1.4.3") rather than ranged versions ("node-emoji": "^1.0.4"), then adjust dependency versions in a controlled manner if/when needed (e.g., using npm outdated).

@Daniel15
Copy link
Member Author

Daniel15 commented Dec 3, 2016

You really should lock dependency versions in package.json

That won't lock transitive dependencies. Even if we depend on an exact version of package Foo (say, version 1.2.3), Foo could depend on ^1.0.0 of some other package (or even *, meaning any version).

Locking (including transitive dependencies) can be done with npm shrinkwrap, but apparently it doesn't handle optional dependencies correctly

@palmerj3
Copy link

palmerj3 commented Dec 4, 2016

@Daniel15 you should be able to do npm shrinkwrap --no-optionals to make a shrinkwrap that doesn't contain optional dependencies.

@Daniel15
Copy link
Member Author

Daniel15 commented Dec 4, 2016 via email

@palmerj3
Copy link

palmerj3 commented Dec 4, 2016

Optional deps, by definition, shouldn't be required in order for a project to function :) If they are then that's a much bigger problem.

@jamiebuilds
Copy link
Contributor

Yeah, but there's a difference in them not being there and them being there and being broken.

@Daniel15
Copy link
Member Author

Daniel15 commented Dec 4, 2016

@thejameskyle - hopefully it's good now, let me know what you think 😃

@jamiebuilds jamiebuilds merged commit f7f1af5 into yarnpkg:master Dec 4, 2016
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.

4 participants