Skip to content

move away from express helmet #927

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 19, 2025
Merged

move away from express helmet #927

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d5069e0
move away from express helmet
nichtsam Feb 11, 2025
43b7b3b
Merge branch 'main' into pr/helmet
kentcdodds Feb 14, 2025
207ea9e
simplify helmet import
nichtsam Feb 14, 2025
a9e09a3
turn off referrerPolicy security header
nichtsam Feb 18, 2025
db540cb
Update server/index.ts
kentcdodds Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion app/entry.server.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { PassThrough } from 'node:stream'
import { contentSecurity } from '@nichtsam/helmet/rules/content/index'
Copy link
Member

@kentcdodds kentcdodds Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nitpick, but is there any chance you could add exports config in your package.json so that we don't have to dive deep into your package? Instead, this could be more like this for example:

Suggested change
import { contentSecurity } from '@nichtsam/helmet/rules/content/index'
import { contentSecurity } from '@nichtsam/helmet/content'

Copy link
Contributor Author

@nichtsam nichtsam Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also didn't like that, but didn't know how to path them better, thanks for the idea!
I will update the package to simplify the import path.

import { createReadableStreamFromReadable } from '@react-router/node'

import * as Sentry from '@sentry/node'
import chalk from 'chalk'
import { isbot } from 'isbot'
Expand All @@ -21,6 +21,8 @@ export const streamTimeout = 5000
init()
global.ENV = getEnv()

const MODE = process.env.NODE_ENV ?? 'development'

type DocRequestArgs = Parameters<HandleDocumentRequestFunction>

export default async function handleRequest(...args: DocRequestArgs) {
Expand Down Expand Up @@ -65,6 +67,33 @@ export default async function handleRequest(...args: DocRequestArgs) {
const body = new PassThrough()
responseHeaders.set('Content-Type', 'text/html')
responseHeaders.append('Server-Timing', timings.toString())

contentSecurity(responseHeaders, {
crossOriginEmbedderPolicy: false,
contentSecurityPolicy: {
// NOTE: Remove reportOnly when you're ready to enforce this CSP
reportOnly: true,
directives: {
fetch: {
'connect-src': [
MODE === 'development' ? 'ws:' : undefined,
process.env.SENTRY_DSN ? '*.sentry.io' : undefined,
"'self'",
],
'font-src': ["'self'"],
'frame-src': ["'self'"],
'img-src': ["'self'", 'data:'],
'script-src': [
"'strict-dynamic'",
"'self'",
`'nonce-${nonce}'`,
],
'script-src-attr': [`'nonce-${nonce}'`],
},
},
},
})

resolve(
new Response(createReadableStreamFromReadable(body), {
headers: responseHeaders,
Expand Down
20 changes: 10 additions & 10 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"@epic-web/totp": "^2.1.1",
"@mjackson/form-data-parser": "^0.7.0",
"@nasa-gcn/remix-seo": "^2.0.1",
"@nichtsam/helmet": "0.2.0",
"@oslojs/crypto": "^1.0.1",
"@oslojs/encoding": "^1.1.0",
"@paralleldrive/cuid2": "^2.2.2",
Expand Down Expand Up @@ -90,7 +91,6 @@
"express-rate-limit": "^7.5.0",
"get-port": "^7.1.0",
"glob": "^11.0.1",
"helmet": "^8.0.0",
"input-otp": "^1.4.2",
"intl-parse-accept-language": "^1.0.0",
"isbot": "^5.1.22",
Expand Down
40 changes: 6 additions & 34 deletions server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import crypto from 'node:crypto'
import { helmet } from '@nichtsam/helmet/node-http'
import { createRequestHandler } from '@react-router/express'
import * as Sentry from '@sentry/node'
import { ip as ipAddress } from 'address'
Expand All @@ -8,7 +9,6 @@ import compression from 'compression'
import express from 'express'
import rateLimit from 'express-rate-limit'
import getPort, { portNumbers } from 'get-port'
import helmet from 'helmet'
import morgan from 'morgan'
import { type ServerBuild } from 'react-router'

Expand Down Expand Up @@ -68,6 +68,11 @@ app.use(compression())
// http://expressjs.com/en/advanced/best-practice-security.html#at-a-minimum-disable-x-powered-by-header
app.disable('x-powered-by')

app.use((_, res, next) => {
helmet(res)
next()
})

Copy link
Member

@kentcdodds kentcdodds Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand what this does, why we need this, and what's in the entry.server.tsx?

Copy link
Contributor Author

@nichtsam nichtsam Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines ensure that all routes are protected, not just Remix routes.

When working on CSP and other security headers, I realized that some security headers are specific to the response type. Some apply to all types of resources, such as X-Content-Type-Options, Strict-Transport-Security, and Cross-Origin-Resource-Policy. Others are content-specific, typically for text/html, some (like CSP) can also apply to application/xml, application/pdf,image/svg+xml and more (see this).

To improve clarity and control, I categorized the headers into three groups:
1. General – Applied to all resources.
2. Content – Applied to content types.
3. Resource Sharing – Related to cross-origin policies.

Here, we apply the general security headers, ensuring that assets, JS files, and other resources are covered. In entry.server.tsx, we handle content-specific headers for text/html, such as CSP.

The main helmet works on Web Fetch API's Headers, this import is essentially a wrapper on it to work with http.ServerResponse.

I hope this is clear, feel free to ask for more details!

if (viteDevServer) {
app.use(viteDevServer.middlewares)
} else {
Expand Down Expand Up @@ -110,39 +115,6 @@ app.use((_, res, next) => {
next()
})

app.use(
helmet({
xPoweredBy: false,
referrerPolicy: { policy: 'same-origin' },
crossOriginEmbedderPolicy: false,
contentSecurityPolicy: {
// NOTE: Remove reportOnly when you're ready to enforce this CSP
reportOnly: true,
directives: {
'connect-src': [
MODE === 'development' ? 'ws:' : null,
process.env.SENTRY_DSN ? '*.sentry.io' : null,
"'self'",
].filter(Boolean),
'font-src': ["'self'"],
'frame-src': ["'self'"],
'img-src': ["'self'", 'data:'],
'script-src': [
"'strict-dynamic'",
"'self'",
// @ts-expect-error
(_, res) => `'nonce-${res.locals.cspNonce}'`,
],
'script-src-attr': [
// @ts-expect-error
(_, res) => `'nonce-${res.locals.cspNonce}'`,
],
'upgrade-insecure-requests': null,
},
},
}),
)

// When running tests or running in development, we want to effectively disable
// rate limiting because playwright tests are very fast and we don't want to
// have to wait for the rate limit to reset between tests.
Expand Down
Loading