Bug 198181 - Cookies with SameSite=None or SameSite=invalid treated as Strict
Summary: Cookies with SameSite=None or SameSite=invalid treated as Strict
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-23 09:48 PDT by Rowan Merewood
Modified: 2019-10-17 17:02 PDT (History)
15 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rowan Merewood 2019-05-23 09:48:12 PDT
Cookies specifying `SameSite=None` or sending an invalid value, e.g. `SameSite=nonsense` appear to be treated as `Strict` and are therefore not being sent with cross-site requests as intended.

There is this recent bug: https://bugs.webkit.org/show_bug.cgi?id=196927 which appears related and also references https://httpwg.org/http-extensions/rfc6265bis.html#the-samesite-attribute-1 for the correct behaviour.

You can see this demonstrated in this example:

1. Set a range of cookies with different `SameSite` values:
https://peaceful-wing.glitch.me/setcookies

2. Verify those cookies are set in a same-site context:
https://peaceful-wing.glitch.me/

3. See which cookies are sent in a cross-site context:
https://merewood.org/samesite.html

From the example site's output
Expected cookies in the cross-site context:
{
    "remote-site-unspecified": "unspecified",
    "remote-site-none-nosecure": "none",
    "remote-site-none-secure": "none-secure",
    "remote-site-nonsense": "nonsense"
}

Actual cookies in the cross-site context:
{
    "remote-site-unspecified": "unspecified"
}
Comment 1 Mike West 2019-05-23 13:08:36 PDT
The patch in https://bugs.webkit.org/show_bug.cgi?id=196927 is only addressing the way cookies are rendered in WebKit's Inspector. I suspect that the underlying behavior is in `NSHTTPCookie`, and will require someone with access to that code to dig into the parser a bit.

FWIW, Chrome, Firefox, and Edge (old and new) all treat `SameSite=UnknownValue` as though the `SameSite` attribute wasn't present. That's fairly forward-compatible for cases in which we decide collectively that a new value is reasonable to add.

Would it be possible for y'all to revisit this behavior? It's going to make shipping https://tools.ietf.org/html/draft-west-cookie-incrementalism-00 somewhat difficult for developers to handle without carving out special behavior for Safari, for example.

