Skip to content
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.

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

Content-Security-Policy: reimplement directive values as functions #243

Closed
EvanHahn opened this issue Aug 9, 2020 · 5 comments · Fixed by #245
Closed

Content-Security-Policy: reimplement directive values as functions #243

EvanHahn opened this issue Aug 9, 2020 · 5 comments · Fixed by #245
Assignees

Comments

@EvanHahn
Copy link
Member

EvanHahn commented Aug 9, 2020

In Helmet version 4, I removed the ability to use functions in Content Security Policy directive values. Several issues (#240, fastify/fastify-helmet#89) arose as a result of this, and I don't think the deprecation was worth it.

I plan to re-add this feature in the next few days.

@einfallstoll
Copy link

Out of curiosity: Why was it removed in the first place?

EvanHahn added a commit that referenced this issue Aug 10, 2020
This was removed in Helmet 4, which I regret. You can read more about it
on [this GitHub issue][0].

Closes [#243][0].

[0]: #243
@EvanHahn
Copy link
Member Author

This will be fixed in helmet@4.1.0, which you can test out with npm install helmet@4.1.0-rc.1. You can see the v4.1.0 pull request at #245 for more information about that release.

@einfallstoll It was removed for a few reasons. Again, I regret the removal, but here was my thought process:

  1. Getting out of the business of "conditional" behavior. I received a number of issues about conditional behavior across Helmet. The most common were about nonces in CSP, but there were plenty of others, such as only serving the Strict-Transport-Security header in HTTPS mode. All of that conditional behavior was subtly different, and I thought it'd be best to get out of the business of doing this.

    This turned out to be fraught, as lots of people were using dynamic features and it made upgrading difficult.

  2. Simplifying the module. The CSP module was messy to maintain, and Helmet 4 simplified it significantly. This made it easier for me, as a maintainer, to work on it. This has downstream benefits: faster releases, easier comprehensibility by new contributors, less bug-free code, etc.

    I think most of those simplifications, like the removal of browser sniffing, were good, but not this one.

  3. Performance. If you serve a static CSP, we only need to compute and validate it once. Dynamic directives have to be computed every time. That's not so bad (especially when browser sniffing is removed) but it can be wasteful.

    But if you're really concerned about this, you could (1) write your own CSP middleware (2) set the header "higher up" in your application's chain, such as in nginx or Apache.

  4. My philosophy on breaking changes. I've maintained a few open source projects and APIs over the years, and I've learned to be conservative about what you add, because you'll have to maintain it for a long time. Generally, the initial development work is the easiest part—maintenance is the much bigger task. I thought it best to remove features to make Helmet easier for me to maintain (which has downstream benefits, as mentioned above).

    However, this was a removed feature that people needed, and it caused some headache for users, so I'm adding it back.

Again, you can test this out in npm install helmet@4.1.0-rc.1, and follow the release's progress in #245.

EvanHahn added a commit that referenced this issue Aug 15, 2020
This was removed in Helmet 4, which I regret. You can read more about it
on [this GitHub issue][0].

Closes [#243][0].

[0]: #243
@EvanHahn
Copy link
Member Author

This has been released in helmet@4.1.0 and helmet-csp@3.1.0.

@airtonix
Copy link

airtonix commented Oct 31, 2020

But if you're really concerned about this, you could
 (1) write your own CSP middleware
 (2) set the header "higher up" in your application's chain, such as in nginx or Apache.

(1) the Helmet documentation suggests doing exactly this, and for NestJS it's what we did for Helmet v3.
(2) Isn't possible when your app is running in docker.

I'm also still getting problems with this:

Screenshot from 2020-10-31 11-44-11

package.json

...

    "helmet": "^4.1.1",
    "helmet-csp": "^3.1.0",
...

src/core/middleware/helmet.middleware.ts

import { Injectable, NestMiddleware } from '@nestjs/common'
import { Request, Response } from 'express'
import { v4 as uuidv4 } from 'uuid'
import helmet from 'helmet'

@Injectable()
export class CspNonceMiddleware implements NestMiddleware {
  use (
    request: Request,
    response: Response,
    next: CallableFunction
  ) {
    response.locals.nonce = uuidv4()
    next()
  }
}

@Injectable()
export class HelmetMiddleware implements NestMiddleware {
    use = helmet({
      contentSecurityPolicy: {
        directives: {
          scriptSrc: [
            "'self'",
            (req, res) => `'nonce-${res.locals.nonce}'`
          ]
        }
      }
    })
}

@EvanHahn
Copy link
Member Author

EvanHahn commented Nov 1, 2020

That error is happening because Helmet types res as an http.ServerResponse when you probably want it to be an express.Request. What happens if you change the line to this?

(req: Request, res: Response) => `'nonce-${res.locals.nonce}'`

If that doesn't work, Helmet might want to add some kind of override to let you say that res is something other than http.ServerResponse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants