Repository navigation
docs: security hub page initial draft - #2522
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Direct link to the deploy preview of the new page: |
|
Off the bat:
or
|
|
|
||
| ## Who Is This For? | ||
|
|
||
| We assume you can already build an Express application and know what an HTTP request is. Beyond that, this page should be useful whether it is your first Express application or your 100th. Advanced developers are not expected to be security experts, and someone brand new to Express may bring plenty of security experience with them. To respect your time, we focus on the risks and mitigations specific to Express rather than teaching fundamentals or going deep on any one topic. |
There was a problem hiding this comment.
A bit wordy tbh, and I dont mention what someone should do if they dont understand this stuffg
There was a problem hiding this comment.
It's important to have a section like this to set expectations. Often, it's titled "What you should already know" but your title is OK, too.
The wording needs to be cleaned up, as it switches person a few times between 2nd "We assume you ..." and 3rd "...developers are not expected ...". Also, I recommend avoiding the use of "we" because although atm it's clear who that is, since this is open-source doc, in a while it will not be! Consider instead:
What you should already know
This security documentation assumes you have a general familiarity with Express and Node.js applications.
It assumes you already know:
- What HTTP requests are and how they work.
- Basics of Node.js development.
- How to build an Express application.
The security documentation focuses on the risks and mitigations specific to Express rather than teaching security fundamentals or going deep on any one topic.
There was a problem hiding this comment.
Taking this section suggestion verbatim, thank you!
Tweaked the sentences where I had said "we" instead of just replacing with "the project" etc.
Those specific changes:
Each question links to a page covering what Express does, what can go wrong, and what we recommend.
to
Each question links to a page covering what Express does, what can go wrong, and what to do about it.
Our Threat Model explains where that boundary sits
to
The Express threat model explains where that boundary sits
For "The Express threat model" I picked that vs "the security wg threat model" or "the express project threat model" to try and communicate it is official policy scoped to Express the framework. Curious if "The Express threat model..." falls into anthropomorphizing in your opinion?
There was a problem hiding this comment.
Curious if "The Express threat model..." falls into anthropomorphizing in your opinion?
No, I think that's OK.
I strongly prefer the first example, where the link is in the wording of the sentence. To my eye, having a "hanging link" at the end, which is not a sentence just looks weird. Alternatively, or perhaps in addition, consider adding a "References" alert (e.g. note) to each section with the links(s). |
|
This is a good start. I don't have time right now for a more comprehensive review, but in addition to my comments above, here are my initial thoughts: I've often heard it said that "security shouldn't be an afterthought, but should be considered up front." By its nature, a checklist is likely to be consulted once an app is at least partially done (which is better than never ;-) ... but would it be feasible to list the most important things to consider BEFORE developing the app and the the things that can be addressed a bit later? Or some other way to align this with the overall development timeline? If not, then we should stress that you should consult this checklist before going into production. One of the (valid IMO) criticisms of the existing security doc was that it was a "grab bag" of considerations--so in general is there a sensible way to order the sections, e.g. by importance/severity or by stage of development: design, implementation, debugging/testing, etc? Perhaps not, but I wanted to ask... All this content should be edited to conform to the Doc style guide. For example, most of the titles are in headline case vs. sentence case. This is easy stuff and IMO can be offloaded to AI, but I can also help if needed. This can be done later once more content is in place and before publishing. |
|
Thank you for the very good feedback folks! I have addressed comments and pushed up changes. For @crandmck, the order is currently my attempt at ordering it based on development phase, and I added back a sentence I had dropped from the intro paragraph: "The questions are ordered roughly by when you meet them while building an application." Keep that thought though, as I think we will be better able to address that feedback further into this process as I discover more about the logical flow/cohesion as I write the rest of the content. Phrase links ✅ Ran a pass w/ claude using the "Doc style guide", it caught some stuff like the Page title not being sentence case, punctuation nits, Node.js vs Node (using Node.js here), full list in commit message. |
ada8264 to
cd1af84
Compare
|
Also I meant to open this against a new branch |
bjohansebas
left a comment
bjohansebas
left a comment
There was a problem hiding this comment.
LGTM! By the way, you can use stacked PRs on GitHub if needed to track progress
That's good. I wonder if we can also group the subsections into higher-level sections, e.g. "Design phase", "Implementation phase", "Testing phase", etc. Definitely something that can come later. |
|
Approved, as it LGTM for the initial PR. |
First implementation PR for the security docs work proposed in [expressjs/discussions#522](expressjs/discussions#522). Adds `/security`: a checklist page of yes/no questions covering the security decisions Express leaves to the application, ordered roughly by when you meet them while building an app. Each item links to a topic page ("spoke") that will explain what Express does, what can go wrong, and what to do. The spoke pages land in follow-up PRs, their links on this page are intentionally dead until then. I expect the links to be anchor based, so we can link someone directly to a heading in a given "spoke" page from the hub. Hopefully that lines up nice, but we will see. Opening as a draft to get the structure and item list reviewed before I write the spokes against it. ## In this PR - `src/content/pages/en/security/index.mdx` (new, global page) - Sidebar entry under Guide (`src/config/menu/docs.ts`) + `menu.items.security` / `menu.aria.security` in `en.json`. Other locales fall back to English until Crowdin picks it up. ## Not in this PR - Spoke pages. P1 spokes (untrusted input, route patterns, response sinks, serving files, proxies, error handling, resource exhaustion) come first. P2 spokes (sessions, CSRF, CORS, logging) come after and may end up pointing at the middleware's own docs instead of a page here. Need to have a cut line somewhere at least for this workstream, hence the P1/P2, open to opinions here. - Removing `/advanced/best-practice-security` and adding its redirect. Deferred until enough spokes exist that the redirect doesn't lose content. - Security Updates page retirement (separate PR, per the discussion). - API reference notes on `res.sendFile`, `res.download`, `res.render`, `express.static`. ## Where the old page's content goes | Section of `/advanced/best-practice-security` | destination | |---|---| | Don't use deprecated or vulnerable versions | Hub item, link to `/support` | | Use TLS | Hub item, outbound links, no spoke (not Express-specific) | | Do not trust user input (incl. open redirect) | Hub items, link to untrusted input/response sinks spokes | | Use Helmet | Hub item, reframed as "sets a bundle of response headers", link to response sinks spoke | | Reduce fingerprinting | Folded into response sinks and error handling items | | Use cookies securely | Hub items → sessions spoke | | Prevent brute-force attacks | Hub item, link to resource exhaustion spoke | | Ensure your dependencies are secure | Hub item, hub-only (npm audit); vendor mentions dropped | | Avoid other known vulnerabilities | Hub item pointing at GitHub Releases and advisories | | Additional considerations | Dropped; threat-model link in the opener covers the pointer | cc @crandmck @bjohansebas
sentence case titles expand on trust proxy info de-anthropomorphize the framework in threat model section add note that the checklist is ordered by development stage
take rand's "What you should already know" section drop first person perspective widen auth item to "authentication and authorization middleware,"
- punctuation inside quotes - spell out acronyms on first use - Node.js, not Node - sentence-case title - "application" consistently - serial comma in heading
cd1af84 to
1bfab44
Compare
4 participants
First implementation PR for the security docs work proposed in expressjs/discussions#522.
Adds
/security: a checklist page of yes/no questions covering the security decisions Express leaves to the application, ordered roughly by when you meet them while building an app. Each item links to a topic page ("spoke") that will explain what Express does, what can go wrong, and what to do.The spoke pages land in follow-up PRs, their links on this page are intentionally dead until then. I expect the links to be anchor based, so we can link someone directly to a heading in a given "spoke" page from the hub. Hopefully that lines up nice, but we will see.
Opening as a draft to get the structure and item list reviewed before I write the spokes against it.
In this PR
src/content/pages/en/security/index.mdx(new, global page)src/config/menu/docs.ts) +menu.items.security/menu.aria.securityinen.json. Other locales fall back to English until Crowdin picks it up.Not in this PR
/advanced/best-practice-securityand adding its redirect. Deferred until enough spokes exist that the redirect doesn't lose content.res.sendFile,res.download,res.render,express.static.Where the old page's content goes
/advanced/best-practice-security/supportcc @crandmck @bjohansebas