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

Should include sameSite cookie attribute in write interface #36

Closed
bsittler opened this issue Sep 8, 2016 · 26 comments
Closed

Should include sameSite cookie attribute in write interface #36

bsittler opened this issue Sep 8, 2016 · 26 comments

Comments

@bsittler
Copy link
Contributor

bsittler commented Sep 8, 2016

The 'SameSite' cookie attribute (draft: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00 ) has received some public exposure, e.g. http://www.sjoerdlangkemper.nl/2016/04/14/preventing-csrf-with-samesite-cookie-attribute/

The attribute is live in Chrome: https://www.chromestatus.com/feature/4672634709082112

It is being tracked for possible inclusion in Firefox too: https://bugzilla.mozilla.org/show_bug.cgi?id=795346

The possible values for this attribute in WebIDL should probably be a nullable three-valued enumeration:

  • sameSite: 'no_restriction' or sameSite: null - the default behavior, equivalent to no SameSite attribute in Set-Cookie
  • sameSite: 'strict' - equivalent to SameSite=Strict in Set-Cookie, the cookie will only be sent in same-site contexts
  • sameSite: 'lax' - equivalent to SameSite=Lax in Set-Cookie, the cookie will be sent in same-site contexts and in top-level navigations
@bsittler
Copy link
Contributor Author

bsittler commented Sep 8, 2016

What do you think, @mikewest ? Is this a reasonable interface for this feature?

@bsittler
Copy link
Contributor Author

bsittler commented Sep 8, 2016

@domenic - any advice on how to handle the maybe-tristate value in WebIDL? Is nullability with two enum values better, or is three enum values better? Is null-as-explicit-default a reasonable way to write that case?

@FagnerMartinsBrack
Copy link

X-Ref: js-cookie/js-cookie#264

@annevk
Copy link
Collaborator

annevk commented Sep 9, 2016

Just enum is better than nullable enum. Use undefined to get the default and let null keep its default behavior (probably ends up throwing).

@domenic
Copy link

domenic commented Sep 9, 2016

As @annevk says. Although I think it'd be cool to change the default to 'strict', in which case you'd need a tri-state enum.

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

I was concerned that defaulting to same site 'strict' would break
everything due to its producing cookieless top-level navigation. Am I too
paranoid on that point?

On Fri, Sep 9, 2016, 08:45 Domenic Denicola notifications@github.com
wrote:

As @annevk https://github.com/annevk says. Although I think it'd be
cool to change the default to 'strict', in which case you'd need a
tri-state enum.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#36 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD3R5bAwUgsTxJ0_uzClIyLYoeXXqKgks5qoX8ZgaJpZM4J4fzo
.

@domenic
Copy link

domenic commented Sep 9, 2016

I mean, it wouldn't break existing pages, and new pages that use this would notice the breakage and use another enum value. But maybe it would cause too much hair-pulling during that process. Would be curious to hear what others think.

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

I suppose another possibility would be to bifurcate set into setCrossSiteCookie and setSameSiteCookie which would make the choice conscious and explicit for every single call site. What do you think? There whould still be a parameter to get the intermediate 'lax' behavior in setSameSiteCookie, though - perhaps a Boolean sameSiteLax: true or sameSiteStrict: false for the more-permissive behavior.

@domenic
Copy link

domenic commented Sep 9, 2016

Meh, that doesn't seem like much of an improvement to me due to complicating all call sites.

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

What if it were set/setShared instead?

On Sep 9, 2016 11:18, "Domenic Denicola" notifications@github.com wrote:

Meh, that doesn't seem like much of an improvement to me due to
complicating all call sites.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#36 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD3R3bOHQ8BLpvE1k_xMnTGNXlyqQsXks5qoaMNgaJpZM4J4fzo
.

@domenic
Copy link

domenic commented Sep 9, 2016

Why is that better than set(...) vs. set(..., { shared: "strict" })?

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

Actually I like your proposal better :)

Also, I think making shared: 'strict' (ditto for 'lax') the default is really problematic because when the script is running in a shared context it won't be able to read the cookies it writes, by default

@domenic
Copy link

domenic commented Sep 9, 2016

Hmm, what is a shared context and when would script be running in it?

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

I refer to cross-site IFRAME documents, as described in https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-2.1.1

If I follow the description correctly, scripts running in cross-site IFRAME contexts won't be able to see SameSite cookies at all.

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

