Skip to content

fix(network): request.postData() returns null for empty string body override#41882

Open
tsushanth wants to merge 3 commits into
microsoft:mainfrom
tsushanth:fix-postdata-empty-string-null
Open

fix(network): request.postData() returns null for empty string body override#41882
tsushanth wants to merge 3 commits into
microsoft:mainfrom
tsushanth:fix-postdata-empty-string-null

Conversation

@tsushanth

Copy link
Copy Markdown

Summary

request.postData() incorrectly returns null when the POST body is set to an empty string via route.continue({ postData: '' }) or route.fallback({ postData: '' }).

Root cause

In packages/playwright-core/src/client/network.ts, the postData() method uses || null at the end:

// before
postData(): string | null {
  return (this._fallbackOverrides.postDataBuffer || this._initializer.postData)?.toString('utf-8') || null;
}

When the override is set to an empty string, Buffer.from('', 'utf-8') is stored in _fallbackOverrides.postDataBuffer. Since an empty Buffer is a truthy object, the left-hand side resolves correctly to the empty buffer, and .toString('utf-8') yields ''. However, '' || null evaluates to null, discarding the empty-string body.

This is inconsistent with postDataBuffer(), which returns the correct empty Buffer for the same input.

Fix

Switch the trailing || null to ?? null (nullish coalescing), which only falls back to null when the value is null or undefined, not when it is an empty string:

// after
postData(): string | null {
  return (this._fallbackOverrides.postDataBuffer ?? this._initializer.postData)?.toString('utf-8') ?? null;
}

The inner || is similarly changed to ?? so that a zero-length override buffer isn't accidentally skipped in favour of the original request's body.

Reproduction

test('postData returns empty string for empty body override', async ({ page }) => {
  await page.route('**/api', async route => {
    await route.continue({ postData: '' });
  });

  page.on('request', request => {
    // Before fix: null  After fix: ''
    expect(request.postData()).toBe('');
  });

  await page.evaluate(() => fetch('/api', { method: 'POST', body: 'original' }));
});

@dcrousso dcrousso left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a good fix (minus the \n issue) but it needs a test

Comment on lines +224 to +225
].join('
'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops? i think you may have accidentally replaced all \n with an actual newline elsewhere in this file

@tsushanth

Copy link
Copy Markdown
Author

Thanks for catching those — addressed in the latest push:

  • Restored the three escaped \n sequences that were accidentally converted to literal newlines (.join('\n'), rewriteErrorMessage, get() header join)
  • Added a test in tests/page/page-request-continue.spec.ts that overrides a POST body with '' and asserts request.postData() returns '' rather than null

Comment on lines +1022 to +1025
page.on('request', request => {
if (request.url().includes('empty-post'))
captured.push(request.postData());
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think this will work as im pretty sure it runs before the route applies

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "MCP"

3 failed
❌ [chrome] › mcp/annotate.spec.ts:57 › should capture multiple screenshots in one annotation @mcp-windows-latest-chrome
❌ [chrome] › mcp/annotate.spec.ts:110 › should abort annotation when last screenshot is removed @mcp-windows-latest-chrome
❌ [webkit] › mcp/cli-killall.spec.ts:42 › kill-all kills filtered dashboard pid @mcp-ubuntu-latest-webkit

7757 passed, 1249 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

9 failed
❌ [chromium-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @frozen-time-library-chromium-linux
❌ [chromium-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @chromium-ubuntu-22.04-arm-node20
❌ [chromium-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @realtime-time-library-chromium-linux
❌ [chromium-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @chromium-ubuntu-22.04-node24
❌ [chromium-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @chromium-ubuntu-22.04-node20
❌ [chromium-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @chromium-ubuntu-22.04-node22
❌ [firefox-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @firefox-ubuntu-22.04-node20
❌ [webkit-page] › page/page-request-continue.spec.ts:1014 › postData should return empty string when overriding body with empty string @webkit-ubuntu-22.04-node20
❌ [playwright-test] › worker-index.spec.ts:324 › should respect project.workers>1 @windows-latest-node22

4 flaky ⚠️ [chromium-library] › library/video.spec.ts:664 › screencast › should capture full viewport `@frozen-time-library-chromium-linux`
⚠️ [chromium-library] › library/browsercontext-page-event.spec.ts:173 › should work with Ctrl-clicking `@chromium-ubuntu-22.04-arm-node20`
⚠️ [chromium-library] › library/video.spec.ts:736 › screencast › should work with video+trace `@chromium-ubuntu-22.04-node22`
⚠️ [firefox-library] › library/inspector/cli-codegen-3.spec.ts:255 › cli codegen › should generate frame locators (4) `@firefox-ubuntu-22.04-node20`

49987 passed, 1189 skipped


Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants