docs: v1 secure coding guidelines #58
Conversation
Gudahtt
changed the title
docs/secure-coding-guidelines.md
Outdated
| 4. Reduce Dependencies and Keep Them Minimal: | ||
| 1. Minimize dependencies to reduce potential vulnerabilities | ||
| 5. Audit and Monitor Dependencies: | ||
| 1. Projects should use the npm audit, Dependabot and Socket.dev tooling for periodic automated auditing of dependencies |
There was a problem hiding this comment.
| 1. Projects should use the npm audit, Dependabot and Socket.dev tooling for periodic automated auditing of dependencies | |
| 1. Projects should use tools (npm audit, Dependabot and Socket.dev) for periodic automated auditing of dependencies | |
| 2. Use npm audit locally as a tool that informs you of the backlog of issues you have. Use dependabot security alerts to react to new vulnerability reports in your dependencies. | |
| - Triage dependabot PRs early, merge quickly if updates are minor. Don't hesitate to close them if not useful or could never be applied - but leave a comment why. Call attention to PRs where a security update is a major version update and cannot be easily introduced. | |
| 3. If vulnerability management is particularly important for the project, run audit as a CI step and fail if it finds new issues. This may be prone to false positives and be blocked by transitive dependencies not being updated upstream. Use `resolutions` to override or `npm-audit-resolver` package to build a process around safely ignoring selected issues (requires discipline) | |
| 4. Socket.dev will automatically report on suspicious dependencies in your pull requests. For best results use the following etiquette when dealing with it: | |
| - avoid merging the PR without addressing Socket warnings | |
| - avoid the ignore-all command unless you're introducing socket to a new project that's known to be safe (very rare) | |
| - always add a short comment explaining why you're ignoring a warning of potential malice/danger. eg. for a publisher change 'known maintainer' is enough of a comment indicating that you've verified the new publisher of the package is legitimate. | |
| - if Socket is being annoying, remember i can be configured. contact @naugtur (Zbyszek) and report if a warning category is useless. |
There was a problem hiding this comment.
Thanks, great suggestions! I haven't taken this exactly as-is, but I've tried to capture the spirit of it here: 43e5282
It's been condensed a bit, and the Dependabot part is omitted. I omitted Dependabot because I tend not to think of it as a security tool; it's quite slow and unreliable, sometimes taking many days to create a PR to address vulnerabilities, and it will silently fail to address them sometimes. It's also often not configured or not configured correctly in our organization. I left a "Update dependencies quickly" bullet there to nudge people to follow your advice about responding quickly though.
There was a problem hiding this comment.
the dependabot part was based on actual problems we've recently had. dependabot is not a tool helping to react fast. It's a reminder and it does some of the work for you creating the PR. It's a tool helping not to stay many versions behind on stuff. And we've had dependabot PRs for security updates sit there for over a year.
There was a problem hiding this comment.
always add a short comment explaining why you're ignoring a warning of potential malice/danger. eg. for a publisher change 'known maintainer' is enough of a comment indicating that you've verified the new publisher of the package is legitimate.
became
If you've investigated a warning and found that it's not indicative of a malicious dependency, ignore it with a bot comment and explain your investigation with a short comment
The latter is IMHO too vague to be memorable and I'm afraid people won't internalize what it means and either ignore the recommendation or put too much effort into creating writeups leading to annoyance.
This is my main issue with this doc - it's too terse and open to interpretation and could use much more examples.
There was a problem hiding this comment.
Point taken on the wording, I'll restore something closer to yours. Can't recall the thinking behind that right now.
For the most part the terseness of the document isn't intended, we just haven't had time to expand on most points yet. More detail would definitely be appreciated.
There was a problem hiding this comment.
I appreciate that dependabot has been serving an important role absent other reminders about security problems, but I think there are good reasons to prefer alternatives. We haven't rolled out an npm audit CI job yet in libraries, but it's a far more reliable solution. Dependabot on the other hand is extremely slow, frequently fails to update things correctly, and will silently give up if it doesn't find a way to update something. It's not a reliable way to flag security advisories. I intended to add an npm audit CI job to the module template to serve that purpose.
There was a problem hiding this comment.
Actually no I take that back, maybe we should mention Dependabot anyway despite its flaws. It's still a helpful tool, even just as a backup.
Also, the GitHub alerts about security advisories are great for visibility too. I hadn't realized that those are called "Dependabot alerts" now, they used to be separate from that system.
There was a problem hiding this comment.
Dependabot is only useful together with the alerts, yes
There was a problem hiding this comment.
The example Socket.dev resolution was restored here, along with a second example: #85
I started restoring the Dependabot advice in this PR but didn't get far: #86
It felt wrong to include advice on triaging Dependabot issues given that it's not used in many repos at the moment, and even in those where it is enabled it's often not setup correctly (such that the Dependabot PRs typically don't pass CI checks).
docs/secure-coding-guidelines.md
Outdated
| 1. Ensure dependencies' licenses align with your project's requirements | ||
| 4. Reduce Dependencies and Keep Them Minimal: | ||
| 1. Minimize dependencies to reduce potential vulnerabilities | ||
| 5. Audit and Monitor Dependencies: |
There was a problem hiding this comment.
| 5. Audit and Monitor Dependencies: | |
| 5. Audit and Monitor Dependencies: | |
| 0. Discuss an establish a process for maintaining your dependencies and dealing with old dependencies you get stuck on because of breaking changes. |
There was a problem hiding this comment.
Hmm, this is a curious one. I'm not aware of any teams that have an explicit process like this, so I'm hesitant to suggest we all should have one. Would like to discuss this further though.
There was a problem hiding this comment.
This was the most important point I wanted to make here. Insert a mandate to start the conversations across teams. I will push for this via various means, but I was hoping to get it in front of everyone and this first edition of the training was a great opportunity.
There was a problem hiding this comment.
Part of the reason that I'm having trouble with this is that I don't fully understand the suggestion. Specifically it's the condition you think teams should have a plan for, this "stuck on old dependencies" situation. Most often the only obstacle to updating dependencies is a little investment in time, so it's rare that we enter a situation where we're "stuck".
Did you have any examples in mind of a situation where we'd benefit from having a process like this, and what that process might look like?
Gudahtt
force-pushed
the
chore/secure-coding-guidelines
branch
2 times, most recently
from
6168835
to
c485e75
Compare
Gudahtt
force-pushed
the
chore/secure-coding-guidelines
branch
from
c485e75
to
4afebca
Compare
Gudahtt
force-pushed
the
chore/secure-coding-guidelines
branch
3 times, most recently
from
5ecd2e4
to
8153531
Compare
Gudahtt
force-pushed
the
chore/secure-coding-guidelines
branch
from
8153531
to
2154c1d
Compare
The two validation guidelines have been combined. The same advice applies to user inputted data and data from third-parties, even down to the eaxmples. Two of the examples have been combined as well, and the wording of the guideline and examples has been simplified.
It seemed like this line was suggesting validation libraries over custom validation, but I'm not completely sure that's what it meant. That doesn't seem like appropriate advice for all contexts anyway, so it has been removed for now.
I'm not sure what this guideline was intended to communicate. We don't typically manage access to "external data".
I think this is covered by the guideline about making tokens publicly accessible. If we're OK with a token being publicly accessible, presumably it should have no privileges that we care about
The guideline about third party systems having audit logs has been removed. It needs more detail about what needs to be logged in order to be useful, and typically we don't have a whole lot of systems to choose from anyway, so we might have little choice in the matter.
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: sethkfman <[email protected]>
| #### LavaMoat (JavaScript projects only) | ||
|
|
||
| - LavaMoat `allow-scripts` should be enabled on all projects | ||
| - This project relies upon install scripts being disabled in your package manager. This can be verified by adding the dependency `@lavamoat/preinstall-always-fail`, which will cause installation to fail if scripts are enabled. |
There was a problem hiding this comment.
allow-scripts setup adds that dependency. Also, yarn 3 and 4 will not fail the entire install process because they swallow errors.
There was a problem hiding this comment.
Ah I see, I hadn't realized that about Yarn v3/v4. I guess your point here is that it'd be safer to advise using allow-scripts setup because users might fail to properly disable install scripts in the package manager otherwise, is that correct? Just wanted to confirm that setting ignore-scripts is also something handled by the setup script.
An example has been added to our "Data Validation and Sanitization" guideline about the risk of dynamic behavior on objects from external sources. This was originally suggested here: #58 (comment) The suggest has been reformatted/reworded here in this PR to match the context.
An example has been added to our "Data Validation and Sanitization" guideline about the risk of dynamic behavior on objects from external sources. This was originally suggested here: #58 (comment) The suggest has been reformatted/reworded here in this PR to match the context.
Two examples have been added that explain in further detail how to properly document that you've reviewed a Socket.dev warning. This is an improvement suggested in this comment: #58 (comment)
These examples were suggested in this comment: #58 (comment) I was going to include further advice on how to use Dependabot, but I am not yet sure what advice to offer here because we don't yet have a Dependabot workflow that we're happy with. It's currently disabled in some places, and it's configured ineffectively in others. We can revisit this later.
An example has been added to our "Data Validation and Sanitization" guideline about the risk of dynamic behavior on objects from external sources. This was originally suggested here: #58 (comment) The suggest has been reformatted/reworded here in this PR to match the context.
These examples were suggested in this comment: #58 (comment) I was going to include further advice on how to use Dependabot, but I am not yet sure what advice to offer here because we don't yet have a Dependabot workflow that we're happy with. It's currently disabled in some places, and it's configured ineffectively in others. We can revisit this later.
Two examples have been added that explain in further detail how to properly document that you've reviewed a Socket.dev warning. This is an improvement suggested in this comment: #58 (comment)
This PR adds secure coding guidelines base on concepts from OWASP Top Ten and modified to the specific at MetaMask when developing client application and libraries.
Link to formatted version