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

Allow 'reportOnly' option to be set dynamically #35

Merged
merged 3 commits into from May 18, 2016

Conversation

Projects
None yet
2 participants
@mfinifter
Contributor

mfinifter commented May 10, 2016

I have found it helpful to have a dynamic kill switch for CSP that turns it from enforcing to report-only in case of an emergency, especially during the initial roll-out. This feature avoids having to do an emergency deploy in order to switch back to report-only mode.

Let me know whether you'd be open to this in principle, and I'd be happy to make any changes you request in order to get this landed.

Thanks.

Show outdated Hide outdated index.js
Show outdated Hide outdated index.js
Show outdated Hide outdated index.js
assert.throws(function () {
csp({ reportOnly: true })
}, Error)
})
it('does not throw an error when reportOnly is a function and there is no report-uri', function () {

This comment has been minimized.

@EvanHahn

EvanHahn May 18, 2016

Member

I feel like this should require a report-uri. If it might be Report-Only, don't you want a report-uri?

@EvanHahn

EvanHahn May 18, 2016

Member

I feel like this should require a report-uri. If it might be Report-Only, don't you want a report-uri?

This comment has been minimized.

@mfinifter

mfinifter May 18, 2016

Contributor

I'm imagining a scenario in which my CSP is stable, so I decide to remove the report-uri in order to reduce the unnecessary load on my reporting endpoint. I don't think this should require that I must also remove my dynamic kill switch.

I actually have an even stronger opinion here. I believe there is value in allowing a report-only policy with no report-uri. For example, imagine I don't yet have a reporting endpoint set up, but I want to get started on CSP as soon as possible. I put a report-only policy on the application, and ask the web developers to report any CSP errors they notice in the console during development. I can't do this with helmet today. Having said that, I understand that this is somewhat of a larger change that you may not be interested in.

If it were up to me (which it most certainly is not), I would not throw an error in any of these cases, but instead simply log a warning to the console.

@mfinifter

mfinifter May 18, 2016

Contributor

I'm imagining a scenario in which my CSP is stable, so I decide to remove the report-uri in order to reduce the unnecessary load on my reporting endpoint. I don't think this should require that I must also remove my dynamic kill switch.

I actually have an even stronger opinion here. I believe there is value in allowing a report-only policy with no report-uri. For example, imagine I don't yet have a reporting endpoint set up, but I want to get started on CSP as soon as possible. I put a report-only policy on the application, and ask the web developers to report any CSP errors they notice in the console during development. I can't do this with helmet today. Having said that, I understand that this is somewhat of a larger change that you may not be interested in.

If it were up to me (which it most certainly is not), I would not throw an error in any of these cases, but instead simply log a warning to the console.

This comment has been minimized.

@EvanHahn

EvanHahn May 18, 2016

Member

Good point. It's not really relevant to this PR, so I opened #37.

@EvanHahn

EvanHahn May 18, 2016

Member

Good point. It's not really relevant to this PR, so I opened #37.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn May 18, 2016

Member

Thanks for this (and sorry it took me a week to get to it!

Could you add something in the README for this, too?

Member

EvanHahn commented May 18, 2016

Thanks for this (and sorry it took me a week to get to it!

Could you add something in the README for this, too?

@mfinifter

This comment has been minimized.

Show comment
Hide comment
@mfinifter

mfinifter May 18, 2016

Contributor

Thanks for reviewing. Happy to continue iterating here as necessary.

Contributor

mfinifter commented May 18, 2016

Thanks for reviewing. Happy to continue iterating here as necessary.

@@ -158,12 +224,18 @@ describe('csp middleware', function () {
})
})
it('throws an error reportOnly is true and there is no report-uri', function () {
it('throws an error when reportOnly is true and there is no report-uri', function () {

This comment has been minimized.

@EvanHahn

EvanHahn May 18, 2016

Member

Good catch!

@EvanHahn

EvanHahn May 18, 2016

Member

Good catch!

@EvanHahn EvanHahn merged commit 186c2fc into helmetjs:master May 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn May 18, 2016

Member

Thanks! Releasing a new version soon...

Member

EvanHahn commented May 18, 2016

Thanks! Releasing a new version soon...

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn May 18, 2016

Member

Released in helmet-csp@1.2.0 and helmet@2.1.0. Thanks for your contribution!

Member

EvanHahn commented May 18, 2016

Released in helmet-csp@1.2.0 and helmet@2.1.0. Thanks for your contribution!

@mfinifter mfinifter deleted the mfinifter:dynamic-report-only-setting branch May 19, 2016

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Aug 5, 2016

Member

@mfinifter I'm making a new website for Helmet.js and I want to credit everyone who's contributed. Do you have a name and/or website you'd like me to use to credit you?

Member

EvanHahn commented Aug 5, 2016

@mfinifter I'm making a new website for Helmet.js and I want to credit everyone who's contributed. Do you have a name and/or website you'd like me to use to credit you?

@mfinifter

This comment has been minimized.

Show comment
Hide comment
@mfinifter

mfinifter Aug 7, 2016

Contributor

You can credit me as Matthew Finifter, if you'd like.

Cheers.

Contributor

mfinifter commented Aug 7, 2016

You can credit me as Matthew Finifter, if you'd like.

Cheers.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Aug 7, 2016

Member

Added you to the list! Stay tuned for the new Helmet website.

Member

EvanHahn commented Aug 7, 2016

Added you to the list! Stay tuned for the new Helmet website.

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