upgrade-insecure-requests directive is always set for falsey config values

[helmetjs]

upgrade-insecure-requests directive is always set for falsey config values

[EvanHahn]

This has been released in [email protected] and [email protected]. Sorry for the bug!

[EvanHahn]

This turns out to be intentional.

Sometimes, you want to specify a directive with no keys. For example, to omit all JavaScript, you might send a header like this:

Content-Security-Policy: script-src; style-src 'self'

You can do something like this to get that behavior:

app.use(csp({
  directives: {
    scriptSrc: null,
    styleSrc: ["'self'"]
  }
})

This is why csp takes any key to mean "include this directive", even falsy values.

Luckily, CSP lets you do this:

var directives = {}

if (app.get('env') === 'production') {
  directives.upgradeInsecureRequests = true
}

app.use(csp({
  directives: directives
}))

Does that help?

[muffinresearch]

@EvanHahn Thanks for the quick reply

Content-Security-Policy: script-src; style-src 'self'

Is there somewhere in the spec that say that's OK? I would have thought script-src 'none' would be more correct since script-src requires a source-list. But then you'd need to be aware of directive value types and it doesn't seem like Helmet knows about that afaict.

The downside of setting the key only where it's needed is that it means I am now having to not have upgrade-insecure-requests set in my default conf (which should be production) and then override it for the deployed hosts just so I don't get upgrade-insecure-requests in local development.

The reason our default conf is production is that it's better to accidentally have production values in development than it is to have development values accidentally in production.

[EvanHahn]

I think you're right that script-src 'none'; is correct and script-src; isn't—I'm not sure what the behavior is in that case. I probably shouldn't allow it, or coerce it to 'none', or at least give a warning.

There is the case where some directives don't have any "arguments", like sandbox or upgrade-insecure-requests, so this module needs some way of specifying that.

I assume you would expect this to set no upgrade-insecure-requests directive, right?

app.use(csp({
  directives: {
    defaultSrc: ['foo'],
    upgradeInsecureRequests: false
  }
})

I think that API makes sense, but it is a breaking change.

[muffinresearch]

Yes I think if upgradeInsecureRequests: false meant that directive isn't sent it would be more intuitive for that specific case.

There is the case where some directives don't have any "arguments", like sandbox or upgrade-insecure-requests, so this module needs some way of specifying that.

Fwiw sandbox also allows optional values to specify the sandbox features as well as no value. See https://www.w3.org/TR/CSP2/#directive-sandbox

For sandbox, maybe false would mean don't set it and true would provide just sandbox and if a list was provided those values should be used as the value.

I do think if it is possible to move in the direction of having helmet-csp understand more of the spec re: the different directive types and their values e.g. when a src-list is expected or not that would be really helpful for end-users.

Once you have that you could also catch incorrect config e.g. iframe-ancestors vs frame-ancestors since the former is not a directive but that's a really easy mistake to make.

It's really easy to misconfigure CSP so any help helmet can provide in that regard would be great.

[EvanHahn]

I think there are two interesting points here. (1) How do you set an "empty" directive and what does helmet-csp do with falsy values? (2) Should helmet-csp understand more of the spec?

Setting empty/falsy directives

I'll start by saying that I'm hesitant to make breaking changes, especially if you can do it (albeit inelegantly) now.

I think we can agree that we know what happens when this option comes along:

defaultSrc: ['example.com']

But what about these?

defaultSrc: [],
scriptSrc: '',
styleSrc: true,
childSrc: false,
imgSrc: null,
frameSrc: void 0,

What should happen in these cases? And are they fundamentally different from sandbox or upgrade-insecure-requests?

sandbox: false,
sandbox: true,
sandbox: ''

I tried to make the directive builder support these things, but perhaps I did things wrong. Perhaps all directives should be specified as arrays, and if it's anything falsy, the directive isn't included? That seems to be what you're suggesting—pardon me if I'm misunderstanding—which seems pretty reasonable.

Once again, however, I'm hesitant to make breaking changes unless I can be convinced that the current setup is horrible.

Making this module understand more of the CSP spec

This module used to catch errors, but I found that it was pretty tricky to do right. Things that were tricky:

• There used to be a list of supported directives (default-src, script-src, etc). csp would throw an error if you used an unsupported directive. This worked well until I learned about new directives, like form-action or plugin-types. I found myself having to keep up with all of the new directives. You could certainly argue this is my job as the maintainer of this module.
• On a related note, there was an issue with frame-src. This directive is considered deprecated and child-src should be used. What should this module do? Should it warn/throw when using frame-src? Should it automatically coerce child-src for unsupported browsers, even though it's subtly different?
• It becomes less flexible if you're trying to use this module in weird ways. For example, You could argue that this is too weird and this module shouldn't support that.

Perhaps the right answer for some of these is to show a warning (and then have some way to disable warnings if you're sure you're doing the right thing).

[muffinresearch]

I'm probably of the (biased) opinon that fixing the first problem re: falsey value handling is easiest if you deal with the 2nd issue of helment understanding more of the spec. The context of what the directive is is really the only way to know to deal with a given value. Mainly it would be a case of dealing with src-lists but there's the few other cases as we've previously covered.

As an example, a non-list source-list value should probably be an error. Assuming ['none'] could eat someone's site resulting in all script being blocked, and the alternative of not sending that directive could be too liberal and bad things might happen because it's completely wide-open.

Avoiding guessing intent would probably be best e.g. providing guidance through warnings and errors is better than actively making changes to the output in case the guess is wrong.

If helmet did understand more of the spec it should be possible with careful design to not lock the user into only being able to configure the defined directives. Maybe that configuration could be user-extendable too, or as you say, the "strict mode" could be disabled if the user wanted to, although that should be gently discouraged!

[EvanHahn]

I think you're totally right...ready for a fat list?

Here's how I think directives should be passed in the future.

For base-uri, child-src, connect-src, default-src, font-src, form-action, frame-ancestors, frame-src, img-src, media-src, object-src, script-src, and style-src:

what's passed in	what happens in "strict mode"	what happens in "loose mode"	different from current?
thingSrc: []	default-src 'none'	default-src 'none'
thingSrc: ['self']	default-src 'self' A warning like: "converting unquoted 'self'"	default-src self	✓
thingSrc: ['none']	default-src 'none' A warning like: "converting unquoted 'none'"	default-src none	✓
thingSrc: ['']	An error like: "'' is not a valid source"	default-src	✓
thingSrc: [123]	An error like: "123 is not a valid source"	default-src 123	✓
thingSrc: true	An error like: "true makes no sense"	An error like: "true makes no sense"	✓
thingSrc: null	An error like: "null makes no sense"	nothing is set	✓
thingSrc: ''	An error like: "'' makes no sense"	nothing is set	✓
thingSrc: false	nothing is set	nothing is set	✓
scriptSrc: ['unsafe-inline']	script-src 'unsafe-inline' A warning like: "converting unquoted 'unsafe-inline'"	script-src unsafe-inline	✓
scriptSrc: ['unsafe-eval']	script-src 'unsafe-eval' A warning like: "converting unquoted 'unsafe-eval'"	script-src unsafe-eval	✓
fontSrc: ['unsafe-inline']	A warning like: "converting unquoted 'unsafe-inline'" An error like: "unsafe-inline makes no sense for font-src"	font-src unsafe-inline	✓
fontSrc: ['unsafe-eval']	A warning like: "converting unquoted 'unsafe-eval'" An error like: "unsafe-eval makes no sense for font-src"	font-src unsafe-eval	✓

For plugin-types:

what's passed in	what happens in "strict mode"	what happens in "loose mode"	different from current?
pluginTypes: []	plugin-types	plugin-types
pluginTypes: []	plugin-types	plugin-types
pluginTypes: ['application/flash']	plugin-types application/flash	plugin-types application/flash
pluginTypes: ['self']	An error like: "'self' is not a valid plugin type"	plugin-types self	✓
pluginTypes: ["'self'"]	An error like: "'self' is not a valid plugin type"	plugin-types 'self'	✓
pluginTypes: ['']	An error like: "'' is not a valid plugin type"	plugin-types	✓
pluginTypes: [123]	An error like: "123 is not a valid plugin type"	plugin-types 123	✓
pluginTypes: true	An error like "true makes no sense"	An error like "true makes no sense"	✓
pluginTypes: null	An error like "null makes no sense"	nothing is set	✓
pluginTypes: ''	An error like "'' makes no sense"	nothing is set	✓
pluginTypes: false	nothing is set	nothing is set	✓

For sandbox:

what's passed in	what happens in "strict mode"	what happens in "loose mode"	different from current?
sandbox: []	sandbox	sandbox
sandbox: ['allow-forms']	sandbox allow-forms	sandbox allow-forms
sandbox: ['allow-garbage']	An error like "allow-garbage is not a sandbox directive"	sandbox allow-garbage	✓
sandbox: true	sandbox	sandbox
sandbox: 123	An error like "123 makes no sense"	An error like "123 makes no sense"	✓
sandbox: null	An error like "null makes no sense"	nothing is set	✓
sandbox: false	nothing is set	nothing is set	✓

For report-uri:

what's passed in	what happens in "strict mode"	what happens in "loose mode"	different from current?
Report-Only with no report URI	An error like "please set a report-uri"	Content-Security-Policy-Report-Only: ...	✓
reportUri: '/foo-boo'	report-uri /foo-boo	report-uri /foo-boo
reportUri: ['/foo', '/boo']	report-uri /foo /boo	report-uri /foo /boo
reportUri: ''	An error like "'' makes no sense"	report-uri
reportUri: []	An error like "[] needs to have at least one element"	report-uri	✓
reportUri: ['']	An error like: "'' is not a valid report URI"	report-uri	✓
reportUri: [123]	An error like: "123 is not a valid report URI"	report-uri 123	✓
reportUri: true	An error like "true makes no sense"	An error like "true makes no sense"	✓
reportUri: null	An error like "null makes no sense"	nothing is set	✓
reportUri: ''	An error like "'' makes no sense"	nothing is set	✓
reportUri: false	nothing is set	nothing is set	✓

Let me know how those look to you.

[muffinresearch]

Looks good on the whole (sorry for taking a while to reply there's lots going on at the moment).

I think strict mode could be a little more brutal e.g. an empty source-list type should probably be an error and not assume "'none'".

Similarly for the mixed type sandbox directive, an empty list should be an error saying that the expected values are either true, false or a list containg the valid values. (The only point where this might become tricky is if browser vendors diverge re: implementation but if that was the case it would be necessary to warn should helmet encounter a value that is browser-specific).

A empty list value for plugin-types is probably never valid iiuc and should always error.

So in a nutshell strict mode should only allow known sane values and everything else should be an error. With the exception of converting un-quoted values is probably ok - although that could potentially be an option.

If the loose mode had the same errors as the strict mode, except it allowed unknown directive values for things like src-lists, or unknown mime-types for plugin-types etc then that would mean that even the loose mode will help the user not make configuration mistakes just it's more lax as what precise data is allowed which I think is the right balance.

[EvanHahn]

I agree with everything you've said.

This is a fairly large change but I'll work on it when I get a chance.

[EvanHahn]

@muffinresearch Do you happen to know how you specify a completely empty plugin-types? Chrome seems to block all plugins with either of these:

Content-Security-Policy: default-src *; plugin-types
Content-Security-Policy: default-src *; plugin-types 'none'

I can't find much in the spec about this.

[EvanHahn]

@muffinresearch I believe this is done and ready for you to take a look at! It's on the 2.0.0 branch, so I believe you can grab it like this in your package.json:

"dependencies": {
  "helmet-csp": "helmetjs/csp#2.0.0"
}

Let me know if it looks good!

[muffinresearch]

@EvanHahn sorry I missed replying to this. Thanks for all the changes!

[t47io]

Hi, I'm using the latest version of the lib (helmet 3.9.0 and helmet-csp 2.6.0) but when I do this:

  helmet({
    contentSecurityPolicy: {
      directives: {
        styleSrc: [
          '\'self\'',
          '\'unsafe-inline\'',
        ],
        scriptSrc: [
          '\'self\'',
          '\'unsafe-inline\'',
          'www.google-analytics.com',
        ],
        upgradeInsecureRequests: !DEBUG, // false for dev
      },
    },
    hsts: { ... },
  })

I still see upgrade-insecure-requests being set in the header.

[EvanHahn]

@t47io Did some quick investigation—this still seems like a bug in Helmet. It might be a few weeks before I can get to this. In the meantime, you'll have to remove the key. Here's an example:

const omit = require('lodash/omit')

let directives = {
  styleSrc: [/* ... */],
  upgradeInsecureRequests: true
  // ...
}
if (DEBUG) {
  directives = omit(directives, 'upgradeInsecureRequests')
}

helmet({
  contentSecurityPolicy: { directives },
  // ...
})

[t47io]

@EvanHahn So I tracked down the issue. The directives are passed as is (i.e. containing a k/v of upgradeInsecureRequests: false) from helmet to helmet-csp to content-security-policy-builder. At this line (https://github.com/helmetjs/content-security-policy-builder/blob/master/index.js#L26), it adds the directive 'upgrade-insecure-requests` to the return string.

Two possible approaches to resolve:

1. Modify content-security-policy-builder, e.g.:

  value = ...
  if (...) {
    ...
  } else if (value === true) {
    ...
  } else if (!value) { // add this, skip directive that has falsy value
    return result
  }

  if (value) {
    ...
  }

2. Modify helmet-csp to remove falsy k/v pairs before passing into content-security-policy-builder.

Which would be better in your opinion in terms of conforming with specs?

[t47io]

I'd prefer approach 1 since it's a lower-level fix for all. One discussion would be whether to restrict the falsy checking to value === false, but I wonder how often do we see directives with (mistaken / misconfigured) values of e.g. undefined and null.

[EvanHahn]

Working on this now.