1377325 - proxy does not fall back to direct requests if DIRECT is specified

Status: RESOLVED DUPLICATE of bug 1381290

(WebExtensions :: Request Handling, defect, P3)

Description

[Rob Wu [:robwu]]

Attachment: proxyDIRECT.zip

Steps to reproduce:

1. Register a PAC script via browser.proxy.registerProxyScript('pac.js'):
// this is pac.js
function FindProxyForURL(host, url) {
    // Some unreachable proxy, followed by "DIRECT".
    return 'PROXY 127.0.0.1:12345; DIRECT ignore-this-work-around-for-bugzil.la/1355198';
}

2. Visit a URL (e.g. example.com) (MAKE SURE THAT THIS PAGE IS NOT IN YOUR BROWSER CACHE).

Expected behavior:
- The actual page is shown (=direct request).

Actual behavior:
- The request is blocked, "The proxy server is refusing connections"

Additional information:
https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/toolkit/components/extensions/ProxyScriptContext.jsm#220
Instead of "return null", it should
return ProxyService.newProxyInfo('direct', '', '', 0, PROXY_TIMEOUT_SEC, null);
(or maybe defaultProxyInfo from applyFilter?)

Note that if the return value is "DIRECT", that the default handler is used, because "proxyInfo" is null, and then "proxyInfo || defaultProxyInfo;" results in a fall back to the defaultProxyInfo (https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/toolkit/components/extensions/ProxyScriptContext.jsm#144).

Comment 1

[Eric Jung [:ericjung]]

Attachment: Bug 1377325 - Refactor ProxyScriptContext.js

Review commit: https://reviewboard.mozilla.org/r/153584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153584/

Comment 2

[Eric Jung [:ericjung]]

Attachment: Bug 1377325 - Refactor ProxyScriptContext.js

Review commit: https://reviewboard.mozilla.org/r/153586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153586/

Comment 3

[Eric Jung [:ericjung]]

Comment on attachment 8882461 [details]
Bug 1377325 - Refactor ProxyScriptContext.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153586/diff/1-2/

Comment 4

[Matthew Wein [:mattw]]

Comment on attachment 8882460 [details]
Bug 1377325 - Refactor ProxyScriptContext.js

https://reviewboard.mozilla.org/r/153584/#review159188

::: toolkit/components/extensions/ProxyScriptContext.jsm:145
(Diff revision 1)
> +        if (rule.type === PROXY_TYPES.DIRECT) {
> +          break;
> +        }
> +        rules.push(rule);

nit: please move this out of the try/catch.

::: toolkit/components/extensions/ProxyScriptContext.jsm:151
(Diff revision 1)
> +          break;
> +        }
> +        rules.push(rule);
> +      } catch (e) {
> +        this.extension.emit("proxy-error", {
> +          message: "FindProxyForURL: " + (e.message || e),

nit: I don't think `e.message` will ever be undefined. Do you see a case where it will?

::: toolkit/components/extensions/ProxyScriptContext.jsm:157
(Diff revision 1)
>      let proxyInfo = this.createProxyInfo(rules);
>  
>      return proxyInfo || defaultProxyInfo;

Based on my suggestion below, this would be simplified to `return this.createProxyInfo(rules, defaultProxyInfo)`.

::: toolkit/components/extensions/ProxyScriptContext.jsm:163
(Diff revision 1)
>     * Creates a new proxy info object using the return value of FindProxyForURL.
>     *
>     * @param {Array<string>} rules The list of proxy rules returned by FindProxyForURL.
>     *    (e.g. ["PROXY 1.2.3.4:8080", "SOCKS 1.1.1.1:9090", "DIRECT"])

We should update this jsdoc now that the structure of the `rules` param has changed.

::: toolkit/components/extensions/ProxyScriptContext.jsm:170
(Diff revision 1)
>     * @param {Array<string>} rules The list of proxy rules returned by FindProxyForURL.
>     *    (e.g. ["PROXY 1.2.3.4:8080", "SOCKS 1.1.1.1:9090", "DIRECT"])
>     * @returns {nsIProxyInfo} The proxy info to apply for the given URI.
>     */
>    createProxyInfo(rules) {
> -    if (!rules.length) {
> +    let proxyInfo = null;

I think we should pass in defaultProxyInfo and set proxyInfo to defaultProxyInfo here - that way the last failover proxy is always defaultProxyInfo.

::: toolkit/components/extensions/ProxyScriptContext.jsm:175
(Diff revision 1)
> -    if (!rules.length) {
> -      return null;
> +    let proxyInfo = null;
> +    while (rules.length) {
> +      let {type, host, port} = rules.pop();
> +      proxyInfo = ProxyService.newProxyInfo(
> +        type, host, port, 0, PROXY_TIMEOUT_SEC, proxyInfo);
> +      if (type === "DIRECT") {

nit: please use PROXY_TYPES.DIRECT here.

Comment 5

[Rob Wu [:robwu]]

Comment on attachment 8882460 [details]
Bug 1377325 - Refactor ProxyScriptContext.js

https://reviewboard.mozilla.org/r/153584/#review159922

::: toolkit/components/extensions/ProxyScriptContext.jsm:157
(Diff revision 1)
>      let proxyInfo = this.createProxyInfo(rules);
>  
>      return proxyInfo || defaultProxyInfo;

This change is done in the next commit, because doing so results in a behavioral change in functionality.

::: toolkit/components/extensions/ProxyScriptContext.jsm:175
(Diff revision 1)
> -    if (!rules.length) {
> -      return null;
> +    let proxyInfo = null;
> +    while (rules.length) {
> +      let {type, host, port} = rules.pop();
> +      proxyInfo = ProxyService.newProxyInfo(
> +        type, host, port, 0, PROXY_TIMEOUT_SEC, proxyInfo);
> +      if (type === "DIRECT") {

`type` should have been compared with lowercase `"direct"` (`PROXY_TYPES.DIRECT` is also lowercase "direct").


There is something wrong with this logic, I think that you should first iterate forwards over the array and stop at the first occurrence of "direct", and then iterate backwards as currently done.

Otherwise, if you have `PROXY xxx; DIRECT; PROXY yyy`, then the implementation will start with `yyy` and then stop, instead of starting with `xxx` (please add a test for this too).

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8882461 [details]
Bug 1377325 - Refactor ProxyScriptContext.js

https://reviewboard.mozilla.org/r/153586/#review160894

Clearing review until the refactoring in the first commit is finished.

Comment 7

[Eric Jung [:ericjung]]

I think this patch is supplanted by other (forthcoming) patches for related bugs.

Comment 8

[Eric Jung [:ericjung]]

Comment on attachment 8882461 [details]
Bug 1377325 - Refactor ProxyScriptContext.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153586/diff/2-3/

Comment 9

[Eric Jung [:ericjung]]

Comment on attachment 8882461 [details]
Bug 1377325 - Refactor ProxyScriptContext.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153586/diff/3-4/

Comment 10

[Shane Caraveo (:mixedpuppy)]

This is addressed in bug 1381290