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

SameSite Cookies #128

Merged
merged 10 commits into from Oct 8, 2019
Merged

SameSite Cookies #128

merged 10 commits into from Oct 8, 2019

Conversation

stash
Copy link
Collaborator

@stash stash commented Sep 25, 2018

Support for SameSite cookies, see #80 and also the other 6265bis issues.

@stash stash force-pushed the same-site branch 2 times, most recently from bf5a285 to a870e53 Compare November 26, 2018 23:03
@stash stash changed the title WIP: Same site cookies SameSite Cookies Nov 26, 2018
@stash stash requested review from ruoho and awaterma November 26, 2018 23:49
@stash
Copy link
Collaborator Author

stash commented Nov 26, 2018

@ruoho @awaterma this is ready for review.

@awaterma
Copy link
Member

Tried to take a look this evening; a bit tired out. I'll pick it up tomorrow and see if I can get this sorted for you.

Copy link
Member

@awaterma awaterma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider what the draft says about the Same-Site attribute, and whether it being set to 'none' or Null will cause the cookie to be ignored.

4.1.2.7. The SameSite Attribute
The "SameSite" attribute limits the scope of the cookie such that it
will only be attached to requests if those requests are same-site, as
defined by the algorithm in Section 5.2. For example, requests for
"https://example.com/sekrit-image" will attach same-site cookies if
and only if initiated from a context whose "site for cookies" is
"example.com".
If the "SameSite" attribute's value is "Strict", the cookie will only
be sent along with "same-site" requests. If the value is "Lax", the
cookie will be sent with same-site requests, and with "cross-site"
top-level navigations, as described in Section 5.3.7.1. If the
"SameSite" attribute's value is neither of these, the cookie will be
ignored.

if (value === 'none' || value === 'lax' || value === 'strict') {
return value;
} else {
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return 'none' here instead of null? Null will have the same effect as none, on the cookie, but it since we support none, as a a value, it seems more consistent.

If the "SameSite" attribute's value is "Strict", the cookie will only
be sent along with "same-site" requests. If the value is "Lax", the
cookie will be sent with same-site requests, and with "cross-site"
top-level navigations, as described in Section 5.3.7.1. If the
"SameSite" attribute's value is neither of these, the cookie will be
ignored.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of null here is simply that it wasn't one of the three valid values. It also forces the value to lowercase for checking in other parts of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awaterma Check out the two calls to this method; it's just used to branch into returning an Error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that; I think you're definitely right about the return value.

@@ -725,6 +753,7 @@ Cookie.prototype.domain = null;
Cookie.prototype.path = null;
Cookie.prototype.secure = false;
Cookie.prototype.httpOnly = false;
Cookie.prototype.sameSite = "none";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be set? Won't setting this to 'none' make the cookie ignored by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should stick a comment on this about this step in Section 5.4:

  1. If the cookie-attribute-list contains an attribute with an
    attribute-name of "SameSite", set the cookie's same-site-flag to
    attribute-value (i.e. either "Strict" or "Lax"). Otherwise, set
    the cookie's same-site-flag to "None".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, have that in the code below already.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that. :) No worries.

@awaterma
Copy link
Member

I may be misreading the RFC. But I think it merits some discussion.

@stash
Copy link
Collaborator Author

stash commented Jan 7, 2019

@awaterma I re-read your comments and I think this is good to go as-is. Left a comment. Let me know if that makes sense.

lib/cookie.js Show resolved Hide resolved
@stash stash dismissed awaterma’s stale review February 5, 2019 03:19

Approved during meeting today (I had no further changes)

@awaterma awaterma added this to To do in 4.x Release Jul 1, 2019
@awaterma awaterma moved this from To do to In progress in 4.x Release Aug 21, 2019
@TimothyGu
Copy link

What's the status of this PR?

@awaterma
Copy link
Member

@TimothyGu -- unchanged. I think @ShivanKaul will take a look sometime soon.

@ShivanKaul
Copy link
Contributor

@TimothyGu - it was good to go until another PR got merged :D I'll resolve conflicts and merge this week.

@ShivanKaul ShivanKaul merged commit 9cdd54e into master Oct 8, 2019
4.x Release automation moved this from In progress to Done Oct 8, 2019
acomagu added a commit to acomagu/DefinitelyTyped that referenced this pull request Dec 4, 2019
acomagu added a commit to acomagu/DefinitelyTyped that referenced this pull request Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
4.x Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants