995343 - Don't pass through HTTP traffic to the external network

Status: RESOLVED INCOMPLETE

(Testing :: General, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We have a proxy setup for Gecko when running at least mochitest, and possibly other test suites.  We ought to be able to use that to catch attempts to access the external network rather than relying on access controls beyond the test harness.

Comment 1

[(no longer active)]

We could do this much more easily in nsHostResolver::ResolveHost FWIW.

Comment 2

[(no longer active)]

(And I think much more effectively too!)

Comment 3

[Ed Morley [:emorley]]

Thank you for filing this :-)

Comment 4

[Gregory Szorc [:gps]]

Why is this filed under httpd.js? Wouldn't this be a Necko-level policy?

FTR, I would love a feature that made network requests fail fast if they attempted to access anything off the local host. Just this week I was bit by tests failing because they were pulling an external resource without my knowledge (when running on my laptop).

Comment 5

[(no longer active)]

I discussed my solution with Nathan and Kyle on IRC for a bit and didn't seem to be able to convince them, so I filed bug 995417 for my suggestion.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: don't permit DIRECT connections with our test networking proxies

This patch at least prohibits external XHRs in light local testing.  The http
server returns status 400 (Bad Request) for such URLs.

The proxy script continues to permit DIRECT connections for invalid URLs.

This change will redirect things like:

  http://example.com:8123

to:

  http://example.com:80

Not entirely sure how desirable that is.  I think that's easily fixed, though.
This is just proof-of-concept.

It's also worth noting that since this is in mozprofile, it's easy to make it
work for reftests.  xpcshell is still an open question.

Comment 7

[Gregory Szorc [:gps]]

Just a heads up - I wouldn't be surprised if this change broke some xpcshell tests in services/. We have a number of tests that try to exercise various network scenarios and history has taught me that those tests can be quite fragile to network behavior changes.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: don't permit DIRECT connections with our test networking proxies

This patch is somewhat more complicated and more robust.  Unfortunately, the
try results for it are not nearly as good on Patrick's try run in bug 995417:

this patch: https://tbpl.mozilla.org/?tree=Try&rev=9a656db7e715
Patrick's local-address-only patch: https://tbpl.mozilla.org/?tree=Try&rev=acd81b7ac542

I don't understand the perma-crash for
test_browserElement_inproc_ExposableURI.html in M2.  The other M2 failure is
because we connect to a non-existant domain and instead of getting a
non-existant domain error, we get redirected to the proxy server and attempt to
fetch a URL that doesn't exist.  Not a problem per se, just not what the test
expected.

The Moth failures are because those tests start up an HTTP server and connect
to localhost:$PORT.  The proxy auto-config bits don't understand that localhost
is an OK name to connect to, and fail the connection.

I haven't tried understanding in detail why the crashes Patrick's patch causes
are not also caught by these proxy changes.  It's possible I have bugs in the
proxy script.  It's also possible that the non-local accesses done are not
really a problem (just getting index.html or similar) and redirecting them to
our local server works just fine.  Not sure about this.

I didn't do this for crashtests/reftests because I wanted to get this working
for mochitests first.  Flipping things on for crashtests/reftests is an extra
line of code.  But I don't see a good way to turn this on for xpcshell, as
xpcshell doesn't use mozprofile.  Hooking into the netwerk bits and flipping an
environment variable is much easier for xpcshell.

Comment 9

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> The other M2 failure is
> because we connect to a non-existant domain and instead of getting a
> non-existant domain error, we get redirected to the proxy server and attempt
> to
> fetch a URL that doesn't exist.  Not a problem per se, just not what the test
> expected.

That's actually a deal breaker, isn't it?  We have some tests that exercise the network error page, and it seems like with your patch they will load a regular HTTP response instead, right?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> (In reply to Nathan Froyd (:froydnj) from comment #8)
> > The other M2 failure is
> > because we connect to a non-existant domain and instead of getting a
> > non-existant domain error, we get redirected to the proxy server and attempt
> > to
> > fetch a URL that doesn't exist.  Not a problem per se, just not what the test
> > expected.
> 
> That's actually a deal breaker, isn't it?  We have some tests that exercise
> the network error page, and it seems like with your patch they will load a
> regular HTTP response instead, right?

I think so, yes.  I was trying to figure out a way to say I now support your solution that would minimize your gloating. ;)

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #9)
> > (In reply to Nathan Froyd (:froydnj) from comment #8)
> > > The other M2 failure is
> > > because we connect to a non-existant domain and instead of getting a
> > > non-existant domain error, we get redirected to the proxy server and attempt
> > > to
> > > fetch a URL that doesn't exist.  Not a problem per se, just not what the test
> > > expected.
> > 
> > That's actually a deal breaker, isn't it?  We have some tests that exercise
> > the network error page, and it seems like with your patch they will load a
> > regular HTTP response instead, right?
> 
> I think so, yes.  I was trying to figure out a way to say I now support your
> solution that would minimize your gloating. ;)

Haha!  Fair enough.  :-)

Comment 12

[(not currently active) Ted Mielczarek]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> That's actually a deal breaker, isn't it?  We have some tests that exercise
> the network error page, and it seems like with your patch they will load a
> regular HTTP response instead, right?

We could presumably list them in server-locations.txt with some special flag and pass them through DIRECT.

Also re: above, Reftest doesn't currently use this proxy PAC, it simply starts its own httpd.js server in-process and loads URLs that point at it. Presumably we could fix that if we wanted.

I wouldn't expect this to change xpcshell testing at all, since xpcshell tests don't even get a profile by default. (And if they do get one they control the contents.)

Comment 13

[Nathan Froyd [:froydnj]]

We are doing this the bug 996504 way.  Ehsan wins!

Comment 14

[Ed Morley [:emorley]]

(s/bug 996504/bug 995417/ for anyone following along)