[Presentation API / Media Router] Relax PresentationRequest URL check.

[Presentation API / Media Router] Relax PresentationRequest URL check.

The idea is that instead checking if the scheme of a Presentation URL is
supported in Blink, we will do that in MR. This patch defines a
whitelist of schemes supported by MR.

This patch also enforces the scheme check on the URL given during a
presentation availability call. PresentationServiceDelegateImpl will
report availability = false for URLs of unsupported schemes.

This also fixes a problem where you actually cannot use a cast:* URL
in a https side despite having been "whitelisted" in Blink, due to the
MixedContent check (which isn't needed, since the URL does not refer to
an actual network resource).

The integration tests for desktop are also updated with new URLs,
including test:*. See accompanying extension CL: cl/158049822

BUG=711541
Review-Url: https://codereview.chromium.org/2927503002
Cr-Commit-Position: refs/heads/master@{#477735}
Committed: https://chromium.googlesource.com/chromium/src/+/e86537c744f1410319ce3ea4659f8c022f67ece3

[imcheng, 2017-06-06 01:06:50]

Description was changed from

==========
[Presentation API / Media Router] Relax PresentationRequest URL check.

The idea is that instead using a hardcoded list of schemes in Blink / MR
to determine whether a Presentation URL is valid, we will let the MRPs
decide whether they support the scheme (and the URL itself).

BUG=711541
==========

to

==========
[Presentation API / Media Router] Relax PresentationRequest URL check.

The idea is that instead using a hardcoded list of schemes in Blink / MR
to determine whether a Presentation URL is valid, we will let the MRPs
decide whether they support the scheme (and the URL itself). Thus, the
scheme check is dropped.

This also fixes a problem where you actually cannot use a cast:* URL
in a https side despite having been "whitelisted" in Blink, due to the
MixedContent check (which isn't needed, since the URL does not refer to
an actual network resource).

The integration tests for desktop are also updated with new URLs,
including test:*. See accompanying extension CL: cl/158049822

BUG=711541
==========

[imcheng, 2017-06-06 01:16:09]

imcheng@chromium.org changed reviewers:
+ avayvod@chromium.org, mfoltz@chromium.org

[imcheng, 2017-06-06 01:16:10]

While thinking of how we can support custom schemes in Presentation API (and
update the integration tests to use test: scheme as a start), I was originally
going to extend the hardcoded whitelist of schemes in Blink (which had only
"cast"). But then there are two issues:
- we also need such a list in MR and keep them in sync (media_source_helper)
- they need to be updated whenever a new scheme is supported

So I thought that we should let the MRP decide which scheme they want to
support. To do this we relax the scheme check in Blink / MR. Let me know what
you think. Also, adding Anton to see if we would need to add some check back in
Android's MRP implementation. For desktop, the http/https check already exists
in the MR extension).

[whywhat, 2017-06-06 01:32:39]

We currently parse the Uri without checking for its validity here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Then we get the fragment and parse it for the Cast params. android.net.Uri
doesn't generally throw any exceptions for invalid URLs - we'll basically find
the first #, get the rest of the string as the fragment and will try to parse it
for SDK parameters. If some stricter checks need to be added, it can be done in
this CL.

Note: we'd need to switch to cast:// scheme in sync with desktop to avoid
breaking Cast Web SDK and modify the check/parsing accordingly.

[mark a. foltz, 2017-06-06 18:51:59]

On 2017/06/06 at 01:32:39, avayvod wrote:
> We currently parse the Uri without checking for its validity here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> Then we get the fragment and parse it for the Cast params. android.net.Uri
doesn't generally throw any exceptions for invalid URLs - we'll basically find
the first #, get the rest of the string as the fragment and will try to parse it
for SDK parameters. If some stricter checks need to be added, it can be done in
this CL.
> 
> Note: we'd need to switch to cast:// scheme in sync with desktop to avoid
breaking Cast Web SDK and modify the check/parsing accordingly.

I'd be fine with removing the checks from PresentationRequest.cpp (since these
non-http schemes are not really part of the Web platform) but would prefer
keeping a whitelist of schemes in media_source_helper.

Reasoning:

1. This provides public documentation and assurance of the exact schemes are
supported by Chrome.
2. We can reject early and cleanly on invalid URLs before invoking the MRPs.
3. We know we won't be sending completely bogus schemes to the MRP.
4. We do have a couple of places in chrome/ with Cast-specific handling so we
need to be able to recognize those URLs in chrome/ anyway.

I don't think we'll be adding any schemes beyond cast:, dial: and test: so I am
not too worried about ongoing maintenance.

WDYT?

[whywhat, 2017-06-06 18:56:27]

