fix: support cookies with empty value (microsoft#9376)

yury-s · web-flow · commit 9164fc71ef06 · 2021-10-07T15:37:47.000-07:00
diff --git a/src/server/browserContext.ts b/src/server/browserContext.ts
@@ -327,7 +327,7 @@ export abstract class BrowserContext extends SdkObject {
 
   async storageState(): Promise<types.StorageState> {
     const result: types.StorageState = {
-      cookies: (await this.cookies()).filter(c => c.value !== ''),
+      cookies: await this.cookies(),
       origins: []
     };
     if (this._origins.size)  {
diff --git a/src/server/network.ts b/src/server/network.ts
@@ -26,9 +26,6 @@ export function filterCookies(cookies: types.NetworkCookie[], urls: string[]): t
   const parsedURLs = urls.map(s => new URL(s));
   // Chromiums's cookies are missing sameSite when it is 'None'
   return cookies.filter(c => {
-    // Firefox and WebKit can return cookies with empty values.
-    if (!c.value)
-      return false;
     if (!parsedURLs.length)
       return true;
     for (const parsedURL of parsedURLs) {
@@ -50,7 +47,6 @@ export function filterCookies(cookies: types.NetworkCookie[], urls: string[]): t
 export function rewriteCookies(cookies: types.SetNetworkCookieParam[]): types.SetNetworkCookieParam[] {
   return cookies.map(c => {
     assert(c.name, 'Cookie should have a name');
-    assert(c.value, 'Cookie should have a value');
     assert(c.url || (c.domain && c.path), 'Cookie should have a url or a domain/path pair');
     assert(!(c.url && c.domain), 'Cookie should have either url or domain');
     assert(!(c.url && c.path), 'Cookie should have either url or path');
diff --git a/tests/browsercontext-add-cookies.spec.ts b/tests/browsercontext-add-cookies.spec.ts
@@ -45,6 +45,24 @@ it('should work with expires=-1', async ({ context, page }) => {
   expect(await page.evaluate(() => document.cookie)).toEqual('username=John Doe');
 });
 
+it('should add cookies with empty value', async ({ context, page, server }) => {
+  await context.addCookies([{
+    name: 'marker',
+    value: '',
+    domain: 'www.example.com',
+    path: '/',
+    expires: -1,
+    httpOnly: false,
+    secure: false,
+    sameSite: 'Lax',
+  }]);
+  await page.route('**/*', route => {
+    route.fulfill({ body: '<html></html>' }).catch(() => {});
+  });
+  await page.goto('https://www.example.com');
+  expect(await page.evaluate(() => document.cookie)).toEqual('marker=');
+});
+
 it('should roundtrip cookie', async ({ context, page, server }) => {
   await page.goto(server.EMPTY_PAGE);
   // @see https://en.wikipedia.org/wiki/Year_2038_problem
diff --git a/tests/browsercontext-cookies.spec.ts b/tests/browsercontext-cookies.spec.ts
@@ -202,14 +202,19 @@ it('should work with subdomain cookie', async ({ context, browserName, isWindows
   }]);
 });
 
-it('should not return cookies with empty value', async ({ context, page, server }) => {
+it('should return cookies with empty value', async ({ context, page, server }) => {
   server.setRoute('/empty.html', (req, res) => {
     res.setHeader('Set-Cookie', 'name=;Path=/');
     res.end();
   });
   await page.goto(server.EMPTY_PAGE);
   const cookies = await context.cookies();
-  expect(cookies.length).toBe(0);
+  expect(cookies).toEqual([
+    expect.objectContaining({
+      name: 'name',
+      value: ''
+    })
+  ]);
 });
 
 it('should return secure cookies based on HTTP(S) protocol', async ({ context, browserName, isWindows }) => {
diff --git a/tests/browsercontext-fetch.spec.ts b/tests/browsercontext-fetch.spec.ts
@@ -229,6 +229,23 @@ it('should add cookies from Set-Cookie header', async ({ context, page, server }
   expect((await page.evaluate(() => document.cookie)).split(';').map(s => s.trim()).sort()).toEqual(['foo=bar', 'session=value']);
 });
 
+it('should support cookie with empty value', async ({ context, page, server }) => {
+  server.setRoute('/setcookie.html', (req, res) => {
+    res.setHeader('Set-Cookie', ['first=']);
+    res.end();
+  });
+  await context.request.get(server.PREFIX + '/setcookie.html');
+  await page.goto(server.EMPTY_PAGE);
+  expect(await page.evaluate(() => document.cookie)).toBe('first=');
+  const cookies = await context.cookies();
+  expect(cookies.map(c => ({ name: c.name, value: c.value }))).toEqual([
+    {
+      name: 'first',
+      value: ''
+    },
+  ]);
+});
+
 it('should not lose body while handling Set-Cookie header', async ({ context, server }) => {
   server.setRoute('/setcookie.html', (req, res) => {
     res.setHeader('Set-Cookie', ['session=value', 'foo=bar; max-age=3600']);
diff --git a/tests/browsercontext-storage-state.spec.ts b/tests/browsercontext-storage-state.spec.ts
@@ -103,3 +103,41 @@ it('should round-trip through the file', async ({ contextFactory }, testInfo) =>
   expect(cookie).toEqual('username=John Doe');
   await context2.close();
 });
+
+it('should capture cookies', async ({ server, context, page, contextFactory }) => {
+  server.setRoute('/setcookie.html', (req, res) => {
+    res.setHeader('Set-Cookie', ['a=b', 'empty=']);
+    res.end();
+  });
+
+  await page.goto(server.PREFIX + '/setcookie.html');
+  expect(await page.evaluate(() => {
+    const cookies = document.cookie.split(';');
+    return cookies.map(cookie => cookie.trim()).sort();
+  })).toEqual([
+    'a=b',
+    'empty=',
+  ]);
+
+  const storageState = await context.storageState();
+  expect(new Set(storageState.cookies)).toEqual(new Set([
+    expect.objectContaining({
+      name: 'a',
+      value: 'b'
+    }),
+    expect.objectContaining({
+      name: 'empty',
+      value: ''
+    })
+  ]));
+  const context2 = await contextFactory({ storageState });
+  const page2 = await context2.newPage();
+  await page2.goto(server.EMPTY_PAGE);
+  expect(await page2.evaluate(() => {
+    const cookies = document.cookie.split(';');
+    return cookies.map(cookie => cookie.trim()).sort();
+  })).toEqual([
+    'a=b',
+    'empty=',
+  ]);
+});
