feat(proxy): unify local network proxy behavior #10719

rwoll · 2021-12-04T18:17:17Z

When configuring a proxy, Chromium requires a magic tokens to get some
local network requests to go through the proxy. This has tripped up a
few users, so we make the behavior default to the expected: proxy
everything including the local requests. This matches the other vendors
as well.

NB: This can be disabled via
PLAYWRIGHT_DISABLE_FORCED_CHROMIUM_PROXIED_LOOPBACK=1

Supercedes: #8345
Fixes: #10631

rwoll · 2021-12-04T18:19:00Z

/cc @pavelfeldman Opening this as WIP. Need to look into FF behavior (and clean up a bit). Can you mark CQ1 so I can see if there are any platform-specific quirks (esp. w/ the non-CR browsers…looking at you WK)? Thanks!

rwoll · 2021-12-04T22:10:42Z

Looks like the FF failures have to do with my test, and not PW impl. Looks like our proxyServer is too naïve for FF to startup properly (if used at the launch- but not context-level). The following blows up the newPage call in a funny way logging Error: Parse Error: Invalid method encountered. The only request I see hit the proxy before that is maybe an OCSP related one to http://ocsp.digicert.com/. Need to dig in later…

it.only('debug', async ({ browserType, server, proxyServer }) => {
  server.setRoute('/target.html', async (req, res) => {
    res.end('<html><title>Served by the proxy</title></html>');
  });
  proxyServer.forwardTo(server.PORT);
  const browser = await browserType.launch({
    proxy: { server: `localhost:${proxyServer.PORT}` }
  });
  const page = await browser.newPage();
  await browser.close();
});

rwoll · 2021-12-05T21:29:26Z

Looks like the FF failures have to do with my test, and not PW impl. Looks like our proxyServer is too naïve for FF to startup properly (if used at the launch- but not context-level). The following blows up the newPage call in a funny way logging Error: Parse Error: Invalid method encountered. The only request I see hit the proxy before that is maybe an OCSP related one to http://ocsp.digicert.com/. Need to dig in later…
it.only('debug', async ({ browserType, server, proxyServer }) => {
  server.setRoute('/target.html', async (req, res) => {
    res.end('<html><title>Served by the proxy</title></html>');
  });
  proxyServer.forwardTo(server.PORT);
  const browser = await browserType.launch({
    proxy: { server: `localhost:${proxyServer.PORT}` }
  });
  const page = await browser.newPage();
  await browser.close();
});

Looks like the issue is with trying to proxy the HTTPS/connect requests in proxyServer which originate from browser startup.

rwoll · 2021-12-05T21:40:41Z

Looks like the FF failures have to do with my test, and not PW impl. Looks like our proxyServer is too naïve for FF to startup properly (if used at the launch- but not context-level). The following blows up the newPage call in a funny way logging Error: Parse Error: Invalid method encountered. The only request I see hit the proxy before that is maybe an OCSP related one to http://ocsp.digicert.com/. Need to dig in later…
it.only('debug', async ({ browserType, server, proxyServer }) => {
  server.setRoute('/target.html', async (req, res) => {
    res.end('<html><title>Served by the proxy</title></html>');
  });
  proxyServer.forwardTo(server.PORT);
  const browser = await browserType.launch({
    proxy: { server: `localhost:${proxyServer.PORT}` }
  });
  const page = await browser.newPage();
  await browser.close();
});
Looks like the issue is with trying to proxy the HTTPS/connect requests in proxyServer which originate from browser startup.

