FindProxyForURL() should use object return type and deprecate string format

RESOLVED FIXED in Firefox 57

Status

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: ericjung, Assigned: mixedpuppy)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla57
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [proxy])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
In the WebExtensions proxyAPI, addons return a string format that does not allow the return of enough information to fully configure an nsIProxyInfo instance. In order to provide the most flexibility and configurability to addons, we should allow the return type to be a Javascript object with the following properties:

|type| -- string, one of "http"|"https|"socks5"|"socks4"|"socks"|"direct"|"ignore"|. note that "socks" is a synonym for socks5. "ignore" means Firefox should handle this URI through its global proxy settings (which could be wpad, pac, system, direct/none, or a proxy server) or other installed addons.
|host| -- string
|port| -- integer between 1 and 65536 (TCP/IP does not allow for ports outside that range)
|username| -- optional string
|password| -- optional string
|proxyDNS| -- optional boolean. default false. if true, TRANSPARENT_PROXY_RESOLVES_HOST is set as a flag on nsIProxyInfo.flags so that the proxy server is used to resolve certain DNS queries.
|failoverTimeout| -- optional integer. default 1. Number of seconds before timing out and trying the next proxy in the failover array
|failover| -- optional array of objects with these same properties. null to terminate. default null (no failover, which is the desired case 99% of the time in my experience).

example:

{
type: "socks",
host: "foo.com",
port: 1080,
proxyDNS: true,
failoverTimeout: 1,
failover: {
  type: "socks",
  host: "bar.com",
  port: 1080,
  proxyDNS: true,
  failoverTimeout: 0,
  failover: null
}
}
Reporter

Updated

2 years ago
Reporter

Updated

2 years ago
Depends on: 1360404
Reporter

Updated

2 years ago
Priority: -- → P3
Whiteboard: [proxy]
Reporter

Updated

2 years ago
Priority: P3 → P1
Reporter

Comment 1

2 years ago
Note proxyDNS == true is only valid if |type| is a socks server. See nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST in https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp and https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsSocketTransport2.cpp

Updated

2 years ago
Assignee: nobody → eric
Summary: WebExtensions ProxyAPI: FindProxyForURL() should use object return type and deprecate string format → FindProxyForURL() should use object return type and deprecate string format

Comment 2

2 years ago
Please keep in mind case of invalid credentials for servers with authorization. And some sort of async waiting for credentials if possible
Reporter

Comment 3

2 years ago
> Please keep in mind case of invalid credentials for servers with authorization.

The first implementation won't notify the addon of invalid credentials. We'll have to do that in a separate bug

>And some sort of async waiting for credentials if possible

I do not think this is possible since FindProxyForPAC() is synchronous and executes on the main thread. Can you provide example--why can't you get the credentials somewhere else in the addon? For example, at startup?

Comment 4

2 years ago
> I do not think this is possible since FindProxyForPAC() is synchronous and executes on the main thread. Can you provide example--why can't you get the credentials somewhere else in the addon? For example, at startup?
For example, AJAX call for credentials at the start. And in case of invalid credentials delay for AJAX for new credentials.
Reporter

Comment 5

2 years ago
> AJAX call for credentials at the start
This can be done at startup or other points. I dont see how we can support asynch calls from FindProxyForURL() without changing a lot of necko/network code. Where were you executing this code before WebExtensions?

Comment 6

2 years ago
In Chrome, we are using webRequest.onAuthRequired with async return to make this waiting. And Proxy API without delay from the start
Reporter

Comment 7

2 years ago
> In Chrome, we are using webRequest.onAuthRequired with async return

But this bug is for Firefox.

Comment 8

2 years ago
Maybe you did not understand: in Chrome it's working perfectly. 

In Firefox Proxy API is not firing webRequest.onAuthRequired and by your description, there will be no way to know about authorization error and no way to change invalid credentials to valid.
Reporter

Updated

2 years ago
Blocks: 1383426
Reporter

Comment 9

2 years ago
> there will be no way to know about authorization error and no way to change invalid credentials to valid.

Opened bug 1383426
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(mixedpuppy)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8889231 [details]
Bug 1381290 - FindProxyForURL() should use object return type and deprecate string formati;

https://reviewboard.mozilla.org/r/160292/#review168546

