Adds secure, httponly, and SameSite=lax to cookies by default #341
Conversation
|
@stve you're the most familiar with this code. Can you take a look 👀? |
There was a problem hiding this comment.
@anglinb nice work!
I think this all looks good with one point we may need to discuss further.
Main question is how we want to handle a configuration that doesn't specify secure or httponly. If a custom configuration is used, do defaults still get applied or should a custom config supercede any defaults?
@oreoshake how is this handled elsewhere within the configuration?
| @@ -5,37 +5,42 @@ module SecureHeaders | |||
| describe Cookie do | |||
| let(:raw_cookie) { "_session=thisisatest" } | |||
|
|
|||
| it "does not tamper with cookies when unconfigured" do | |||
| cookie = Cookie.new(raw_cookie, {}) | |||
| it "does not tamper with cookies when using OPT_OUT is used" do | |||
There was a problem hiding this comment.
👍 for adding support for SecureHeaders::OPT_OUT this was a HUGE miss on my part in the initial implementation.
lib/secure_headers/headers/cookie.rb
Outdated
| @@ -16,6 +16,11 @@ def validate_config!(config) | |||
|
|
|||
| def initialize(cookie, config) | |||
| @raw_cookie = cookie | |||
| unless config == SecureHeaders::OPT_OUT | |||
| config ||= {} | |||
There was a problem hiding this comment.
I wonder if it would make sense to define the default as { secure: true, httponly: true} as opposed to checking for nil in the configuration.
I'm of the opinion that if a configuration is defined (even if it disables secure or httponly), you shouldn't have to opt-out of each configuration explicitly.
There was a problem hiding this comment.
Per #341 (comment), I think we should go with
config[:secure] = true unless config[:secure] == OPT_OUT
config[:httponly] = true unless config[:httponly] == OPT_OUT
You should have to explicitly say |
@anglinb can you add tests for when |
|
@anglinb sorry for not identifying the |
|
@oreoshake @stve Thanks for taking a look! I'll address your feedback this afternoon--totally missed these notifications 🙃 |
|
While you're at it... let's default /me ducks Break all the things. I'll add a post-install message to broadcast this change (along with the others). |
anglinb
force-pushed
the
update_cookie_defaults
branch
from
54f677f
to
810e2c1
Compare
|
@oreoshake Sounds good. Will take care of the two changes:
I'm still going to allow the global, OPT_OUT. So if |
👍 |
|
^ Just added |
oreoshake
changed the title
Adds
secureandhttponlyflags to cookies by default, targeting a 4.0 release tracked by #286./CC @stve @oreoshake
Outstanding Questions:
Should 4.0 also include
SameSite=Lax?Suggested CHANGELOG entry
(I'm not sure if I should add this to the actual CHANGELOG file b/c I don't know which release this will get pulled into)
All Prs