refactor configuration

[jelhan]

The configuration of this addon doesn't feel inline with the rest of the ecosystem. This gets even worse if new configuration option are needed as it's the case for #91.

I like to suggest to refactor the configuration API as the following:

ENV['ember-cli-content-security-policy'] = {
  enabled: true,
  metaTag: false,
  policies: {
    'default-src': ["'none'"],
    'script-src':  ["'self'"],
    'font-src':    ["'self'"],
    'connect-src': ["'self'"],
    'img-src':     ["'self'"],
    'style-src':   ["'self'"],
    'media-src':   ["'self'"]
  },
  reportOnly: true,
}

The values shown above are suggested defaults.

For a transition period we might want to support existing configuration API. In that case the defaults would look like:

ENV['ember-cli-content-security-policy'] = {
  enabled: true,
  metaTag: ENV.contentSecurityPoliyMeta,
  policies: ENV.contentSecurityPolicy || {
    'default-src': ["'none'"],
    'script-src':  ["'self'"],
    'font-src':    ["'self'"],
    'connect-src': ["'self'"],
    'img-src':     ["'self'"],
    'style-src':   ["'self'"],
    'media-src':   ["'self'"]
  },
  reportOnly: ENV.contentSecurityPolicyHeader === 'Content-Security-Policy-Report-Only',
}

If we consider disabling the addon by setting ENV.contentSecurityPolicy === {} as a public API as suggested in #77, we might want to change the default for enabled to !ENV.contentSecurityPolicy || Object.keys(ENV.contentSecurityPolicy).length !== 0. We should deprecate the old configuration API at the same time. In that case this wouldn't be a breaking change but add some additional maintenance costs.

Please note that this API wouldn't require a change for #91. It would allow to disable throwing in tests by setting enabled = environment !== 'test'. metaTag option would be forced to true if running tests.

[jelhan]

Just recognized that this isn't runtime configuration but for build and server only. Due to this it should also be moved from config/environment.js to ember-cli-build.js.

[lifeart]

is it possible to have more dummers-frendly and declarative configuration, like

    csp
        policies:
        
            images:
                local: true,
                raw-inline: true,
                fromHosts: [ 'http://...', 'http://...' ]
            fonts:
                local: true,
            styles:
                local: true,
                inline: true
                ...

I think we can replace none, self etc to something more meaningful and less formal

'font-src': ["'self'"] - scares me, comparing policies.fonts.local = true - looks friendly

I think CSP - it's kinda do and forget configuration, and we able to add sugar for it, to not ask developers about formal notation

[jelhan]

@rwjblue I'm willing to provide a PR for this but would like to know beforehand if you are willing to accept such a change of configuration API. Also the status of this addon is unclear to me. It has still a high weekly download numbers on NPM but PRs are open for a long time without feedback and it's missing any test coverage. 😞 Let me know if there is anything I could help with. This addon provides valuable feature for the ecosystem and could provide big value if known limitations (e.g. testing support) are addressed IMO.

[sandstrom]

@jelhan Looks good overall!

Some thoughts:

1. Should we use a key for delivery, instead of meta true/false?

and basically only deliver via header or meta, but not both. I think that may avoid some confusion + some issues around the directives that can only be delivered via headers.

  // …
  enabled: true,
  delivery: 'meta', // or 'header'
  // …

2. How about the directives that don't have a value[1], for example block-all-mixed-content? Perhaps we could use "block-all-mixed-content": true, as the syntax for them

3. Your example on the transition ENV.contentSecurityPolicyHeader === 'Content-Security-Policy', shouldn't that be ENV.contentSecurityPolicyHeader === 'Content-Security-Policy-Report-Only',?

4. Do we know how many addons would be affected? It's great that you are thinking about the transition + supporting existing addons. Would very much prefer if this won't need to be a breaking change.

[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/block-all-mixed-content

I know rwjblue still care about this repo and want to see it progress, but he's a busy person. So I'm certain it's not neglect, it's just a lack of bandwidth. Anyway, I have commit bits to this repo so I can help shepherd it through (though I'll try to get rwjblue to sign off on it first).

[jelhan]

Should we use a key for delivery, instead of meta true/false?

I like that one but would additionally support both as an option to not add a breaking change.

How about the directives that don't have a value[1], for example block-all-mixed-content? Perhaps we could use "block-all-mixed-content": true, as the syntax for them

I like that proposal. Are these ones currently supported?

Your example on the transition ENV.contentSecurityPolicyHeader === 'Content-Security-Policy', shouldn't that be ENV.contentSecurityPolicyHeader === 'Content-Security-Policy-Report-Only',?

💯 Updated first post accordingly.

Do we know how many addons would be affected? It's great that you are thinking about the transition + supporting existing addons. Would very much prefer if this won't need to be a breaking change.

A change here would not only affect addons but also a lot of application. This addon has 24.758 weekly downloads. That's a lot for our ecosystem. For comparison ember-cli has 120.459 weekly downloads. It was part of default blueprint of applications and addons for a long time.

[sandstrom]

1. Good point, how about allowing an array for the delivery key, so that the default could be:

// it would accept this (default, for backwards compatibility):
delivery: ['meta', 'header'],

// and also accept a single value
delivery: 'meta',

(in case there would be some future mechanism for CSP delivery)

2. Not really, right now you could do this and it would kind of work:

block-all-mixed-content: '',

3. Sounds good!

4. Yes, that's a lot, we'll have to make sure this won't cause an unnecessary interruption.

[jelhan]

Most of the configuration refactoring discussed here has landed in #94. The policy object isn't refactored yet. @lifeart do you have time to work on that one as it was your suggestion? I'm totally in favor for it but like to focus on other parts of this addon. Would be great to change the configuration in one release only and not in several ones.

[lifeart]

@jelhan sorry, this week I have no time for it(.
I think we need more polishing on proposed config format, to have all items consistent.

[sandstrom]

@lifeart Although I really appreciate that you'd like to help out improving this addon, I'm not convinced that we should move to something "dummers-frendly". Although I agree that CSP is complicated and sometimes confusing, there are also disadvantages to adding another layer of abstraction.

Happy to discuss further though! I could certainly change my mind if there are good arguments in favor of dumbing it down.

But just wanted to make this note so you don't start working on something that may not be merged in the end.

cc: @jelhan

[sandstrom]

I'll go ahead and close this issue. But feel free to open a new if you want to discuss more changes to the config format! 😄

Thanks @jelhan for landing the first refactor!