The implementation is looking good, but I'd like to see more code coverage before I'll feel comfortable giving an r+.

::: toolkit/components/extensions/ProxyScriptContext.jsm:164
(Diff revision 1)
>        }
>      }
> +    return this.createProxyInfo(rules, defaultProxyInfo);
> +  }
>  
> -    let proxyInfo = this.createProxyInfo(rules);
> +  parseRule(rule) {

Please add jsdoc for this function.

::: toolkit/components/extensions/ProxyScriptContext.jsm:165
(Diff revision 1)
>      }
> +    return this.createProxyInfo(rules, defaultProxyInfo);
> +  }
>  
> -    let proxyInfo = this.createProxyInfo(rules);
> +  parseRule(rule) {
> +    let {type, host, port, username, password, proxyDNS, failoverTimeout} = rule;

`rule` could be undefined and isn't garunteed to be an object at this point. You should probably validate each item in the array above where you check to see if the rules are a string or an array.

::: toolkit/components/extensions/ProxyScriptContext.jsm:195
(Diff revision 1)
> -      proxyInfo = ProxyService.newProxyInfo(
> -        type, host, port, 0, PROXY_TIMEOUT_SEC, proxyInfo);
>        if (type === "DIRECT") {
>          return proxyInfo;
>        }
> +      // TODO: After bug1360404 is fixed, use ProxyService.newProxyInfoWithAuth() all the time--not just for SOCKS

nit: please put a space between "bug" and 1360404.

::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:82
(Diff revision 1)
>        var FindProxyForURL = 5; // eslint-disable-line mozilla/var-only-at-top-level
>      }, {
>        message: "The proxy script must define FindProxyForURL as a function",
>      });
>  });
>  

It looks like you are still missing a few tests. Please add tests for the following return conditions:
    
1. An empty array
2. An array of something other than objects
3. An array of empty objects
4. Missing type
5. Missing host
6. Missing port
7. A username which is not a string
8. A password which is not a string
7. A host which isn't a string
8. A host which has whitespace
9. A host which is only whitespace
10. A port which is not an intenger
11. A port which is less than 1
12. A proxyDNS which is not a boolean
13. A proxyDNS set to true when the type is not socks
14. A failoverTimeout which is not an integer
15. A failoverTimeout which is less than 1

Please add similar tests to test_proxy_scripts.js

::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:96
(Diff revision 1)
>      });
>  
>    await testProxyScript(
>      () => {
>        function FindProxyForURL() {
> -        return "INVALID";
> +        return [{type: "pptp", host: "foo.bar", port: 1080, username: "mungosantamaria", password: "pass123", proxyDNS: true, failoverTimeout: 3},

You shouldn't need to provide username, password, proxyDNS, or failoverTimeout to test the type.

::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:106
(Diff revision 1)
>      });
>  
>    await testProxyScript(
>      () => {
>        function FindProxyForURL() {
> -        return "SOCKS";
> +        return [{type: "pptp", host: "foo.bar", port: 65536, username: "mungosantamaria", password: "pass123", proxyDNS: true, failoverTimeout: 3},

I think you should use a valid type when testing the port, and you also shouldn't need to provide username, password, proxyDNS, or failoverTimeout to test the port.

::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:125
(Diff revision 1)
>      });
>  
>    await testProxyScript(
>      () => {
>        function FindProxyForURL() {
>          return "PROXY :";

Shouldn't this file only be testing the new object return type?

::: toolkit/components/extensions/test/mochitest/test_ext_proxy_legacy.html:54
(Diff revision 1)
> +  }
> +
> +  win.close();
> +  await extension.unload();
> +}
> +

Please add out-of-range tests for the port now that it's being validated.

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:128
(Diff revision 1)
>        type: "http",
> -      failoverProxy: null,
>      },
>    });
>  });
>  

