fix: do not cancel CORS preflight request on proxy auth. #29266

marekharanczyk · 2021-05-20T16:35:26Z

Description of Change

If connecting via proxy, preflight request can receive 407 header response from proxy. This does not mean request was finished even though it received headers (from proxy, not the destination server), so prevent "completing" and most importantly deleting it, which causes request to be canceled in network layer. Just continue to monitor it and await proper response from server. Also add circuit breaker to cancel request if proxy auth failed 3 times (for example user keeps cancelling auth). This behavior happens only when app registered WebRequest api listeners.

Fixes #25184
Fixes #25758

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed CORS preflight request always being cancelled when connecting via proxy requiring authentication for apps that had registered WebRequest listeners.

marekharanczyk · 2021-05-20T16:37:01Z

This PR fixes the problem described in #29265

If connecting via proxy, preflight request can receive 407 header response from proxy. This does not mean request was finished even though it received headers (from proxy, not the destination server), so prevent "completing" and most importantly deleting it, which causes request to be canceled in network layer. Just continue to monitor it and await proper response from server. Also add circut breaker to cancel request if proxy auth failed 3 times (for example user keeps cancelling auth). This behavior happens only when app registered WebRequest api listeners.

marekharanczyk · 2021-05-21T09:04:13Z

Marked issue is a duplicate of #25758.

nornagon

This doesn't match the related code in Chromium: https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc;l=857-869;drc=924ddf7fffcdc465838e48a40b08ed41bf480a5c

Can you expand on why this is necessary?

marekharanczyk · 2021-05-24T12:29:19Z

I've already reported https://chromium-review.googlesource.com/c/chromium/src/+/2910336 to chromium, which is similar change in their code.
This change is necessary because electron always sets "extraHeader" mode for WebRequests api. When connection is established through proxy requiring auth, 407 response is received from proxy to force client to authenticate itself with proxy. Extension/electron code assumed that any response that is received for CORS preflight (HTTP OPTIONS) completes the requests, but for proxy auth case, request is not yet finished, so it should be kept alive to allow network service to start authentication process. Deleting preflight request immediately after receiving 407 caused it to be cancelled, causing original request to be cancelled as well, making CORS unusable and breaking pages by rpeventing resource loads.
The only difference between CL and this PR is to provide failsafe and abort the request after 3 auth attempts to prevent unintentional infinite auth request loop. Authentication hooks in electron are different from chromium auth code and UI.

zcbenz

I'm good with this change, but we should wait for how Chromium reviewers say before merging.

lukeapage · 2021-06-11T04:01:31Z

Amazing this is being fixed now. I’ve received the issue intermittently in chrome too where by I have to reset my settings and it goes away - I can only guess something in my setup pushes chrome into this extra header mode.

miniak · 2021-06-11T05:16:25Z

@marekharanczyk the Chromium change does not have the maximum of 3 attempts, why do we need it here?
https://chromium-review.googlesource.com/c/chromium/src/+/2910336/12/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc

miniak · 2021-06-11T06:00:04Z

@marekharanczyk looks like we should also include https://chromium-review.googlesource.com/c/chromium/src/+/2011781
I hit the DCHECK when testing the fix with our repro

marekharanczyk · 2021-06-11T06:27:44Z

@miniak The retry check is because electron does not have same handling of auth that chromium has, just a safety measure to prevent infinite loop. I can remove it if it bothers you. Regarding second chromium fix you pointed: I will move it as well.

Move relevant parts of chromium WebRequestProxyingURLLoaderFactory from https://chromium-review.googlesource.com/c/chromium/src/+/2011781 into electron ProxyingURLLoaderFactory.

miniak · 2021-06-11T07:13:04Z

@marekharanczyk I was mostly just wondering why the fix is different between Chromium and Electron. If you believe the retry limit makes sense we can keep it.

marekharanczyk · 2021-06-14T10:22:32Z

Updated code to final version that just got merged upstream and removed fail safe.

nornagon · 2021-06-16T23:24:45Z

shell/browser/net/proxying_url_loader_factory.cc

+  // CorsURLLoader::FollowRedirect. In such a case the old network::URLLoader
+  // is going to be detached fairly soon, so we don't need to take care of it.
+  // We need this explicit reset to avoid a DCHECK failure in mojo::Receiver.
+  header_client_receiver_.reset();


Is this .reset() needed given we immediately .Bind() on the next line?

This is a cherry-pick of https://chromium-review.googlesource.com/c/chromium/src/+/2011781. Reason for reset is stated above in comment by original author, it is here to prevent DCHECK in mojo receiver.

nornagon · 2021-06-17T16:16:58Z

11.x is security fixes only now.

release-clerk · 2021-06-21T05:06:55Z

Release Notes Persisted

Fixed CORS preflight request always being cancelled when connecting via proxy requiring authentication for apps that had registered WebRequest listeners.

* fix: do not cancel CORS preflight request on proxy auth. If connecting via proxy, preflight request can receive 407 header response from proxy. This does not mean request was finished even though it received headers (from proxy, not the destination server), so prevent "completing" and most importantly deleting it, which causes request to be canceled in network layer. Just continue to monitor it and await proper response from server. Also add circut breaker to cancel request if proxy auth failed 3 times (for example user keeps cancelling auth). This behavior happens only when app registered WebRequest api listeners. * Port chromium webrequest changes to electron code. Move relevant parts of chromium WebRequestProxyingURLLoaderFactory from https://chromium-review.googlesource.com/c/chromium/src/+/2011781 into electron ProxyingURLLoaderFactory. * Update code to upstreamed version and remove retyr count failsafe. Co-authored-by: Milan Burda <milan.burda@gmail.com>

trop · 2021-06-21T05:07:33Z

I have automatically backported this PR to "12-x-y", please check out #29810

trop · 2021-06-21T05:07:36Z

I have automatically backported this PR to "13-x-y", please check out #29811

trop · 2021-06-21T05:07:43Z

I have automatically backported this PR to "14-x-y", please check out #29812

* fix: do not cancel CORS preflight request on proxy auth. (#29266) * fix: do not cancel CORS preflight request on proxy auth. If connecting via proxy, preflight request can receive 407 header response from proxy. This does not mean request was finished even though it received headers (from proxy, not the destination server), so prevent "completing" and most importantly deleting it, which causes request to be canceled in network layer. Just continue to monitor it and await proper response from server. Also add circut breaker to cancel request if proxy auth failed 3 times (for example user keeps cancelling auth). This behavior happens only when app registered WebRequest api listeners. * Port chromium webrequest changes to electron code. Move relevant parts of chromium WebRequestProxyingURLLoaderFactory from https://chromium-review.googlesource.com/c/chromium/src/+/2011781 into electron ProxyingURLLoaderFactory. * Update code to upstreamed version and remove retyr count failsafe. Co-authored-by: Milan Burda <milan.burda@gmail.com> * chore: add required header Co-authored-by: marekharanczyk <48673767+marekharanczyk@users.noreply.github.com> Co-authored-by: Milan Burda <milan.burda@gmail.com> Co-authored-by: Cheng Zhao <zcbenz@gmail.com>

) * fix: do not cancel CORS preflight request on proxy auth. If connecting via proxy, preflight request can receive 407 header response from proxy. This does not mean request was finished even though it received headers (from proxy, not the destination server), so prevent "completing" and most importantly deleting it, which causes request to be canceled in network layer. Just continue to monitor it and await proper response from server. Also add circut breaker to cancel request if proxy auth failed 3 times (for example user keeps cancelling auth). This behavior happens only when app registered WebRequest api listeners. * Port chromium webrequest changes to electron code. Move relevant parts of chromium WebRequestProxyingURLLoaderFactory from https://chromium-review.googlesource.com/c/chromium/src/+/2011781 into electron ProxyingURLLoaderFactory. * Update code to upstreamed version and remove retyr count failsafe. Co-authored-by: Milan Burda <milan.burda@gmail.com>

electron-cation bot added the new-pr 🌱 PR opened recently label May 20, 2021

marekharanczyk force-pushed the CORS-preflight-fix branch from 4ac1eac to b28ef25 Compare May 20, 2021 16:53

nornagon suggested changes May 21, 2021

View reviewed changes

zcbenz approved these changes May 25, 2021

View reviewed changes

zcbenz added semver/patch backwards-compatible bug fixes target/12-x-y labels May 25, 2021

electron-cation bot removed the new-pr 🌱 PR opened recently label May 25, 2021

zcbenz added the target/14-x-y label May 27, 2021

Merge branch 'main' into CORS-preflight-fix

8c439d2

miniak mentioned this pull request Jun 11, 2021

Second socket on a session does not send Proxy-Authenticate for ntlm #25184

Closed

3 tasks

miniak added the target/11-x-y label Jun 11, 2021

Port chromium webrequest changes to electron code.

752b8cc

Move relevant parts of chromium WebRequestProxyingURLLoaderFactory from https://chromium-review.googlesource.com/c/chromium/src/+/2011781 into electron ProxyingURLLoaderFactory.

Update code to upstreamed version and remove retyr count failsafe.

84c3e9c

jkleinsc requested a review from nornagon June 14, 2021 13:42

nornagon reviewed Jun 16, 2021

View reviewed changes

nornagon approved these changes Jun 17, 2021

View reviewed changes

nornagon removed the target/11-x-y label Jun 17, 2021

nornagon removed the semver/patch backwards-compatible bug fixes label Jun 17, 2021

VerteDinde added semver/none semver/patch backwards-compatible bug fixes and removed semver/none labels Jun 17, 2021

zcbenz merged commit 507cbdc into electron:main Jun 21, 2021

This was referenced Jun 21, 2021

fix: do not cancel CORS preflight request on proxy auth. #29810

Merged

fix: do not cancel CORS preflight request on proxy auth. #29811

Merged

trop bot mentioned this pull request Jun 21, 2021

fix: do not cancel CORS preflight request on proxy auth. #29812

Merged

trop bot added in-flight/12-x-y and removed target/12-x-y labels Jun 21, 2021

trop bot added in-flight/13-x-y and removed target/14-x-y labels Jun 21, 2021

trop bot added merged/13-x-y and removed in-flight/14-x-y labels Jun 21, 2021

trop bot added the merged/12-x-y label Jun 21, 2021

trop bot removed the in-flight/12-x-y label Jun 21, 2021

fix: do not cancel CORS preflight request on proxy auth. #29266

fix: do not cancel CORS preflight request on proxy auth. #29266

Uh oh!

Conversation

marekharanczyk commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

marekharanczyk commented May 20, 2021

Uh oh!

marekharanczyk commented May 21, 2021

Uh oh!

nornagon left a comment

Choose a reason for hiding this comment

Uh oh!

marekharanczyk commented May 24, 2021

Uh oh!

zcbenz left a comment

Choose a reason for hiding this comment

Uh oh!

lukeapage commented Jun 11, 2021

Uh oh!

miniak commented Jun 11, 2021

Uh oh!

miniak commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marekharanczyk commented Jun 11, 2021

Uh oh!

miniak commented Jun 11, 2021

Uh oh!

marekharanczyk commented Jun 14, 2021

Uh oh!

nornagon Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

marekharanczyk Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

nornagon commented Jun 17, 2021

Uh oh!

release-clerk bot commented Jun 21, 2021

Uh oh!

trop bot commented Jun 21, 2021

Uh oh!

trop bot commented Jun 21, 2021

Uh oh!

trop bot commented Jun 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marekharanczyk commented May 20, 2021 •

edited

Loading

miniak commented Jun 11, 2021 •

edited

Loading