From eababcf511b354ee0ef841ed45fed31ff5351e81 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 24 May 2024 16:21:13 -0700 Subject: [PATCH] fix(webkit): make cookie SameSite=Lax by default Reference https://github.com/microsoft/playwright/issues/30305 Reference https://github.com/microsoft/playwright-browsers/issues/795 --- .../src/server/webkit/wkBrowser.ts | 11 +++++- tests/config/browserTest.ts | 4 +- .../browsercontext-add-cookies.spec.ts | 4 +- tests/library/browsercontext-cookies.spec.ts | 37 ++++++++++--------- tests/library/browsercontext-fetch.spec.ts | 2 +- 5 files changed, 33 insertions(+), 25 deletions(-) diff --git a/packages/playwright-core/src/server/webkit/wkBrowser.ts b/packages/playwright-core/src/server/webkit/wkBrowser.ts index 4b10dfd3fe..b0c516d1ce 100644 --- a/packages/playwright-core/src/server/webkit/wkBrowser.ts +++ b/packages/playwright-core/src/server/webkit/wkBrowser.ts @@ -256,7 +256,11 @@ export class WKBrowserContext extends BrowserContext { async doGetCookies(urls: string[]): Promise { const { cookies } = await this._browser._browserSession.send('Playwright.getAllCookies', { browserContextId: this._browserContextId }); return network.filterCookies(cookies.map((c: channels.NetworkCookie) => { - const copy: any = { ... c }; + const copy: any = { ...c }; + // "Prevent cross-site tracking." is on by default in Safary, which essentially + // disables SameSite=None and such cookies become Lax. + if (copy.sameSite === 'None') + copy.sameSite = 'Lax'; copy.expires = c.expires === -1 ? -1 : c.expires / 1000; delete copy.session; return copy as channels.NetworkCookie; @@ -267,7 +271,10 @@ export class WKBrowserContext extends BrowserContext { const cc = network.rewriteCookies(cookies).map(c => ({ ...c, session: c.expires === -1 || c.expires === undefined, - expires: c.expires && c.expires !== -1 ? c.expires * 1000 : c.expires + expires: c.expires && c.expires !== -1 ? c.expires * 1000 : c.expires, + // With "Prevent cross-site tracking." on by default in Safary, SameSite=None cookies + // behave as Lax. + sameSite: (!c.sameSite || c.sameSite === 'None') ? 'Lax' : c.sameSite, })) as Protocol.Playwright.SetCookieParam[]; await this._browser._browserSession.send('Playwright.setCookies', { cookies: cc, browserContextId: this._browserContextId }); } diff --git a/tests/config/browserTest.ts b/tests/config/browserTest.ts index 02555d42eb..8dbf9a31a9 100644 --- a/tests/config/browserTest.ts +++ b/tests/config/browserTest.ts @@ -67,11 +67,11 @@ const test = baseTest.extend await run(false); }, { scope: 'worker' }], - defaultSameSiteCookieValue: [async ({ browserName, browserMajorVersion, channel }, run) => { + defaultSameSiteCookieValue: [async ({ browserName, browserMajorVersion, channel, platform }, run) => { if (browserName === 'chromium') await run('Lax'); else if (browserName === 'webkit') - await run('None'); + await run('Lax'); else if (browserName === 'firefox' && channel === 'firefox-beta') await run(browserMajorVersion >= 103 && browserMajorVersion < 110 ? 'Lax' : 'None'); else if (browserName === 'firefox' && channel !== 'firefox-beta') diff --git a/tests/library/browsercontext-add-cookies.spec.ts b/tests/library/browsercontext-add-cookies.spec.ts index bdfe14ef0a..b6cd243c79 100644 --- a/tests/library/browsercontext-add-cookies.spec.ts +++ b/tests/library/browsercontext-add-cookies.spec.ts @@ -224,7 +224,7 @@ it('should have |expires| set to |-1| for session cookies', async ({ context, se expect(cookies[0].expires).toBe(-1); }); -it('should set cookie with reasonable defaults', async ({ context, server, browserName }) => { +it('should set cookie with reasonable defaults', async ({ context, server, browserName, defaultSameSiteCookieValue }) => { await context.addCookies([{ url: server.EMPTY_PAGE, name: 'defaults', @@ -239,7 +239,7 @@ it('should set cookie with reasonable defaults', async ({ context, server, brows expires: -1, httpOnly: false, secure: false, - sameSite: browserName === 'chromium' ? 'Lax' : 'None', + sameSite: defaultSameSiteCookieValue, }]); }); diff --git a/tests/library/browsercontext-cookies.spec.ts b/tests/library/browsercontext-cookies.spec.ts index a53f989886..5dbf9e1b84 100644 --- a/tests/library/browsercontext-cookies.spec.ts +++ b/tests/library/browsercontext-cookies.spec.ts @@ -147,7 +147,7 @@ it('should get cookies from multiple urls', async ({ context, browserName, isWin url: 'https://foo.com', name: 'doggo', value: 'woofs', - sameSite: 'None', + sameSite: 'Lax', }, { url: 'https://bar.com', name: 'catto', @@ -168,7 +168,7 @@ it('should get cookies from multiple urls', async ({ context, browserName, isWin expires: -1, httpOnly: false, secure: true, - sameSite: (browserName === 'webkit' && isWindows) ? 'None' : 'Lax', + sameSite: 'Lax', }, { name: 'doggo', value: 'woofs', @@ -177,7 +177,7 @@ it('should get cookies from multiple urls', async ({ context, browserName, isWin expires: -1, httpOnly: false, secure: true, - sameSite: 'None', + sameSite: 'Lax', }])); }); @@ -279,7 +279,7 @@ it('should add cookies with an expiration', async ({ context }) => { url: 'https://foo.com', name: 'doggo', value: 'woofs', - sameSite: 'None', + sameSite: 'Lax', expires, }]); const cookies = await context.cookies(['https://foo.com']); @@ -292,7 +292,7 @@ it('should add cookies with an expiration', async ({ context }) => { expires, httpOnly: false, secure: true, - sameSite: 'None', + sameSite: 'Lax', }]); { // Rollover to 5-digit year @@ -300,14 +300,14 @@ it('should add cookies with an expiration', async ({ context }) => { url: 'https://foo.com', name: 'doggo', value: 'woofs', - sameSite: 'None', + sameSite: 'Lax', expires: 253402300799, // Fri, 31 Dec 9999 23:59:59 +0000 (UTC) }]); await expect(context.addCookies([{ url: 'https://foo.com', name: 'doggo', value: 'woofs', - sameSite: 'None', + sameSite: 'Lax', expires: 253402300800, // Sat, 1 Jan 1000 00:00:00 +0000 (UTC) }])).rejects.toThrow(/Cookie should have a valid expires/); } @@ -316,7 +316,7 @@ it('should add cookies with an expiration', async ({ context }) => { url: 'https://foo.com', name: 'doggo', value: 'woofs', - sameSite: 'None', + sameSite: 'Lax', expires: -42, }])).rejects.toThrow(/Cookie should have a valid expires/); }); @@ -350,26 +350,26 @@ it('should be able to send third party cookies via an iframe', async ({ browser, } }); -it('should support requestStorageAccess', async ({ page, server, channel, browserName, isMac, isLinux, isWindows }) => { +it('should support requestStorageAccess', async ({ browser, httpsServer, channel, browserName, isLinux, isMac }) => { + const page = await browser.newPage({ ignoreHTTPSErrors: true }); it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/17285' }); it.skip(browserName === 'chromium', 'requestStorageAccess API is not available in Chromium'); it.skip(channel === 'firefox-beta', 'hasStorageAccess returns true, but no cookie is sent'); - - server.setRoute('/set-cookie.html', (req, res) => { - res.setHeader('Set-Cookie', 'name=value; Path=/'); + httpsServer.setRoute('/set-cookie.html', (req, res) => { + res.setHeader('Set-Cookie', 'name=value; Path=/; SameSite=None; Secure'); res.end(); }); // Navigate once to the domain as top level. - await page.goto(server.CROSS_PROCESS_PREFIX + '/set-cookie.html'); - await page.goto(server.EMPTY_PAGE); - await page.setContent(``); + await page.goto(httpsServer.CROSS_PROCESS_PREFIX + '/set-cookie.html'); + await page.goto(httpsServer.EMPTY_PAGE); + await page.setContent(``); const frame = page.frames()[1]; if (browserName === 'firefox') { expect(await frame.evaluate(() => document.hasStorageAccess())).toBeTruthy(); { const [serverRequest] = await Promise.all([ - server.waitForRequest('/title.html'), + httpsServer.waitForRequest('/title.html'), frame.evaluate(() => fetch('/title.html')) ]); expect(serverRequest.headers.cookie).toBe('name=value'); @@ -381,7 +381,7 @@ it('should support requestStorageAccess', async ({ page, server, channel, browse expect(await frame.evaluate(() => document.hasStorageAccess())).toBeFalsy(); { const [serverRequest] = await Promise.all([ - server.waitForRequest('/title.html'), + httpsServer.waitForRequest('/title.html'), frame.evaluate(() => fetch('/title.html')) ]); if (!isMac && browserName === 'webkit') @@ -393,12 +393,13 @@ it('should support requestStorageAccess', async ({ page, server, channel, browse expect(await frame.evaluate(() => document.hasStorageAccess())).toBeTruthy(); { const [serverRequest] = await Promise.all([ - server.waitForRequest('/title.html'), + httpsServer.waitForRequest('/title.html'), frame.evaluate(() => fetch('/title.html')) ]); expect(serverRequest.headers.cookie).toBe('name=value'); } } + await page.close(); }); it('should parse cookie with large Max-Age correctly', async ({ server, page, defaultSameSiteCookieValue, browserName, platform }) => { diff --git a/tests/library/browsercontext-fetch.spec.ts b/tests/library/browsercontext-fetch.spec.ts index f4088359cf..be111aff64 100644 --- a/tests/library/browsercontext-fetch.spec.ts +++ b/tests/library/browsercontext-fetch.spec.ts @@ -1211,7 +1211,7 @@ it('should support set-cookie with SameSite and without Secure attribute over HT }); await page.request.get(server.EMPTY_PAGE); const [cookie] = await page.context().cookies(); - if (browserName === 'chromium' && value === 'None') + if (['chromium', 'webkit'].includes(browserName) && value === 'None') expect(cookie).toBeFalsy(); else if (browserName === 'webkit' && isWindows) expect(cookie.sameSite).toBe('None');