Please add tests for the other proxy types.

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:180
(Diff revision 1)
>    }, {
>      proxyInfo: {
> -      host: "1.2.3.4",
> -      port: "8080",
> +      type: "socks",
> +      host: "foo.bar",
> +      port: 1080,
> +      proxyDNS: true,

nit: Shouldn't this be TRANSPARENT_PROXY_RESOLVES_HOST?

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:201
(Diff revision 1)
> -        type: "socks",
> +            type: "socks",
> -        failoverProxy: null,
> -      },
> -    },
> -  });
> -});
> +            proxyDNS: TRANSPARENT_PROXY_RESOLVES_HOST,
> +            username: "mungosantamaria",
> +            password: "foobar",
> +            failoverProxy: {
> +              type: "dect",

What is this?
Attachment #8889231 - Flags: review?(matthewjwein) → review-

Comment 12

2 years ago
As a WebExtension developer, may I ask whether it is possible to detect whether object return values are supported? For example, an extension can allow the user to specify `username` and `proxyDNS` on the UI if object return values are supported, but hides these fields and falls back to string format in earlier Firefox versions.

Of course, it is possible to just return an object first and look for entries such as `FindProxyForURL: Return type must be a string` in the proxy error event. However, the first request will fail and there will be no way to retry the request or return a string value instead unless the user refreshes the page, which leads to sub-optimal UX.

Is it possible to expose a constant like `browser.proxy.SUPPORTS_OBJECT_PROXY_INFO === true`?
Reporter

Comment 13

2 years ago
>is possible to detect whether object return values are supported?

You can do find the version of Firefox in which your addon is executing:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/getBrowserInfo

then decide which return type to use based on when this bug lands.
Reporter

Comment 14

2 years ago
The |type| return value should, in the future, support:

* wpad (use WPAD to find a PAC and use that PAC for proxying decisions)
* pac |url| (use the PAC file at |url| for proxying decisions)
* system (use the user prefs for proxying decisions)

in such cases, most of the other values in the returned object can be ignored.
Reporter

Updated

2 years ago
Assignee: eric → mixedpuppy
Assignee

Comment 15

2 years ago
Taking this over to finish it.  My other hopes were vanquished, this is the most logical way forward for 57.
Flags: needinfo?(mixedpuppy)
Comment hidden (mozreview-request)
Assignee

Comment 17

2 years ago
Comment on attachment 8901397 [details]
Bug 1381290 support proxyInfo object return from FindProxyForURL,

The other patch here apparently was built on top of some unpublished patch.  I started over, and reorganized things a bit.  Eric, can you make sure no basic logic is lost.  I haven't run this through tests yet, primarily reviving/reorganizing.
Attachment #8901397 - Flags: review?(matthewjwein) → feedback?(eric)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8901397 - Flags: review?(tomica)
Attachment #8901397 - Flags: review?(matthewjwein)
Attachment #8901397 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8889231 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8901397 - Flags: review?(matthewjwein) → review?(kmaglione+bmo)
Assignee

Updated

2 years ago
No longer depends on: 1360404
Assignee

Updated

2 years ago
No longer blocks: 1383426

Comment 21

2 years ago
mozreview-review
Comment on attachment 8901397 [details]
Bug 1381290 support proxyInfo object return from FindProxyForURL,

https://reviewboard.mozilla.org/r/172862/#review179614

I didn't look too deep at the tests.  The code itself looks good, apart from the performance question and some nits.

::: toolkit/components/extensions/ProxyScriptContext.jsm:28
(Diff revision 3)
>                                     "@mozilla.org/network/protocol-proxy-service;1",
>                                     "nsIProtocolProxyService");
>  
>  const CATEGORY_EXTENSION_SCRIPTS_CONTENT = "webextension-scripts-content";
>  
> +// Should DNS be resolved on the SOCKS proxy server? (does not apply to HTTP and HTTP proxy servers)

This reads as a question for whoever is reading this code, please reword.

::: toolkit/components/extensions/ProxyScriptContext.jsm:29
(Diff revision 3)
>                                     "nsIProtocolProxyService");
>  
>  const CATEGORY_EXTENSION_SCRIPTS_CONTENT = "webextension-scripts-content";
>  
> +// Should DNS be resolved on the SOCKS proxy server? (does not apply to HTTP and HTTP proxy servers)
> +const TRANSPARENT_PROXY_RESOLVES_HOST = Ci.nsIProxyInfo.TRANSPARENT_PROXY_RESOLVES_HOST;

nit: `const {TRANSPARENT_...} = ...`

