Closed Bug 1457213 Opened 2 years ago Closed 2 years ago

proxy extensions should start before requests bypass proxies

Categories

(WebExtensions :: Request Handling, enhancement, P1)

58 Branch
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

Probably pretty similar to the webrequest changes in bug 1447551.
Assignee: nobody → mixedpuppy
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Priority: -- → P1
Comment on attachment 8971408 [details]
Bug 1457213 support early startup for proxy api,

https://reviewboard.mozilla.org/r/240164/#review246744

Looks good, I think the test could be more thorough.  E.g., it looks like it tests that the background page starting and event dispatch happen as intended, but it doesn't actually check that the event result is correctly handled?  (i.e., that the request is properly proxied).  Also making a request that doesn't match the filter would give us some coverage to ensure that the filter is properly saved and restored.

::: toolkit/components/extensions/parent/ext-proxy.js:128
(Diff revision 3)
> +    // Leaving as non-persistent.  By itself it's not useful since proxy-error
> +    // is emitted from the proxy filter.
>      let onError = new EventManager({

Any reason not to just inline this below?
(that doesn't need to happen in this patch, it just jumped out at my while reading the patch)
Attachment #8971408 - Flags: review?(aswan) → review+
(In reply to Andrew Swan [:aswan] from comment #4)
> Comment on attachment 8971408 [details]

> ::: toolkit/components/extensions/parent/ext-proxy.js:128
> (Diff revision 3)
> > +    // Leaving as non-persistent.  By itself it's not useful since proxy-error
> > +    // is emitted from the proxy filter.
> >      let onError = new EventManager({
> 
> Any reason not to just inline this below?
> (that doesn't need to happen in this patch, it just jumped out at my while
> reading the patch)

I need to followup on that and deprecate the onProxyError handler.  Then it could be inlined.
Comment on attachment 8971408 [details]
Bug 1457213 support early startup for proxy api,

https://reviewboard.mozilla.org/r/240164/#review246844

Looks good, one comment below.  The whole thing might be easier to follow if you register a handler on the proxied server as well then you can just count both proxied and non-proxied requests separately rather than inferring that a proxied request worked from the fact that it finished but didn't hit the non-proxied server.

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_startup.js:79
(Diff revisions 3 - 4)
> +  let request = ExtensionTestUtils.fetch("http://test1.example.com/data/file_sample.html");
> +  await Promise.all([sawRequest, request]);
> +  equal(0, nonProxiedRequests, "non proxied request ok");
> +  ok(true, "initial proxy test passed");
> +  await ExtensionTestUtils.fetch("http://example.com/?a=0");
> +  equal(1, nonProxiedRequests, "non proxied request ok");

This should be proxied (not "non proxied") right?
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec8911b8f9c7
support early startup for proxy api, r=aswan
https://hg.mozilla.org/mozilla-central/rev/ec8911b8f9c7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(mixedpuppy)
Manual testing may be a touch complex.  You'd have to configure an extension to use a proxy, then use a network monitor to be sure no requests from firefox bypass the configured proxy.  I'll attach a simple extension you could use for the firefox part of the testing.  It is similar to the extension in bug 1455755 with the ability to use the onRequest listener, see the STR there for some documentation.  Be sure to use the dynamic checkbox in the browser action to use the listener.

Once you set a proxy, you can restart firefox.  The console should show that all requests are going through the proxy listener.  

Having said all that, there are automated tests on this, so the important part is validating with an external network monitor.
Flags: needinfo?(mixedpuppy)
Attached file proxyTesting.xpi (obsolete) —
extension for testing either onRequest listener or pref settings.
I tried Wireshark to intercept the requests made by the firefox process but was unable to make a filter for it.
I've also found this articles https://bugzilla.mozilla.org/show_bug.cgi?id=1457213 that mentions that filtering by PID/process is not possible.
Please let me know if there is any other way or any other network monitoring program that I can use to validate this issue.
Testing is proving to be a bit more complex than I anticipated as well.
Flags: needinfo?(mixedpuppy)
There are a number of simple command line tools to do this.  Here's one I tried:


brew install tcpflow
tcpflow -p -c -i en0 port 80 | grep -oE '(GET|POST|HEAD) .* HTTP/1.[01]|Host: .*'


You should see a couple requests when firefox starts.  Set a "dynamic" proxy via the extension here, and restart.  You should see no requests on startup.
Flags: needinfo?(mixedpuppy)
Product: Toolkit → WebExtensions
(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> There are a number of simple command line tools to do this.  Here's one I
> tried:
> 
> 
> brew install tcpflow
> tcpflow -p -c -i en0 port 80 | grep -oE '(GET|POST|HEAD) .*
> HTTP/1.[01]|Host: .*'
> 
> 
> You should see a couple requests when firefox starts.  Set a "dynamic" proxy
> via the extension here, and restart.  You should see no requests on startup.

I tried to set up a "dynamic" proxy via the attached extension, but this is not working(I am not able to open any addresses) and I suspect that the extension is somehow broken because if I set same proxies directly in browser or if I don't check the dynamic checkbox in the extension, everything works as expected.

Shane, please let me know if you can check if the extension is not broken around that "dynamic" checkbox.
Flags: needinfo?(mixedpuppy)
Attached file proxyTesting.xpi
This version is a little cleaned up to make it obvious if a proxy is configured, and whether it is dynamic.  If the fields in the panel are empty, no proxy is configured.
Attachment #8974773 - Attachment is obsolete: true
Flags: needinfo?(mixedpuppy)
You need to log in before you can comment on or make changes to this bug.