Budget API calls should only succeed on secure origins

Budget API calls should only succeed on secure origins

BUG=617971
Committed: https://crrev.com/a838aec4845f3f6d4c702e0336d7ef41eccbd681
Cr-Commit-Position: refs/heads/master@{#420630}

[harkness, 2016-09-22 10:12:29]

harkness@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2016-09-22 14:27:29]

Have you considered adding checks to BudgetServiceImpl? It'd be great if we
could call ReceivedBadMessage() if a renderer tries anyway, instead of failing
with NOT_SUPPORTED.

https://codereview.chromium.org/2366533002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager.cc (right):

https://codereview.chromium.org/2366533002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.cc:73: if
(!url.SchemeIsHTTPOrHTTPS() || !content::IsOriginSecure(url)) {
What's the added value of checking SchemeIsHTTPOrHTTPS()? I think we can just
rely on IsOriginSecure(). Elsewhere too.

[harkness, 2016-09-22 15:35:47]

I did consider putting the checks in budget_service_impl.cc. I decided on
budget_manager.cc because I definitely want to check for secure origin on calls
from within chrome, and that's the first place that both Mojo and chrome calls
pass through.

https://codereview.chromium.org/2366533002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager.cc (right):

https://codereview.chromium.org/2366533002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.cc:73: if
(!url.SchemeIsHTTPOrHTTPS() || !content::IsOriginSecure(url)) {
On 2016/09/22 14:27:29, Peter Beverloo wrote:
> What's the added value of checking SchemeIsHTTPOrHTTPS()? I think we can just
> rely on IsOriginSecure(). Elsewhere too.

By my reading of IsOriginSecure, it looks like it will allow file schemes, which
are not allowed by ServiceWorkers. 

I suppose it's really up to us what we want to allow in the spec, but my plan
was to mirror the service worker limits, and OriginCanAccessServiceWorkers has
botht he IsOriginSecure and the SchemeIsHTTPOrHTTPS check.

[harkness, 2016-09-23 10:32:19]

https://codereview.chromium.org/2366533002/diff/20001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager.cc (right):

https://codereview.chromium.org/2366533002/diff/20001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager.cc:72: if (origin.unique() ||
!content::IsOriginSecure(GURL(origin.Serialize()))) {
As discussed, the HTTP/HTTPS check has been updated to origin.unique().

[Peter Beverloo, 2016-09-23 12:33:05]

lgtm

[harkness, 2016-09-23 12:36:26]

The CQ bit was checked by harkness@chromium.org

[commit-bot: I haz the power, 2016-09-23 12:36:43]

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, 2016-09-23 12:47:46]

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

[commit-bot: I haz the power, 2016-09-23 12:47:47]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[harkness, 2016-09-23 15:28:41]

The CQ bit was checked by harkness@chromium.org

[commit-bot: I haz the power, 2016-09-23 15:29:01]

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, 2016-09-23 16:38:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-23 16:40:15]

Description was changed from

==========
Budget API calls should only succeed on secure origins

BUG=617971
==========

to

==========
Budget API calls should only succeed on secure origins

BUG=617971

Committed: https://crrev.com/a838aec4845f3f6d4c702e0336d7ef41eccbd681
Cr-Commit-Position: refs/heads/master@{#420630}
==========

[commit-bot: I haz the power, 2016-09-23 16:40:16]

Patchset 3 (id:??) landed as
https://crrev.com/a838aec4845f3f6d4c702e0336d7ef41eccbd681
Cr-Commit-Position: refs/heads/master@{#420630}