Closed Bug 1377325 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: Request Handling, defect, P3)

54 Branch
defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1381290

People

(Reporter: robwu, Assigned: ericjung)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proxy])

Attachments

(2 files, 1 obsolete file)

Attached file 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 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)
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 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)
Priority: -- → P3
I think this patch is supplanted by other (forthcoming) patches for related bugs.
Attachment #8882460 - Attachment is obsolete: true
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
Closed: 7 years ago
Resolution: --- → DUPLICATE
Attachment #8882461 - Flags: review?(matthewjwein)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: