Consider limiting helmet to document requests or add a note

[mariomc]

Currently helmet middleware is applied to every single requese-response pair.

This makes it so that mixed applications (that serve documents, files or XHR requests) are applying security headers to non-document requests.

Example:

const express = require('express');
const app = express();
const port = 3010;
const path = require('path');
const helmet = require('helmet');

app.use(
  helmet({
    contentSecurityPolicy: {
      directives: {
        scriptSrc: ["'self'", () => `'nonce-stuff'`],
      },
    },
  })
);
app.use(express.static('static'));

app.get('/', (req, res) => {
  res.sendFile(path.resolve('pages/index.html'));
});

app.get('/api', (req, res) => {
  res.send({ test: 1 });
});

app.listen(port, () => {
  console.log(`Example app listening at http://localhost:${port}`);
});

Result:

CSS	XRH/API

This is unnecessarily inflating response sizes for clients and the documentation is not warning users about it.

I can help raise a PR if you find this a relevant issue but first, some questions:

• Is this considered a bug, misconfiguration or a new feature?
• How should this be tackled? Via code or documentation?
• If via code, should it be within each internal middleware (ex: via a req.accepts('html') or req.accepts(internalAllowListForMiddleware))? OR
• Given that all of them seem document related (are they?), should it be inside the main entry point?
• Should that be default behavior or should it be hidden behind an option?
• If behind an option, should it be a boolean or something else? What would be the default?

[EvanHahn]

This is a good and tricky question.

• Is this considered a bug, misconfiguration or a new feature?

It's considered a side-effect of the design.

Helmet is designed to be easy to use. It's not always possible to do this, but I try to make something that can be included with one line. Ideally, you'd just be able to do this:

// The ideal
app.use(helmet());

As you point out, this creates a problem: many of Helmet's headers are wasteful for non-documents.

This is further complicated by the fact that not all of Helmet's headers are wasteful in these cases. I believe Strict-Transport-Security, X-Content-Type-Options, X-Permitted-Cross-Domain-Policies, and X-Powered-By can be useful for non-documents.

• How should this be tackled? Via code or documentation?

• If via code, should it be within each internal middleware (ex: via a req.accepts('html') or req.accepts(internalAllowListForMiddleware))? OR

Documentation, IMO. I think it's too difficult to reliably set the appropriate headers automatically.

Helmet is designed to work with Express but it doesn't require it. req.accepts() is Express-specific, so we can't use it.

But even if it were supported, the Accept header isn't the right way to determine whether Helmet headers should be set. Just because a request accepts HTML doesn't mean that's what will be used in the response.

We might be able to use on-headers for this, but I fear that's too brittle.

• Given that all of them seem document related (are they?), should it be inside the main entry point?

As I said above, they're not all document-related.

I could see adding an API for document-specific headers, but I'm not sure what that looks like. I'd love suggestions.

[mariomc]

It's considered a side-effect of the design.

Helmet is designed to be easy to use. It's not always possible to do this, but I try to make something that can be included with one line. Ideally, you'd just be able to do this:

// The ideal
app.use(helmet());

As you point out, this creates a problem: many of Helmet's headers are wasteful for non-documents.

Thank you for the reply.

I'll open a PR adding a note below that line in the documentation.

But even if it were supported, the Accept header isn't the right way to determine whether Helmet headers should be set. Just because a request accepts HTML doesn't mean that's what will be used in the response.

You're right. For something like this to exist, it would have to be derived on the response type somewhere.
Given how middlewares work, nothing ensures this plugin that - even though the response isn't yet a document, it
could be transformed by the next middleware up in the chain.

I now understand what you mean't by being difficult to "reliably set the appropriate headers automatically." 😄

Given how chaining works, should we also recommend (in the documentation) that this middleware should be the last one to be executed?

[EvanHahn]

Thanks for opening a PR.

Given how chaining works, should we also recommend (in the documentation) that this middleware should be the last one to be executed?

Sorry, I don't understand this. Could you explain?

[mariomc]

Sorry, I don't understand this. Could you explain?

Disregard my comment. 😄
I was needlessly concerned about conflicting middleware that adds X-Powered-By or others.

Instead of:

app.use(helmet());
app.use(middlewareThatAddsXPoweredBy());

Ensuring:

app.use(middlewareThatAddsXPoweredBy());
app.use(helmet());