Skip to content

fix(proxy): preserve HTTP env proxy request target#40395

Merged
Skn0tt merged 1 commit into
microsoft:mainfrom
magus:noah/proxy-invalid-url
Apr 27, 2026
Merged

fix(proxy): preserve HTTP env proxy request target#40395
Skn0tt merged 1 commit into
microsoft:mainfrom
magus:noah/proxy-invalid-url

Conversation

@magus
Copy link
Copy Markdown
Contributor

@magus magus commented Apr 24, 2026

Summary

Fixes HTTP proxy handling in httpRequest() when proxy settings come from environment variables.

When http_proxy/HTTP_PROXY is set and NO_PROXY is empty, connectOverCDP("http://localhost:<port>") performs CDP discovery through the proxy by requesting /json/version/.

In the broken path, Playwright mutates the proxy URL pathname

parsedProxyURL.pathname = url.toString();

Because URL.pathname normalizes the value as a path, this serializes the request target as

GET /http://localhost:<port>/json/version/ HTTP/1.1

For HTTP proxying, the request target must remain absolute-form

GET http://localhost:<port>/json/version/ HTTP/1.1

This patch stores the original target URL in options.path instead, preserving the correct proxy request target while still sending the request to the proxy server.

Related context

Testing

npx playwright test \
--config=tests/library/playwright.config.ts \
--project=chromium-library \
tests/library/chromium/connect-over-cdp.spec.ts \
-g "should use env proxy with connectOverCDP discovery request" \
--reporter=line

Before the fix, the regression test fails with

- "http://localhost:<port>/json/version/"
+ "/http://localhost:<port>/json/version/"

After the fix, the test passes


fixes #40394

@magus
Copy link
Copy Markdown
Contributor Author

magus commented Apr 24, 2026

@magus please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@ agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@ agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@ agree company="Microsoft"

Contributor License Agreement

@ agree company="OpenAI"

@magus
Copy link
Copy Markdown
Contributor Author

magus commented Apr 24, 2026

@magus please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="OpenAI"

Copy link
Copy Markdown
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

Hi Noah, thanks for the PR. The change looks good, what a subtle bug :/

Could you shorten the test a little? This should be good, I think:

test('should use env proxy with connectOverCDP discovery request', async ({ browserType, server, proxyServer, mode }) => {
  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/40394' });
  test.skip(mode !== 'default'); // Out of process transport does not allow us to set env vars dynamically.
  proxyServer.forwardTo(server.PORT);

  const oldValue = process.env.HTTP_PROXY;
  try {
    process.env.HTTP_PROXY = proxyServer.URL;
    await browserType.connectOverCDP(server.PREFIX).catch(() => {});
    expect(proxyServer.requestUrls).toEqual([`${server.PREFIX}/json/version/`]);
  } finally {
    process.env.HTTP_PROXY = oldValue;
  }
});

The HTTPS_PROXY env var doesn't matter in this context.

@magus magus force-pushed the noah/proxy-invalid-url branch from 3c518d7 to 56f6bbf Compare April 24, 2026 21:05
@magus
Copy link
Copy Markdown
Contributor Author

magus commented Apr 24, 2026

Hi Noah, thanks for the PR. The change looks good, what a subtle bug :/

Could you shorten the test a little? This should be good, I think:

test('should use env proxy with connectOverCDP discovery request', async ({ browserType, server, proxyServer, mode }) => {
  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/40394' });
  test.skip(mode !== 'default'); // Out of process transport does not allow us to set env vars dynamically.
  proxyServer.forwardTo(server.PORT);

  const oldValue = process.env.HTTP_PROXY;
  try {
    process.env.HTTP_PROXY = proxyServer.URL;
    await browserType.connectOverCDP(server.PREFIX).catch(() => {});
    expect(proxyServer.requestUrls).toEqual([`${server.PREFIX}/json/version/`]);
  } finally {
    process.env.HTTP_PROXY = oldValue;
  }
});

The HTTPS_PROXY env var doesn't matter in this context.

Done, thanks for simplifying!

@github-actions
Copy link
Copy Markdown
Contributor

Test results for "MCP"

2 failed
❌ [webkit] › mcp/dashboard.spec.ts:185 › should start dashboard and annotate when no dashboard is running @mcp-macos-latest
❌ [webkit] › mcp/dashboard.spec.ts:207 › should enter annotate mode on fresh dashboard.tsx mount with -s --annotate @mcp-macos-latest

6675 passed, 928 skipped


Merge workflow run.

@github-actions
Copy link
Copy Markdown
Contributor

Test results for "tests 1"

10 flaky ⚠️ [chromium-library] › library/chromium/connect-over-cdp.spec.ts:449 › should be able to connect via localhost `@ubuntu-22.04-chromium-tip-of-tree`
⚠️ [chromium-library] › library/chromium/connect-over-cdp.spec.ts:449 › should be able to connect via localhost `@chromium-ubuntu-22.04-arm-node20`
⚠️ [chromium-library] › library/video.spec.ts:719 › screencast › should work with video+trace `@chromium-ubuntu-22.04-arm-node20`
⚠️ [chromium-library] › library/chromium/connect-over-cdp.spec.ts:449 › should be able to connect via localhost `@chromium-ubuntu-22.04-node24`
⚠️ [chromium-library] › library/popup.spec.ts:261 › should not throw when click closes popup `@chromium-ubuntu-22.04-node24`
⚠️ [chromium-library] › library/chromium/connect-over-cdp.spec.ts:449 › should be able to connect via localhost `@chromium-ubuntu-22.04-node20`
⚠️ [chromium-library] › library/video.spec.ts:476 › screencast › should capture static page in persistent context @smoke `@chromium-ubuntu-22.04-node20`
⚠️ [chromium-library] › library/chromium/connect-over-cdp.spec.ts:449 › should be able to connect via localhost `@chromium-ubuntu-22.04-node22`
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1080 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node20`
⚠️ [playwright-test] › ui-mode-test-output.spec.ts:118 › should collapse repeated console messages for test `@ubuntu-latest-node22`

41452 passed, 847 skipped


Merge workflow run.

@Skn0tt Skn0tt merged commit 48798d9 into microsoft:main Apr 27, 2026
37 of 38 checks passed
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.

[Regression]: connectOverCDP sends malformed request target with HTTP_PROXY

2 participants