doc: add Node.js Security Best Practices #4896
Conversation
RafaelGSS
force-pushed
the
guides/nodejs-security-best-practices
branch
from
9a68d3e to
6b4a9fc
Compare
|
This document recommends some experimental features (frozen intrinsics and policy) and doesn't mention that policies are experimental. At a minimum, I think we should mention that policy is experimental. But more broadly, do we want to encourage the use of experimental features in production for security purposes? @nodejs/security |
|
This is a really valuable document which a lot of effort clearly went into. Nice work! |
That's a valid concern. I think we can recommend its usage but mention at the end of the document the expectations when using an experimental feature. Something like:
What do you think? |
RafaelGSS
force-pushed
the
guides/nodejs-security-best-practices
branch
from
6b4a9fc to
8071fa5
Compare
Something like that would work for me. |
|
This is incredible! |
RafaelGSS
requested review from
bmeck,
feross and
UlisesGascon
and removed request for
bmeck and
feross
Co-authored-by: Ulises Gascon <[email protected]> Co-authored-by: Thomas Gentilhomme <[email protected]> Co-authored-by: Facundo Tuesca <[email protected]> Co-authored-by: Michael Dawson <[email protected]> Co-authored-by: Andrew Hart <[email protected]> Co-authored-by: Zbyszek Tenerowicz <[email protected]> Co-authored-by: Yagiz Nizipli <[email protected]>
RafaelGSS
force-pushed
the
guides/nodejs-security-best-practices
branch
from
e74fdc0 to
5c4e35b
Compare
RafaelGSS
requested review from
feross and
Trott
and removed request for
UlisesGascon and
feross
|
@nodejs/documentation |
Ignore the js error because we really need it. For more at https://github.com/eslint/eslint-plugin-markdown#skipping-blocks
| ### Exposure of Sensitive Information to an Unauthorized Actor (CWE-552) | ||
|
|
||
| All the files and folders included in the current directory are pushed to the | ||
| npm registry during the package publication. | ||
|
|
||
| There are some mechanism to control this behavior by defining a blocklist with | ||
| `.npmignore` and `.gitignore` or by defining an allowlist in the `package.json` | ||
|
|
||
| **Mitigations** | ||
|
|
||
| * Using `npm publish --dry-run` list all the files to publish. Ensure to review the | ||
| content before publishing the package. | ||
| * It’s also important to create and maintain ignore files such as `.gitignore` and | ||
| `.npmignore`. | ||
| Throughout these files, you can specify which files/folders should not be published. | ||
| The [files property][] in `package.json` allows the inverse operation | ||
| -- allowed list. | ||
| * In case of an exposure, make sure to [unpublish the package][]. |
There was a problem hiding this comment.
This is fantastic but doesn't seem like a node concern so much as it is an npm concern. (Of course, the line is blurry and this is absolutely something folks should be concerned about.) The broader problem of exposing sensitive information definitely is; would it make sense to either expand this section to talk about these class of bugs or rename the section to be about publishing code?
There was a problem hiding this comment.
I think these bullets do not necessarily need to be separated or in a unique category as mitigations could exist as a solution in multiple vectors with the same action. This already points out that it is about "the current directory are pushed to the npm registry during the package publication." so idk if there needs to be a full expansion on this. Discussing every attack vector isn't really the purpose of a best practices doc normally.
There was a problem hiding this comment.
I wasn't suggesting being exhaustive. I was trying to say there is a disconnect between the title (which is about a broad category of attacks) and the content (which is narrow and about a particular npm workflow). I think just adding a sentence at the start (I can just suggest something when back on the computer) would bride the gap.
There was a problem hiding this comment.
I'm not quite sure if I get your point. The suggestion to expand it to other registries is definitely a thing, but I'd include it in a second PR.
| Like all runtimes, Node.js is vulnerable to these attacks if your projects runs | ||
| on a shared machine. |
There was a problem hiding this comment.
I'm not sure why the shared machine is relevant here? Do you mean because Node.js uses libc and the libc could be vulnerable?
There was a problem hiding this comment.
No, it's because when sharing resources with other processes, the memory can be violated.
|
I left some notes, but of course feel free to ignore! :-) Really cool to see this! |
Co-authored-by: Deian Stefan <[email protected]>
|
Anybody from @nodejs/nodejs-tr want to translate this document? |
Reference nodejs/security-wg#819.
One of Security WG's initiatives is to create a best practices document for end-users. We've been actively working on that and finally, we have something to share.
This document intends to extend the current threat model(under development) and provide extensive guidelines on how to secure a Node.js application. The target audience is Node.js users/developers.
I think is important to have @nodejs/tsc eyes here as well.
@nodejs/security-wg Thanks to everyone who helped in that journey (I'll include all of you as Co-Author soon as I get your handles)
Co-authored-by: Ulises Gascon [email protected]
Co-authored-by: Thomas Gentilhomme [email protected]
Co-authored-by: Facundo Tuesca [email protected]
Co-authored-by: Michael Dawson [email protected]
Co-authored-by: Andrew Hart [email protected]
Co-authored-by: Zbyszek Tenerowicz [email protected]
Co-authored-by: Yagiz Nizipli [email protected]