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

Add option to disable non-standard CSP headers #32

Closed
Ry7n opened this Issue Feb 19, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@Ry7n
Contributor

Ry7n commented Feb 19, 2016

We're currently running with setAllHeaders: true because we'd like to cache the responses, which you can't do if we're dynamically setting headers based on user agent.

Since the nonstandard versions of the CSP header are supported by browsers that are falling out of use, it would be awesome to provide an option to disable dynamic headers completely, and just use the standard one.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Feb 19, 2016

Member

I agree. I've also thought about a version of this module that does no browser sniffing; is that something you'd want?

Member

EvanHahn commented Feb 19, 2016

I agree. I've also thought about a version of this module that does no browser sniffing; is that something you'd want?

@Ry7n

This comment has been minimized.

Show comment
Hide comment
@Ry7n

Ry7n Feb 20, 2016

Contributor

I'm a fan of the entire helmet suite, so I'd be uninterested in a second library. I would, however be happy to send a PR if the maintainers would consider it.

Contributor

Ry7n commented Feb 20, 2016

I'm a fan of the entire helmet suite, so I'd be uninterested in a second library. I would, however be happy to send a PR if the maintainers would consider it.

@Ry7n

This comment has been minimized.

Show comment
Hide comment
@Ry7n

Ry7n Feb 20, 2016

Contributor

The API change I would make would be a new option, setLegacyHeaders which supports three values:

  • "auto" - the current default behavior
  • any other truthy value - equivalent of setAllHeaders: true
  • any defined falsey value - only sets the standard header

The default value would be derived from the setAllHeaders config for backwards compatibility, and setAllHeaders would become deprecated.

Contributor

Ry7n commented Feb 20, 2016

The API change I would make would be a new option, setLegacyHeaders which supports three values:

  • "auto" - the current default behavior
  • any other truthy value - equivalent of setAllHeaders: true
  • any defined falsey value - only sets the standard header

The default value would be derived from the setAllHeaders config for backwards compatibility, and setAllHeaders would become deprecated.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Feb 21, 2016

Member

Ah, sorry—I didn't mean to suggest I'd make another library.

What about this?

// Current default behavior (these are all equivalent)
csp({})
csp({ setLegacyHeaders: true })
csp({ setAllHeaders: false })

// Always set all headers
csp({ setAllHeaders: true })

// Don't set legacy headers; do no browser sniffing
csp({ setLegacyHeaders: false })

// This throws an error
csp({
  setAllHeaders: true,
  setLegacyHeaders: false
})
Member

EvanHahn commented Feb 21, 2016

Ah, sorry—I didn't mean to suggest I'd make another library.

What about this?

// Current default behavior (these are all equivalent)
csp({})
csp({ setLegacyHeaders: true })
csp({ setAllHeaders: false })

// Always set all headers
csp({ setAllHeaders: true })

// Don't set legacy headers; do no browser sniffing
csp({ setLegacyHeaders: false })

// This throws an error
csp({
  setAllHeaders: true,
  setLegacyHeaders: false
})
@Ry7n

This comment has been minimized.

Show comment
Hide comment
@Ry7n

Ry7n Feb 22, 2016

Contributor

Looks good to me.

Contributor

Ry7n commented Feb 22, 2016

Looks good to me.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Feb 22, 2016

Member

Would you be interested in making a pull request?

Member

EvanHahn commented Feb 22, 2016

Would you be interested in making a pull request?

@Ry7n

This comment has been minimized.

Show comment
Hide comment
@Ry7n

Ry7n Feb 24, 2016

Contributor

Sure, I'll give it a shot.

Contributor

Ry7n commented Feb 24, 2016

Sure, I'll give it a shot.

Ry7n added a commit to Ry7n/csp that referenced this issue Feb 24, 2016

Add opt-out for UA parsing
Currently the only option for caching responses from CSP are to use
`setAllHeaders: true`, lest a browser-specific CSP gets cached for the wrong
UA. This PR adds a new option, `setLegacyHeaders`, which will skip UA parsing
if defined and falsey.

This adds a testing dependency on `lodash.pickBy`, which we can remove by
upgrading lodash to the latest version.

Lastly, I added a constants file for header names in `lib/headers.js`. In
order to minimize change, I didn't refactor existing files to use it, but it
might help cut down on duplication and human error potential to use it in
other places.

Fixes helmetjs#32.

Ry7n added a commit to Ry7n/csp that referenced this issue Feb 24, 2016

Add opt-out for UA parsing
Currently the only option for caching responses from CSP are to use
`setAllHeaders: true`, lest a browser-specific CSP gets cached for the wrong
UA. This PR adds a new option, `setLegacyHeaders`, which will skip UA parsing
if defined and falsey.

This adds a testing dependency on `lodash.pickBy`, which we can remove by
upgrading lodash to the latest version.

Lastly, I added a constants file for header names in `lib/headers.js`. In
order to minimize change, I didn't refactor existing files to use it, but it
might help cut down on duplication and human error potential to use it in
other places.

Fixes helmetjs#32.

Ry7n added a commit to Ry7n/csp that referenced this issue Feb 24, 2016

Add opt-out for UA parsing
Currently the only option for caching responses from CSP are to use
`setAllHeaders: true`, lest a browser-specific CSP gets cached for the wrong
UA. This PR adds a new option, `setLegacyHeaders`, which will skip UA parsing
if defined and falsey.

This adds a testing dependency on `lodash.pickBy`, which we can remove by
upgrading lodash to the latest version.

Lastly, I added a constants file for header names in `lib/headers.js`. In
order to minimize change, I didn't refactor existing files to use it, but it
might help cut down on duplication and human error potential to use it in
other places.

Fixes helmetjs#32.
@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Feb 29, 2016

Member

I took bits and pieces of what you did, committed it with you as the author, and added you as a contributor in the package.json. I decided to go with an API of browserSniff: false which eschews all useragent sniffing.

This is released in helmet-csp@1.1.0 and helmet@1.2.0.

Thanks for your help!

Member

EvanHahn commented Feb 29, 2016

I took bits and pieces of what you did, committed it with you as the author, and added you as a contributor in the package.json. I decided to go with an API of browserSniff: false which eschews all useragent sniffing.

This is released in helmet-csp@1.1.0 and helmet@1.2.0.

Thanks for your help!

@EvanHahn EvanHahn closed this Feb 29, 2016

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Aug 5, 2016

Member

@Ry7n 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

@Ry7n 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?

@RCanine

This comment has been minimized.

Show comment
Hide comment
@RCanine

RCanine Aug 6, 2016

Thanks! You can use the name and URL on my Github profile.

RCanine commented Aug 6, 2016

Thanks! You can use the name and URL on my Github profile.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Aug 6, 2016

Member

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

Member

EvanHahn commented Aug 6, 2016

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

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