foxyproxy / browser-extension

Version 8 and above. Browser extension source code for Firefox, Chrome, and other Chromium-based browsers
GNU General Public License v2.0
198 stars 29 forks source link

Incorrect import of a proxy with `socks` protocol #120

Closed lessneek closed 5 months ago

lessneek commented 5 months ago

On Import Proxy List a proxy with socks:// protocol is imported with empty hostname and port.

Versions:

Firefox 121.0.1
FoxyProxy 8.8
erosman commented 5 months ago

Can you give an example?

I tried with these and there were no problems.

socks://100.10.11.12:1080?title=China&cc=CN&city=Beijing
socks5://100.10.11.12:1080?title=China&cc=CN&city=Beijing
lessneek commented 5 months ago

I've tried these:

socks://100.10.11.12:1080?title=China&cc=CN&city=Beijing
socks5://100.10.11.12:1080?title=China&cc=CN&city=Beijing

and what I've got: image

lessneek commented 5 months ago

A breakpoint while debugging: it seems that the URL doesn't parse a socks:// url correctly: image

erosman commented 5 months ago

Which version of FoxyProxy are you using and which browser?

lessneek commented 5 months ago

Which version of FoxyProxy are you using and which browser?

Firefox 121.0.1 FoxyProxy 8.8 Fedora Linux 39 (Workstation Edition)

erosman commented 5 months ago

I tested it now with v8.8 on Firefox Nightly and I don't get any problems.

Please try the following:

lessneek commented 5 months ago

I have tries with an empty Firefox this in console, and it seems that a URL with the socks protocol is not parsed correctly. I can replace socks with asdf and the result will be the same (but https works as expected): image

I don't know how the URL is implemented, but it seems that it is based on my current system installation. Can you try it in Fedora 39 may be?

I found that there were some regressions in the past few years, but it was fixed, may be it is a new regression. Have no time to make a research of it.

lessneek commented 5 months ago

May be we can replace the URL parsing method with a simple regular expression? Just to have no dependence of a particular system configuration.

erosman commented 5 months ago

I have tries with an empty Firefox this in console, and it seems that a URL with the socks protocol is not parsed correctly.

That is why FoxyProxy code reprocesses it. I don't get any error when I try it.

https://github.com/foxyproxy/browser-extension/blob/172c936be1f6ffa4450f5e5186c48794aa8037ba/src/content/options.js#L1019-L1041

lessneek commented 5 months ago

Sets the type according to the protocol

when I debug, protocol isn't changed after assignment:

url = new URL("socks://10.10.1.2:1080")
< URL { href: "socks://10.10.1.2:1080", origin: "null", protocol: "socks:", username: "", password: "", host: "", hostname: "", port: "", pathname: "//10.10.1.2:1080", search: "" }

url.protocol = "http"
< "http"

url
< URL { href: "socks://10.10.1.2:1080", origin: "null", protocol: "socks:", username: "", password: "", host: "", hostname: "", port: "", pathname: "//10.10.1.2:1080", search: "" }
erosman commented 5 months ago

You are right. :man_facepalming: I have tested it on Chrome and Firefox and while Chrome allows changing the protocol, Firefox doesn't. I am going to ask Mozilla for a reason and rewrite the code accordingly.

(() => {
  let url = new URL("socks://10.10.1.2:1080");
  console.log(url);

  url.protocol = 'http:';
  console.log(url);
})();
erosman commented 5 months ago

I have updated the code with a fix for v8.9.

erosman commented 5 months ago

It seems new URL() has changed.

On Firefox 121.0.1, hostname & port are not shown.

image

On Nightly 124.0a1, hostname & port are shown.

image

erosman commented 5 months ago

It seems new URL() was updated in Firefox 122 (Bug 1603699: Enable DefaultURI use for unknown schemes).

erosman commented 5 months ago

v8.9 uploaded to AMO Basic version is available now and the standard version will be available after the AMO review.