-
Notifications
You must be signed in to change notification settings - Fork 147
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
Proposal to provide package safety check #96
Open
viztor
wants to merge
1
commit into
yarnpkg:master
Choose a base branch
from
viztor:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+55
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
- Start Date: 2018-08-15 | ||
- RFC PR: (leave this empty) | ||
- Yarn Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Static safety-check for packages | ||
|
||
# Motivation | ||
|
||
The recent eslint-scope security incident has came as a surprise for many, if not all developers, as very few of us would expect a widely-used package on npm would be `hacked` in such a way. | ||
|
||
Although measures must and have be taken in the publishing process both for eslint and npm team, human error and negligence are in its nature unavoidable; Thereof, for the developers to trust and embrace open source packages, actions must also be taken from the package manager's side, which developer interact everyday to download their packages, to provide an extra safety-net from malicious attackers. | ||
|
||
The most concerning malware are the ones that steals data, thereof, we need to let developers know if the package they install have the ability to access the internet or not, and it should be up to the developers to decide if it is desired. | ||
|
||
# Detailed design | ||
|
||
The design is pretty straightforward: | ||
|
||
Any network access would end up in requiring certain node modules or using certain browser api, therefore, it should not be difficult to do static analyses of those packages and provide basic safety check as for whether those packages requires those modules and/or calls those globals. | ||
|
||
For packages that requires other packages, if any of their required package has `network capacity`, it should be marked as to have `network capacity`. | ||
|
||
User need to manually confirm when they are adding a package with network capacity. | ||
|
||
If the yarn.lock file indicates the package has no `network capacity` and yarn found that the currently pulled one does, it should throw an error. | ||
|
||
In an CI/CD environment, the confirmation can be skipped if a lockfile is provided. | ||
|
||
The result of such analysis can and should be cached on server. // it can and probably should be signed by the server of the package hash, analysis result and version. | ||
|
||
When a publisher tries to publish a package with `network capacity` which previously doesn't, they will be asked to re-authenticate with their credentials, or provide a 2FA code. // not sure if yarn can do this. maybe only npm? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would have to be on the registry side, and would be impossible to detect or prevent without runtime cooperation from both node and the OS. |
||
|
||
# How We Teach This | ||
|
||
As this RFC improves existing API and extra warnings, the change should mostly be unobstrusive. | ||
|
||
# Drawbacks | ||
|
||
It may over-complicate the yarn package. | ||
|
||
Those packages that uses network interface but only provides wrapper function are still considered package that has `network-capacity` and user would still be warned. | ||
|
||
# Alternatives | ||
|
||
The other designs might be to develop or use a separate package to perform static safety check and use that package within yarn to provide such functionality. | ||
|
||
Or such measure could be and/or taken from the registry's side by providing an extra field in its response and relevant hash and signature from trusted server. | ||
|
||
# Unresolved questions | ||
|
||
Should we add extra endpoint to allow manual checking of existing access? | ||
|
||
Perhaps we should let the user decide if they want this feature through config? |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in fact impossible to statically determine anything like that with certainty in JavaScript or in node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. The purpose here isn't to build an anti-virus software, but rather to check what we can detect. The reason I recommend static check at this stage is because the implementation cost would be relatively low, consider other measures taken by Chrome or macOS - Sandbox and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the implementation cost would be that low. Also consider that such a measure would be pointless unless followed by the ecosystem. And the ecosystem would only follow it if it actually worked. So imo, any solution will have to be designed to be the silver bullet and solve all the use cases from the very beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, any "check" against malice is either 100% effective, or it's useless.