Pushed a workaround in latest commit. (Skips rewriting some HTTPS requests FF requires for startup so we don't have to deal with MITMing them better just for the tests.)

tests/browsercontext-proxy.spec.ts

pavelfeldman · 2021-12-06T23:40:46Z

tests/browsercontext-proxy.spec.ts

+        }
+      ]) {
+        it(`${params.description}`, async ({ platform, browserName, contextFactory, server, proxyServer }) => {
+          it.fail(platform !== 'linux' && browserName === 'webkit' && additionalBypass && ['localhost', '127.0.0.1'].includes(params.target), 'WK fails to proxy 127.0.0.1 and localhost if additional bypasses are present');


nit, style: given the complexity of the fail predicate, I'd copy-paste this test 3 times instead.

nit, style: given the complexity of the fail predicate, I'd copy-paste this test 3 times instead.

@pavelfeldman how strong of a nit is this? Hehe

I found the more complex fail predicate much easier to read/maintain/edit than trying to find the subtle differences in the otherwise lots of setup in each of the cases.

Mind if I revert d307a87 before merging?

how strong of a nit is this?

Not too strong, do it the way you like.

Happy to merge as is if this is ready.

Thanks @pavelfeldman! Just reverted! Ready for a merge whenever assuming tests pass.

rwoll · 2021-12-08T07:33:57Z

Fixed up the tests based on the CQ1 run. Ready for another run. (Ran all the proxy tests locally {macOS, Linux/Docker} x {headless, head full}. Don’t have a windows box to verifies this will pass now, but they should :)

rwoll · 2021-12-08T17:09:49Z

@mxschmitt Thanks for toggling the label. Looks like npm ci exploded on a lot of the bots and didn’t even run the tests. Is that just runner flakiness, or should I rebase and try again?

mxschmitt · 2021-12-08T23:33:03Z

@mxschmitt Thanks for toggling the label. Looks like npm ci exploded on a lot of the bots and didn’t even run the tests. Is that just runner flakiness, or should I rebase and try again?

Seems like a temporary GitHub/NPM outage, I'd recommend to rebase.

When configuring a proxy, Chromium requires a magic tokens to get some local network requests to go through the proxy. This has tripped up a few users, so we make the behavior default to the expected: proxy everything including the local requests. This matches the other vendors as well. NB: This can be disabled via `PLAYWRIGHT_DISABLE_FORCED_CHROMIUM_PROXIED_LOOPBACK=1` Supercedes: microsoft#8345 Fixes: microsoft#10631

…illa.com

This reverts commit d307a87.

rwoll · 2021-12-09T02:42:15Z

Thanks for the re-trigger! Everything passed on the rebase except one unrelated test (https://github.com/microsoft/playwright/runs/4464958397?check_suite_focus=true)

So should be good to merge! 🎉

cc: @pavelfeldman

rwoll force-pushed the feat/unify-proxy-behavior branch from 0fae359 to 8d342e2 Compare December 4, 2021 18:21

mxschmitt added the CQ1 label Dec 4, 2021

rwoll force-pushed the feat/unify-proxy-behavior branch 2 times, most recently from ea7801f to 36ad482 Compare December 5, 2021 22:39

rwoll marked this pull request as ready for review December 6, 2021 00:35

pavelfeldman approved these changes Dec 6, 2021

View reviewed changes

mxschmitt added CQ1 and removed CQ1 labels Dec 8, 2021

rwoll added 9 commits December 8, 2021 15:42

tests: workaround FF exploding on newPage b/c of contile.services.moz…

9afea15

…illa.com

tests: ensure explicit bypasses still work, parallelize

75c56c2

tests: account for WK linux vs. macOS diffs

c57eb5e

nit: reduce fail complexity, parameterized -> copy paste

8eda600

Revert "nit: reduce fail complexity, parameterized -> copy paste"

6560bdf

This reverts commit d307a87.

nit: remove explicit parallel calls

b69660c

tests: skip attempting to filter connect requests

b755031

tests: windows did something mac couldn't do!

2f8aeb0

rwoll force-pushed the feat/unify-proxy-behavior branch from 0cef729 to 2f8aeb0 Compare December 8, 2021 23:46

mxschmitt added CQ1 and removed CQ1 labels Dec 9, 2021

mxschmitt merged commit fde427d into microsoft:main Dec 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proxy): unify local network proxy behavior #10719

feat(proxy): unify local network proxy behavior #10719

rwoll commented Dec 4, 2021 •

edited

rwoll commented Dec 4, 2021

rwoll commented Dec 4, 2021

rwoll commented Dec 5, 2021

rwoll commented Dec 5, 2021

pavelfeldman Dec 6, 2021

rwoll Dec 7, 2021

pavelfeldman Dec 7, 2021

pavelfeldman Dec 7, 2021

rwoll Dec 7, 2021

rwoll commented Dec 8, 2021

rwoll commented Dec 8, 2021

mxschmitt commented Dec 8, 2021

rwoll commented Dec 9, 2021

feat(proxy): unify local network proxy behavior #10719

feat(proxy): unify local network proxy behavior #10719

Conversation

rwoll commented Dec 4, 2021 • edited

rwoll commented Dec 4, 2021

rwoll commented Dec 4, 2021

rwoll commented Dec 5, 2021

rwoll commented Dec 5, 2021

pavelfeldman Dec 6, 2021

Choose a reason for hiding this comment

rwoll Dec 7, 2021

Choose a reason for hiding this comment

pavelfeldman Dec 7, 2021

Choose a reason for hiding this comment

pavelfeldman Dec 7, 2021

Choose a reason for hiding this comment

rwoll Dec 7, 2021

Choose a reason for hiding this comment

rwoll commented Dec 8, 2021

rwoll commented Dec 8, 2021

mxschmitt commented Dec 8, 2021

rwoll commented Dec 9, 2021

rwoll commented Dec 4, 2021 •

edited