fix(browse): defensive hardening for cookie-import CDP pipe path

Three narrow robustness fixes flagged during review:

1. CdpPipeTransport.readStream 'end' handler now sets this.closed = true
   before aborting pending. Without this, a send() after end would queue
   a promise forever (the 'closed' guard wouldn't trigger).

2. extractCookiesViaCdpPipe guards cookiesResp.cookies with ?? [] so
   a CDP response missing the cookies field returns an empty array
   instead of throwing TypeError.

3. extractCookiesViaCdpPipe validates attached.sessionId is a non-empty
   string. flatten:true should always return one, but defensive guards
   beat silent fallback to the browser-level session.

Three new tests lock each fix:
- send-after-end rejects with cdp_error/closed
- empty getAllCookies response returns []
- attachToTarget without sessionId throws cdp_error

Refs #1136.
This commit is contained in:
Nicolas Gomes Ferreira Dos Santos 2026-04-23 21:32:25 -07:00
parent 7df4295f10
commit 952a3220fd
2 changed files with 55 additions and 2 deletions

View File

@ -989,6 +989,7 @@ export class CdpPipeTransport {
});
this.readStream.on('end', () => {
if (!this.closed) {
this.closed = true;
this.abortAll(new CookieImportError('CDP read stream ended unexpectedly', 'cdp_error'));
}
});
@ -1113,7 +1114,13 @@ export async function extractCookiesViaCdpPipe(
targetId: pageTarget.targetId,
flatten: true,
});
const sessionId = attached.sessionId as string;
const sessionId = attached.sessionId as string | undefined;
if (typeof sessionId !== 'string' || sessionId.length === 0) {
throw new CookieImportError(
'Target.attachToTarget returned no sessionId',
'cdp_error',
);
}
await transport.send('Network.enable', undefined, sessionId);
const cookiesResp = await transport.send('Network.getAllCookies', undefined, sessionId);
@ -1126,7 +1133,7 @@ export async function extractCookiesViaCdpPipe(
}
const result: PlaywrightCookie[] = [];
for (const c of cookiesResp.cookies as CdpCookie[]) {
for (const c of (cookiesResp.cookies ?? []) as CdpCookie[]) {
if (!domainSet.has(c.domain)) continue;
result.push({
name: c.name,

View File

@ -649,6 +649,25 @@ describe('CdpPipeTransport', () => {
expect(caught!.code).toBe('cdp_error');
});
test('send after read-stream end rejects immediately (closed flag flipped)', async () => {
const pipes = makeMockPipes();
const t = new CdpPipeTransport(pipes.writeToChild, pipes.readFromChild);
pipes.closeFromChild();
// Give the 'end' handler a tick to fire.
await flush();
let caught: any = null;
try {
await t.send('Network.enable');
} catch (e) {
caught = e;
}
expect(caught).toBeInstanceOf(CookieImportError);
expect(caught.code).toBe('cdp_error');
expect(caught.message.toLowerCase()).toContain('closed');
});
test('send with explicit sessionId includes it in the frame', async () => {
const pipes = makeMockPipes();
const t = new CdpPipeTransport(pipes.writeToChild, pipes.readFromChild);
@ -784,6 +803,33 @@ describe('extractCookiesViaCdpPipe', () => {
expect(cookies).toEqual([]);
expect(transport.sent).toHaveLength(0);
});
test('returns empty array when Network.getAllCookies response has no cookies field', async () => {
const transport = new ScriptedTransport();
transport.script('Target.getTargets', { targetInfos: [{ targetId: 'P', type: 'page' }] });
transport.script('Target.attachToTarget', { sessionId: 'S1' });
transport.script('Network.enable', {});
transport.script('Network.getAllCookies', {}); // no 'cookies' field
const cookies = await extractCookiesViaCdpPipe(transport as any, ['example.com']);
expect(cookies).toEqual([]);
});
test('throws cdp_error when attachToTarget returns no sessionId', async () => {
const transport = new ScriptedTransport();
transport.script('Target.getTargets', { targetInfos: [{ targetId: 'P', type: 'page' }] });
transport.script('Target.attachToTarget', {}); // missing sessionId
let caught: any = null;
try {
await extractCookiesViaCdpPipe(transport as any, ['example.com']);
} catch (e) {
caught = e;
}
expect(caught).toBeInstanceOf(CookieImportError);
expect(caught.code).toBe('cdp_error');
expect(caught.message.toLowerCase()).toContain('sessionid');
});
});
describe('buildCdpSpawnArgs', () => {