On 2017/06/06 at 18:51:59, mfoltz wrote:
> On 2017/06/06 at 01:32:39, avayvod wrote:
> > We currently parse the Uri without checking for its validity here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > 
> > Then we get the fragment and parse it for the Cast params. android.net.Uri
doesn't generally throw any exceptions for invalid URLs - we'll basically find
the first #, get the rest of the string as the fragment and will try to parse it
for SDK parameters. If some stricter checks need to be added, it can be done in
this CL.
> > 
> > Note: we'd need to switch to cast:// scheme in sync with desktop to avoid
breaking Cast Web SDK and modify the check/parsing accordingly.
> 
> I'd be fine with removing the checks from PresentationRequest.cpp (since these
non-http schemes are not really part of the Web platform) but would prefer
keeping a whitelist of schemes in media_source_helper.
> 
> Reasoning:
> 
> 1. This provides public documentation and assurance of the exact schemes are
supported by Chrome.
> 2. We can reject early and cleanly on invalid URLs before invoking the MRPs.
> 3. We know we won't be sending completely bogus schemes to the MRP.
> 4. We do have a couple of places in chrome/ with Cast-specific handling so we
need to be able to recognize those URLs in chrome/ anyway.
> 
> I don't think we'll be adding any schemes beyond cast:, dial: and test: so I
am not too worried about ongoing maintenance.

And remote-playback:// ?

> 
> WDYT?

[whywhat, 2017-06-06 18:57:27]

On 2017/06/06 at 18:56:27, whywhat wrote:
> On 2017/06/06 at 18:51:59, mfoltz wrote:
> > On 2017/06/06 at 01:32:39, avayvod wrote:
> > > We currently parse the Uri without checking for its validity here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > > 
> > > Then we get the fragment and parse it for the Cast params. android.net.Uri
doesn't generally throw any exceptions for invalid URLs - we'll basically find
the first #, get the rest of the string as the fragment and will try to parse it
for SDK parameters. If some stricter checks need to be added, it can be done in
this CL.
> > > 
> > > Note: we'd need to switch to cast:// scheme in sync with desktop to avoid
breaking Cast Web SDK and modify the check/parsing accordingly.
> > 
> > I'd be fine with removing the checks from PresentationRequest.cpp (since
these non-http schemes are not really part of the Web platform) but would prefer
keeping a whitelist of schemes in media_source_helper.
> > 
> > Reasoning:
> > 
> > 1. This provides public documentation and assurance of the exact schemes are
supported by Chrome.
> > 2. We can reject early and cleanly on invalid URLs before invoking the MRPs.
> > 3. We know we won't be sending completely bogus schemes to the MRP.
> > 4. We do have a couple of places in chrome/ with Cast-specific handling so
we need to be able to recognize those URLs in chrome/ anyway.
> > 
> > I don't think we'll be adding any schemes beyond cast:, dial: and test: so I
am not too worried about ongoing maintenance.
> 
> And remote-playback:// ?

Ah, disregard if the whitelist check is not performed below PresentationRequest
though :)

> 
> > 
> > WDYT?

[imcheng, 2017-06-06 19:55:58]

On 2017/06/06 18:57:27, whywhat wrote:
> On 2017/06/06 at 18:56:27, whywhat wrote:
> > On 2017/06/06 at 18:51:59, mfoltz wrote:
> > > On 2017/06/06 at 01:32:39, avayvod wrote:
> > > > We currently parse the Uri without checking for its validity here:
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > > > 
> > > > Then we get the fragment and parse it for the Cast params.
android.net.Uri
> doesn't generally throw any exceptions for invalid URLs - we'll basically find
> the first #, get the rest of the string as the fragment and will try to parse
it
> for SDK parameters. If some stricter checks need to be added, it can be done
in
> this CL.
> > > > 
> > > > Note: we'd need to switch to cast:// scheme in sync with desktop to
avoid
> breaking Cast Web SDK and modify the check/parsing accordingly.
> > > 
> > > I'd be fine with removing the checks from PresentationRequest.cpp (since
> these non-http schemes are not really part of the Web platform) but would
prefer
> keeping a whitelist of schemes in media_source_helper.
> > > 
> > > Reasoning:
> > > 
> > > 1. This provides public documentation and assurance of the exact schemes
are
> supported by Chrome.
> > > 2. We can reject early and cleanly on invalid URLs before invoking the
MRPs.
> > > 3. We know we won't be sending completely bogus schemes to the MRP.
> > > 4. We do have a couple of places in chrome/ with Cast-specific handling so
> we need to be able to recognize those URLs in chrome/ anyway.
> > > 
> > > I don't think we'll be adding any schemes beyond cast:, dial: and test: so
I
> am not too worried about ongoing maintenance.
> > 

Yeah, that sounds good. I agree with removing the check from Blink since the
custom schemes support are dictated by Chrome. 

> > And remote-playback:// ?
> 
> Ah, disregard if the whitelist check is not performed below
PresentationRequest
> though :)
> 

We can add the scheme whitelist checking in Media Router code. We can also add
remote-playback://, since the check is performed when constructing a
MediaSource.

It also looks like we will to perform whitelist checking on the URLs passed from
Presentation API availability checking. We can do so in
PresentationServiceDelegateImpl.

> > 
> > > 
> > > WDYT?

[imcheng, 2017-06-06 22:02:17]

Description was changed from

==========
[Presentation API / Media Router] Relax PresentationRequest URL check.

The idea is that instead using a hardcoded list of schemes in Blink / MR
to determine whether a Presentation URL is valid, we will let the MRPs
decide whether they support the scheme (and the URL itself). Thus, the
scheme check is dropped.

This also fixes a problem where you actually cannot use a cast:* URL
in a https side despite having been "whitelisted" in Blink, due to the
MixedContent check (which isn't needed, since the URL does not refer to
an actual network resource).

The integration tests for desktop are also updated with new URLs,
including test:*. See accompanying extension CL: cl/158049822

BUG=711541
==========

to

==========
[Presentation API / Media Router] Relax PresentationRequest URL check.

The idea is that instead checking if the scheme of a Presentation URL is
supported in Blink, we will do that in MR. This patch defines a
whitelist of schemes supported by MR.

This patch also enforces the scheme check on the URL given during a
presentation availability call. PresentationServiceDelegateImpl will
report availability = false for URLs of unsupported schemes.

This also fixes a problem where you actually cannot use a cast:* URL
in a https side despite having been "whitelisted" in Blink, due to the
MixedContent check (which isn't needed, since the URL does not refer to
an actual network resource).

The integration tests for desktop are also updated with new URLs,
including test:*. See accompanying extension CL: cl/158049822

BUG=711541
==========

[imcheng, 2017-06-06 22:08:08]

Updated patch with tests. PTAL.

[mark a. foltz, 2017-06-06 22:49:08]

Thanks for being flexible.

