1233360 - proxy bypass ignored on Mac when using system proxy settings

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[bonjour]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/601.3.9 (KHTML, like Gecko) Version/9.0.2 Safari/601.3.9

Steps to reproduce:

Firefox 43 on Mac OS X El-Capitan ignores system proxy bypass settings when it is using system proxy settings. I have set http and https proxies in Mac network preferences and the default proxy bypass string is *.local, 169.254/16, 192.168/16

This works flawlessly in Safari but Firefox fails to connect to local sites in 192.168.* network. On the other hand, Firefox fully honours proxy bypass patterns described in Automatic Network Configuration scripts.

This issue seems to be Mac specific and it is not reproducible on Windows. On an interesting note, I also found a similar issue 470207 which was resolved back in 2009.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:43.0) Gecko/20100101 Firefox/43.0

Comment 1

[Gary Chen [:xeonchen]]

Attachment: MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder

Review commit: https://reviewboard.mozilla.org/r/50697/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50697/

Comment 2

[Gary Chen [:xeonchen]]

Attachment: MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder

Review commit: https://reviewboard.mozilla.org/r/50699/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50699/

Comment 3

[Gary Chen [:xeonchen]]

Attachment: MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder

Review commit: https://reviewboard.mozilla.org/r/50701/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50701/

Comment 4

[Gary Chen [:xeonchen]]

We didn't handle the form '<IP>/<prefix>', add implementation and tests here.

Comment 5

[Gary Chen [:xeonchen]]

Comment on attachment 8749023 [details]
MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50697/diff/1-2/

Comment 6

[Gary Chen [:xeonchen]]

Comment on attachment 8749024 [details]
MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/1-2/

Comment 7

[Gary Chen [:xeonchen]]

Comment on attachment 8749025 [details]
MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/1-2/

Comment 8

[Daniel Stenberg [:bagder]]

Comment on attachment 8749023 [details]
MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder

https://reviewboard.mozilla.org/r/50697/#review48017

::: toolkit/system/osxproxy/moz.build:12
(Diff revision 2)
>  SOURCES += [
>      'nsOSXSystemProxySettings.mm',
>  ]
>  
> +UNIFIED_SOURCES += [
> +    'ProxyUtils.cpp',

This makes it a lovely mix of .cpp and .mm files here. Did you make a decision here or how come the code extracted from a .mm got to be a .cpp?

I'm not aware of how we've decided what to use where, I'm just noticing the inconsistency here.

Comment 9

[Daniel Stenberg [:bagder]]

Comment on attachment 8749024 [details]
MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder

https://reviewboard.mozilla.org/r/50699/#review48019

::: toolkit/system/osxproxy/ProxyUtils.cpp:16
(Diff revision 2)
>  namespace mozilla {
>  namespace toolkit {
>  namespace system {
>  
> -bool IsHostProxyEntry(const nsACString& aHost, const nsACString& aOverride)
> +static bool
> +IPv4Parser(const nsACString& aAddr, uint32_t& ipv4)

Why have your own implemenation that looks pretty much like a subset of inet_pton() ?

Comment 10

[Gary Chen [:xeonchen]]

https://reviewboard.mozilla.org/r/50699/#review48019

> Why have your own implemenation that looks pretty much like a subset of inet_pton() ?

Yes, but we'll have to handle the form like '10/8' instead of '10.0.0.0/8'.
Also the result of inet_pton can't be masked directly, or is there a better solution?

Comment 11

[Gary Chen [:xeonchen]]

https://reviewboard.mozilla.org/r/50697/#review48017

> This makes it a lovely mix of .cpp and .mm files here. Did you make a decision here or how come the code extracted from a .mm got to be a .cpp?
> 
> I'm not aware of how we've decided what to use where, I'm just noticing the inconsistency here.

I expect this function would be reused by other platforms.
But you're right, this should be done when it have to be.

Comment 12

[Gary Chen [:xeonchen]]

Comment on attachment 8749023 [details]
MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50697/diff/2-3/

Comment 13

[Gary Chen [:xeonchen]]

Comment on attachment 8749024 [details]
MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/2-3/

Comment 14

[Gary Chen [:xeonchen]]

Comment on attachment 8749025 [details]
MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/2-3/

Comment 15

[Gary Chen [:xeonchen]]

https://reviewboard.mozilla.org/r/50699/#review48019

> Yes, but we'll have to handle the form like '10/8' instead of '10.0.0.0/8'.
> Also the result of inet_pton can't be masked directly, or is there a better solution?

Hi bagder, I've just made some modification, would you please see if it looks good to you?

Comment 16

[Gary Chen [:xeonchen]]

Comment on attachment 8749024 [details]
MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/3-4/

Comment 17

[Gary Chen [:xeonchen]]

Comment on attachment 8749025 [details]
MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/3-4/

Comment 18

[Daniel Stenberg [:bagder]]

Comment on attachment 8749023 [details]
MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder

https://reviewboard.mozilla.org/r/50697/#review49041

Comment 19

[Daniel Stenberg [:bagder]]

Comment on attachment 8749024 [details]
MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder

https://reviewboard.mozilla.org/r/50699/#review49043

Apart the little format nit, it looks fine!

::: toolkit/system/osxproxy/ProxyUtils.mm:58
(Diff revision 4)
> +
> +  if (aMaskLen > 96) {
> +    aAddr.pr_s6_addr32[3] = PR_htonl(
> +        PR_ntohl(aAddr.pr_s6_addr32[3]) & (~0L << (128 - aMaskLen)));
> +  }
> +  else if (aMaskLen > 64) {

Nit: our style guide says we write them '} else if' on the same line. You have this on several places.

Comment 20

[Daniel Stenberg [:bagder]]

Comment on attachment 8749025 [details]
MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder

https://reviewboard.mozilla.org/r/50701/#review49045

Comment 21

[Gary Chen [:xeonchen]]

Comment on attachment 8749024 [details]
MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/4-5/

Comment 22

[Gary Chen [:xeonchen]]

Comment on attachment 8749025 [details]
MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/4-5/

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccbedc9308f
https://hg.mozilla.org/integration/mozilla-inbound/rev/81add9b7d007
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b83aa3ab364

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ccbedc9308f
https://hg.mozilla.org/mozilla-central/rev/81add9b7d007
https://hg.mozilla.org/mozilla-central/rev/1b83aa3ab364