1507110 - Always no-proxy localhost/127.0.0.1 except if user very explicitly opts out

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[:Gijs (he/him)]

Per discussion in bug 1503393, we want to make the default no proxy pref list an empty string, and always connect directly to localhost, to make it harder for people to shoot themselves in the foot by removing localhost from the list. There can be an appropriately-scarily-named hidden (ie about:config-only) pref to disable this, e.g. network.proxy.allow_hijacking_localhost or something similar.

Comment 1

[Honza Bambas (:mayhemer)]

This should likely not be forgotten.

Comment 2

[:Gijs (he/him)]

Should we find an assignee for this?

Comment 3

[Dragana Damjanovic [:dragana]]

Junior, can you take this?

Comment 4

[Junior [inactive]]

Yes.

Comment 5

[Junior [inactive]]

work in progress, plan to have a patch next week

Comment 6

[Junior [inactive]]

Attachment: Bug 1507110 - Allow hijacking localhost only if network.proxy.allow_hijacking_localhost is set

Comment 7

[Justin Dolske [:Dolske]]

The original bug notes enterprise stuff, so we'll likely want support for an enterprise policy for this new pref too. See bug 1523810 or :mkaply for a recent example.

Comment 8

[Junior [inactive]]

(In reply to Justin Dolske [:Dolske] from comment #7)

The original bug notes enterprise stuff, so we'll likely want support for an enterprise policy for this new pref too. See bug 1523810 or :mkaply for a recent example.

Got it. Will update the enterprise policy part in the next update.
Thanks!

Comment 9

[Junior [inactive]]

Hello :mkaply,
cc you for https://phabricator.services.mozilla.com/D19325#504725
Personally I believe we need the flexibility to let enterprise decide proxy-ing localhost or not. See bug 1503393 Comment 22-24 for a discussion before. What do you think?

Comment 10

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=703a93a1643667574bd27e6d7e2aa06cbebc58db

Comment 11

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d5db9c79493949250865cb26d1c843ea5de5e72

Comment 12

[Mike Kaply [:mkaply]]

We can easily create a policy for this. How different is how we are fixing this versus how Chrome is fixing it?

Comment 13

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #12)

We can easily create a policy for this. How different is how we are fixing this versus how Chrome is fixing it?

https://chromium.googlesource.com/chromium/src/+/da790f920bbc169a6805a4fb83b4c2ab09532d91 looks like the Chromium commit.

From the commit message:

The compatibility risk of this change should be low as proxying through localhost was not universally supported. It is however an idiom used in testing (a number of our own tests had such a dependency). Impacted users can use the "<-loopback>" bypass rule as a workaround.

So it looks like they added special syntax to the list of proxy bypass addresses to say "and by the way, remove local addresses from this list". I don't know how their policy stuff works and if that means we have to make our policy stuff recognize the same syntax to trip the same policy or what. Pinging Mike again to feed back on this.

FWIW, it also seems like they made a similar default-on exception for link-local IP addresses, not just for 127.0.0.1/localhost/::1. I don't know if we need a similar fix. Junior, thoughts?

Comment 14

[Junior [inactive]]

as Honza said, we might treat other loopback host as follow up. fwiw another open bug for <loopback> syntax

Comment 15

[Mike Kaply [:mkaply]]

I don't see anything in their policy code that covers this:

https://www.chromium.org/administrators/policy-list-3

And the original asks (and issues) don't seem enterprise related.

I'd be OK with not doing a policy for now. If folks really need this, they can set the pref. I doubt it needs to go out company wide.

Comment 16

[Junior [inactive]]

Remove checkin-needed since we’d like to remove enterprise support

Comment 17

[Junior [inactive]]

Enterprise policy is removed. Let's see if treeherder is happy
https://treeherder.mozilla.org/#/jobs?repo=try&revision=446fe294f5fa809ca237f18065f5fe9dd852559d

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/78d3e59ea9fa

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/78d3e59ea9fa

Comment 20

[:Gijs (he/him)]

Junior, did you want to uplift this to 66 or mark wontfix for 66?

Comment 21

[Junior [inactive]]

(In reply to :Gijs (he/him) from comment #20)

Junior, did you want to uplift this to 66 or mark wontfix for 66?

To me, it's a wonfix since no big behaviour difference and same esr.
However, I'd like to ask for information from the point of security.

What do you think, :dveditz?

Comment 22

[Daniel Veditz [:dveditz]]

We don't need to uplift sec-want bugs.

Comment 24

[:Gijs (he/him)]

There were at least 7 bugs filed about this behaviour (see bug 1535581 and dupes) since it hit release. That's quite a lot.

Junior, should we consider adding some text underneath the box that says something to the effect of "localhost never uses the proxy"?

Comment 25

[Junior [inactive]]

Yes, it's in my mind. Thanks for raising this. I'll file another bug and take it.