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

'self' and 'none' values lack quotes #454

Closed
DougReeder opened this issue Feb 22, 2024 · 4 comments
Closed

'self' and 'none' values lack quotes #454

DougReeder opened this issue Feb 22, 2024 · 4 comments
Assignees
Milestone

Comments

@DougReeder
Copy link

To reproduce:

Set helmet with a CSP:

app.use(helmet({contentSecurityPolicy: {
    directives: {
      sandbox: ['allow-scripts', 'allow-forms', 'allow-popups', 'allow-same-origin'],
      defaultSrc: ['self'],
      childSrc: ['none'],
      connectSrc: ['none'],
      baseUri: ['self'],
      frameAncestors: ['none'],
    },
  }}));

Expected result:

All pages have CSP header with ... default-src 'self'; child-src 'none'...

Actual result

All pages have CSP header with ... default-src self; child-src none...
which are interpreted by Firefox as origins named "self" and "none". Thus, the actual CSP is not what is intended.

@EvanHahn
Copy link
Member

In the short term:

You need to quote these values yourself. For example, defaultSrc: ["'self'"].

In the long term:

It seems reasonable that Helmet would do this for you, as it's unlikely that users intend to use self/none as origins. I'll think about whether this is a better API in future versions. Feedback welcome!

@webketje
Copy link

webketje commented Apr 24, 2024

Also encountered:

app.use(helmet({
  contentSecurityPolicy: {
    directives: {
      'script-src': [`'self'`, `'sha256-${serviceWorkerHash.digest('base64')}'`]
    }
  }
}))

Our current CSP:

script-src  'self'  'sha256-dJ+NYptz3LoT6dbm1JzANoJYN4KGvIEG7mmjqsW9l0Q=';default-src  'self';base-uri 'self';font-src 'self' https: data:;form-action  'self';frame-ancestors 'self';img-src 'self' data:;object-src  'none';script-src-attr 'none';style-src 'self' https:  'unsafe-inline';upgrade-insecure-requests

Of course this will be a semver-major change unless code is added to wrap the values only if no quote is present and only on the fixed values of "self" and "none". What is the use case for not auto-adding quotes to these? Don't understand what is implied here:

it's unlikely that users intend to use self/none as origins

@EvanHahn
Copy link
Member

EvanHahn commented Apr 24, 2024

It's theoretically possible that someone wants to specify self or none without quotes. They could have configured one of these as an origin (likely some weird company intranet thing) and deliberately want to allow something like https://self/foo.js.

However, I think this is so unlikely that Helmet doesn't need to support it. My plan:

  • In the current version of Helmet, warn when you try to do this.
  • In the next version of Helmet, auto-quote "self" and "none" throw when you try to do this.

@EvanHahn EvanHahn added this to the v8.0.0 milestone Apr 24, 2024
webketje added a commit to webketje/helmet that referenced this issue Apr 24, 2024
- replaces HTML5Rocks URL with web.dev (redirect), add links to relevant MDN docs
- adds doc sections/ anchors for defaults, computed directives, disabling directives, and report only header
- clarifies that defaultSrc will default to 'self' (and is thus not required to the user) when useDefaults: true
- solves helmetjs#404, documents function signature and adds conditional CDN script-src loading example
- adds a common recipe to generate subresource-integrity hashes
- documents caveat of non-hostname values mentioned in helmetjs#454
@EvanHahn EvanHahn self-assigned this Apr 27, 2024
@EvanHahn
Copy link
Member

This was done in Helmet v7 (the current version) in 6475da1, and for Helmet v8 (the next version) in 7b94a6c.

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

No branches or pull requests

3 participants