-
Notifications
You must be signed in to change notification settings - Fork 214
Allow for specifying schema in "proxy" #1922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
d64580f
4a7ee1b
e6a351f
73f2d59
3d84a8f
1e0853d
b0a959b
b22e68f
63e21ca
1cc6648
d43a0ee
df77dbc
e370abf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1621,10 +1621,9 @@ <h3>Proxy</h3> | |
| <tr> | ||
| <td><code>httpProxy</code> | ||
| <td>string | ||
| <td>Defines the proxy <a>host</a> for HTTP traffic when | ||
| <td>Defines the proxy configuration for HTTP traffic when | ||
| the <a><code>proxyType</code></a> is "<code>manual</code>". | ||
| <td>A <a>host and optional port</a> for | ||
| scheme "<code>http</code>". | ||
| <td>A <a>proxy specifier</a> for "<code>http</code>". | ||
| </tr> | ||
|
|
||
| <tr> | ||
|
|
@@ -1638,36 +1637,57 @@ <h3>Proxy</h3> | |
| <tr> | ||
| <td><code>sslProxy</code> | ||
| <td>string | ||
| <td>Defines the proxy <a>host</a> for encrypted TLS traffic | ||
| <td>Defines the proxy configuration for encrypted TLS traffic | ||
| when the <a><code>proxyType</code></a> is "<code>manual</code>". | ||
| <td>A <a>host and optional port</a> for | ||
| scheme "<code>https</code>". | ||
| <td>A <a>proxy specifier</a> "<code>https</code>". | ||
| </tr> | ||
|
|
||
| <tr> | ||
| <td><code>socksProxy</code> | ||
| <td>string | ||
| <td>Defines the proxy <a>host</a> for a <a>SOCKS proxy</a> | ||
| when the <a><code>proxyType</code></a> is "<code>manual</code>". | ||
| <td>A <a>host and optional port</a> with an <a>undefined</a> scheme. | ||
| <td>Defines the proxy configuration for SOCKS traffic when the | ||
| <a><code>proxyType</code></a> is "<code>manual</code>". | ||
| <td>A <a>proxy specifier</a> for "<code>socks</code>". | ||
| </tr> | ||
|
|
||
| <tr> | ||
| <td><code>socksVersion</code> | ||
| <td>number | ||
| <td>Defines the <a>SOCKS proxy</a> version | ||
| when the <a><code>proxyType</code></a> is "<code>manual</code>". | ||
| <td>Defines the <a>SOCKS proxy</a> version when the <a>proxy specifier</a>'s | ||
| <a>proxy scheme</a> is "<code>socks</code>". | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to my comment above. It would be a backward compatibility breaking change when we now introduce a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated algorithm. The |
||
| <td>Any <a>integer</a> between 0 and 255 inclusive. | ||
| </tr> | ||
|
|
||
| </table> | ||
|
|
||
| <p>A <dfn>host and optional port</dfn> for a <var>scheme</var> is | ||
| defined as being a valid <a>host</a>, optionally followed by a colon | ||
| and a valid <a>port</a>. The <a>host</a> may | ||
| <a data-lt="includes credentials">include credentials</a>. If the | ||
| port is omitted and <var>scheme</var> has a <a>default port</a>, | ||
| this is the implied port. Otherwise, the port is left undefined. | ||
| <p>A <dfn>proxy specifier</dfn> for <a>proxy scheme</a> <var>protocol</var> can | ||
| be either "<code>direct</code>" or a string consists of an optional | ||
| <a>proxy scheme</a> <var>scheme</var> followed by the string | ||
| "<code>://</code>", a valid <a>host</a> <var>host</var>, and optionally a | ||
| colon followed by a valid <a>port</a> <var>port</var>. | ||
|
|
||
| <p class=note>The <var>scheme</var> can be different from <var>protocol</var>. | ||
| If so, it means that the <var>protocol</var> traffic will be proxied via | ||
| <var>scheme</var>. | ||
|
|
||
| <ol> | ||
| <li><p>If <var>scheme</var> is omitted, let <var>scheme</var> be null. | ||
|
|
||
| <li><p>If <var>scheme</var> is null, let <var>scheme</var> be | ||
| <var>protocol</var>. | ||
|
|
||
| <li><p>If <var>scheme</var> is "<code>http</code>" or "<code>https</code>", | ||
| the <var>host</var> may | ||
| <a data-lt="includes credentials">include credentials</a>. | ||
|
|
||
| <li><p>If the <var>port</var> is omitted and <var>scheme</var> has a | ||
sadym-chromium marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <a>default port</a>, then <a>remote end</a> should use this port. Otherwise, | ||
| the <var>port</var> is left undefined. | ||
| </ol> | ||
|
|
||
| <p>A <dfn>proxy scheme</dfn> is defined as being one of the following strings: | ||
| "<code>http</code>", "<code>https</code>", "<code>socks</code>", | ||
| "<code>socks4</code>", "<code>socks5</code>". | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think that is a backward incompatible given that we didn't use a scheme for socks so far and I'm unsure how clients actually specify the socks proxy. @jgraham maybe you have some additional feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the algorithm.
However, if the user WANTS to specify schema, now they can do it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is incredibly poorly defined to begin with, but I think it's unclear from the specification text what the new semantics actually are. If we want this to actually work, I think it's worth spending the time to define the right data structures to represent the configuration, even if it's a larger lift. It seems like we more or less have two things: a source protocol (http, https, etc. or "all") and a destination proxy configuration, which consists of a proxy type, and a host address (host/ip and port) for the server. I think the assumption that the meaning is clear just from a URL-like serialization of the destination proxy is unfounded. In particular there doesn't seem to be a |
||
|
|
||
| <p>A <a><code>proxyType</code></a> of "<code>direct</code>" indicates | ||
| that the browser should not use a proxy at all. | ||
|
|
@@ -1720,10 +1740,10 @@ <h3>Proxy</h3> | |
| <a>own property</a> for "<code>proxyAutoconfigUrl</code>" return | ||
| an <a>error</a> with <a>error code</a> <a>invalid argument</a>. | ||
|
|
||
| <li><p>If <var>proxy</var> has an <a>own property</a> for | ||
| "<code>socksProxy</code>" and does not have an <a>own property</a> | ||
| for "<code>socksVersion</code>" return an <a>error</a> with <a>error | ||
| code</a> <a>invalid argument</a>. | ||
| <li><p>If <var>proxy</var> contains <a>proxy specifier</a> with | ||
| <a>proxy scheme</a> "<code>socks</code>" and does not have an | ||
| <a>own property</a> for "<code>socksVersion</code>", return an <a>error</a> | ||
| with <a>error code</a> <a>invalid argument</a>. | ||
|
|
||
| <li><p>Return <a>success</a> with data <var>proxy</var>. | ||
| </ol> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you removed the
socksVersionfield? I thought that for socks proxies it is a mandatory field. Also if clients specify this field we would run into a backward incompatible change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key change is that the "
socksProxy" field sets how to proxy ALL the socks connections, not only of the specific version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WRT backward compatibility, we can figure things out if we agree on what we want to achieve.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the Chromium perspective, the
socksVersiondoes not make actually sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the socks version will cause a backward incompatible change, means it will break clients using that field. We probably should fine a way to deprecate if it is really not needed. But note that when defining a socks proxy in Firefox you need to specify the version. So I don't think that we can get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I re-worked the PR so that the
socksVersioncan be used together withsocks://scheme.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can default
socksVersionto5to ease the API. @whimboo WDYT?