Closed
Bug 1457213
Opened 6 years ago
Closed 6 years ago
proxy extensions should start before requests bypass proxies
Categories
(WebExtensions :: Request Handling, enhancement, P1)
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 | ||
Updated•6 years ago
|
Assignee: nobody → mixedpuppy
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 5•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
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?
Comment hidden (mozreview-request) |
Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ec8911b8f9c7 support early startup for proxy api, r=aswan
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec8911b8f9c7
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
extension for testing either onRequest listener or pref settings.
Comment 14•6 years ago
|
||
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)
Assignee | ||
Comment 15•6 years ago
|
||
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)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 16•6 years ago
|
||
(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)
Assignee | ||
Comment 17•6 years ago
|
||
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.
Description
•