Fix to respect --explicitly-allowed-ports command line option

Fix to respect --explicitly-allowed-ports command line option

Removing portAllowed from KnownPorts.h, we will check it in chromium side.

[PATCH 1]: https://codereview.chromium.org/1036823003/
[PATCH 2]: This Patch

BUG=223927
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193818

[Paritosh Kumar, 2015-02-06 12:29:38]

paritosh.in@samsung.com changed reviewers:
+ a.cavalcanti@samsung.com, habib.virji@samsung.com

[Paritosh Kumar, 2015-02-06 12:29:38]

@Habib, @Adenilson: Please have a look.

[Habib Virji, 2015-02-06 13:18:11]

On 2015/02/06 12:29:38, Paritosh Kumar wrote:
> @Habib, @Adenilson: Please have a look.

1. remove postAllowed from KnownPorts.h ->
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....


2. Comment https://code.google.com/p/chromium/issues/detail?id=223927#c4
mentions about adding isPortAllowed() in SocketStream. Is there a separate patch
for that fix?

[cavalcantii1, 2015-02-06 17:03:36]

A tip: remember to ask us to quick the bots.
:-)

[Paritosh Kumar, 2015-02-09 13:40:15]

Thanks

Removed the portAllowed method from, KnownPorts.h file.


>Comment https://code.google.com/p/chromium/issues/detail?id=223927#c4
>mentions about adding isPortAllowed() in SocketStream. Is there a separate
patch
>for that fix?

As at
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...
we are checking for this, I think this is not required again in SocketStream.


>A tip: remember to ask us to quick the bots.

Sure, but all the times trybot exception is coming.

[Paritosh Kumar, 2015-02-13 02:24:52]

paritosh.in@samsung.com changed reviewers:
+ haraken@chromium.org, tyoshino@chromium.org

[Paritosh Kumar, 2015-02-13 02:24:52]

@tyoshino, @haraken: Please have a look.

[haraken, 2015-02-13 02:31:32]

On 2015/02/13 02:24:52, Paritosh Kumar wrote:
> @tyoshino, @haraken: Please have a look.

I want to have yoshino-san take a look first.

[tyoshino (SeeGerritForStatus), 2015-02-16 04:26:00]

On 2015/02/13 02:31:32, haraken wrote:
> On 2015/02/13 02:24:52, Paritosh Kumar wrote:
> > @tyoshino, @haraken: Please have a look.
> 
> I want to have yoshino-san take a look first.

Thanks for working on this. But I'd like to keep the WebSocket API conform to
this step of the spec.

https://html.spec.whatwg.org/multipage/comms.html

> If port is a port to which the user agent is configured to block access, then
throw a SecurityError exception and abort these steps. (User agents typically
block access to well-known ports like SMTP.)

Does http/tests/websocket/url-parsing.html pass even with this patch?

If not, ... I guess it's not a small work to return the result of port checking
from net/ to Blink and fail the WebSocket constructor synchronously. Could you
please try to export the well known port checking function implemented in net/
via content/child/blink_platform_impl.cc and use it in DOMWebSocket.cpp?

[Paritosh Kumar, 2015-03-01 05:11:57]

Thanks tyoshino


I tested http/tests/websocket/url-parsing.html, its failing.

As per your suggestion I tried to return result of port checking
from net/ via "content/child/blink_platform_impl.cc".

I'm trying this change:

at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

