Closed Bug 1295807 Opened 8 years ago Closed 8 years ago

Support proxy configuration from WebExtensions

Categories

(WebExtensions :: Request Handling, defect)

Product:
WebExtensions
Component:
Request Handling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: mattw, Assigned: mattw, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [proxy] triaged)

Attachments

(2 files, 3 obsolete files)

Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts.
58 bytes, text/x-review-board-request
robwu
: review+
kmag
: review+
Details
Part 2: Bug 1295807 - Implement the Proxy API.
58 bytes, text/x-review-board-request
kmag
: review+
Details
The PAC scripts should run in a sandbox in the main process to avoid round trip requests to the extension process with each execution. Similar to content scripts[1], we should provide the scripts with access to a limited subset of the WebExtension APIs. For more information, see the proxy API design document[2] [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts [2] https://docs.google.com/documen/d/1W45o5X2bFRPrTaQDFp9IzTJ8njCVfEgyENS7i2owaUI/edit?usp=sharing
Blocks: 1296081
Assignee: nobody → mwein
Comment on attachment 8783034 [details] Part 3: Bug 1295807 - Add in the schema for the network API. https://reviewboard.mozilla.org/r/73008/#review70782 ::: toolkit/components/extensions/ExtensionProxyManager.jsm:19 (Diff revision 1) > +let gPPS = Components.classes["@mozilla.org/network/protocol-proxy-service;1"] > + .getService(Components.interfaces.nsIProtocolProxyService); `defineLazyServiceGetter(this, "proxyService", ...)` ::: toolkit/components/extensions/ExtensionProxyManager.jsm:91 (Diff revision 1) > - // TODO: Set up the proxy filter based on the result of FindProxyForUrl. > + let filter = new ProxyFilter(result, url); > + gPPS.registerFilter(filter, 0); > + this.filters.push(filter); No, one proxy filter that calls `FindProxyForUrl` every time `applyFilter` is called.
Attachment #8783034 - Flags: review?(kmaglione+bmo)
Comment on attachment 8783034 [details] Part 3: Bug 1295807 - Add in the schema for the network API. https://reviewboard.mozilla.org/r/73008/#review70782 > No, one proxy filter that calls `FindProxyForUrl` every time `applyFilter` is called. This sounds a lot more efficient than creating a proxy filter for each URL that loads. Would it be efficient to pass in the sandbox to the ProxyFilter constructor and access it each time `applyFilter` runs?
Comment on attachment 8783034 [details] Part 3: Bug 1295807 - Add in the schema for the network API. https://reviewboard.mozilla.org/r/73008/#review70782 > This sounds a lot more efficient than creating a proxy filter for each URL that loads. Would it be efficient to pass in the sandbox to the ProxyFilter constructor and access it each time `applyFilter` runs? Either the context itself should act as the proxy filter, or we should have one global proxy filter that handles the PACs for all extensions.
Component: WebExtensions → WebExtensions: Request Handling
No longer blocks: 1296081
Depends on: 1287010
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. The objectives for this bug seem to be changing frequently. I'm happy to work with you on it if you like but from what I can tell it looks like Kris has been more actively involved...
Attachment #8782306 - Flags: review?(aswan)
Summary: [tracking] Add support for running sandboxed PAC scripts in the main process. → [tracking] Implement the Network API
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review73288 The restrictions look good, but by default messages should not be sent to PAC scripts, unless the extension explicitly requests so. runtie.sendMessage/connect is specified to deliver messages to destinations in an extension process, while PAC scripts run in the default process. Add an extra option to runtime.sendMessage and runtime.connect to allow extensions to send messages to PAC scripts (both APIs already have an optional object, you can store the new optional property here). And use `FindProxyForURL` instead of `FindProxyForUrl`. ::: toolkit/components/extensions/ProxyScript.jsm:35 (Diff revision 5) > + BaseContext, > + Messenger, > + SchemaAPIManager, > +} = ExtensionUtils; > + > +const FAILOVER_TIMEOUT = 10; Which unit is being used here? Seconds? Milliseconds? ::: toolkit/components/extensions/ProxyScript.jsm:39 (Diff revision 5) > + > +const FAILOVER_TIMEOUT = 10; > + > +function createMessenger(extension, context) { > + let sender = {id: extension.uuid}; > + sender.url = extension.baseURI.spec; Is it possible to use the PAC script's URL instead? ::: toolkit/components/extensions/ProxyScript.jsm:42 (Diff revision 5) > +function createMessenger(extension, context) { > + let sender = {id: extension.uuid}; > + sender.url = extension.baseURI.spec; > + let delegate = { > + getSender() {}, > + }; FYI delegate is going away with bugzil.la/1298979 (in the patch called "Bug 1298979 - Add ProxyMessenger, change message managers and getSender") With that change, this would become a 3-line function, so can you inline it? ::: toolkit/components/extensions/ProxyScript.jsm:57 (Diff revision 5) > + let ret; > + try { > + let code = this.generateCode(uri); > + ret = Components.utils.evalInSandbox(code, this.sandbox); > + } catch (e) { > + return null; Why are you shallowing all errors instead of doing something like `Cu.reportError(e);` ? ::: toolkit/components/extensions/ProxyScript.jsm:78 (Diff revision 5) > + isPrivate = PrivateBrowsingUtils.isWindowPrivate(win); > + windowId = win.id; > + if (win.gBrowser) { > + tabId = win.gBrowser.selectedTab && win.gBrowser.selectedTab.id; > + } > + } Isn't this information misleading? There is no guarantee that the most recently focused window initiated the request, is there? ::: toolkit/components/extensions/ProxyScript.jsm:89 (Diff revision 5) > + let details = {tabId, windowId, userContextId, isPrivate}; > + > + return `FindProxyForUrl("${url}", "${host}", ${JSON.stringify(details)});`; > + } > + > + parseRule(rule, failoverRules) { Firefox already knows how to parse the results of PAC scripts. Can't you reuse the functionality? A quick search yields the non-public `nsProtocolProxyService::ProcessPACString` method. ::: toolkit/components/extensions/ProxyScript.jsm:151 (Diff revision 5) > + browserObj /* dest */ > + ); > + > + try { > + Services.scriptloader.loadSubScript(url, this.sandbox_, "UTF-8"); > + } catch (error) { `url` is an arbitrary user-defined script, so you should not ignore errors. ::: toolkit/components/extensions/ProxyScript.jsm:155 (Diff revision 5) > + Services.scriptloader.loadSubScript(url, this.sandbox_, "UTF-8"); > + } catch (error) { > + return; > + } > + > + if (!this.sandbox_.FindProxyForUrl || typeof this.sandbox_.FindProxyForUrl != "function") { Print a warning to the console or notify the extension developer in some other way. And access the variable only once, like this: ``` let {FindProxyForURL} = this.sandbox_; if (typeof FindProxyForURL != "function") { // error handling return; } ``` (also note that I use `FindProxyForURL` with capital `URL` instead of `Url` because PAC scripts use the capitalized function name) ::: toolkit/components/extensions/ext-runtime.js:23 (Diff revision 5) > } = ExtensionUtils; > > XPCOMUtils.defineLazyModuleGetter(this, "NativeApp", > "resource://gre/modules/NativeMessaging.jsm"); > > -extensions.registerSchemaAPI("runtime", "addon_parent", context => { > +extensions.registerSchemaAPI("runtime", "addon_parent", (context) => { This change is unnecessary, please undo it. ::: toolkit/components/extensions/test/xpcshell/test_proxy_script.js:242 (Diff revision 5) > + host: "1.2.3.4", > + port: "8080", > + type: "https", > + }, > + }, runtimeMessage); > +}); Need tests for: - FindProxyForURL not defined. - FindProxyForURL defined but not a function. - FindProxyForURL returning junk (not a string).
Attachment #8782305 - Flags: review?(rob) → review-
Summary: [tracking] Implement the Network API → [tracking] Support proxy configuration from WebExtensions
Depends on: 1298979
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review73288 > Which unit is being used here? Seconds? Milliseconds? Seconds - I'll update the name to include the units. > Is it possible to use the PAC script's URL instead? Yeah, that makes sense, I'll switch to using that. > FYI delegate is going away with bugzil.la/1298979 (in the patch called "Bug 1298979 - Add ProxyMessenger, change message managers and getSender") > > With that change, this would become a 3-line function, so can you inline it? Thanks. I'll inline it and add Bug 1298979 as a dependency. > Why are you shallowing all errors instead of doing something like `Cu.reportError(e);` ? In Part 4, I emit all of these errors to the extension for handling, but you're right, I'll also report these errors to the error console. I should probably actually return |proxyInfo| here, since that would pass on control to other addons that have registered scripts rather than skipping them and returning |direct| when errors like this occur. I updated this and added more tests. > Isn't this information misleading? There is no guarantee that the most recently focused window initiated the request, is there? The problem was that I was trying to get the information for the xpcshell tests when they don't really have windows or tabs. I'm going to move the code that retrieves this information into the API code, and then add mochitests for it. > Firefox already knows how to parse the results of PAC scripts. Can't you reuse the functionality? A quick search yields the non-public `nsProtocolProxyService::ProcessPACString` method. We could potentially use this, but we would have to modify it to support the additional return types being added ("PASS", "SYSTEM", "WPAD"), and I'm not sure if we want to do that yet. > `url` is an arbitrary user-defined script, so you should not ignore errors. I'll report this error to the error console. I'm also sending these errors to the extension's onProxyError listener in Part 4. > Print a warning to the console or notify the extension developer in some other way. > > And access the variable only once, like this: > > ``` > let {FindProxyForURL} = this.sandbox_; > if (typeof FindProxyForURL != "function") { > // error handling > return; > } > ``` > > (also note that I use `FindProxyForURL` with capital `URL` instead of `Url` because PAC scripts use the capitalized function name) That's a really great catch, thank you! > This change is unnecessary, please undo it. Done. > Need tests for: > > - FindProxyForURL not defined. > - FindProxyForURL defined but not a function. > - FindProxyForURL returning junk (not a string). Done. The tests I added confirm that the correct ProxyInfo is returned when these failures occur, and I also added tests to confirm the correct failover proxies are included. In part 4, I add tests to confirm that the correct errors are thrown.
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review74940 Context and messenger changes look good with the next points addresseed. ::: toolkit/components/extensions/ProxyScript.jsm:23 (Diff revision 6) > +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil", > + "resource://gre/modules/NetUtil.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "Task", > + "resource://gre/modules/Task.jsm"); > + Nit: In other files I don't see blank lines between the lazy definitions. ::: toolkit/components/extensions/ProxyScript.jsm:142 (Diff revision 6) > + // TODO(matt): Consider validating the information taken from |contextInfo|. > + let {tabId, windowId, userContextId, isPrivate} = this.contextInfo; > + > + let url = `${uri.scheme}://${uri.host}`; > + let host = `${uri.host}`; > + let contextInfo = `${JSON.stringify({tabId, windowId, userContextId, isPrivate})}`; Nit: These template literals are unnecessary. ::: toolkit/components/extensions/ProxyScript.jsm:148 (Diff revision 6) > + > + return `FindProxyForURL("${url}", "${host}", ${contextInfo});`; > + } > +} > + > +class ProxyContext extends BaseContext { Oops. Please call this `ProxyScriptContext`. We already have a `ProxyContext` in Extension.jsm, so this is confusing. ::: toolkit/components/extensions/ProxyScript.jsm:162 (Diff revision 6) > + this.messageManager = Services.ppmm; > + > + let sender = {id: extension.uuid, url: this.url_}; > + > + // Only allow messages to the sandbox if the sender uses this filter. > + let filter = {toProxyScriptSandbox: extension.id}; Change to `{extensionId: extension.id, toProxyScriptSandbox: true}` and adjust the caller accordingly. extensionId is automatically set in http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/toolkit/components/extensions/ExtensionUtils.jsm#310 (as `options.recipient.extensionId`). ::: toolkit/components/extensions/ext-c-runtime.js:41 (Diff revision 6) > } else if (args.length == 2) { > if (typeof args[0] == "string" && args[0]) { > [extensionId, message] = args; > } else { > [message, options] = args; > + dump(`L0G options ${JSON.stringify(options)}`); Remove this. ::: toolkit/components/extensions/ext-c-runtime.js:57 (Diff revision 6) > return Promise.reject({message: "runtime.sendMessage's extensionId argument is invalid"}); > } > if (options != null && typeof options != "object") { > return Promise.reject({message: "runtime.sendMessage's options argument is invalid"}); > } > // TODO(robwu): Validate option keys and values when we support it. Since you've introduced an option, please add validation :)
Attachment #8782305 - Flags: review?(rob) → review+
Comment on attachment 8783351 [details] Part 4: Bug 1295807 - Add in the implementation for the network API. https://reviewboard.mozilla.org/r/73182/#review74950 ::: toolkit/components/extensions/ProxyScript.jsm:192 (Diff revision 7) > ); > > try { > Services.scriptloader.loadSubScript(this.url_, this.sandbox_, "UTF-8"); > } catch (error) { > + this.extension.emit("network-error", error); FYI: `error` need not be an Error object. The script could throw anything, e.g. `throw null` and down when you use `error.message` that piece of code itself will throw. ::: toolkit/components/extensions/ext-network.js:12 (Diff revision 7) > +XPCOMUtils.defineLazyModuleGetter(this, "ProxyScript", > + "resource://gre/modules/ProxyScript.jsm"); > + > +var {EventManager} = ExtensionUtils; > + > +// extension -> ProxyContext `ProxtContext` -> `ProxyScriptContext` (see my previous comment for why). ::: toolkit/components/extensions/ext-network.js:18 (Diff revision 7) > +let proxyScriptMap = new Map(); > + > +/* eslint-disable mozilla/balanced-listeners */ > +extensions.on("shutdown", (type, extension) => { > + if (proxyScriptMap.has(extension)) { > + let proxyScript = proxyScriptMap.get(extension); I've seen this pattern a couple of times in WebExtensions code. Is there any reason why you're not using the following? ``` let proxyScript = proxyScriptMap.get(extension); if (proxyScript) { proxyScript.unload(); proxyScriptMap.delete(extension); } ``` ::: toolkit/components/extensions/ext-network.js:33 (Diff revision 7) > + network: { > + setProxyScript: (url) => { > + let proxyScript; > + if (proxyScriptMap.has(extension)) { > + proxyScript = proxyScriptMap.get(extension); > + proxyScript.unload(); In case the next code throws, add this: `proxyScriptMap.delete(extension);`
Comment on attachment 8786422 [details] Part 5: Bug 1295807 - Add unit tests for the network API. https://reviewboard.mozilla.org/r/75364/#review74952 ::: toolkit/components/extensions/ext-network.js:36 (Diff revision 4) > if (proxyScriptMap.has(extension)) { > proxyScript = proxyScriptMap.get(extension); > proxyScript.unload(); > } > > - proxyScript = new ProxyScript(extension); > + proxyScript = new ProxyScript(extension, url); This change belongs to the fourth patch.
Following-up on an email conversation between me and K-9: this API needs to support username/password authentication to proxy servers. Chrome does it with chrome.webRequest.onAuthRequired.addListener, see https://developer.chrome.com/extensions/webRequest. A trivial example: chrome.webRequest.onAuthRequired.addListener(function(details) { if (details.isProxy === true) return {authCredentials: {username: "eric", password: "mypass"}}; // Can also return {cancel: true} } The problem with this implementation, IMO, is that it is completely separated from the rest of the API. It has no access to window/tab information, for example. It's much more straightforward to extend the return values from PAC script to specify username/password info. For example: return 'PROXY eric@mypass:foobar.com:3128; PROXY eric@mypass:foobarz.com; DIRECT';
Don't forget FindProxyForURL() can also return 'HTTPS'; e.g. return 'HTTPS secure-proxy.example.com:443'; see https://bugzilla.mozilla.org/show_bug.cgi?id=378637 and https://www.chromium.org/developers/design-documents/secure-web-proxy As I proposed in the Proxy API Design Document that K-9 and I authored, I believe it should be extended to permit other values, too: Current supported values: DIRECT: no proxy server PROXY: HTTP proxy server HTTPS: HTTPS/SSL proxy server New values: HTTP: same as PROXY ('PROXY' was spec'd when other types of proxy servers probably didn't yet exist) SOCKS4: SOCKS4 or 4A proxy server SOCKS5: SOCKS 5 proxy server SPDY: SPDY proxy server (future use; I do not know if netwerk has SPDY proxy support ATM) QUIC: QUICK UDP proxy server (future use; I do not know if netwerk has support for it ATM). Chrome supports this; see https://developer.chrome.com/extensions/proxy#type-Scheme
> New values: > HTTP: same as PROXY ('PROXY' was spec'd when other types of proxy servers > probably didn't yet exist) > SOCKS4: SOCKS4 or 4A proxy server > SOCKS5: SOCKS 5 proxy server > SPDY: SPDY proxy server (future use; I do not know if netwerk has SPDY proxy > support ATM) > QUIC: QUICK UDP proxy server (future use; I do not know if netwerk has > support for it ATM). Chrome supports this; see > https://developer.chrome.com/extensions/proxy#type-Scheme Forgot to mention a couple more: SYSTEM: use system settings. No other data needed in string; i.e., return 'SYSTEM'; WPAD: use WPAD for proxy settings. No other data needed in string; i.e. return 'WPAD'; You can see partial implementations of this in the FoxyProxy code: http://code.getfoxyproxy.org/Foxyproxy_Firefox/tree/src/components/proxy.js#n656 http://code.getfoxyproxy.org/Foxyproxy_Firefox/tree/src/components/proxy.js#n738
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review76456 ::: toolkit/components/extensions/ExtensionUtils.jsm:1275 (Diff revisions 6 - 7) > this.context = context; > this.messageManagers = messageManagers; > this.sender = sender; > this.filter = filter; > this.delegate = delegate; > + this.strict = strict; Why `this.strict`? To filter messages, just add a `messageFilter`. You don't have to use `MessageChannel.matchesFilter`, see the small diff and commit message at https://hg.mozilla.org/mozilla-central/rev/d074142f6055 ::: toolkit/components/extensions/ProxyScript.jsm:172 (Diff revisions 6 - 7) > > let sender = {id: extension.uuid, url: this.url_}; > > // Only allow messages to the sandbox if the sender uses this filter. > - let filter = {toProxyScriptSandbox: extension.id}; > - this.messenger = new Messenger(this, [this.messageManager], sender, filter); > + let filter = {extensionId: extension.id, toProxyScriptSandbox: true}; > + this.messenger = new Messenger(this, [this.messageManager], sender, filter, null /* delegate */, true /* strict */); There is no delegate any more. It was removed in https://hg.mozilla.org/mozilla-central/rev/329102a4c682 What's up with `strict`? ::: toolkit/components/extensions/ext-c-runtime.js:63 (Diff revisions 6 - 7) > > + if (options) { > + if ("toProxyScriptSandbox" in options && typeof options.toProxyScriptSandbox == "boolean") { > + recipient.toProxyScriptSandbox = options.toProxyScriptSandbox; > + } > + if ("includeTlsChannelId" in options && typeof options.includeTlsChannelId == "boolean") { `includeTlsChannelId` is not supported by Firefox. Let's not check its type. ::: toolkit/components/extensions/Schemas.jsm:477 (Diff revision 7) > /** > * @property {Array<string>} [restrictions] > * A list of restrictions to consider before generating the API. > * These are not parsed by the schema, but passed to `shouldInject`. > */ > - this.restrictions = schema.restrictions || null; > + this.restrictions = schema.restrictions || []; Do you plan to land these patches soon? If not can you create a separate patch for the `restrictions` field? Luca's patch can benefit from this simplification. ::: toolkit/components/extensions/test/xpcshell/xpcshell.ini:57 (Diff revision 7) > [test_ext_storage.js] > [test_getAPILevelForWindow.js] > [test_locale_converter.js] > [test_locale_data.js] > [test_native_messaging.js] > +[test_proxy_script.js] Move this one line below. The `skip-if = os == 'android'` should be under the `test_native_messaging.js` section, not under your new test file.
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review76456 > Why `this.strict`? > > To filter messages, just add a `messageFilter`. You don't have to use `MessageChannel.matchesFilter`, see the small diff and commit message at https://hg.mozilla.org/mozilla-central/rev/d074142f6055 I want to make sure that the messages are only delivered to the sandbox when they include the filter |toProxyScriptSandbox: true|, and it seems like the only way to do that is to use strict filtering rather than permissive filtering. > Do you plan to land these patches soon? If not can you create a separate patch for the `restrictions` field? Luca's patch can benefit from this simplification. I'll create a separate patch for this.
Depends on: 1302020
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review76456 > I want to make sure that the messages are only delivered to the sandbox when they include the filter |toProxyScriptSandbox: true|, and it seems like the only way to do that is to use strict filtering rather than permissive filtering. Adding `strict` twice would cause the filter to be applied twice. I've fixed the root cause by splitting up the filter in two parts, see bugzil.la/1302020
Depends on: 1302898
(In reply to Eric H. Jung from comment #48) > return 'PROXY eric@mypass:foobar.com:3128; PROXY eric@mypass:foobarz.com; > DIRECT'; The other benefit of this implementation over Chrome's is that the PAC file is much more self-contained. The logic for proxy and proxy auth decisions becomes much more portable.
Attachment #8783034 - Attachment is obsolete: true
Attachment #8783034 - Flags: review?(kmaglione+bmo)
Attachment #8783351 - Attachment is obsolete: true
Attachment #8783351 - Flags: review?(kmaglione+bmo)
Attachment #8786422 - Attachment is obsolete: true
Attachment #8786422 - Flags: review?(kmaglione+bmo)
Summary: [tracking] Support proxy configuration from WebExtensions → Support proxy configuration from WebExtensions
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review94954 ::: toolkit/components/extensions/ProxyScriptContext.jsm:62 (Diff revision 8) > + try { > + let code = this.generateCode(uri); > + ret = Components.utils.evalInSandbox(code, this.sandbox); > + } catch (e) { > + this.extension.emit("proxy-error", new Error( > + `FindProxyForURL: ${e.message}`)); You don't know the exception is coming from FindProxyForURL(). There can be other code in the PAC script (e.g. helper functions or variables defined outside the scope of the FindProxyForURL() function) that is causing the exception.
@Matthew, are you allowing the "HTTP" to be an alias for "PROXY" in the FindProxyForURL() return types? As we discussed in the specification drafts? Also, anything about authentication in the implementation?
Blocks: 1319630
Blocks: 1319631
Blocks: 1319634
Blocks: 1319641
Blocks: 1319642
matthew - can you re-link to the proxy design documentation? I haven't reviewed it. The link in comment 0 is stale. thanks
(In reply to Patrick McManus [:mcmanus] from comment #63) > matthew - can you re-link to the proxy design documentation? I haven't > reviewed it. > > The link in comment 0 is stale. > thanks Sure, thanks! https://docs.google.com/document/d/1W45o5X2bFRPrTaQDFp9IzTJ8njCVfEgyENS7i2owaUI/edit?usp=sharing
thanks for sharing.. I'm a little unclear with the interaction between web-extensions and pac.. are you proposing extending normal pac not loaded via a web-ext addon, or is this just in the scope of things loaded with explicit permissions. for the former I would note that there is a very conservative security model that includes not being able to leak much information out of the script.. changes to it probably belong in some standards forum.
> I'm a little unclear with the interaction between web-extensions and pac.. > are you proposing extending normal pac not loaded via a web-ext addon, or is > this just in the scope of things loaded with explicit permissions. The latter - it only extends PAC scripts for webextensions with the associated permission.
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 ::: toolkit/components/extensions/ExtensionProxyManager.jsm:13 (Diff revision 2) > + > +/* exported ExtensionProxyManager */ > + > +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > + > +Cu.import("resource://gre/modules/Services.jsm"); This isn't used. ::: toolkit/components/extensions/ExtensionProxyManager.jsm:16 (Diff revision 2) > +XPCOMUtils.defineLazyModuleGetter(this, "Task", > + "resource://gre/modules/Task.jsm"); This isn't used. ::: toolkit/components/extensions/ExtensionProxyManager.jsm:19 (Diff revision 2) > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "Task", > + "resource://gre/modules/Task.jsm"); > + > +class ProxyScript { This should be a Context, and extend BaseContext. ::: toolkit/components/extensions/ExtensionProxyManager.jsm:21 (Diff revision 2) > +XPCOMUtils.defineLazyModuleGetter(this, "Task", > + "resource://gre/modules/Task.jsm"); > + > +class ProxyScript { > + constructor(extension) { > + let principal = extension.createPrincipal(); This principal should be created with the URL that we're going to load into the sandbox. ::: toolkit/components/extensions/ExtensionProxyManager.jsm:24 (Diff revision 2) > +class ProxyScript { > + constructor(extension) { > + let principal = extension.createPrincipal(); > + > + this.sandbox = Cu.Sandbox(principal, { > + sandboxName: `proxyscript:${extension.id}`, This should also include the URL. ::: toolkit/components/extensions/ExtensionProxyManager.jsm:25 (Diff revision 2) > + constructor(extension) { > + let principal = extension.createPrincipal(); > + > + this.sandbox = Cu.Sandbox(principal, { > + sandboxName: `proxyscript:${extension.id}`, > + }); `metadata: {addonID: extension.id},` ::: toolkit/components/extensions/ExtensionProxyManager.jsm:34 (Diff revision 2) > + > + load(url) { > + try { > + Services.scriptloader.loadSubScript(url, this.sandbox, "UTF-8"); > + } catch (e) { > + return Promise.reject(e.message); Promises should be rejected with actual error objects. However, I see no reason for this function to return a promise. ::: toolkit/components/extensions/ExtensionProxyManager.jsm:39 (Diff revision 2) > + return Promise.reject(e.message); > + } > + return Promise.resolve(); > + } > + > + run() { `run` doesn't seem to be a good name for this function. Also, it could use a doc comment. ::: toolkit/components/extensions/ExtensionProxyManager.jsm:44 (Diff revision 2) > + let result; > + try { > + // TODO: Pass in the correct arguments to FindProxyForUrl(url, host, details). > + result = Components.utils.evalInSandbox("FindProxyForUrl()", this.sandbox); > + } catch (e) { > + return Promise.reject(e.message); > + } `this.callSafe(this.sandbox.FindProxyForURL, ...)` ::: toolkit/components/extensions/ExtensionProxyManager.jsm:57 (Diff revision 2) > + // TODO: Set up the proxy filter based on the result of FindProxyForUrl. > + > + return Promise.resolve(result); > + } > + > + destroy() { `unload` ::: toolkit/components/extensions/ProxyScript.jsm:46 (Diff revision 5) > + getSender() {}, > + }; > + return new Messenger(context, [context.messageManager], sender, {}, delegate); > +} > + > +class ProxyFilter { Most of this belongs in the `ProxyContext` class. ::: toolkit/components/extensions/ProxyScript.jsm:82 (Diff revision 5) > + } > + } > + > + // TODO(matt): Get the user context ID. > + > + let url = `${uri.scheme}://${uri.host}`; Why isn't this the full URL? ::: toolkit/components/extensions/ProxyScript.jsm:83 (Diff revision 5) > + } > + > + // TODO(matt): Get the user context ID. > + > + let url = `${uri.scheme}://${uri.host}`; > + let host = `${uri.host}`; No need for a template string here. ::: toolkit/components/extensions/ProxyScript.jsm:86 (Diff revision 5) > + > + let url = `${uri.scheme}://${uri.host}`; > + let host = `${uri.host}`; > + let details = {tabId, windowId, userContextId, isPrivate}; > + > + return `FindProxyForUrl("${url}", "${host}", ${JSON.stringify(details)});`; No eval. `context.callSafe(sandbox.FindProxyForUrl, ...)` ::: toolkit/components/extensions/ProxyScript.jsm:136 (Diff revision 5) > + this.principal_ = extension.principal; > + this.sandbox_ = Cu.Sandbox(this.principal_); > + this.filter_ = new ProxyFilter(this.sandbox_); No underscored properties. Just define properties with the expected names. ::: toolkit/components/extensions/ProxyScriptContext.jsm:30 (Diff revision 10) > +const CATEGORY_EXTENSION_SCRIPTS_CONTENT = "webextension-scripts-content"; > + > +/** > + * An Error subclass for which proxy error messages are passed o extensions. > + */ > +class ProxyScriptError extends Error {} What's the purpose of this class? ::: toolkit/components/extensions/ProxyScriptContext.jsm:77 (Diff revision 10) > + "Return type must be a string")); > + return defaultProxyInfo; > + } > + > + let rules = ret.split(";"); > + let proxyInfo = this.parseRule(rules.shift(), rules); I don't see why we'd pass the first rule separately here. Just pass `rules.split(";")` ::: toolkit/components/extensions/ProxyScriptContext.jsm:91 (Diff revision 10) > + this.extension.emit("proxy-error", new ProxyScriptError( > + "FindProxyForUrl: Missing Proxy Rule")); > + return null; > + } > + > + let parts = rule.split(" "); `.split(/\s+/)` ::: toolkit/components/extensions/ProxyScriptContext.jsm:101 (Diff revision 10) > + return null; > + } > + > + parts[0] = parts[0].toLowerCase(); > + > + let host, port, type, timeout; Please declare these when you use them. ::: toolkit/components/extensions/ProxyScriptContext.jsm:131 (Diff revision 10) > + type = PROXY_TYPES.HTTPS; > + } > + > + let failoverProxyInfo = null; > + if (failoverRules.length && failoverRules[0]) { > + failoverProxyInfo = this.parseRule(failoverRules.shift(), failoverRules); You shouldn't be modifying an array that you recieve as an argument. ::: toolkit/components/extensions/ProxyScriptContext.jsm:136 (Diff revision 10) > + failoverProxyInfo = this.parseRule(failoverRules.shift(), failoverRules); > + } > + > + return ProxyService.newProxyInfo(type, host, port, 0, timeout, failoverProxyInfo); > + case PROXY_TYPES.DIRECT: > + return ProxyService.newProxyInfo("direct", "", -1, 0, 0, null); I'd rather we use the default proxy in this case, until we have a way to sanely handle multiple filters. ::: toolkit/components/extensions/ProxyScriptContext.jsm:144 (Diff revision 10) > + /** > + * Constructs a function call to `FindProxyForURL` for evaluation in the proxy sandbox. > + * > + * @param {object} uri The URI of the current tab. > + * @returns {string} The `FindProxyForURL` code to execute in the proxy sandbox. > + */ > + generateCode(uri) { > + // TODO(k9): Use `this.contextInfo` to provide contextual information once we have some to provide. > + let contextInfo = {}; > + return `FindProxyForURL("${uri.scheme}://${uri.host}", "${uri.host}", ${JSON.stringify(contextInfo)});`; > + } Just use `context.callSafe` to call `FindProxyForURL` directly. There's no need to eval any code. ::: toolkit/components/extensions/ProxyScriptContext.jsm:170 (Diff revision 10) > + } > + return super.generateAPIs(...args); > + } > + > + registerSchemaAPI(namespace, envType, getAPI) { > + if (envType == "proxy_script" && namespace == "runtime") { The namespace check should be an assertion, if it needs to be there at all. If we're trying to register other APIs in this environment, we shouldn't be silently ignoring it. But I also don't see why we wouldn't want to allow the other content script namespaces here. ::: toolkit/components/extensions/ProxyScriptContext.jsm:184 (Diff revision 10) > + this.localAPIs = localAPIs; > + } > + > + shouldInject(namespace, name, allowedContexts) { > + // Do not generate proxy script APIs unless explicitly allowed. > + return this.context.envType === "proxy_script" && allowedContexts.includes("proxy"); This should also be an assertion. We shouldn't see any other context type here. ::: toolkit/components/extensions/ProxyScriptContext.jsm:202 (Diff revision 10) > + super("proxy_script", extension); > + > + this.extension = extension; > + this.messageManager = Services.cpmm; > + this.sandbox = Cu.Sandbox(this.extension.principal); > + this.filter = new ProxyFilter(this.sandbox, extension, contextInfo); Either pass the context object to the ProxyFilter class, or merge the ProxyFilter class into the Context class. ::: toolkit/components/extensions/ProxyScriptContext.jsm:208 (Diff revision 10) > + this.url = url; > + } > + > + load() { > + Schemas.exportLazyGetter(this.sandbox, "browser", () => this.chromeObj); > + Schemas.exportLazyGetter(this.sandbox, "chrome", () => this.chromeObj); This environment doesn't exist in Chrome, so let's just define the browser namespace here. ::: toolkit/components/extensions/ProxyScriptContext.jsm:257 (Diff revision 10) > + let sender = {id: this.extension.uuid, url: this.url}; > + let filter = {extensionId: this.extension.id, toProxyScriptSandbox: true}; > + return new Messenger(this, [this.messageManager], sender, filter); > +}); > + > +defineLazyGetter(ProxyScriptContext.prototype, "chromeObj", function() { s/chrome/browser/ ::: toolkit/components/extensions/ProxyScriptContext.jsm:259 (Diff revision 10) > + return new Messenger(this, [this.messageManager], sender, filter); > +}); > + > +defineLazyGetter(ProxyScriptContext.prototype, "chromeObj", function() { > + let localAPIs = {}; > + let proxyScriptAPIManager = new ProxyScriptAPIManager(); We don't need a separate instance of this for every context. Just one global instance. ::: toolkit/components/extensions/ext-c-runtime.js:52 (Diff revision 10) > > extensionId = extensionId || extension.id; > let recipient = {extensionId}; > > + if (options != null) { > + if (typeof options != "object") { Please use `===` and `!==`. Same goes for other places you're checking equality. ::: toolkit/components/extensions/ext-c-runtime.js:55 (Diff revision 10) > + if ("toProxyScriptSandbox" in options && typeof options.toProxyScriptSandbox == "boolean") { > + recipient.toProxyScriptSandbox = options.toProxyScriptSandbox; > + } I'm not sure this is necessary. If it is, please don't use the word `Sandbox`. ::: toolkit/components/extensions/ext-c-runtime.js:55 (Diff revision 10) > > + if (options != null) { > + if (typeof options != "object") { > + return Promise.reject({message: "runtime.sendMessage's options argument is invalid"}); > + } > + if ("toProxyScriptSandbox" in options && typeof options.toProxyScriptSandbox == "boolean") { The `in options` check is not necessary. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:18 (Diff revision 10) > + browser.test.onMessage.addListener((message, data) => { > + if (message == "runtime-message") { > + browser.runtime.onMessage.addListener((msg, sender, respond) => { > + if (msg == "finish-from-pac-script") { > + browser.test.notifyPass("proxy"); > + respond(msg); Please don't use response callbacks. Just return a promise. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:39 (Diff revision 10) > + > + let extension = ExtensionTestUtils.loadExtension(extensionData); > + let extension_internal = extension.extension; > + > + yield extension.startup(); > + yield extension.awaitMessage("ready"); This isn't necessary. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:46 (Diff revision 10) > + let script = new ProxyScriptContext(extension_internal, extension_internal.getURL("proxy.js")); > + > + try { > + yield script.load(); > + } catch (error) { > + equal(error, expected.error); Please include descriptions for all assertions. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:75 (Diff revision 10) > + }, > + }); > + }); > + > + if (!expected.proxyInfo) { > + equal(proxyInfo, null); Please include messages for all assertions. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:83 (Diff revision 10) > + equal(proxyInfo.port, expected.proxyInfo.port); > + equal(proxyInfo.type, expected.proxyInfo.type); > + > + let failoverProxyInfo = proxyInfo.failoverProxy; > + let expectedFailoverProxyInfo = expected.proxyInfo.failoverProxy; > + while (failoverProxyInfo) { Please use a `for` loop for this. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:109 (Diff revision 10) > + }); > +}); > + > +add_task(function* testWrongTypeForFindProxyForURL() { > + yield testProxyScript({ > + scriptData: `let FindProxyForURL = "foo";`, Please write these as functions rather than string literals. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:197 (Diff revision 10) > +}); > + > +add_task(function* testProxyReturnType() { > + let scriptData = ` > + function FindProxyForURL(url, host) { > + return "PROXY 1.2.3.4:8080"; Please add tests with unusual whitespace configurations. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:322 (Diff revision 10) > + } > + browser.runtime.onMessage.addListener((msg, sender, respond) => { > + if (msg.host) { > + settings.host = msg.host; > + browser.runtime.sendMessage("finish-from-pac-script"); > + respond(msg); Atain, please return a promise rather than using response callbacks.
Attachment #8782305 - Flags: review?(kmaglione+bmo)
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. https://reviewboard.mozilla.org/r/72516/#review106452 ::: toolkit/components/extensions/ext-proxy.js:1 (Diff revision 12) > +"use strict"; Please add license and modeline headers. ::: toolkit/components/extensions/ext-proxy.js:10 (Diff revision 12) > +Cu.import("resource://gre/modules/ExtensionUtils.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "ProxyScriptContext", > + "resource://gre/modules/ProxyScriptContext.jsm"); > + > +var {EventManager} = ExtensionUtils; Please use the same multi-line destructuring style that we use elsewhere. ::: toolkit/components/extensions/ext-proxy.js:30 (Diff revision 12) > +extensions.registerSchemaAPI("proxy", "addon_parent", context => { > + let {extension} = context; > + return { > + proxy: { > + registerProxyScript: (url) => { > + let proxyScriptContext; Please declare this where it's used. ::: toolkit/components/extensions/ext-proxy.js:32 (Diff revision 12) > + return { > + proxy: { > + registerProxyScript: (url) => { > + let proxyScriptContext; > + if (proxyScriptContextMap.has(extension)) { > + proxyScriptContext = proxyScriptContextMap.get(extension); No need for this variable. Just `proxyScriptContextMap.get(extension).unload()` ::: toolkit/components/extensions/ext-proxy.js:39 (Diff revision 12) > + proxyScriptContextMap.delete(extension); > + } > + > + proxyScriptContext = new ProxyScriptContext(extension, url); > + proxyScriptContext.load(); > + proxyScriptContextMap.set(extension, proxyScriptContext); We shouldn't save this if the load fails. ::: toolkit/components/extensions/ext-proxy.js:44 (Diff revision 12) > + proxyScriptContextMap.set(extension, proxyScriptContext); > + }, > + > + onProxyError: new EventManager(context, "proxy.onProxyError", fire => { > + let listener = (name, error) => { > + fire(error.message); This should probably be an object with a `message` property. ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:59 (Diff revision 12) > + background: backgroundScript, > + manifest: { > + "permissions": ["proxy"], > + }, > + files: { > + "proxy_script.js": `let FindProxyForURL = 5`, Please use functions rather than string literals for these. ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:86 (Diff revision 12) > + manifest: { > + "permissions": ["proxy"], > + }, > + files: { > + "proxy_script.js": `function FindProxyForURL() { > + return not_defined; We should also include a stack/fileName/lineNumber in the error when the error comes from the extension code.
Attachment #8782306 - Flags: review?(kmaglione+bmo)
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 It looks like you reviewed revision 2, not the latest revision which is revision 10. Quite a lot has changed since then. Could you take a look at the latest revision? In the meantime I'll try to fix the issues that are still relavent. > What's the purpose of this class? My initial intention was to make it so error handling didn't depend on the error strings, but to do this I think I'd need to add error types for each error. I'll remove this for now. > I don't see why we'd pass the first rule separately here. Just pass `rules.split(";")` Yeah, that's cleaner, no need to distinguish the first rule from the failover rules. > Just use `context.callSafe` to call `FindProxyForURL` directly. There's no need to eval any code. I tried using `context.callSafe` to replace `evalInSandbox` but it wasn't defined and I couldn't see any references to `callSafe` in searchfox. Is there a different method I should be using? > Please write these as functions rather than string literals. Could you please give me an example of how to do this?
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. https://reviewboard.mozilla.org/r/72516/#review106452 > Please use functions rather than string literals for these. Could you give me an example of how to do this?
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 > Please use a `for` loop for this. I'd be happy to, but I'm not sure how I would because each proxyInfo points to the next proxyInfo with the last one being null.
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 > I'd be happy to, but I'm not sure how I would because each proxyInfo points to the next proxyInfo with the last one being null. I could do something like: for (let failover = proxyInfo.failoverProxy; failover != null; failover = failover.failoverProxy) { equal(failover.host, expectedFailoverProxyInfo.host, `Expected failover proxy host to be ${expected.proxyInfo.host}`); equal(failover.port, expectedFailoverProxyInfo.port, `Expected failover proxy port to be ${expected.proxyInfo.port}`); equal(failover.type, expectedFailoverProxyInfo.type, `Expected failover proxy type to be ${expected.proxyInfo.type}`); expectedFailoverProxyInfo = expectedFailoverProxyInfo.failoverProxy; } But I don't think that's any better.
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 > I could do something like: > > for (let failover = proxyInfo.failoverProxy; failover != null; failover = failover.failoverProxy) { > equal(failover.host, expectedFailoverProxyInfo.host, `Expected failover proxy host to be ${expected.proxyInfo.host}`); > equal(failover.port, expectedFailoverProxyInfo.port, `Expected failover proxy port to be ${expected.proxyInfo.port}`); > equal(failover.type, expectedFailoverProxyInfo.type, `Expected failover proxy type to be ${expected.proxyInfo.type}`); > expectedFailoverProxyInfo = expectedFailoverProxyInfo.failoverProxy; > } > > But I don't think that's any better. for (let proxy = proxyInfo; proxy; proxy = proxy.failoverProxy) You can deal with the expected info in the same loop head, or not. Or you could just make it an array. Of you could just use `Assert.deepEqual`.
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. https://reviewboard.mozilla.org/r/72516/#review106452 > Could you give me an example of how to do this? let scriptData = String(options.scriptData).replace(/^.*?\{(.*)\}$/, "$1");
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 > I tried using `context.callSafe` to replace `evalInSandbox` but it wasn't defined and I couldn't see any references to `callSafe` in searchfox. Is there a different method I should be using? http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/toolkit/components/extensions/ExtensionCommon.jsm#117
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 > Why isn't this the full URL? The path and query components of the URL are supposed to be stripped when it's HTTPS, so I just left it out for now. This should probably be uri.prePath instead of what I currently have, though. I filed bug 1337001 to make sure we provide the path and query components for non-HTTPS URLs. > http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/toolkit/components/extensions/ExtensionCommon.jsm#117 Thanks > for (let proxy = proxyInfo; proxy; proxy = proxy.failoverProxy) > > You can deal with the expected info in the same loop head, or not. Or you could just make it an array. Of you could just use `Assert.deepEqual`. Sounds good, thanks
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review70780 > This should also include the URL. Done. > `metadata: {addonID: extension.id},` Done. > `run` doesn't seem to be a good name for this function. Also, it could use a doc comment. Done. > `this.callSafe(this.sandbox.FindProxyForURL, ...)` Fixed. > `unload` Fixed. > No need for a template string here. Fixed. > No eval. `context.callSafe(sandbox.FindProxyForUrl, ...)` Done. > No underscored properties. Just define properties with the expected names. Done. > `.split(/\s+/)` Thanks. > Please declare these when you use them. Done. > You shouldn't be modifying an array that you recieve as an argument. Fixed. > I'd rather we use the default proxy in this case, until we have a way to sanely handle multiple filters. Done. > The namespace check should be an assertion, if it needs to be there at all. If we're trying to register other APIs in this environment, we shouldn't be silently ignoring it. > > But I also don't see why we wouldn't want to allow the other content script namespaces here. Makes sense, I removed the check for the namespace. > This should also be an assertion. We shouldn't see any other context type here. Done. > Either pass the context object to the ProxyFilter class, or merge the ProxyFilter class into the Context class. Done. > This environment doesn't exist in Chrome, so let's just define the browser namespace here. Done. > s/chrome/browser/ Fixed. > We don't need a separate instance of this for every context. Just one global instance. Done. > Please use `===` and `!==`. Same goes for other places you're checking equality. Fixed. > The `in options` check is not necessary. Removed. > I'm not sure this is necessary. If it is, please don't use the word `Sandbox`. Fixed. > Please don't use response callbacks. Just return a promise. Fixed. > This isn't necessary. Removed. > Please include descriptions for all assertions. Fixed.
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. https://reviewboard.mozilla.org/r/72516/#review106452 > Please add license and modeline headers. Done. > Please use the same multi-line destructuring style that we use elsewhere. Done. > Please declare this where it's used. Done. > No need for this variable. Just `proxyScriptContextMap.get(extension).unload()` Done. > We shouldn't save this if the load fails. Done. > This should probably be an object with a `message` property. Done. > let scriptData = String(options.scriptData).replace(/^.*?\{(.*)\}$/, "$1"); Neat, thanks > We should also include a stack/fileName/lineNumber in the error when the error comes from the extension code. Done.
Is this bug going to be released before Firefox 57 (Nov 14, 2017)? If not, addons/extensions will be able to control proxy behavior. There won't be any Firefox proxy addons/extensions which work.
(In reply to Eric H. Jung from comment #87) > Is this bug going to be released before Firefox 57 (Nov 14, 2017)? If not, > addons/extensions will be able to control proxy behavior. will *NOT* be able to control proxy behavior
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review116156 Getting close, but still a few issues. ::: toolkit/components/extensions/ExtensionChild.jsm:296 (Diff revision 14) > * messages. Unlike `filter`, the keys from `optionalFilter` are allowed to > * be omitted from `recipient`. Only keys that are present in both > * `optionalFilter` and `recipient` are applied to filter incoming messages. > */ > class Messenger { > - constructor(context, messageManagers, sender, filter, optionalFilter) { > + constructor(context, messageManagers, sender, filter, optionalFilter = {}) { Why this change? ::: toolkit/components/extensions/MessageChannel.jsm (Diff revision 14) > - * Returns true if the properties of the `data` object match those in > - * the `filter` object. Matching is done on a strict equality basis, > - * and the behavior varies depending on the value of the `strict` > - * parameter. > - * Why did you remove this? ::: toolkit/components/extensions/ProxyScriptContext.jsm:121 (Diff revision 14) > + } catch (error) { > + this.extension.emit("proxy-error", new Error(error.message)); > + return; > + } > + > + let {FindProxyForURL} = this.sandbox; Please do something like `this.FindProxyForURL = Cu.unwaiveXrays(this.sandbox.FindProxyForURL)` ::: toolkit/components/extensions/ProxyScriptContext.jsm:123 (Diff revision 14) > + return; > + } > + > + let {FindProxyForURL} = this.sandbox; > + if (typeof FindProxyForURL !== "function") { > + this.extension.emit("proxy-error", new Error( Not `new Error`. We don't want to expose privileged error objects to extension code. Same for the instances below. ::: toolkit/components/extensions/ProxyScriptContext.jsm:156 (Diff revision 14) > + applyFilter(service, uri, defaultProxyInfo) { > + let ret; > + try { > + // Bug 1337001 - provide path and query components to non-https URLs. > + ret = this.runSafe(this.sandbox.FindProxyForURL, uri.prePath, uri.host, this.contextInfo); > + } catch (error) { `runSafe` catches any errors the function throws. This will never happen. ::: toolkit/components/extensions/ProxyScriptContext.jsm:157 (Diff revision 14) > + let ret; > + try { > + // Bug 1337001 - provide path and query components to non-https URLs. > + ret = this.runSafe(this.sandbox.FindProxyForURL, uri.prePath, uri.host, this.contextInfo); > + } catch (error) { > + this.extension.emit("proxy-error", new Error(error.message)); We never want to expose arbitrary errors to extension code. They need to go through `context.normalizeError`. Same for other instances. ::: toolkit/components/extensions/ext-c-runtime.js:44 (Diff revision 14) > return Promise.reject({message: "runtime.sendMessage's last argument is not a function"}); > } else { > return Promise.reject({message: "runtime.sendMessage received too many arguments"}); > } > > - if (extensionId != null && typeof extensionId != "string") { > + if (extensionId !== null && typeof extensionId !== "string") { This actually needs to be `!= null` so that it matches undefined. ::: toolkit/components/extensions/ext-c-runtime.js:51 (Diff revision 14) > - // TODO(robwu): Validate option keys and values when we support it. > > extensionId = extensionId || extension.id; > let recipient = {extensionId}; > > + if (options !== null) { This also needs to be `!=` rather than `!==` ::: toolkit/components/extensions/ext-c-runtime.js:55 (Diff revision 14) > > + if (options !== null) { > + if (typeof options !== "object") { > + return Promise.reject({message: "runtime.sendMessage's options argument is invalid"}); > + } > + if (typeof options.toProxyScriptSandbox === "boolean") { We should throw if this exists and is not a boolean. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:100 (Diff revision 14) > +}); > + > +add_task(function* testWrongTypeForFindProxyForURL() { > + yield testProxyScript({ > + scriptData() { > + let FindProxyForURL = "foo"; // eslint-disable-line no-unused-vars You want `var FindProxyForURL` or `this.FindProxyForURL`. `let` places the variable in a block scope, which isn't accessible via the sandbox global. ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:108 (Diff revision 14) > + proxyInfo: null, > + }); > +}); > + > +add_task(function* testInvalidReturnTypeForFindProxyForURL() { > + function scriptData() { Is there a particular reason you don't just use the method syntax everywhere? ::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:109 (Diff revision 14) > + }); > +}); > + > +add_task(function* testInvalidReturnTypeForFindProxyForURL() { > + function scriptData() { > + function FindProxyForURL(url, host) { // eslint-disable-line no-unused-vars Just add something like this to the top of the script: no-unused-vars": ["error", {"args": "none", "varsIgnorePattern": "^(FindProxyForURL)$"}]
Attachment #8782305 - Flags: review?(kmaglione+bmo)
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. https://reviewboard.mozilla.org/r/72516/#review116892 ::: toolkit/components/extensions/ProxyScriptContext.jsm:161 (Diff revision 16) > let ret; > try { > // Bug 1337001 - provide path and query components to non-https URLs. > ret = this.runSafe(this.sandbox.FindProxyForURL, uri.prePath, uri.host, this.contextInfo); > } catch (error) { > - this.extension.emit("proxy-error", new Error(error.message)); > + this.extension.emit("proxy-error", error); Not sure why this isn't in the first patch, but same issues as elsewhere. ::: toolkit/components/extensions/ext-proxy.js:22 (Diff revision 16) > +var { > + SingletonEventManager, > +} = ExtensionUtils; > + > +// WeakMap[Extension -> ProxyScriptContext] > +let proxyScriptContextMap = new Map(); WeakMap, please. ::: toolkit/components/extensions/ext-proxy.js:52 (Diff revision 16) > + proxyScriptContextMap.set(extension, proxyScriptContext); > + } > + }, > + > + onProxyError: new SingletonEventManager(context, "proxy.onProxyError", fire => { > + let listener = (name, {message, stack, fileName, lineNumber}) => { Need to use `context.normalizeError` here, and pass the normalized error object. ::: toolkit/components/extensions/ext-proxy.js:53 (Diff revision 16) > + } > + }, > + > + onProxyError: new SingletonEventManager(context, "proxy.onProxyError", fire => { > + let listener = (name, {message, stack, fileName, lineNumber}) => { > + fire.sync({message, stack, fileName, lineNumber}); s/sync/withoutClone/ ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:20 (Diff revision 16) > + > +add_task(function* test_empty_proxyscript() { > + function backgroundScript() { > + browser.proxy.onProxyError.addListener(error => { > + browser.test.assertEq("The proxy script must define FindProxyForURL as a function", > + error.message, "Correct error message received"); Please either align after opening paren or move the first argument to the next line. Same elsewhere. ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:42 (Diff revision 16) > + yield extension.startup(); > + yield extension.awaitFinish("proxy-empty-script"); > + yield extension.unload(); > +}); > + > +add_task(function* test_invalid_type_for_FindProxyForURL() { Lots of repetition. Please add a helper that creates the extension, starts it, sends back the error, and shuts down. Probably checks that the error is what we expected, too. ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:59 (Diff revision 16) > + background: backgroundScript, > + manifest: { > + "permissions": ["proxy"], > + }, > + files: { > + "proxy_script.js": `let FindProxyForURL = 5`, Again, no `let`, please, and please avoid stringified scripts, as you did in the other tests. ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:72 (Diff revision 16) > + browser.test.assertTrue("stack" in error, "Error should include stack trace"); > + browser.test.assertTrue("fileName" in error, "Error should include file name"); > + browser.test.assertTrue("lineNumber" in error, "Error should include line number"); Need to check that these have the expected values.
Attachment #8782306 - Flags: review?(kmaglione+bmo)
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review116156 > Why this change? Good question.. I don't remember why I added it, and it doesn't seem necessary so I'll revert it. > Why did you remove this? I think most of the information is redundant. I was able to get all the information I need about this function by looking at the `@returns` jsdoc and then reading what the `strict` param does. I added any of the information that wasn't redundant to the description of `strict`. > Please do something like `this.FindProxyForURL = Cu.unwaiveXrays(this.sandbox.FindProxyForURL)` Done. > Not `new Error`. We don't want to expose privileged error objects to extension code. Same for the instances below. Done. > `runSafe` catches any errors the function throws. This will never happen. Oh interesting, I'll need to remove the use of runSafe then because we want to catch runtime errors and send the info to the extension. > We never want to expose arbitrary errors to extension code. They need to go through `context.normalizeError`. Same for other instances. Done. > This actually needs to be `!= null` so that it matches undefined. Done. One of my tests was broken and it turned out that this was the reason it was failing. > This also needs to be `!=` rather than `!==` Done. > We should throw if this exists and is not a boolean. Done. > You want `var FindProxyForURL` or `this.FindProxyForURL`. `let` places the variable in a block scope, which isn't accessible via the sandbox global. Interesting, thanks. > Is there a particular reason you don't just use the method syntax everywhere? I just didn't get around to simplifying this part. I'll switch them. > Just add something like this to the top of the script: > > no-unused-vars": ["error", {"args": "none", "varsIgnorePattern": "^(FindProxyForURL)$"}] Oh, neat, thanks.
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. https://reviewboard.mozilla.org/r/72516/#review116892 > Not sure why this isn't in the first patch, but same issues as elsewhere. Fixed. > WeakMap, please. Done. > Need to use `context.normalizeError` here, and pass the normalized error object. Thanks, done. > s/sync/withoutClone/ Done. > Please either align after opening paren or move the first argument to the next line. Same elsewhere. Done. > Lots of repetition. Please add a helper that creates the extension, starts it, sends back the error, and shuts down. Probably checks that the error is what we expected, too. Done. > Again, no `let`, please, and please avoid stringified scripts, as you did in the other tests. Done. > Need to check that these have the expected values. Done.
Comment on attachment 8782305 [details] Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. https://reviewboard.mozilla.org/r/72514/#review118018 ::: toolkit/components/extensions/ProxyScriptContext.jsm:7 (Diff revision 15) > +this.EXPORTED_SYMBOLS = ["ProxyScriptContext", "Error"]; > + > +/* exported ProxyScriptContext, Error */ Remove `Error`
Attachment #8782305 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8782306 [details] Part 2: Bug 1295807 - Implement the Proxy API. https://reviewboard.mozilla.org/r/72516/#review118022 ::: toolkit/components/extensions/ext-proxy.js:53 (Diff revision 17) > + } > + }, > + > + onProxyError: new SingletonEventManager(context, "proxy.onProxyError", fire => { > + let listener = (name, error) => { > + fire.asyncWithoutClone(error); `fire.async`, not `asyncWithoutClone`, please. ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:54 (Diff revision 17) > +add_task(function* test_invalid_FindProxyForURL_type() { > + yield testProxyScript(() => { }, { > + message: "The proxy script must define FindProxyForURL as a function", > + }); > + > + yield testProxyScript(() => { Please move start of arrow function to next line. Same for the other tests. ::: toolkit/components/extensions/test/mochitest/test_ext_proxy.html:58 (Diff revision 17) > + > + yield testProxyScript(() => { > + var FindProxyForURL = 5; > + }, { > + message: "The proxy script must define FindProxyForURL as a function", > + }); Needs another indentation level. Same for other instances.
Attachment #8782306 - Flags: review?(kmaglione+bmo) → review+
webextensions: --- → +
Keywords: checkin-needed
Thanks for the r+!
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 976a80260f72 -d af332c1ddc74: rebasing 378699:976a80260f72 "Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. r=kmag,robwu" merging toolkit/components/extensions/moz.build rebasing 378700:aad81be915cb "Part 2: Bug 1295807 - Implement the Proxy API. r=kmag" (tip) merging toolkit/components/extensions/extensions-toolkit.manifest merging toolkit/components/extensions/jar.mn merging toolkit/components/extensions/schemas/jar.mn warning: conflicts while merging toolkit/components/extensions/extensions-toolkit.manifest! (edit, then use 'hg resolve --mark') warning: conflicts while merging toolkit/components/extensions/jar.mn! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
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 b31b8d06422a -d b9e7efc94543: rebasing 378760:b31b8d06422a "Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. r=kmag,robwu" merging toolkit/components/extensions/moz.build rebasing 378761:0d5a99ab55a5 "Part 2: Bug 1295807 - Implement the Proxy API. r=kmag" (tip) merging toolkit/components/extensions/extensions-toolkit.manifest merging toolkit/components/extensions/jar.mn merging toolkit/components/extensions/schemas/jar.mn warning: conflicts while merging toolkit/components/extensions/extensions-toolkit.manifest! (edit, then use 'hg resolve --mark') warning: conflicts while merging toolkit/components/extensions/jar.mn! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1a9f4c3c782d Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. r=kmag,robwu https://hg.mozilla.org/integration/autoland/rev/1b162d4857b5 Part 2: Bug 1295807 - Implement the Proxy API. r=kmag
Keywords: checkin-needed
Flags: needinfo?(mwein)
Keywords: checkin-needed
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 1c17d8930826 -d 3436c6cf4123: rebasing 379408:1c17d8930826 "Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. r=kmag,robwu" merging toolkit/components/extensions/ExtensionCommon.jsm merging toolkit/components/extensions/MessageChannel.jsm merging toolkit/components/extensions/ext-c-runtime.js merging toolkit/components/extensions/moz.build merging toolkit/components/extensions/schemas/runtime.json merging toolkit/components/extensions/test/xpcshell/xpcshell.ini warning: conflicts while merging toolkit/components/extensions/ext-c-runtime.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Blocks: webext-port-hola
I think this should land nicely now :)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/488acde695af Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. r=kmag,robwu https://hg.mozilla.org/integration/autoland/rev/3ae228897932 Part 2: Bug 1295807 - Implement the Proxy API. r=kmag
Keywords: checkin-needed
Backed out for formatting failure in extensions-toolkit.manifest and eslint failure: https://hg.mozilla.org/integration/autoland/rev/4276bb86fa19278d8ab760ad4733ed507cbcb074 https://hg.mozilla.org/integration/autoland/rev/792b992e6ecd5866fcb4c7690be7edf0357e9e53 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3ae2288979324be626bc23971d03927ac495e379&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=81746438&repo=autoland [task 2017-03-05T19:12:20.375759Z] 19:12:20 INFO - Error: /home/worker/workspace/build/src/obj-firefox/dist/bin/components/extensions-toolkit.manifest:61: Unknown flag: scripts [task 2017-03-05T19:12:20.375888Z] 19:12:20 INFO - Traceback (most recent call last): [task 2017-03-05T19:12:20.376074Z] 19:12:20 INFO - File "/home/worker/workspace/build/src/toolkit/mozapps/installer/packager.py", line 397, in <module> [task 2017-03-05T19:12:20.376192Z] 19:12:20 INFO - main() [task 2017-03-05T19:12:20.376383Z] 19:12:20 INFO - File "/home/worker/workspace/build/src/toolkit/mozapps/installer/packager.py", line 349, in main [task 2017-03-05T19:12:20.376502Z] 19:12:20 INFO - copier.add(mozpath.join(respath, 'removed-files'), removals) [task 2017-03-05T19:12:20.376634Z] 19:12:20 INFO - File "/usr/lib/python2.7/contextlib.py", line 24, in __exit__ [task 2017-03-05T19:12:20.376813Z] 19:12:20 INFO - self.gen.next() [task 2017-03-05T19:12:20.376951Z] 19:12:20 INFO - File "/home/worker/workspace/build/src/python/mozbuild/mozpack/errors.py", line 131, in accumulate [task 2017-03-05T19:12:20.377076Z] 19:12:20 INFO - raise AccumulatedErrors() [task 2017-03-05T19:12:20.377252Z] 19:12:20 INFO - mozpack.errors.AccumulatedErrors [task 2017-03-05T19:12:20.377395Z] 19:12:20 INFO - /home/worker/workspace/build/src/toolkit/mozapps/installer/packager.mk:41: recipe for target 'stage-package' failed Eslint log: https://treeherder.mozilla.org/logviewer.html#?job_id=81746454&repo=autoland > TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/ext-c-runtime.js:66:9 | Unexpected console statement. (no-console)
Flags: needinfo?(mwein)
Please link to a green Try push before setting checkin-needed again on this.
All of the Tier 1 tests are passing now that I've added in the missing proxy.json file and removed the unnecessary log statements. The tests that have failed are unrelated to this change.
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7a576717126b Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. r=kmag,robwu https://hg.mozilla.org/integration/autoland/rev/a9beac770cec Part 2: Bug 1295807 - Implement the Proxy API. r=kmag
Keywords: checkin-needed
Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0a4cfaacb4dd Backed out changeset a9beac770cec https://hg.mozilla.org/integration/autoland/rev/37ccffb8930a Backed out changeset 7a576717126b for unresolved review issues. DONTBUILD
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4322967ebd09 Part 1: Bug 1295807 - Add a Javascript Module for registering proxy scripts. r=kmag,robwu https://hg.mozilla.org/integration/autoland/rev/0700ffb4a067 Part 2: Bug 1295807 - Implement the Proxy API. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4322967ebd09 https://hg.mozilla.org/mozilla-central/rev/0700ffb4a067
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1345491
Keywords: dev-doc-needed
No longer blocks: webext-port-hola
Matt, I've had a go at some docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy and an example: https://github.com/mdn/webextensions-examples/tree/master/proxy-blocker The compat tables are waiting review, but you don't have to review them. In particular, I'm quite unclear about how we handle multiple PAC-registering add-ons. I've tried to explain what I seem to observe: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy/registerProxyScript but I'd be very grateful if you could pay particular attention to that bit :). Thanks!
Flags: needinfo?(mwein)
Will, I reviewed the docs and the sample. I will make some changes tomorrow since Matt is on vacation.
Hi Mattew, With this implementation, the pac script is accessed before the webRequest.onBeforeRequest hook. Is it a bug or intended? Is there a way for me to notify pac script on new proxy settings before the request is fired, on every request?
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: