Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter validation on samesite configuration #398

Open
vcsjones opened this issue Sep 28, 2018 · 1 comment
Open

Stricter validation on samesite configuration #398

vcsjones opened this issue Sep 28, 2018 · 1 comment

Comments

@vcsjones
Copy link
Member

vcsjones commented Sep 28, 2018

Hi! 馃憢

We started using SameSite configuration on a few of our cookies, I misread the docs, goofed, and did this:

samesite: {
  strict: ['butter_cookie']
}

This does not cause a startup error, it just doesn't work, silently.

In other cases when I have made mistakes, secure_headers told me I was doing something wrong, e.g. Unknown config directive: dogs=["better"]. This caused a bit of head scratching because I have come to expect nice errors from here when I make a mistake (thanks for that!)

Is there a use where samesite.strict can be set to an array and do something useful? If not, does it make sense to validate that the strict key can only be assigned to a TrueClass, FalseClass, or a Hash via is_a??

Along the same lines, if it is a hash, it can also silently fail like so

samesite: {
  strict: {
    rly: ['biscotti_cookie']
  }
}

So that leads me to a question of what is the philosophy around validation for configuration? I'm happy to take a stab at a PR to improve validation around cookie configuration, but I wanted to get my head around what should be done, first.

@oreoshake
Copy link
Contributor

oreoshake commented Sep 29, 2018

I'm sorry, the correct answer was "oreo_cookie". That's what unlocks all security features :trollface:

Historically, the validation was incredibly valuable. Nowadays, because the validation hasn't kept up with the set of valid configurations, it's limits functionality that can only be fixed in a pull request. It's a trade off.

Currently, the only validation is that a hash is supplied: https://github.com/twitter/secure_headers/blob/90597531a705ddb5fe9cd3ec222191786068dc27/lib/secure_headers/utils/cookies_config.rb#L23

"Surely you wrote a test ensuring the functionality works as expected and your cookies are extra secured" but I don't like calling people names. I like validation. I don't see this spec changing too frequently. Philosophically I think adding validation for this would be nice. It would be really nice to move away from a nested hash structure to an object but I'll settle for something basic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants