1545421 - Propagate useful proxy errors to WebExtensions

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Comment 1

[Dragana Damjanovic [:dragana]]

We want to trigger proxy.onError

Comment 2

[Honza Bambas (:mayhemer)]

Turning to defect, because we really lack here.

My proposal:

• this bug will only help to distinguish errors of CONNECT requests
• I intend to add new NS_ERROR_PROXY_* error codes for some of the common scenarios like 407 (to work around bug 1554218), 502, 504 and some other codes we may find relevant
• the response codes will convert to error codes somewhere in the h2 tunneling code and be propagated to the connection object to end up on the channel as the error status
• appropriate error pages for top level loads has to be implemented; this means localization is needed

Test from bug 1554190 has several TODOs that will be updated in this bug.

Plain connections problem:

• When we are connecting to a plain server through an h2 proxy, we don't use CONNECT, because from the privacy point of view it's just one round trip added and most proxies will fail to connect port 80 this way (it's usually forbidden). But, this brings one major problem: how do we recognize what went wrong? 502/504/5XX are general HTTP response codes that the end server may return well enough as the proxy that simply can't reach the server (from legit reasons.) It is not possible/reliable to recognize if our configured forward h2 proxy can't reach the end server server or a remote reverse proxy can't reach it's back-end server. If our proxy can't reach out, we have to fail the load and show an error page; if the response comes from the end server (rev proxy) we /should/ show the response as the content.

For consideration:

• tl;dr: special MIME type for proxies to describe their errors
• long version: there is a proposal (not adopted and normalized) for application/proxy-explanation+json content type that proxies may return (with an appropriate content) to let clients safely and reliably recognize the error code is coming from the proxy and not from the server. (If the server misuse this mime type, it would only lead to blocking of the content, on our side it would only convert to an internal failure code. It could only disrupt our telemetry, when widely abused - unlikely.)

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: Bug 1545421 - New nsresult error codes for 407, 502 and 504 http response codes returned by proxies + test, r=dragana!

Comment 6

[Honza Bambas (:mayhemer)]

As I was afraid, our test harness doesn't provide newer (8+) nodejs. I filed Update nodejs for testharness to at least 8.4 to support newer http2 APIs · Issue #2150 · mozilla/release-services in tooltool to change that.

I will update the patch to comment out the proxy code from moz-http2.js and disable the h2 proxy test and land it like that. That test can still be run locally if you have nodejs 8.4+.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/684c934ad7c5
New nsresult error codes for 407, 502 and 504 http response codes returned by proxies + test, r=dragana

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/684c934ad7c5

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 9067995 [details]
Bug 1545421 - New nsresult error codes for 407, 502 and 504 http response codes returned by proxies + test, r=dragana!

Beta/Release Uplift Approval Request

• User impact if declined: Needed for the secure proxy project
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just propagates few error codes we didn't propagate before, doesn't change/have impact on the internal connection logic.
• String changes made/needed: none

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9067995 [details]
Bug 1545421 - New nsresult error codes for 407, 502 and 504 http response codes returned by proxies + test, r=dragana!

this doesn't seem appropriate for the last week of beta

Comment 11

[Honza Bambas (:mayhemer)]

Julien, this is critical for the secure proxy project that is planned for 68. This patch MUST be there.

Comment 12

[Julien Cristau [:jcristau]]

Why did the uplift request not come earlier then? Are you and others in the necko team going to be available in the next two weeks if there are issues?

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Julien Cristau [:jcristau] from comment #12)

Why did the uplift request not come earlier then?

Because I was unclear on dates, perhaps.

Are you and others in the necko team going to be available in the next two weeks if there are issues?

We definitely will be.

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9067995 [details]
Bug 1545421 - New nsresult error codes for 407, 502 and 504 http response codes returned by proxies + test, r=dragana!

approved for 68.0b14

Comment 15

[Julien Cristau [:jcristau]]

Could this cause visible changes depending on the user's proxy configuration? Anything that's worth testing before going to release?

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Julien Cristau [:jcristau] from comment #15)

Could this cause visible changes depending on the user's proxy configuration? Anything that's worth testing before going to release?

This will only effect error pages that we show when a proxy fails to connect an end server - actually will make them more accurate. Instead of a general "proxy connect error" we may show more proper "server connect error" instead.

Comment 17

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/66f621aa4cfa