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

RESOLVED DUPLICATE of bug 1381290

Status

P3
normal
RESOLVED DUPLICATE of bug 1381290
a year ago
3 months ago

People

(Reporter: robwu, Assigned: ericjung)

Tracking

(Blocks: 1 bug)

54 Branch

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [proxy])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8882416 [details]
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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.
Attachment #8882460 - Flags: review?(mwein)
(Reporter)

Comment 5

a year ago
mozreview-review
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

a year ago
mozreview-review
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.
Attachment #8882461 - Flags: review?(mwein)

Updated

a year ago
Priority: -- → P3
(Assignee)

Comment 7

a year ago
I think this patch is supplanted by other (forthcoming) patches for related bugs.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8882460 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1283639
(Assignee)

Updated

a year ago
Summary: [proxy] proxy does not fall back to direct requests if DIRECT is specified → proxy does not fall back to direct requests if DIRECT is specified
Whiteboard: [proxy]
This is addressed in bug 1381290
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1381290
Attachment #8882461 - Flags: review?(matthewjwein)

Updated

3 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.