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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of SameSite to WOCookie #790

Merged
merged 3 commits into from
Aug 8, 2016
Merged

Conversation

darkv
Copy link
Member

@darkv darkv commented Aug 2, 2016

This patch adds the SameSite support to WOCookie.

Full info on what this cookie flag does can be found at https://tools.ietf.org/html/draft-west-first-party-cookies-07 or less technical but better readable at https://www.sjoerdlangkemper.nl/2016/04/14/preventing-csrf-with-samesite-cookie-attribute/. Currently this flag is supported in Chrome 51 and Opera 39.

@darkv darkv added the Wonder7 label Aug 2, 2016
@darkv darkv merged commit f8f5614 into wocommunity:master Aug 8, 2016
@darkv darkv deleted the cookie_patch branch August 8, 2016 06:11
@paulhoadley
Copy link
Contributor

This is very cool Johann.

Say you have an app that stores session ID in cookies:

  1. Is this a viable defence against CSRF?
  2. How would you get WebObjects to use SameSite.STRICT for the wosid cookie?

@darkv
Copy link
Member Author

darkv commented Sep 9, 2016

Hi Paul,

  1. it helps against CSRF but does of course not solve it completely
  2. you could overwrite the method _appendCookieToResponse in your session class and modify the wosid cookie there, should be the easiest way

We could of course introduce a new property and set that value accordingly within Wonder classes automagically.

@paulhoadley
Copy link
Contributor

We could of course introduce a new property and set that value accordingly within Wonder classes automagically.

I think we should definitely do this. It would seem fairly analogous with the handling of the useSecureSessionCookies and useHttpOnlySessionCookies in ERXSession (which I didn't even know existed until five minutes ago). We could add one more property, default to existing behaviour, and add another if-block in _appendCookieToResponse().

I'll add this to my to-do list, unless you've already done it while I've been typing!

@darkv
Copy link
Member Author

darkv commented Sep 9, 2016

I'll add this to my to-do list, unless you've already done it while I've been typing!

Now that you say it, I actually did ;-) see #799

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

Successfully merging this pull request may close these issues.

2 participants