Nathan Froyd [:froydnj] Reporter
	Description • 10 years ago

From bug 995417 comment 12, with the patches from that bug applied (I will refresh part 2 in a moment so that xpcshell tests actually get the appropriate environment variable set):

I am seeing test_speculative_connect.js xpcshell failures locally:

 7:48.00 TEST-PASS | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | [test_hostnames_resolving_to_addresses : 204] 1 == 1
 7:48.00 BAD CONNECT: connecting to 10.0.0.1 0
 7:48.00 TEST-PASS | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | [test_hostnames_resolving_to_addresses : 207] "object" != undefined
 7:48.00 TEST-PASS | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | [test_hostnames_resolving_to_addresses : 212] "object" != undefined
 7:48.00 <<<<<<<
PROCESS-CRASH | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | application crashed [Unknown top frame]

Happens on try, too:

https://tbpl.mozilla.org/php/getParsedLog.php?id=39672912&tree=Try

Should we be checking for IsIPAddrLocal in addition to IsLoopBackAddress:

http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/DNS.cpp#181

?

And from bug 995417 comment 13:

(In reply to Nathan Froyd (:froydnj) from comment #12)
> I am seeing test_speculative_connect.js xpcshell failures locally:

I think that test does a bunch of off host things well beyond the rfc 1918 stuff (that we should keep around). can you file a separate bug about it, assign to :sworkman and cc me?

	Nathan Froyd [:froydnj] Reporter
	Comment 1 • 10 years ago

Patrick, do you have an ETA on when this work will be scheduled?

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 2 • 10 years ago

Sorry for the delay here.

If I understand this correctly, I think adding a check for !IsIPAddrLocal to your branch in nsSocketTransport::InitiateSocket would be good. This should still crash on external connection attempts, but should be ok for connection attempts in the local network.

Question - is this code to be permanent in nsSocketTransport::InitiateSocket? Or is it just means to catch external connections now so that external access can be restricted/disabled at the network level?

Like Pat, I'd like to keep the external connections defined here: http://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_speculative_connect.js#46, e.g. google.com.

From what I've read of the related bugs, there was to be a restricted whitelist of external network accesses. Can the hosts used in this test be part of that? Or can you tell us which these hosts are so we can rewrite the test?

And speaking of whitelists, if you are keeping that code in nsSocketTransport::InitiateSocket, it will need to check the whitelist itself, no?

	Nathan Froyd [:froydnj] Reporter
	Comment 3 • 10 years ago

(In reply to Steve Workman [:sworkman] from comment #2)
> If I understand this correctly, I think adding a check for !IsIPAddrLocal to
> your branch in nsSocketTransport::InitiateSocket would be good. This should
> still crash on external connection attempts, but should be ok for connection
> attempts in the local network.

OK, great.

> Question - is this code to be permanent in
> nsSocketTransport::InitiateSocket? Or is it just means to catch external
> connections now so that external access can be restricted/disabled at the
> network level?

Yes, the intention is that the code in InitiateSocket would be permanent, guarded by an environment variable that our testsuites set.

> Like Pat, I'd like to keep the external connections defined here:
> http://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/
> test_speculative_connect.js#46, e.g. google.com.

OK.  Can you elaborate on the rationale for doing this?  We've spent some time removing external connections to *.google.com, *.mozilla.*, etc. elsewhere and I'd like to understand why this test should be considered different.

> From what I've read of the related bugs, there was to be a restricted
> whitelist of external network accesses. Can the hosts used in this test be
> part of that? Or can you tell us which these hosts are so we can rewrite the
> test?

Where did you see this?  I don't recall seeing a discussion of such a whitelist.  We have a proxy redirection scheme for a whitelist-like selection of origins for mochitests, but those connections are still localhost-only.

	Patrick McManus [:mcmanus]
	Comment 4 • 10 years ago

I think its ok to allow the 1918 addresses to the extent that this test is specifically testing 1918 semantcs (ther are some associated with speculative connections), but not the general internet routable ones..

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 5 • 10 years ago

(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Steve Workman [:sworkman] from comment #2)
> > Like Pat, I'd like to keep the external connections ...
> 
> OK.  Can you elaborate on the rationale for doing this? ...

So, if Pat's ok with not testing the external speculative connections (comment 4), that's fine by me. I'll work up a patch and upload it shortly. It will still need !IsIPAddrLocal() to be added to the condition, but that should stop the failures.

> > From what I've read of the related bugs, there was to be a restricted
> > whitelist of external network accesses...
> 
> Where did you see this?  I don't recall seeing a discussion of such a
> whitelist.  We have a proxy redirection scheme for a whitelist-like
> selection of origins for mochitests, but those connections are still
> localhost-only.

Ah, I may have misunderstood that. No worries.

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 6 • 10 years ago

Nathan, here's a patch to try out to see if it solves the problem. It removes all remote/external tests from test_speculative_connect.js.

Runs fine locally, but I don't have your patches applied here.

	Nathan Froyd [:froydnj] Reporter
	Comment 7 • 10 years ago

Comment on attachment 8435244 [details] [diff] [review]
v1.0 Remove external addresses from test_speculative_connect.js

Review of attachment 8435244 [details] [diff] [review]:
-----------------------------------------------------------------

This works for me if I add an IsIPAddrLocal check in InitiateSocket.  Thanks for the quick turnaround!

::: netwerk/protocol/websocket/WebSocketChannelChild.h
@@ +23,5 @@
>   public:
>    WebSocketChannelChild(bool aSecure);
>    ~WebSocketChannelChild();
>  
> +  NS_DECL_THREADSAFE_ISUPPORTS

Presumably this is part of a different patch?

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 8 • 10 years ago

Thanks for verifying it works Nathan! Apologies about the other patch bits - thought I had switched queues cleanly, sorry.

	Patrick McManus [:mcmanus]
	Comment 9 • 10 years ago

Comment on attachment 8435380 [details] [diff] [review]
v1.1 Remove external addresses from test_speculative_connect.js

Review of attachment 8435380 [details] [diff] [review]:
-----------------------------------------------------------------

this doesn't leave any positive test cases :)

I think we need to add a pref that controls whether or not we filter speculative connections to local(). (right now iirc that's true all the time right?). then we can write one test
that does sc to localhost and test it. sound good?

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 10 • 10 years ago

(In reply to Patrick McManus [:mcmanus] from comment #9)
> Comment on attachment 8435380 [details] [diff] [review]
> v1.1 Remove external addresses from test_speculative_connect.js
> 
> Review of attachment 8435380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this doesn't leave any positive test cases :)
> 
> I think we need to add a pref that controls whether or not we filter
> speculative connections to local(). (right now iirc that's true all the time
> right?). then we can write one test
> that does sc to localhost and test it. sound good?

I left the function test_speculative_connect which is a positive test case for localhost and should do what you suggest, no?

	Patrick McManus [:mcmanus]
	Comment 11 • 10 years ago

Comment on attachment 8435380 [details] [diff] [review]
v1.1 Remove external addresses from test_speculative_connect.js

Review of attachment 8435380 [details] [diff] [review]:
-----------------------------------------------------------------

steve - sorry for a moment I thought that localhost was filtered out as part of the 1918 stuff.. but assuming its not (which I now think you're right) we're good. sorry about that.

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 12 • 10 years ago

No worries :) Thanks for r+!

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 13 • 10 years ago

Should have done this earlier. try run: https://tbpl.mozilla.org/?tree=Try&rev=08a36a46cfc2

	Steve Workman [:sworkman] (INACTIVE) Assignee
	Comment 14 • 10 years ago

On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd94a4d8be6f

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/cd94a4d8be6f

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/b40d5a52870b
https://hg.mozilla.org/releases/mozilla-beta/rev/8151e54f1500
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ab0f0b3787fa
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/728a5643db0f