Use SameSite=None for canary cookie. #24670
Conversation
With Chrome 78, cookies will be set with SameSite=Lax by default. This prevents the canary/rc cookie from being sent when the page is not on the cdn.ampproject.org domain when requesting v0.js. Use SameSite=None to set the cookie, working around iOS, which does not treat it correctly by not setting the SameSite value. - Include v0.js on the experiments page, so the cookies code can access the platform service.
tools/experiments/experiments.html
Outdated
| execution order so that the AMP_CONFIG from v0.js does not interfere | ||
| with the one from the cache-busted v0.js loaded via experiments.js. | ||
| --> | ||
| <script defer src="https://cdn.ampproject.org/v0.js"></script> |
There was a problem hiding this comment.
Why include the v0.js script here?
There was a problem hiding this comment.
It is needed to get the Platform service, otherwise the Services.platformFor(win) returns null.
src/cookies.js
Outdated
| } | ||
|
|
||
| const platform = Services.platformFor(win); | ||
| const isWebkit = platform.isIos() || platform.isSafari(); |
There was a problem hiding this comment.
Is Chrome's behavior on iOS also broken?
There was a problem hiding this comment.
Yes, but after researching this a bit more,it looks like the issue is fixed in iOS 13, which was just recently released. For the experiments page, I think it would be fine to just omit the platform check and have this cookie not work correctly on iOS 12. I'm less familiar with the other uses of setCookie in cid-impl and amp-analytics. For those usages, I assume we would want to check for iOS at least until 14 is released.
There was a problem hiding this comment.
Actually, on second thought, I'm not sure samesite even matters for those cookies (they are set on the domain itself, not for a subresource), so it is probably okay to just remove the platform checks.
src/cookies.js
Outdated
| const platform = Services.platformFor(win); | ||
| const isWebkit = platform.isIos() || platform.isSafari(); | ||
|
|
||
| if (isWebkit && sameSite == SameSite.NONE) { |
There was a problem hiding this comment.
Nit: We can skip UA checks if we do the sameSite check first.
src/cookies.js
Outdated
| @@ -14,6 +14,7 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| import {Services} from './services'; | |||
There was a problem hiding this comment.
I don't know if this is safe. When is the platform code first installed, and where is the first setCookie call?
There was a problem hiding this comment.
I'm not sure it is safe. The platform service is installed as one of the first things in the first startup chunk here:
Lines 91 to 94 in 1c1e722
The first call to setCookie on the page is when the user clicks button to opt in/out of canary (or RC). Since the scripts use defer, we can be sure that v0 runs on this page prior to the code that listens to the button click is registered, but since the platform service install is in a startup chunk, I'm pretty sure there is a window where the user could click the button first and have the event listener present, before the macro task for the first startup chunk is run.
There was a problem hiding this comment.
It's not the experiments page that I'm worried about, but ads/analytics on a regular amp page that calls setCookie.
There was a problem hiding this comment.
So since this is fixed in iOS 13, and I think only the experiments page needs samesite=None, how does removing the platform check sound? I'll update the PR with that for now.
With Chrome 78 (beta) and Chrome 80 (stable), cookies will be set with
SameSite=Laxby default. This prevents the canary/rc cookie from being sent when the page is not on the cdn.ampproject.org domain when requesting v0.js.Fixes #24643
/cc @jridgewell