Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs

[Dave Cahill <dc...@midokura.com>]

Hi Daan / Alex,

Sorry I missed this change at the time, but it looks problematic to me.

The code is trying to check for a scheme part in the incoming value, and
doesn't add
a scheme if value already has one. Therefore, if the scheme check has a
false positive
(it thinks the value has a scheme, but it doesn't), we are guaranteed to
produce an
invalid URI (no scheme part) and an exception.

I'm pretty sure the current scheme check is invalid given the URI spec, but
definitely
shout if I'm off base.

Here's the check:
    if (value.toString().contains(":"))
        return new URI(value.toString());
    else
        return new URI(scheme, value.toString(), null);

"value" in this case is ssp, the Scheme Specific Part (see signature of the
URI
constructor we're using [1]). There are basically no restrictions on the
scheme
specific part of a URI, and certainly no restriction on the presence of
colons.
In fact, many URIs have an authority part in the SSP (e.g. user:password)
 which
*requires* a colon [2]. http is another example, where a colon is
used within the
SSP to specify the port.

In summary, it's perfectly valid to have a colon in the scheme specific
part of a URI,
so we shouldn't be creating invalid URIs in that case.

What are we trying to protect against in the first place with the check?
Should we
just remove the check?

Thanks,
Dave.

[1] http://docs.oracle.com/javase/6/docs/api/java/net/URI.html#URI(java.lang.String,
java.lang.String, java.lang.String)
[2] http://www.ietf.org/rfc/rfc2396.txt

Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs

[Dave Cahill <dc...@midokura.com>]

Hi,

Adding Daan's reply - I sent to the wrong dev@ first time around, so the
reply went astray:

> On Tue, Aug 6, 2013 at 10:30 AM, Dave Cahill <dc...@midokura.com> wrote:
> > I'm pretty sure the current scheme check is invalid given the URI spec,
but
> > definitely
> > shout if I'm off base.
>
> You are not 'way' of but please review https://reviews.apache.org/r/12849/
>
> If the caller needs BroadcastDomainType the reason why should be known
> and the value-member can be called taking your foreseen problem into
> account. The code you are quoting would 'just' be a default behavior.
>
> regards,
> Daan

If you're around, maybe discussing on IRC would be faster?

I took a read of the code you posted, and the default behavior still causes
an exception if colons are
present in the value passed to toUri.

I might be interpreting wrong, but it sounds like you're saying anyone
using BroadcastDomainType
should actively work around the new (incorrect) default colon handling by
changing what value they're
passing. This seems like a strange choice, since the value they're passing
may be perfectly correct and
other parts of the system may depend on it. Why would we intentionally change
the existing behavior
to start handling colons incorrectly, and put the burden on the caller?

The rest of the code looks fine to me, but the colon part breaks existing
callers (the MidoNet plugin
for one), is confusing for anyone using BroadcastDomainType in future, and
I don't yet see what it
gains us.

I'll add comments on the review. Thanks for the quick reply; feel free to
grab me on IRC, we'll probably
figure this out faster than on a mail thread. :)

Thanks,
Dave.




On Tue, Aug 6, 2013 at 5:38 PM, Dave Cahill <dc...@midokura.com> wrote:

> Hi Daan / Alex,
>
> Sorry I missed this change at the time, but it looks problematic to me.
>
> The code is trying to check for a scheme part in the incoming value, and
> doesn't add
> a scheme if value already has one. Therefore, if the scheme check has a
> false positive
> (it thinks the value has a scheme, but it doesn't), we are guaranteed to
> produce an
> invalid URI (no scheme part) and an exception.
>
> I'm pretty sure the current scheme check is invalid given the URI spec,
> but definitely
> shout if I'm off base.
>
> Here's the check:
>     if (value.toString().contains(":"))
>         return new URI(value.toString());
>     else
>         return new URI(scheme, value.toString(), null);
>
> "value" in this case is ssp, the Scheme Specific Part (see signature of
> the URI
> constructor we're using [1]). There are basically no restrictions on the
> scheme
> specific part of a URI, and certainly no restriction on the presence of
> colons.
> In fact, many URIs have an authority part in the SSP (e.g. user:password)
>  which
> *requires* a colon [2]. http is another example, where a colon is
> used within the
> SSP to specify the port.
>
> In summary, it's perfectly valid to have a colon in the scheme specific
> part of a URI,
> so we shouldn't be creating invalid URIs in that case.
>
> What are we trying to protect against in the first place with the check?
> Should we
> just remove the check?
>
> Thanks,
> Dave.
>
> [1] http://docs.oracle.com/javase/6/docs/api/java/net/URI.html#URI(java.lang.String,
> java.lang.String, java.lang.String)
> [2] http://www.ietf.org/rfc/rfc2396.txt
>