From 64e7557fb9052a14b02161a9e341e7b350d5c004 Mon Sep 17 00:00:00 2001 From: Ross Wollman Date: Mon, 24 Jan 2022 16:06:36 -0700 Subject: [PATCH] fix: falsey behavior in route.continue, page.post, testInfo.attach (#11421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In several of the Playwright APIs, falsey values were not handled correctly. This changeset adds tests (and some fixes): - route.continue: If options.postData was the empty string, the continue failed to override the post data. - page.post (application/json with options.data: false|''|0|null): Raw falsey values were getting dropped (i.e. you can't do the equivalent of curl --header application/json … -d 'false'). This has been fixed with most values across all browsers, but an additional fix is needed for 'null' which the channel serializer treats extra specially. - testInfo.attach: This didn't get reported as an error when options.path was the empty string, but should have been. #11413 (and its fix #11414) inspired this search as they are the same class of bug. --- .../playwright-core/src/client/network.ts | 2 +- .../src/dispatchers/networkDispatchers.ts | 2 +- packages/playwright-core/src/server/fetch.ts | 4 +-- packages/playwright-test/src/dispatcher.ts | 2 +- packages/playwright-test/src/workerRunner.ts | 2 +- tests/browsercontext-route.spec.ts | 25 +++++++++++++++++++ tests/global-fetch.spec.ts | 7 +++++- .../reporter-attachment.spec.ts | 13 ++++++++++ 8 files changed, 50 insertions(+), 7 deletions(-) diff --git a/packages/playwright-core/src/client/network.ts b/packages/playwright-core/src/client/network.ts index c3471c03ef..4938cb1425 100644 --- a/packages/playwright-core/src/client/network.ts +++ b/packages/playwright-core/src/client/network.ts @@ -78,7 +78,7 @@ export class Request extends ChannelOwner implements ap if (this._redirectedFrom) this._redirectedFrom._redirectedTo = this; this._provisionalHeaders = new RawHeaders(initializer.headers); - this._postData = initializer.postData ? Buffer.from(initializer.postData, 'base64') : null; + this._postData = initializer.postData !== undefined ? Buffer.from(initializer.postData, 'base64') : null; this._timing = { startTime: 0, domainLookupStart: -1, diff --git a/packages/playwright-core/src/dispatchers/networkDispatchers.ts b/packages/playwright-core/src/dispatchers/networkDispatchers.ts index 6889072886..8607912a32 100644 --- a/packages/playwright-core/src/dispatchers/networkDispatchers.ts +++ b/packages/playwright-core/src/dispatchers/networkDispatchers.ts @@ -123,7 +123,7 @@ export class RouteDispatcher extends Dispatcher im url: params.url, method: params.method, headers: params.headers, - postData: params.postData ? Buffer.from(params.postData, 'base64') : undefined, + postData: params.postData !== undefined ? Buffer.from(params.postData, 'base64') : undefined, }); } diff --git a/packages/playwright-core/src/server/fetch.ts b/packages/playwright-core/src/server/fetch.ts index bf51a954fd..0f83928bc5 100644 --- a/packages/playwright-core/src/server/fetch.ts +++ b/packages/playwright-core/src/server/fetch.ts @@ -579,7 +579,7 @@ function isJsonParsable(value: any) { function serializePostData(params: channels.APIRequestContextFetchParams, headers: { [name: string]: string }): Buffer | undefined { assert((params.postData ? 1 : 0) + (params.jsonData ? 1 : 0) + (params.formData ? 1 : 0) + (params.multipartData ? 1 : 0) <= 1, `Only one of 'data', 'form' or 'multipart' can be specified`); - if (params.jsonData) { + if (params.jsonData !== undefined) { const json = isJsonParsable(params.jsonData) ? params.jsonData : JSON.stringify(params.jsonData); headers['content-type'] ??= 'application/json'; return Buffer.from(json, 'utf8'); @@ -599,7 +599,7 @@ function serializePostData(params: channels.APIRequestContextFetchParams, header } headers['content-type'] ??= formData.contentTypeHeader(); return formData.finish(); - } else if (params.postData) { + } else if (params.postData !== undefined) { headers['content-type'] ??= 'application/octet-stream'; return Buffer.from(params.postData, 'base64'); } diff --git a/packages/playwright-test/src/dispatcher.ts b/packages/playwright-test/src/dispatcher.ts index 9edccad679..a6ec8881c9 100644 --- a/packages/playwright-test/src/dispatcher.ts +++ b/packages/playwright-test/src/dispatcher.ts @@ -204,7 +204,7 @@ export class Dispatcher { name: a.name, path: a.path, contentType: a.contentType, - body: typeof a.body !== 'undefined' ? Buffer.from(a.body, 'base64') : undefined + body: a.body !== undefined ? Buffer.from(a.body, 'base64') : undefined })); result.status = params.status; test.expectedStatus = params.expectedStatus; diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index 19b5f70f33..9ec0e56901 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerRunner.ts @@ -267,7 +267,7 @@ export class WorkerRunner extends EventEmitter { attach: async (name: string, options: { path?: string, body?: string | Buffer, contentType?: string } = {}) => { if ((options.path !== undefined ? 1 : 0) + (options.body !== undefined ? 1 : 0) !== 1) throw new Error(`Exactly one of "path" and "body" must be specified`); - if (options.path) { + if (options.path !== undefined) { const hash = calculateSha1(options.path); const dest = testInfo.outputPath('attachments', hash + path.extname(options.path)); await fs.promises.mkdir(path.dirname(dest), { recursive: true }); diff --git a/tests/browsercontext-route.spec.ts b/tests/browsercontext-route.spec.ts index a448211c98..46576d5408 100644 --- a/tests/browsercontext-route.spec.ts +++ b/tests/browsercontext-route.spec.ts @@ -212,3 +212,28 @@ it('should support the times parameter with route matching', async ({ context, p await page.goto(server.EMPTY_PAGE); expect(intercepted).toHaveLength(1); }); + +it('should overwrite post body with empty string', async ({ context, server, page, browserName }) => { + await context.route('**/empty.html', route => { + route.continue({ + postData: '', + }); + }); + + const [req] = await Promise.all([ + server.waitForRequest('/empty.html'), + page.setContent(` + + `), + ]); + + const body = (await req.postBody).toString(); + expect(body).toBe(''); +}); diff --git a/tests/global-fetch.spec.ts b/tests/global-fetch.spec.ts index e15fb32697..1ca5e138cf 100644 --- a/tests/global-fetch.spec.ts +++ b/tests/global-fetch.spec.ts @@ -266,12 +266,17 @@ it('should remove content-length from reidrected post requests', async ({ playwr }); -const serialization = [ +const serialization: [string, any][] = [ ['object', { 'foo': 'bar' }], ['array', ['foo', 'bar', 2021]], ['string', 'foo'], + ['string (falsey)', ''], ['bool', true], + ['bool (false)', false], ['number', 2021], + ['number (falsey)', 0], + ['null', null], + ['literal string undefined', 'undefined'], ]; for (const [type, value] of serialization) { const stringifiedValue = JSON.stringify(value); diff --git a/tests/playwright-test/reporter-attachment.spec.ts b/tests/playwright-test/reporter-attachment.spec.ts index 095f300a7f..ade6795ba6 100644 --- a/tests/playwright-test/reporter-attachment.spec.ts +++ b/tests/playwright-test/reporter-attachment.spec.ts @@ -103,6 +103,19 @@ test(`testInfo.attach errors`, async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); }); +test(`testInfo.attach errors with empty path`, async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.js': ` + const { test } = pwt; + test('fail', async ({}, testInfo) => { + await testInfo.attach('name', { path: '' }); + }); + `, + }, { reporter: 'line', workers: 1 }); + expect(stripAscii(result.output)).toMatch(/Error: ENOENT: no such file or directory, copyfile ''/); + expect(result.exitCode).toBe(1); +}); + test(`testInfo.attach error in fixture`, async ({ runInlineTest }) => { const result = await runInlineTest({ 'a.test.js': `