::: toolkit/components/extensions/ProxyScriptContext.jsm:61
(Diff revision 3)
> +  validate(proxyData) {
> +    if (proxyData.type && proxyData.type.toLowerCase() == "direct") {
> +      return {type: proxyData.type};
> +    }
> +    for (let prop of ["type", "host", "port", "username", "password", "proxyDNS", "failoverTimeout"]) {
> +      this[prop](proxyData);

This looks like hand-rolled "schema validation/normalization", perhaps worth it to use Schema.normalize()?

::: toolkit/components/extensions/ProxyScriptContext.jsm:81
(Diff revision 3)
> +    proxyData.type = type;
> +  },
> +
> +  host(proxyData) {
> +    let {host} = proxyData;
> +    if (typeof host != "string" || host.split(/\s+/).length != 1) {

this would be more readable (and avoid regex): ` || host.includes(" ")`

::: toolkit/components/extensions/ProxyScriptContext.jsm:84
(Diff revision 3)
> +  host(proxyData) {
> +    let {host} = proxyData;
> +    if (typeof host != "string" || host.split(/\s+/).length != 1) {
> +      throw new ExtensionError(`FindProxyForURL: Invalid proxy server host: "${host}"`);
> +    }
> +    host = host.trim();

`host` can't have a space here

::: toolkit/components/extensions/ProxyScriptContext.jsm:137
(Diff revision 3)
> +    if (failoverTimeout !== undefined && (!Number.isInteger(failoverTimeout) || failoverTimeout < 1)) {
> +      throw new ExtensionError(`FindProxyForURL: Invalid failover timeout: "${failoverTimeout}"`);
> +    }
> +  },
> +
> +  createProxyInfoFromData(proxyDataList, defaultProxyInfo) {

This got fairly complicated real fast, with schema validation/normalization, regular expressions, recursion (and even some string parsing if the result was a string).

And we do all of this per every loaded URI * every proxy extension?

Can we perhaps do some caching here, since I'm guessing an extension is likely to ever return one of a few different proxy configurations?

::: toolkit/components/extensions/ProxyScriptContext.jsm:156
(Diff revision 3)
> +   * Creates a new proxy info data object using the return value of FindProxyForURL.
> +   *
> +   * @param {Array<string>} rule A proxy rule 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.

This looks like it only deals with a single rule, not an array, so update the comment please.

::: toolkit/components/extensions/ProxyScriptContext.jsm:167
(Diff revision 3)
> +  parseProxyInfoDataFromPAC(rule) {
> +    if (!rule) {
> +      throw new ExtensionError("FindProxyForURL: Missing Proxy Rule");
> +    }
> +
> +    let parts = rule.split(/\s+/);

nit: how about converting to lowercase once, and naming the parts `{type, arg} = ` upfront?

::: toolkit/components/extensions/ProxyScriptContext.jsm:169
(Diff revision 3)
> +      throw new ExtensionError("FindProxyForURL: Missing Proxy Rule");
> +    }
> +
> +    let parts = rule.split(/\s+/);
> +    if (!parts[0] || parts.length > 2) {
> +      throw new ExtensionError(`FindProxyForURL: Too many arguments passed for proxy rule: "${rule}"`);

Wrong error message when `!parts[0]`.

::: toolkit/components/extensions/ProxyScriptContext.jsm:183
(Diff revision 3)
> +        if (!parts[1]) {
> +          throw new ExtensionError(`FindProxyForURL: Missing argument for proxy type: "${parts[0]}"`);
> +        }
> +
> +        if (parts.length != 2) {
> +          throw new ExtensionError(`FindProxyForURL: Too many arguments for proxy rule: "${rule}`);

This is already confirmed by `parts.length > 2` and `!parts[1]` checks above.

::: toolkit/components/extensions/ProxyScriptContext.jsm:273
(Diff revision 3)
> +      case "string":
> +        let proxyRules = [];
> +        for (let result of proxyData.split(";")) {
> +          try {
> +            let pi = ProxyInfoData.parseProxyInfoDataFromPAC(result.trim());
> +            if (pi) {

It doesn't look like `pi` can be null or falsy here.

::: toolkit/components/extensions/ProxyScriptContext.jsm:289
(Diff revision 3)
> +              message: error.message,
> +              fileName: error.fileName,
> +              lineNumber: error.lineNumber,
> +              stack: error.stack,
> +            });
> +            break;

This `break` is slightly confusing.  It might be more readable if you inverted and put the `for` loop inside the `try` block.

::: toolkit/components/extensions/ProxyScriptContext.jsm:299
(Diff revision 3)
> +      case "object":
> +        if (!Array.isArray(proxyData)) {
> +          throw new ExtensionError("FindProxyForURL: Return type must be a string or array of objects");
> +        }
> +        if (proxyData.length < 1) {
> +          throw new ExtensionError("FindProxyForURL: Empty result array");

This error message might mention "array" even when the string was returned.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Duplicate of this bug: 1377325
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 27

2 years ago
(In reply to Tomislav Jovanovic :zombie from comment #21)
> Comment on attachment 8901397 [details]
> Bug 1381290 support proxyInfo object return from FindProxyForURL
> 
> https://reviewboard.mozilla.org/r/172862/#review179614
> 
> I didn't look too deep at the tests.  The code itself looks good, apart from
> the performance question and some nits.

Thanks, I've addressed everything except below.

> ::: toolkit/components/extensions/ProxyScriptContext.jsm:137
> (Diff revision 3)
> > +    if (failoverTimeout !== undefined && (!Number.isInteger(failoverTimeout) || failoverTimeout < 1)) {
> > +      throw new ExtensionError(`FindProxyForURL: Invalid failover timeout: "${failoverTimeout}"`);
> > +    }
> > +  },
> > +
> > +  createProxyInfoFromData(proxyDataList, defaultProxyInfo) {
> 
> This got fairly complicated real fast, with schema validation/normalization,
> regular expressions, recursion (and even some string parsing if the result
> was a string).

That can be done in a followup if really necessary.

> And we do all of this per every loaded URI * every proxy extension?
> 
> Can we perhaps do some caching here, since I'm guessing an extension is
> likely to ever return one of a few different proxy configurations?

They can dynamically change what is returned.
Assignee

Updated

2 years ago
Attachment #8901397 - Flags: review?(matthewjwein) → review?(kmaglione+bmo)
Reporter

Comment 28

2 years ago
Shane, are you implemented anything from comment c14? Most importantly, the return type of "pac <url>" since there is an ongoing discussion on amo-admins@mozilla.org about how proxyAPI WebExtensions can defer to an external PAC script. netwerk/base/ProxyAutoConfig.cpp and netwerk/base/nsPACMan.cpp are the tools to do this.
Reporter

Comment 29

2 years ago
> Shane, are you implemented anything from comment 14?

See also https://gist.github.com/ericjung/77844ccda80feb5def66339dac0b1bcc for an example of implementing nsIProxyAutoConfig
Assignee

Comment 30

2 years ago
(In reply to Eric Jung [:ericjung] from comment #28)
> Shane, are you implemented anything from comment c14?

There are other bugs for that (e.g. 1319630 and 1319631), and you didn't indicate those were blockers any time I asked.  We're not going to get this all in for 57.
Reporter

Comment 31

2 years ago
> There are other bugs for that (e.g. 1319630 and 1319631)

Those are for "system" and "wpad", not "pac" -- which is the most important of the three IMHO. Do you want me to open a new bug for "pac"? I didn't see one in the tracker bug 1283639.

> you didn't indicate those were blockers any time I asked.

True, because I thought the ongoing discussion in amo-admins@mozilla.org would permit a work-around (combining XMLHttpRequest() with eval() inside the sandbox'd FindProxyForURL())... but it's not clear the AMO admins are going to permit that.
Assignee

Comment 32

2 years ago
(In reply to Eric Jung [:ericjung] from comment #31)
> > There are other bugs for that (e.g. 1319630 and 1319631)
> 
> Those are for "system" and "wpad", not "pac" -- which is the most important
> of the three IMHO. Do you want me to open a new bug for "pac"? I didn't see
> one in the tracker bug 1283639.
> 
> > you didn't indicate those were blockers any time I asked.
> 
> True, because I thought the ongoing discussion in amo-admins@mozilla.org
> would permit a work-around (combining XMLHttpRequest() with eval() inside
> the sandbox'd FindProxyForURL())... but it's not clear the AMO admins are
> going to permit that.

You can't do that in FindProxyForURL unless it's a sync call, and that is bad.  Yes another bug, w/use case and why the only way to handle it is with a new type.  I'm not certain any further proxy work can be done for 57.
Assignee

Updated

2 years ago
Blocks: 1393940
Reporter

Comment 33

2 years ago
> Yes another bug, w/use case and why the only way to handle it is with a new type.

Opened bug 1396485

Comment 34

2 years ago
mozreview-review
Comment on attachment 8901397 [details]
Bug 1381290 support proxyInfo object return from FindProxyForURL,

https://reviewboard.mozilla.org/r/172862/#review180170

::: toolkit/components/extensions/ProxyScriptContext.jsm:36
(Diff revision 5)
>  // The length of time (seconds) to wait for a proxy to resolve before ignoring it.
>  const PROXY_TIMEOUT_SEC = 10;
>  
>  const {
>    defineLazyGetter,
> +  ExtensionError,

Nit: We usually use case-sensitive sorting.

::: toolkit/components/extensions/ProxyScriptContext.jsm:68
(Diff revision 5)
> +    return proxyData;
> +  },
> +
> +  type(proxyData) {
> +    let {type} = proxyData;
> +    if (typeof type != "string" || !PROXY_TYPES.hasOwnProperty(type.toUpperCase())) {

Nit: `!==`

::: toolkit/components/extensions/ProxyScriptContext.jsm:72
(Diff revision 5)
> +    let {type} = proxyData;
> +    if (typeof type != "string" || !PROXY_TYPES.hasOwnProperty(type.toUpperCase())) {
> +      throw new ExtensionError(`FindProxyForURL: Invalid proxy server type: "${type}"`);
> +    }
> +    type = type.toLowerCase();
> +    if (type == PROXY_TYPES.PROXY) {

Nit: `===`

::: toolkit/components/extensions/ProxyScriptContext.jsm:74
(Diff revision 5)
> +      throw new ExtensionError(`FindProxyForURL: Invalid proxy server type: "${type}"`);
> +    }
> +    type = type.toLowerCase();
> +    if (type == PROXY_TYPES.PROXY) {
> +      // PROXY_TYPES.HTTP and PROXY_TYPES.PROXY are synonyms
> +      return PROXY_TYPES.HTTP;

This return value doesn't seem to be used. Also, I'd expect to just have `PROXY: "http"` in the types map.

::: toolkit/components/extensions/ProxyScriptContext.jsm:104
(Diff revision 5)
> +    proxyData.port = port;
> +  },
> +
> +  username(proxyData) {
> +    let {username} = proxyData;
> +    if (username !== undefined && typeof username != "string") {

Nit: `!==`

::: toolkit/components/extensions/ProxyScriptContext.jsm:111
(Diff revision 5)
> +    }
> +  },
> +
> +  password(proxyData) {
> +    let {password} = proxyData;
> +    if (password !== undefined && typeof password != "string") {

`!==`

::: toolkit/components/extensions/ProxyScriptContext.jsm:120
(Diff revision 5)
> +
> +  proxyDNS(proxyData) {
> +    let {proxyDNS, type} = proxyData;
> +    // Do we want the SOCKS layer to send the hostname and port to the proxy and let it do the DNS?
> +    if (proxyDNS !== undefined) {
> +      if (typeof proxyDNS != "boolean") {

`!==`

::: toolkit/components/extensions/ProxyScriptContext.jsm:123
(Diff revision 5)
> +    // Do we want the SOCKS layer to send the hostname and port to the proxy and let it do the DNS?
> +    if (proxyDNS !== undefined) {
> +      if (typeof proxyDNS != "boolean") {
> +        throw new ExtensionError(`FindProxyForURL: Invalid proxyDNS value: "${proxyDNS}"`);
> +      }
> +      if (proxyDNS && type != PROXY_TYPES.SOCKS && type != PROXY_TYPES.SOCKS4) {

`!==`

::: toolkit/components/extensions/ProxyScriptContext.jsm:139
(Diff revision 5)
> +  },
> +
> +  createProxyInfoFromData(proxyDataList, defaultProxyInfo) {
> +    let {type, host, port, username, password, proxyDNS, failoverTimeout} =
> +        ProxyInfoData.validate(proxyDataList.shift());
> +    if (type == PROXY_TYPES.DIRECT) {

Nit: `===`

You ge the idea...

::: toolkit/components/extensions/ProxyScriptContext.jsm:280
(Diff revision 5)
> +        }
> +        proxyData = proxyRules;
> +        // fall through
> +      case "object":
> +        if (Array.isArray(proxyData) && proxyData.length > 0) {
> +          return ProxyInfoData.createProxyInfoFromData(proxyData.slice(), defaultProxyInfo);

Why `.slice()`? If the function wants to modify the array, it should copy it itself.

::: toolkit/components/extensions/ProxyScriptContext.jsm:353
(Diff revision 5)
>      this.apiCan = apiCan;
>    }
>  
>    shouldInject(namespace, name, allowedContexts) {
>      if (this.context.envType !== "proxy_script") {
> -      throw new Error(`Unexpected context type "${this.context.envType}"`);
> +      throw new ExtensionError(`Unexpected context type "${this.context.envType}"`);

This is an internal error. It shouldn't be exposed to the extension. So please throw a normal Error() instead.

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts_results.js:36
(Diff revision 5)
> +      browser.proxy.register("proxy.js").then(() => {
> +        browser.test.sendMessage("ready");
> +      });
> +    },
> +    files: {
> +      "proxy.js": `

"use strict";
Attachment #8901397 - Flags: review?(kmaglione+bmo) → review+

Comment 35

2 years ago
mozreview-review
Comment on attachment 8902485 [details]
Bug 1381290 add a socks authentication test,

https://reviewboard.mozilla.org/r/174070/#review183084

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:13
(Diff revision 3)
> +const BinaryInputStream = CC("@mozilla.org/binaryinputstream;1",
> +                           "nsIBinaryInputStream",
> +                           "setInputStream");
> +
> +const currentThread = Cc["@mozilla.org/thread-manager;1"]
> +                      .getService().currentThread;

`nsIThread` access from JavaScript is going to be removed. You should probably be using TCPServerSocket instead.

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:20
(Diff revision 3)
> +function buf2str(buf) {
> +  return String.fromCharCode.apply(null, buf);
> +}

:/ All of this should really be using typed arrays, and either the TCPSocket bindings or the more modern ArrayBuffer-based stream wrappers...

But I don't have the energy to complain about it now. Just know that when these things are deprecated and it comes back to bite us, you're going to be the one responsible for making this code continue to work.
Attachment #8902485 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 38

2 years ago
Comment on attachment 8901397 [details]
Bug 1381290 support proxyInfo object return from FindProxyForURL,

carry over r+
Attachment #8901397 - Flags: review?(matthewjwein) → review+
Assignee

Updated

2 years ago
Blocks: 1398860
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8902485 - Attachment is obsolete: true

Comment 40

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s daf0d120a995 -d acb232650faf: rebasing 419248:daf0d120a995 "Bug 1381290 support proxyInfo object return from FindProxyForURL, r=kmag" (tip)
merging toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
warning: conflicts while merging toolkit/components/extensions/test/xpcshell/xpcshell-common.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

2 years ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f3342ea446a5
support proxyInfo object return from FindProxyForURL, r=kmag
https://hg.mozilla.org/mozilla-central/rev/f3342ea446a5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is there necessary any manual testing around this bug? If yes, could you please provide some reliable steps and the proper webextension?
Flags: needinfo?(mixedpuppy)
Assignee

Comment 46

2 years ago
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #45)
> Is there necessary any manual testing around this bug? If yes, could you
> please provide some reliable steps and the proper webextension?

It's covered by tests.
Flags: needinfo?(mixedpuppy)

Updated

2 years ago
Blocks: 1401511
Reporter

Updated

2 years ago
Keywords: dev-doc-needed
I've added some stuff here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy#PAC_file_specification on the return value.

Please let me know if this is right.

I wasn't sure whether it returns an array of objects or an object which may contain an array of failover objects (as comment 0 says). As far as I can tell it returns an array.
Flags: needinfo?(mixedpuppy)
Assignee

Comment 49

2 years ago
It should be noted that username/password is usable for socks5.  For http proxy authorizations they will need to use WebRequest.onAuthRequired in the background script.

It does return an array of objects.
Flags: needinfo?(mixedpuppy)
Thanks Shane, I've updated the doc with this.

Updated

2 years ago
Flags: qe-verify-

Updated

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