feat(security): add XSS sanitization and security headers#1455
feat(security): add XSS sanitization and security headers#1455kelvinkipruto wants to merge 8 commits intomainfrom
Conversation
- Sanitize user-provided embed HTML with xss library - Add Content-Security-Policy and other security headers - Normalize CORS/CSRF origins and set default allowed origin - Update HelplineCard, RowCard, and ActionBanner components to use sanitized embed code
|
It's cool to sanitize @kelvinkipruto if it's not a lot of work. Remember, we're not getting content from end users i.e. the web like reddit. We're rendering content from a CMS managed and reviewed by trusted content editors. |
This comment was marked as resolved.
This comment was marked as resolved.
My question still remains @kelvinkipruto , why go through all of this if the content comes from our CMS which we have full control of? The content doesn't come from the wild web out there. |
The xss library is no longer needed as we rely on CSP headers for security. This removes the dependency, the sanitizeEmbedHtml utility, and updates components to use raw embed code. Also makes HSTS header opt-in via environment variable to support HTTP-only preview deployments.
This comment was marked as resolved.
This comment was marked as resolved.
refactor(security): update CSP comment for clarity chore(deps): remove unused xss dependency and update lockfile
This comment was marked as resolved.
This comment was marked as resolved.
kilemensi
left a comment
There was a problem hiding this comment.
LGTM
May be we should links/description (one line) for each security setting we're doing so that it's easier to know why the settings are the way they are?
| maxWidth={false} | ||
| onClose={onClose} | ||
| open={Boolean(embedCode && open)} | ||
| open={Boolean(hasEmbed && open)} |
There was a problem hiding this comment.
Isn't this used inside HelplineCard and we've already checked embedCode, etc. there?
| `script-src ${scriptSrc.join(" ")}`, | ||
| "style-src 'self' 'unsafe-inline' https:", |
There was a problem hiding this comment.
Aren't script-src and style-src just the same value?
| // we are ready to harden `script-src` further. | ||
| `script-src ${scriptSrc.join(" ")}`, | ||
| "style-src 'self' 'unsafe-inline' https:", | ||
| ].join("; "); |
There was a problem hiding this comment.
Why not just use
`
...;
...;
`on the whole thing instead of
[
'...',
'...',
].join('; ')?
| } | ||
|
|
||
| function isExplicitlyEnabled(value) { | ||
| return ["1", "true", "yes"].includes(value?.trim()?.toLowerCase()); |
There was a problem hiding this comment.
Lets just use true/false... that's what all other env vars use.
| }, | ||
| ]; | ||
|
|
||
| if (nodeEnv === "production" && enableHsts) { |
There was a problem hiding this comment.
Don't you have isProduction already at the top?
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
| const defaultAllowedOrigin = site.url.replace(/\/+$/, ""); |
There was a problem hiding this comment.
| const defaultAllowedOrigin = site.url.replace(/\/+$/, ""); | |
| const serverUrl = normalizeOrigins(site.url); |
- I think serverUrl is much clearer than defaultAllowedOrigin
- Use the created
normalizeOriginto trim trailing/(should it benormalizeornormalise)?
| process.env.PAYLOAD_CSRF?.split(",") | ||
| .map((d) => d.trim()) | ||
| .filter(Boolean) ?? []; | ||
| const cors = normalizeOrigins(process.env.PAYLOAD_CORS); |
There was a problem hiding this comment.
May be:
| const cors = normalizeOrigins(process.env.PAYLOAD_CORS); | |
| const cors = normalizeOrigins(process.env.PAYLOAD_CORS?.trim() || serverUrl); |
So that there is no need to check for if the return array is empty later on?
|
Please update the title / description as we're no longer doing XSS @kelvinkipruto |
Description
Fixes # (issue)
Type of change
Screenshots
Checklist: