-
Notifications
You must be signed in to change notification settings - Fork 229
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 9 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 |
|---|---|---|
|
|
@@ -1623,8 +1623,7 @@ <h3>Proxy</h3> | |
| <td>string | ||
| <td>Defines the proxy <a>host</a> 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 url</a> for "<code>http</code>". | ||
|
sadym-chromium marked this conversation as resolved.
Outdated
|
||
| </tr> | ||
|
|
||
| <tr> | ||
|
|
@@ -1640,34 +1639,64 @@ <h3>Proxy</h3> | |
| <td>string | ||
| <td>Defines the proxy <a>host</a> 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 url</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 <a>host</a> for SOCKS traffic when the | ||
|
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 don't think it makes sense to talk about "SOCKS traffic". After establishing the proxy the traffic is just whatever data is being transmitted e.g. HTTP or HTTPS. |
||
| <a><code>proxyType</code></a> is "<code>manual</code>". | ||
| <td>A <a>proxy url</a> for "<code>socks</code>". | ||
| </tr> | ||
|
|
||
| <tr> | ||
| <td><code>socksVersion</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. Is there a reason why you removed the
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. The key change is that the "
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. WRT backward compatibility, we can figure things out if we agree on what we want to achieve.
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. From the Chromium perspective, the
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. 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.
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 see. I re-worked the PR so that the
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. We can default |
||
| <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 url</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 url</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>. | ||
|
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. This is hard to read. Presumably there are three cases:
I think you need to specify in more detail how to parse the input to get the destination scheme, the destination host and destination port. |
||
|
|
||
| <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 and <var>port</var> is <code>80</code>, | ||
|
sadym-chromium marked this conversation as resolved.
Outdated
|
||
| let <var>scheme</var> be "<code>http</code>". | ||
|
|
||
| <li><p>If <var>scheme</var> is null and <var>port</var> is <code>443</code>, | ||
| let <var>scheme</var> be "<code>https</code>". | ||
|
|
||
| <li><p>If <var>scheme</var> is null and <var>port</var> is <code>1080</code>, | ||
| let <var>scheme</var> be "<code>socks</code>". | ||
|
|
||
| <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.
|
||
| <a>default port</a>, this is the implied 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 +1749,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 url</a> with <a>proxy scheme</a> | ||
| "<code>socks</code>" and does not have an <a>own property</a> | ||
|
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. Is there a reason why you recently replaced
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 extended the description of the issue with detailed list of possible capabilities. This line was changed in order to fulfill new scenarios under "
If we restrict the scheme to |
||
| 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> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.