LGTM with comments addressed.

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
File chrome/common/media_router/media_source_helper.cc (right):

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
chrome/common/media_router/media_source_helper.cc:34: constexpr std::array<const
char* const, 4> kAllowedSchemes{
Is std::array a type alias to POD or does it have a constructor?  If the latter,
it shouldn't have static lifetime.

Can this be constexpr char* kAllowedSchemes[kNumAllowedSchemes] = { ... }
instead?

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
chrome/common/media_router/media_source_helper.cc:37: bool IsUrlAllowed(const
GURL& url) {
IsSchemeAllowed?

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
chrome/common/media_router/media_source_helper.cc:39: std::any_of(
I think a for loop over a kAllowedSchemes array would involve less code overall
by not requiring a lambda definition (unless the compiler can optimize this into
equivalent code).  Not trying to over-optimize, but if there is a more compact
way to do the same thing...

[imcheng, 2017-06-07 00:53:44]

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
File chrome/common/media_router/media_source_helper.cc (right):

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
chrome/common/media_router/media_source_helper.cc:34: constexpr std::array<const
char* const, 4> kAllowedSchemes{
On 2017/06/06 22:49:08, mark a. foltz wrote:
> Is std::array a type alias to POD or does it have a constructor?  If the
latter,
> it shouldn't have static lifetime.
> 
> Can this be constexpr char* kAllowedSchemes[kNumAllowedSchemes] = { ... }
> instead?

Yep, this is a POD.
https://stackoverflow.com/questions/3674247/is-stdarrayt-s-guaranteed-to-be-p...

This seems a bit nicer to use than C style array, since it comes with iterator
support (though not needed, strictly speaking...)

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
chrome/common/media_router/media_source_helper.cc:37: bool IsUrlAllowed(const
GURL& url) {
On 2017/06/06 22:49:08, mark a. foltz wrote:
> IsSchemeAllowed?

Done.

https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
chrome/common/media_router/media_source_helper.cc:39: std::any_of(
On 2017/06/06 22:49:08, mark a. foltz wrote:
> I think a for loop over a kAllowedSchemes array would involve less code
overall
> by not requiring a lambda definition (unless the compiler can optimize this
into
> equivalent code).  Not trying to over-optimize, but if there is a more compact
> way to do the same thing...
> 

A decent compiler should be able to optimize this. The written code using
std::any_of is slightly more compact then using a for loop. Not a huge
difference either way.

[imcheng, 2017-06-07 00:53:52]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 00:54:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 01:09:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 01:09:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mark a. foltz, 2017-06-07 16:32:07]

On 2017/06/07 at 00:53:44, imcheng wrote:
>
https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
> File chrome/common/media_router/media_source_helper.cc (right):
> 
>
https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
> chrome/common/media_router/media_source_helper.cc:34: constexpr
std::array<const char* const, 4> kAllowedSchemes{
> On 2017/06/06 22:49:08, mark a. foltz wrote:
> > Is std::array a type alias to POD or does it have a constructor?  If the
latter,
> > it shouldn't have static lifetime.
> > 
> > Can this be constexpr char* kAllowedSchemes[kNumAllowedSchemes] = { ... }
> > instead?
> 
> Yep, this is a POD.
https://stackoverflow.com/questions/3674247/is-stdarrayt-s-guaranteed-to-be-p...
> 
> This seems a bit nicer to use than C style array, since it comes with iterator
support (though not needed, strictly speaking...)
> 
>
https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
> chrome/common/media_router/media_source_helper.cc:37: bool IsUrlAllowed(const
GURL& url) {
> On 2017/06/06 22:49:08, mark a. foltz wrote:
> > IsSchemeAllowed?
> 
> Done.
> 
>
https://codereview.chromium.org/2927503002/diff/40001/chrome/common/media_rou...
> chrome/common/media_router/media_source_helper.cc:39: std::any_of(
> On 2017/06/06 22:49:08, mark a. foltz wrote:
> > I think a for loop over a kAllowedSchemes array would involve less code
overall
> > by not requiring a lambda definition (unless the compiler can optimize this
into
> > equivalent code).  Not trying to over-optimize, but if there is a more
compact
> > way to do the same thing...
> > 
> 
> A decent compiler should be able to optimize this. The written code using
std::any_of is slightly more compact then using a for loop. Not a huge
difference either way.

The question is whether the compiler can inline the lambda expression and in so
doing not generate an additional stack frame with copies of the pointers to
|url| and |scheme|.

I agree with this argument that inlining is possible:
https://stackoverflow.com/questions/13722426/why-can-lambdas-be-better-optimi...

It would be interesting to know what the generated code looks like.

[imcheng, 2017-06-07 17:52:15]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 17:52:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[imcheng, 2017-06-07 17:57:26]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 17:57:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[whywhat, 2017-06-07 18:50:14]

lgtm

thanks for adding remote-playback:// into the list!

[commit-bot: I haz the power, 2017-06-07 19:41:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 19:41:05]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-06-07 19:43:18]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2017-06-07 19:43:19]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2927503002/#ps60002
(title: "Really add #include")

[commit-bot: I haz the power, 2017-06-07 19:43:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 19:49:05]

CQ is committing da patch.

Bot data: {"patchset_id": 60002, "attempt_start_ts": 1496864598903360,
"parent_rev": "651d12dca852b4abb5e01f18a8bd950e52705605", "commit_rev":
"e86537c744f1410319ce3ea4659f8c022f67ece3"}

[commit-bot: I haz the power, 2017-06-07 19:49:15]

Description was changed from

==========
[Presentation API / Media Router] Relax PresentationRequest URL check.

The idea is that instead checking if the scheme of a Presentation URL is
supported in Blink, we will do that in MR. This patch defines a
whitelist of schemes supported by MR.

This patch also enforces the scheme check on the URL given during a
presentation availability call. PresentationServiceDelegateImpl will
report availability = false for URLs of unsupported schemes.

This also fixes a problem where you actually cannot use a cast:* URL
in a https side despite having been "whitelisted" in Blink, due to the
MixedContent check (which isn't needed, since the URL does not refer to
an actual network resource).

The integration tests for desktop are also updated with new URLs,
including test:*. See accompanying extension CL: cl/158049822

BUG=711541
==========

to

==========
[Presentation API / Media Router] Relax PresentationRequest URL check.

The idea is that instead checking if the scheme of a Presentation URL is
supported in Blink, we will do that in MR. This patch defines a
whitelist of schemes supported by MR.

This patch also enforces the scheme check on the URL given during a
presentation availability call. PresentationServiceDelegateImpl will
report availability = false for URLs of unsupported schemes.

This also fixes a problem where you actually cannot use a cast:* URL
in a https side despite having been "whitelisted" in Blink, due to the
MixedContent check (which isn't needed, since the URL does not refer to
an actual network resource).

The integration tests for desktop are also updated with new URLs,
including test:*. See accompanying extension CL: cl/158049822

BUG=711541

Review-Url: https://codereview.chromium.org/2927503002
Cr-Commit-Position: refs/heads/master@{#477735}
Committed:
https://chromium.googlesource.com/chromium/src/+/e86537c744f1410319ce3ea4659f...
==========

[commit-bot: I haz the power, 2017-06-07 19:49:17]

Committed patchset #6 (id:60002) as
https://chromium.googlesource.com/chromium/src/+/e86537c744f1410319ce3ea4659f...