if (!portAllowed(m_url)) ==>> if
(!Platform::current()->portAllowed(m_url.port(), m_url.protocolIs("ftp"))

at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

virtual bool portAllowed(const unsigned short port, bool isFtp) const { return
false; }

at 
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/blin...
virtual bool portAllowed(const unsigned short port, bool isFtp) const

at 
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/blin...

bool BlinkPlatformImpl::portAllowed(const unsigned short port, bool isFtp) const
{
  if (isFtp)
    return net::IsPortAllowedByFtp(port) || net::IsPortAllowedByOverride(port);
  return net::IsPortAllowedByDefault(port) ||
net::IsPortAllowedByOverride(port); 
}

[Paritosh Kumar, 2015-03-01 11:22:21]

Thanks tyoshino


I tested http/tests/websocket/url-parsing.html, its failing.

As per your suggestion I tried to return result of port checking
from net/ via "content/child/blink_platform_impl.cc".

I'm trying this change:

at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

if (!portAllowed(m_url)) ==>> if
(!Platform::current()->portAllowed(m_url.port(), m_url.protocolIs("ftp"))

at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

virtual bool portAllowed(const unsigned short port, bool isFtp) const { return
false; }

at 
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/blin...
virtual bool portAllowed(const unsigned short port, bool isFtp) const

at 
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/blin...

bool BlinkPlatformImpl::portAllowed(const unsigned short port, bool isFtp) const
{
  if (isFtp)
    return net::IsPortAllowedByFtp(port) || net::IsPortAllowedByOverride(port);
  return net::IsPortAllowedByDefault(port) ||
net::IsPortAllowedByOverride(port); 
}


But, portAllowed() is returning false, because
net::IsPortAllowedByOverride(port) is returning false
always, either we use --explicitly-allowed-ports or not.

[tyoshino (SeeGerritForStatus), 2015-03-19 12:49:51]

tyoshino@chromium.org changed reviewers:
+ eroman@chromium.org

[tyoshino (SeeGerritForStatus), 2015-03-19 12:53:39]

On 2015/03/01 11:22:21, Paritosh Kumar(OOO till 22nd) wrote:
> But, portAllowed() is returning false, because
> net::IsPortAllowedByOverride(port) is returning false
> always, either we use --explicitly-allowed-ports or not.

Oh, I see. Yeah, it doesn't work. Sorry. The global instance holding the allowed
ports in the renderer process is different from the one in the browser process.

We could choose to parse the option also in the renderer or have the browser
initialize the renderer via some IPC message or something like WebSettings.

Using RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer() is the
simplest solution.

I'd like to get some input from net/ OWNERS. +eroman. As the renderer process is
supposed not to make any network connection, it might be slight strange to
initialize g_explicitly_allowed_ports in the renderer process.

[Paritosh Kumar, 2015-03-26 09:11:26]

Thanks tyoshino

>I'd like to get some input from net/ OWNERS.

Waiting for inputs from net/ OWNERS.

But, Uploaded a cl https://codereview.chromium.org/1036823003/
to initialize g_explicitly_allowed_ports in the renderer process
Using RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer().

PTAL.

[Paritosh Kumar, 2015-04-02 10:57:03]

paritosh.in@samsung.com changed reviewers:
+ mkwst@chromium.org

[eroman, 2015-04-10 01:55:56]

Removal of the duplicated code looksgood.
A few comments on implementation specifics

https://codereview.chromium.org/908483002/diff/40001/Source/modules/websocket...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/908483002/diff/40001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocket.cpp:306: if
(!Platform::current()->portAllowed(m_url.port(), m_url.protocolIs("ftp"))) {
A couple things here:

  (1) It is weird to be calling m_url.port() in cases where the URL has no
specified port. In this case it will return 0, and relies on the assumption that
0 is an allowed port to pass. It would be better to only do the test if
m_url.hasPort().

 (2) Is it possible for the URL scheme to be anything other than "ws" or "wss" ?
The test for FTP would make sense in the general case, but it looks weird here.

 (3) The previous implementation of portAllowed() also explicitly checked for
"file" scheme and allowed everything on that. I don't think that is applicable
anymore, but the solution will depend on the answer to (2)

[Paritosh Kumar, 2015-04-10 12:52:36]

Thanks eroman

Addressed your comment inline.

Please have a look.

https://codereview.chromium.org/908483002/diff/40001/Source/modules/websocket...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/908483002/diff/40001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocket.cpp:306: if
(!Platform::current()->portAllowed(m_url.port(), m_url.protocolIs("ftp"))) {
>   (1) It is weird to be calling m_url.port() in cases where the URL has no
> specified port. In this case it will return 0, and relies on the assumption
that
> 0 is an allowed port to pass. It would be better to only do the test if
> m_url.hasPort().

Updated in new CL.

 
>  (2) Is it possible for the URL scheme to be anything other than "ws" or "wss"
?
> The test for FTP would make sense in the general case, but it looks weird
here.

As on line# 296 we are doing protocol check:
"if (!m_url.protocolIs("ws") && !m_url.protocolIs("wss")) {
    m_state = CLOSED;
    exceptionState.throwDOMException(SyntaxError, "The URL's scheme must be
either 'ws' or 'wss'. '" + m_url.protocol() + "' is not allowed.");
    return;
}"

So, URL scheme cannot be other than "ws" or "wss", here. So, should remove test
for FTP. Thanks.


>  (3) The previous implementation of portAllowed() also explicitly checked for
> "file" scheme and allowed everything on that. I don't think that is applicable
> anymore, but the solution will depend on the answer to (2)

Agree. No need of checking for file also. As per (2).

[eroman, 2015-04-10 17:22:48]

https://codereview.chromium.org/908483002/diff/60001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/908483002/diff/60001/public/platform/Platform...
public/platform/Platform.h:346: virtual bool portAllowed(unsigned short port)
const { return false; }
Can you change the API to:
   virtual bool portAllowed(const WebURL&) const;

To mirror the previous implementation.

That way the scheme checks and port relevancy can be internalized to the
function (the hasPort() check will move into the implementation, and we can keep
the FTP check in case future code wants to use this for non-web sockets).

After that the change lookgoodtome

[Paritosh Kumar, 2015-04-13 10:00:25]

Thanks eroman

As per your suggestion changed the Platform API to
   virtual bool portAllowed(const WebURL&) const;
to mirror the previous implementation.

For this change updated cl for https://codereview.chromium.org/1036823003/ also.

Please have a look.

https://codereview.chromium.org/908483002/diff/60001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/908483002/diff/60001/public/platform/Platform...
public/platform/Platform.h:346: virtual bool portAllowed(unsigned short port)
const { return false; }
On 2015/04/10 17:22:48, eroman wrote:
> Can you change the API to:
>    virtual bool portAllowed(const WebURL&) const;
> 
> To mirror the previous implementation.
> 
> That way the scheme checks and port relevancy can be internalized to the
> function (the hasPort() check will move into the implementation, and we can
keep
> the FTP check in case future code wants to use this for non-web sockets).
> 
> After that the change lookgoodtome

Thanks. Done.

[tyoshino (SeeGerritForStatus), 2015-04-13 14:29:33]

lgtm

[eroman, 2015-04-13 15:26:25]

lgtm

[Mike West, 2015-04-14 05:14:24]

public/ LGTM.

[Paritosh Kumar, 2015-04-15 17:43:18]

The CQ bit was checked by paritosh.in@samsung.com

[commit-bot: I haz the power, 2015-04-15 17:43:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908483002/80001

[commit-bot: I haz the power, 2015-04-15 19:20:42]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193818