Closed Bug 1566884 Opened 5 months ago Closed 5 months ago

When proxy.onRequest returns [{type: "some"}, {type: "direct"}] the "direct" item is swallowed

Categories

(WebExtensions :: Request Handling, defect, P1)

defect

Tracking

(firefox69 fixed, firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The problem is here:
https://searchfox.org/mozilla-central/rev/22b330ecb3edba1536a54887060cbdd09db21c59/netwerk/base/nsProtocolProxyService.cpp#2342

Removing the block helps. When the callback returns only [{type: "direct"}], channels get proxyinfo == null, which is expected and desired.

Only major difference here is that when defaultProxyInfo is something else than 'direct' we throw it away with this change; I'm not sure what to do exactly here to not break anything.

Attachment #9078761 - Attachment description: Bug 1566884 - Do not replace "direct" proxy info result from proxy.onRequest with the default proxy info provided by the platform, r=mixedpuppy → Bug 1566884 - Do not replace "direct" proxy info result from proxy.onRequest with a null default proxy info, r=mixedpuppy
Assignee: nobody → honzab.moz
Blocks: secure-proxy
Status: NEW → ASSIGNED
Priority: -- → P1
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/face9338dcbd
Do not replace "direct" proxy info result from proxy.onRequest with a null default proxy info, r=mixedpuppy

Comment on attachment 9078761 [details]
Bug 1566884 - Do not replace "direct" proxy info result from proxy.onRequest with a null default proxy info, r=mixedpuppy

Beta/Release Uplift Approval Request

This change allows the most proper solution for proxying system requests (like various critical updates) and gracefully, safely and automatically fallback to a direct connection when the proxy goes down, selectively only for whitelisted requests.

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Needs manual testing = YES, if the automated tests coverage might be found inefficient. I think testing with some proxy-providing addons may potentially be useful.
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): In the form the patch is we won't bypass any default proxy settings when extensions return "direct" as an option, so the behavior is going to be identical.

Except that a direct connection can now be used as a fallback (returned item #2) when the proxy (returned item #1) is down. Hidden sensitive bugs in addons could be uncovered with this because now requests could go silently unproxied.

  • String changes made/needed:
Attachment #9078761 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
QA Whiteboard: [qa-triaged]

Comment on attachment 9078761 [details]
Bug 1566884 - Do not replace "direct" proxy info result from proxy.onRequest with a null default proxy info, r=mixedpuppy

Ensures that we fallback to a direct connection attempt if the proxied connection doesn't work. Approved for 69.0b7.

Attachment #9078761 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hello,
Using the suggestion provided in the "steps to reproduce" comment, I've tried in reproducing this in previous versions before the Uplift (68.0.2 Release and 69.0b6 Beta on Windows 64-bit) by installing extension proxies and performing various actions but was not successful.
If manual testing is still required, please provide more detailed Steps to reproduce.
Thank you

Flags: needinfo?(honzab.moz)

I think automated testing (existing or updated) should be preferred. I was overly cautious here because we could uncover bugs in existing addons. But that is probably an overkill for QA to do this. Thanks. qe-

Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.