/cc dbates@, wilander@, joepeck@.
Comment 2 Daniel Bates 2019-05-23 15:12:02 PDT
(In reply to Mike West from comment #1)
> The patch in https://bugs.webkit.org/show_bug.cgi?id=196927 is only
> addressing the way cookies are rendered in WebKit's Inspector. I suspect
> that the underlying behavior is in `NSHTTPCookie`, and will require someone
> with access to that code to dig into the parser a bit.
> 

Yep, and it is has already been fixed if I am remembering correctly. Fix may not have shipped, yet.

> FWIW, Chrome, Firefox, and Edge (old and new) all treat
> `SameSite=UnknownValue` as though the `SameSite` attribute wasn't present.
> That's fairly forward-compatible for cases in which we decide collectively
> that a new value is reasonable to add.
> 
> Would it be possible for y'all to revisit this behavior? It's going to make
> shipping https://tools.ietf.org/html/draft-west-cookie-incrementalism-00
> somewhat difficult for developers to handle without carving out special
> behavior for Safari, for example.
> 

Wait, I haven’t seen “None” before. If this does something other than nothing (i.e. implementer does not even need to parse ‘n’, ‘o’, ‘n’, ‘e’, then ^^^ is enough. If there any other visible change then CFNetwork will need to be involved and we need a radar, which you can create yourself assigned to the right place (I think) at bugreport.apple.com. Ehh, as I type this, I guess its not much work to CC the WebKit-radar-importer and then do a radar dance Internally 😕

> /cc dbates@, wilander@, joepeck@.
Comment 3 Daniel Bates 2019-05-23 15:20:00 PDT
(In reply to Daniel Bates from comment #2)
> (In reply to Mike West from comment #1)
> > The patch in https://bugs.webkit.org/show_bug.cgi?id=196927 is only
> > addressing the way cookies are rendered in WebKit's Inspector. I suspect
> > that the underlying behavior is in `NSHTTPCookie`, and will require someone
> > with access to that code to dig into the parser a bit.
> > 
> 
> Yep, and it is has already been fixed if I am remembering correctly. Fix may
> not have shipped, yet.
> 

Fixed in <rdar://problem/42290578>. Not shipped. 🤷‍♂️ when.
Comment 4 Mike West 2019-05-23 22:15:52 PDT
(In reply to Daniel Bates from comment #2)
> (In reply to Mike West from comment #1)
> > The patch in https://bugs.webkit.org/show_bug.cgi?id=196927 is only
> > addressing the way cookies are rendered in WebKit's Inspector. I suspect
> > that the underlying behavior is in `NSHTTPCookie`, and will require someone
> > with access to that code to dig into the parser a bit.
> > 
> 
> Yep, and it is has already been fixed if I am remembering correctly. Fix may
> not have shipped, yet.

Does "fixed" mean that `Set-Cookie: name=value; SameSite=InvalidValue` is interpreted as `Set-Cookie: name=value;` (which is what this bug is asking for)? Or as `Set-Cookie: name=value; SameSite=Strict` (which is what that Inspector patch does)? I'm hoping for the former. :)

> Wait, I haven’t seen “None” before. If this does something other than
> nothing (i.e. implementer does not even need to parse ‘n’, ‘o’, ‘n’, ‘e’,
> then ^^^ is enough.

`SameSite=None` is intended to be a no-op in the status quo. That is, it assumes the first interpretation of "fixed" above, wherein `SameSite=None` is equivalent to omitting the `SameSite` attribute entirely.

I'll paste in a little more context here from the thread you started elsewhere:

"""
That document proposes that `SameSite=Lax` is a better default than we have today, and positions `None` as an explicit way of opting into the status quo. `SameSite=None` should be a no-op based on the above PR, and it seemed like an elegant, backwards-compatible way of allowing developers to explicitly express their intent for a given cookie to be delivered cross-site. I talked about the general direction with folks at the recent HTTP Workshop, and there seemed to be reasonable agreement that incrementally shifting cookes' defaults would be a good way to make progress.

If Safari's able to change the underlying parsing behavior in the near future, this seems like a path we can pursue. If not, we might need to introduce another attribute in order to get that behavior in a way that doesn't break in other browsers.
"""

> If there any other visible change then CFNetwork will
> need to be involved and we need a radar, which you can create yourself
> assigned to the right place (I think) at bugreport.apple.com. Ehh, as I type
> this, I guess its not much work to CC the WebKit-radar-importer and then do
> a radar dance Internally 😕

Thank you. :)
Comment 5 Daniel Bates 2019-05-23 23:15:31 PDT
(In reply to Mike West from comment #4)
> (In reply to Daniel Bates from comment #2)
> > (In reply to Mike West from comment #1)
> > > The patch in https://bugs.webkit.org/show_bug.cgi?id=196927 is only
> > > addressing the way cookies are rendered in WebKit's Inspector. I suspect
> > > that the underlying behavior is in `NSHTTPCookie`, and will require someone
> > > with access to that code to dig into the parser a bit.
> > > 
> > 
> > Yep, and it is has already been fixed if I am remembering correctly. Fix may
> > not have shipped, yet.
> 
> Does "fixed" mean that `Set-Cookie: name=value; SameSite=InvalidValue` is
> interpreted as `Set-Cookie: name=value;` (which is what this bug is asking
> for)? Or as `Set-Cookie: name=value; SameSite=Strict` (which is what that
> Inspector patch does)?

Then it sounds like the inspector patch is wrong (I haven’t looked at it).


> I'm hoping for the former. :)
> 

The former.

> > Wait, I haven’t seen “None” before. If this does something other than
> > nothing (i.e. implementer does not even need to parse ‘n’, ‘o’, ‘n’, ‘e’,
> > then ^^^ is enough.
> 
> `SameSite=None` is intended to be a no-op in the status quo. That is, it
> assumes the first interpretation of "fixed" above, wherein `SameSite=None`
> is equivalent to omitting the `SameSite` attribute entirely.
> 

Ok.

> I'll paste in a little more context here from the thread you started
> elsewhere:
> 
> """
> That document proposes that `SameSite=Lax` is a better default than we have
> today, and positions `None` as an explicit way of opting into the status
> quo. `SameSite=None` should be a no-op based on the above PR, and it seemed
> like an elegant, backwards-compatible way of allowing developers to
> explicitly express their intent for a given cookie to be delivered
> cross-site. I talked about the general direction with folks at the recent
> HTTP Workshop, and there seemed to be reasonable agreement that
> incrementally shifting cookes' defaults would be a good way to make progress.
>

Ok.
 
> If Safari's able to change the underlying parsing behavior in the near
> future, this seems like a path we can pursue. If not, we might need to
> introduce another attribute in order to get that behavior in a way that
> doesn't break in other browsers.
> """
> 

Lost the context... What does Safari need to change? Can you please re-contextify me 🙂.
Comment 6 Mike West 2019-05-24 00:20:13 PDT
(In reply to Daniel Bates from comment #5)
> Then it sounds like the inspector patch is wrong (I haven’t looked at it).

+joepeck@.

> Lost the context... What does Safari need to change? Can you please
> re-contextify me 🙂.

It sounds like shipping the fix from rdar://problem/42290578 would be sufficient. Just because I'm not familiar with y'all's release process: does a `CFNetwork` update require an OS update, or do you bundle it with Safari pushes? Likewise, I imagine it would require changes in both macOS and iOS?

I recognize that it's difficult to make predictions about the future, but we're trying to figure out a reasonable timeline for shifting to `SameSite=Lax` by default, and it would be lovely to not break Safari users as we do it. If this is a fix that will be widely-deployed soon, great! If not, we might need to rethink our plans (possibly requiring adding more complexity to cookies with some additional attribute that Safari would safely ignore). I'd kinda like to avoid doing that if possible...
Comment 7 Joseph Pecoraro 2019-05-24 11:48:54 PDT
> > > Yep, and it is has already been fixed if I am remembering correctly. Fix may
> > > not have shipped, yet.

Correct, I don't believe the CFNetwork change has shipped yet. I waited to change Web Inspector until our Network stack made the change. 


> > Does "fixed" mean that `Set-Cookie: name=value; SameSite=InvalidValue` is
> > interpreted as `Set-Cookie: name=value;` (which is what this bug is asking
> > for)? Or as `Set-Cookie: name=value; SameSite=Strict` (which is what that
> > Inspector patch does)?
>
> Then it sounds like the inspector patch is wrong (I haven’t looked at it).
> 
> 
> > I'm hoping for the former. :)
> > 
> 
> The former.

Web Inspector should be doing the former:
https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/Models/Cookie.js#L117


```
    static parseSameSiteAttributeValue(attributeValue)
    {
        if (!attributeValue)
            return WI.Cookie.SameSiteType.None;

        switch (attributeValue.toLowerCase()) {
        case "lax":
            return WI.Cookie.SameSiteType.Lax;
        case "strict":
            return WI.Cookie.SameSiteType.Strict;
        }

        return WI.Cookie.SameSiteType.None;
    }
```

We also have tests:
https://trac.webkit.org/browser/trunk/LayoutTests/inspector/unit-tests/cookie.html#L173

```
    // SameSite with unknown value is ignored.
    test(`name=value; SameSite=invalid`, {
        name: "name",
        value: "value",
        expires: null,
        maxAge: null,
        path: null,
        domain: null,
        secure: false,
        httpOnly: false,
        sameSite: WI.Cookie.SameSiteType.None,
    });
```

This was all done in:
https://bugs.webkit.org/show_bug.cgi?id=196927

    commit 10b4574f710b67f24dc0b219b9c0f67a2014f101
    Author: pecoraro@apple.com <pecoraro@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
    Date:   Mon Apr 15 21:53:48 2019 +0000

        Web Inspector: SameSite parsing should be stricter
        https://bugs.webkit.org/show_bug.cgi?id=196927
        <rdar://problem/42291601>

--

• Are you seeing otherwise?
• Are you using Safari Technology Preview?
Comment 8 Joseph Pecoraro 2019-05-24 11:52:44 PDT
In STPv83 if I load:
https://peaceful-wing.glitch.me/setcookies

I see "–" in the SameSite column for the `Set-Cookie: remote-site-nonsense=nonsense; SameSite=Nonsense` response cookie.
Comment 9 Mike West 2019-05-28 01:03:27 PDT
(In reply to Joseph Pecoraro from comment #8)
> In STPv83 if I load:
> https://peaceful-wing.glitch.me/setcookies
> 
> I see "–" in the SameSite column for the `Set-Cookie:
> remote-site-nonsense=nonsense; SameSite=Nonsense` response cookie.

Great! In that case, I think I just misread the patch, thanks for the correction; I'm happy to see this works exactly the way I thought it didn't. :)

We still need to wait for an OS update in order to pull in the networking side of the fix, though, correct?
Comment 10 Joseph Pecoraro 2019-05-28 10:37:49 PDT
> We still need to wait for an OS update in order to pull in the networking
> side of the fix, though, correct?

Correct.
Comment 11 Rowan Merewood 2019-06-25 10:32:01 PDT
This does look to be fixed in the iOS 13 beta (which is grand 🎉). Is there any view as to whether this will make it into 12?
Comment 12 John Wilander 2019-06-25 15:35:01 PDT
(In reply to Rowan Merewood from comment #11)
> This does look to be fixed in the iOS 13 beta (which is grand 🎉). Is there
> any view as to whether this will make it into 12?

I don't think the CFNetwork change is in the iOS 12.4 beta but iOS 12.4 beta 5 was released recently if you want to check: https://developer.apple.com/documentation/ios_ipados_release_notes/ios_12_4_beta_5_release_notes
Comment 13 Joseph Pecoraro 2019-06-26 11:25:51 PDT
Great! Can we mark this as resolved?
Comment 14 Mike West 2019-06-28 03:13:57 PDT
(In reply to Joseph Pecoraro from comment #13)
> Great! Can we mark this as resolved?

Yes. It's fixed in y'all's codebase. Now we're just waiting for deployment. Thank you!

Note that Chrome is currently targeting ~M80 (February) to push a `SameSite=None` requirement on third-party cookies (https://www.chromestatus.com/feature/5088147346030592). That seems like it'll be enough time for a critical mass of Safari users to upgrade to macOS 10.15 and iOS 13. It still leaves folks whose devices supported Safari 12 but won't support Safari 13 in a somewhat uncomfortable position, and we're likely going to need to recommend UA sniffing to avoid harm to that population.

If y'all could work out a way to get the underlying network stack change into a point release of either or both macOS 10.14 and iOS 12, it would both make that timeline more solid, and potentially allow us to be more aggressive with the timeline generally with less risk.

Thanks again for your help here!
Comment 15 Radar WebKit Bug Importer 2019-06-28 03:14:16 PDT
<rdar://problem/52320694>
Comment 16 John Wilander 2019-06-28 07:52:33 PDT
(In reply to Mike West from comment #14)
> (In reply to Joseph Pecoraro from comment #13)
> > Great! Can we mark this as resolved?
> 
> Yes. It's fixed in y'all's codebase. Now we're just waiting for deployment.
> Thank you!
> 
> Note that Chrome is currently targeting ~M80 (February) to push a
> `SameSite=None` requirement on third-party cookies
> (https://www.chromestatus.com/feature/5088147346030592). That seems like
> it'll be enough time for a critical mass of Safari users to upgrade to macOS
> 10.15 and iOS 13. It still leaves folks whose devices supported Safari 12
> but won't support Safari 13 in a somewhat uncomfortable position, and we're
> likely going to need to recommend UA sniffing to avoid harm to that
> population.
> 
> If y'all could work out a way to get the underlying network stack change
> into a point release of either or both macOS 10.14 and iOS 12, it would both
> make that timeline more solid, and potentially allow us to be more
> aggressive with the timeline generally with less risk.
> 
> Thanks again for your help here!

I filed rdar://52330350 for investigating a backport of the network stack change.
Comment 17 Lily Chen 2019-08-22 15:19:09 PDT
Hi, is there any update on the status of the backport for this fix?
Comment 18 billy.richardson 2019-09-11 15:28:56 PDT
(In reply to John Wilander from comment #16)
> I filed rdar://52330350 for investigating a backport of the network stack
> change.

Hello John, was a backport feasible for this fix? Will this be backported to iOS 12? Thanks!
Comment 19 John Wilander 2019-09-11 15:32:11 PDT
(In reply to billy.richardson from comment #18)
> (In reply to John Wilander from comment #16)
> > I filed rdar://52330350 for investigating a backport of the network stack
> > change.
> 
> Hello John, was a backport feasible for this fix? Will this be backported to
> iOS 12? Thanks!

Lily and Billy, unfortunately it's not looking good for a backport. We argued the case and the people responsible for the HTTP stack understand the issue. But I think it's wise for you to take into consideration that we may never see support for SameSite=None on iOS 12 (and back).
Comment 20 Kalle 2019-10-09 05:15:29 PDT
This will be a major headache for anyone who needs to use SameSite=None

Does anyone have any good suggestions on what to use for user agent sniffing to exclude all devices that treat SameSite=None as SameSite=Strict?
Comment 21 Robert Slaney 2019-10-16 18:39:38 PDT
We need firm advice on how to detect non-confirming versions of Safari.

UA sniffing is fragile at best and I don't want to have to exclude more than I need to
Comment 22 John Wilander 2019-10-16 19:58:02 PDT
(In reply to Robert Slaney from comment #21)
> We need firm advice on how to detect non-confirming versions of Safari.
> 
> UA sniffing is fragile at best and I don't want to have to exclude more than
> I need to

We understand the concern, but this is not caused by WebKit or CFNetwork. This is Chrome planning to change the default behavior of cookies and nicely asking us if we can backport the change to support them. You should primarily bring your concerns to the Chrome team.
Comment 23 Robert Slaney 2019-10-17 16:56:13 PDT
Hey John...

I appreciate the position that Chrome devs have put us all in.  I have an end user base that I am accountable to and I can whinge till the cows come home but it won't make a difference.

I don't need this fix to be backported but I need to know when I have to change our systems' behaviour and UA sniffing is the only reasonable answer at this point.

so....

Is there a range of version numbers of Safari, MacOS, and/or iOS that I need to detect so I do NOT supply the SameSite=None cookie flag when required.

To be honest, the Safari product set is not our primary browser segment so if I ( and I suspect a majority of corporates are in the same position ) have to put a stake in the ground then the users of non-conforming browsers will be impacted.
Comment 24 billy.richardson 2019-10-17 17:02:17 PDT
Hey Robert,
My team and I are in the same position as you.

I am currently not sending the SameSite cookie attribute to the following User Agents:

^.*iPhone; CPU iPhone OS 1[0-2].*$
^.*iPad; CPU OS 1[0-2].*$
^.*iPod touch; CPU iPhone OS 1[0-2].*$
^.*Macintosh; Intel Mac OS X.*Version\/1[0-2].*Safari.*$

This has been in place in our Production for several weeks without any user complaints. As always, you should verify yourself too :)

Regards,
Billy Richardson