Assuming my understanding of that is correct, I guess we could potentially default to SameSite 'strict' or SameSite 'lax' when the document and all ancestors (assuming any exist) are in the same registrable domain, and otherwise default to a non-SameSite cookie. However this may be exposing new information to scripts (whether they and all parents are in the same registrable domain) which AFAIK they cannot determine today without cross-origin document.domain collusion

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

Also, it's unclear to me whether script running in a top-level document which is itself the result of a top-level navigation should be able to see SameSite 'strict' cookies.

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

Further complications:

  • when intercepting a Fetch or Foreign Fetch, a ServiceWorker should perhaps be able to tell whether the fetch was initiated from a SameSite 'strict', SameSite 'lax', or non-SameSite context.
  • Likewise cookie reading and monitoring APIs for ServiceWorkers and SameSite 'strict' documents should perhaps be able to specify which of the three possible views they will get;
  • likewise SameSite 'lax' documents should perhaps be able to specify which of the two possible views they will get.

I suppose a simple way to handle all of this would be to provide separate interest registration methods for each view (for ServiceWorker monitoring) and separate CookieStore instances for each view

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

@metromoxie @mikewest can you clarify a few points about SameSite cookies?

  • When should document.cookie reveal them? I've been looking at Chrome's implementation and at the draft RFC, but I'm unsure of whether the implementation is correct and the draft RFC doesn't seem to explicitly describe script-level read access
  • Also, is there any way for script to determine which SameSite bucket a given document falls into? (lax, strict, or neither?)

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

@mkruisselbrink any opinions on how this interacts with Fetch and Foreign-Fetch interception? Ditto with Link: ... rel=serviceworker, should that cause the resulting ServiceWorker to run in a non-SameSite mode?

@mkruisselbrink
Copy link
Collaborator

@mkruisselbrink any opinions on how this interacts with Fetch and Foreign-Fetch interception? Ditto with Link: ... rel=serviceworker, should that cause the resulting ServiceWorker to run in a non-SameSite mode?

I don't think that how a service worker was originally installed should influence how future fetches made by that service worker are done, that just seems confusing. Not sure how any of the same site cookie stuff is supposed to interact with service workers. For foreign fetch possibly just treating any fetches the SW makes as not associated with any particular document makes the most sense (as just fetch(event.request) in a foreign fetch handler is already going to result in different behavior compared to the foreign fetch worker not existing, as it turns a cross origin fetch into a same origin fetch).

@bsittler
Copy link
Contributor Author

bsittler commented Sep 9, 2016

For that matter it's not clear to me how same-site cookies should interact with scripts and script-initiated requests generally. Are script-initiated same-origin fetches inside non-samesite IFRAMEs explicit enough that it's safe to imbue them with full samesite ambient authority?

@pwnall
Copy link
Collaborator

pwnall commented May 31, 2018

Picking up the discussion here again -- I think Async Cookies needs to support setting the SameSite attribute, for feature parity with document.cookie. https://crbug.com/848062 will track implementing the outcome of this discussion in Chrome.

I'd like to have strict as the default SameSite value, to follow the principle of picking good defaults. I think this requires a tri-state enum, which means we'll have to settle on the "shared" value (lax and strict are covered in RFC 6265bis).

How about sameSite = "no_restriction" | "lax" | "strict" ?

Alternatives for the third value, with reasons why I think they're worse.

  • "not" - less clear than no_restriction, but it does feel more like a keyword
  • "shared" - it's not clear which of shared and lax is stronger
  • "none" - maps well to the RFC, but sameSite: "none" is weird

Renaming the attribute to sameSiteRestrictions = "none" | "lax" | "strict" looks more readable, at the expense of making the name quite verbose. Renaming introduces the extra cognitive complexity of adding an alias to the RFC name.

@annevk @domenic @inexorabletash Thoughts?

@annevk
Copy link
Collaborator

annevk commented May 31, 2018

"exclude"? I'd definitely keep a shorter attribute. And note that if you need a separator in an enum value, it has to be -: https://w3ctag.github.io/design-principles/#casing-rules.

@pwnall
Copy link
Collaborator

pwnall commented May 31, 2018

@annevk To clarify -- do you mean that exclude is a possible value for sameSite's third value?

@annevk
Copy link
Collaborator

annevk commented May 31, 2018

Yes, though I think "none" is reasonable too and is quite popular as a value for this kind of thing in Fetch at least.

@domenic
Copy link

domenic commented Jun 22, 2018

Drive-by thought: sameSite = "unrestricted" | "lax" | "strict"

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

No branches or pull requests

7 participants