Bug 86860 – KURL returns port equals to 0 if default port for protocol is in use

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Thiago Marcos P. Santos]

This consequently affects WebCore::SecurityOrigin, since port 0 is also returned for malformed URL. You can't really say if the port == 0 fallsback to default or is Invalid.

For this fix, backwards compatibility has to be taken into account since database identifiers are generated like http_example.com_0 when the origin uses the default port (80 in this example).

[Adam Barth]

I'm not sure I understand what problem you're trying to solve.  KURL has an isValid method you're supposed to use to check if the URL is valid.

[Thiago Marcos P. Santos]

(In reply to comment #1)
> I'm not sure I understand what problem you're trying to solve.  KURL has an isValid method you're supposed to use to check if the URL is valid.

The Security Origin API for at least EFL, Qt and GTK returns 0 when you call ::port() if the protocol is the default. Makes more sense to return the real port like 80, 443, etc.

[Adam Barth]

> The Security Origin API for at least EFL, Qt and GTK returns 0 when you call ::port() if the protocol is the default. Makes more sense to return the real port like 80, 443, etc.

Do you mean the API used by the embedder?  If so, that makes some sense.  I wonder if we should handle that at the WebKit layer though.

[Thiago Marcos P. Santos]

Created attachment 142761 [details]
Patch

[Adam Barth]

Comment on attachment 142761 [details]
Patch

Wouldn't it be easier to leave the internal representation as 0 and just change SecurityOrigin::port to look up the default port if the port number is zero?

Also, 0 isn't a valid port, so there's no need to keep around an extra bit.

[Thiago Marcos P. Santos]

(In reply to comment #5)
> (From update of attachment 142761 [details])
> Wouldn't it be easier to leave the internal representation as 0 and just change SecurityOrigin::port to look up the default port if the port number is zero?
> 
> Also, 0 isn't a valid port, so there's no need to keep around an extra bit.

Hmmm, that's one option, but if we do so, KURL will not return a valid port for KURL::port() and everything that uses it will need to do the same lookup.

[Adam Barth]

> Hmmm, that's one option, but if we do so, KURL will not return a valid port for KURL::port() and everything that uses it will need to do the same lookup.

I'm not sure you're right w.r.t. interoperating with other browsers.  Consider this test case, for example:

data:text/html,<a href="http://example.com"></a><script>alert(document.getElementsByTagName("a")[0].port);</script>

In Firefox and Chrome, that alerts the empty string, not 80.  In Safari, it alerts "0".  The right behavior is probably to alert the empty string.

Here's a positive control, for comparison:

data:text/html,<a href="http://example.com:234"></a><script>alert(document.getElementsByTagName("a")[0].port);</script>

In that case, Firefox, Safari, and Chrome alert "234".

[Thiago Marcos P. Santos]

(In reply to comment #7)
> > Hmmm, that's one option, but if we do so, KURL will not return a valid port for KURL::port() and everything that uses it will need to do the same lookup.
> 
> I'm not sure you're right w.r.t. interoperating with other browsers.  Consider this test case, for example:
> 
> data:text/html,<a href="http://example.com"></a><script>alert(document.getElementsByTagName("a")[0].port);</script>
> 
> In Firefox and Chrome, that alerts the empty string, not 80.  In Safari, it alerts "0".  The right behavior is probably to alert the empty string.
> 
> Here's a positive control, for comparison:
> 
> data:text/html,<a href="http://example.com:234"></a><script>alert(document.getElementsByTagName("a")[0].port);</script>
> 
> In that case, Firefox, Safari, and Chrome alert "234".

I took this as source of inspiration:
http://tools.ietf.org/html/rfc6454#section-4

6.  If there is no port component of the URI:
   1.  Let uri-port be the default port for the protocol given by uri-scheme.
   Otherwise:
   2.  Let uri-port be the port component of the URI.

7.  Return the triple (uri-scheme, uri-host, uri-port).

[Thiago Marcos P. Santos]

(In reply to comment #7)
> > Hmmm, that's one option, but if we do so, KURL will not return a valid port for KURL::port() and everything that uses it will need to do the same lookup.
> 
> I'm not sure you're right w.r.t. interoperating with other browsers.  Consider this test case, for example:
> 
> data:text/html,<a href="http://example.com"></a><script>alert(document.getElementsByTagName("a")[0].port);</script>
> 
> In Firefox and Chrome, that alerts the empty string, not 80.  In Safari, it alerts "0".  The right behavior is probably to alert the empty string.

Hmmm, and Opera also alerts empty.

> 
> Here's a positive control, for comparison:
> 
> data:text/html,<a href="http://example.com:234"></a><script>alert(document.getElementsByTagName("a")[0].port);</script>
> 
> In that case, Firefox, Safari, and Chrome alert "234".

[Adam Barth]

> I took this as source of inspiration:
> http://tools.ietf.org/html/rfc6454#section-4

Haha.  Yeah, I know what it says.  :)

That's all just internal representation, however.  What really matters is the observable consequences.  The value we expose via the embedder API is certainly observable, and I agree with you that it makes sense to return 80 to the embedder.  There are many ways to implement that.  The easiest might well be to do the conversion at the last possible moment before returning the value via the API.

[Thiago Marcos P. Santos]

(In reply to comment #10)
> > I took this as source of inspiration:
> > http://tools.ietf.org/html/rfc6454#section-4
> 
> Haha.  Yeah, I know what it says.  :)
> 
> That's all just internal representation, however.  What really matters is the observable consequences.  The value we expose via the embedder API is certainly observable, and I agree with you that it makes sense to return 80 to the embedder.  There are many ways to implement that.  The easiest might well be to do the conversion at the last possible moment before returning the value via the API.

OK, gonna fix that. Thanks for reviewing.

[Thiago Marcos P. Santos]

Will be handled now on bug 87002, with a different context.