fix service worker cross-origin problem in multibuffers

The cross-origin checks in the multibuffer code are not sufficient, as they only trigger when a redirect happens. Service-workers do not need redirects to paste data from diffierent origins together.

This CL fixes that.

BUG=610747
Committed: https://crrev.com/c1f791b319a03f4b3a70e8dbaf0bb4c240a86087
Cr-Commit-Position: refs/heads/master@{#392850}

[hubbe, 2016-05-10 00:05:06]

hubbe@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[ddorwin, 2016-05-10 00:08:22]

Drive-by comments:
* Is there a BUG that provides more information? You might even provide a little
more detail in the body of the commit message.
* The current last line of the commit message seems more like a message to
reviewers than part of a commit message.

[DaleCurtis, 2016-05-10 00:16:08]

+1 to ddorwin@'s comments, this needs a BUG= and a much better description.

https://codereview.chromium.org/1958123004/diff/20001/media/blink/resource_mu...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1958123004/diff/20001/media/blink/resource_mu...
media/blink/resource_multibuffer_data_provider.cc:316: // This test is vital for
security!
Needs unit test too then; did we lose a test from the BufferedDataSource suite?

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.c...
media/blink/url_index.cc:132: have_data_origin_ = true;
Isn't this just data_origin_.is_empty()?

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.h
File media/blink/url_index.h (right):

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.h...
media/blink/url_index.h:103: // to the previous origin and returns weather they
match or not.
s/weather/whether/

https://codereview.chromium.org/1958123004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/service-worker-mixed-response.html
(right):

https://codereview.chromium.org/1958123004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/service-worker-mixed-response.html:72:
var request_num = 1;
Why are these changes needed? Presumably attackers would not rotate their
request numbers? :)

[hubbe, 2016-05-10 17:53:20]

Description was changed from

==========
fix service worker cross-origin problem in multibuffers

This should let us turn multibuffers on on trunk.
==========

to

==========
fix service worker cross-origin problem in multibuffers

This should let us turn multibuffers on on trunk.

BUG=610747
==========

[hubbe, 2016-05-10 17:54:36]

Description was changed from

==========
fix service worker cross-origin problem in multibuffers

This should let us turn multibuffers on on trunk.

BUG=610747
==========

to

==========
The crosss-origin checks in the multibuffer code are not sufficient, as they
only trigger when a redirect happens. Service-workers do not need redirects to
paste data from diffierent origins together.

This CL fixes that.

BUG=610747
==========

[hubbe, 2016-05-10 22:33:23]

hubbe@chromium.org changed reviewers:
+ liberato@chromium.org

[hubbe, 2016-05-10 22:33:23]

+ liberato since dale is out

https://codereview.chromium.org/1958123004/diff/20001/media/blink/resource_mu...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1958123004/diff/20001/media/blink/resource_mu...
media/blink/resource_multibuffer_data_provider.cc:316: // This test is vital for
security!
On 2016/05/10 00:16:08, DaleCurtis_OOO_Until_May_16 wrote:
> Needs unit test too then; did we lose a test from the BufferedDataSource
suite?

Turns out we missed one test (added after fork), and some of the tests were not
working as they should, fixed now.

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.c...
media/blink/url_index.cc:132: have_data_origin_ = true;
On 2016/05/10 00:16:08, DaleCurtis wrote:
> Isn't this just data_origin_.is_empty()?

No, the origin is empty if it was generated by a service worker.

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.h
File media/blink/url_index.h (right):

https://codereview.chromium.org/1958123004/diff/20001/media/blink/url_index.h...
media/blink/url_index.h:103: // to the previous origin and returns weather they
match or not.
On 2016/05/10 00:16:08, DaleCurtis wrote:
> s/weather/whether/

Done.

https://codereview.chromium.org/1958123004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/service-worker-mixed-response.html
(right):

https://codereview.chromium.org/1958123004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/service-worker-mixed-response.html:72:
var request_num = 1;
On 2016/05/10 00:16:08, DaleCurtis wrote:
> Why are these changes needed? Presumably attackers would not rotate their
> request numbers? :)

This file contains multiple tests, each assuming a clean slate.
The test results become somewhat confusing if each test depends on what tests
have been run before.

[DaleCurtis, 2016-05-10 22:42:09]

lgtm

https://codereview.chromium.org/1958123004/diff/60001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1958123004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:193: {
Delete extra {} now.

https://codereview.chromium.org/1958123004/diff/60001/media/blink/resource_mu...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1958123004/diff/60001/media/blink/resource_mu...
media/blink/resource_multibuffer_data_provider.cc:167:
std::unique_ptr<ActiveLoader> active_loader = std::move(active_loader_);
How come active_loader_ needs to out live Fail()? Seems this could just be
active_loader_ = nullptr or done after fail?

[DaleCurtis, 2016-05-10 22:42:09]

lgtm

https://codereview.chromium.org/1958123004/diff/60001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1958123004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:193: {
Delete extra {} now.

https://codereview.chromium.org/1958123004/diff/60001/media/blink/resource_mu...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1958123004/diff/60001/media/blink/resource_mu...
media/blink/resource_multibuffer_data_provider.cc:167:
std::unique_ptr<ActiveLoader> active_loader = std::move(active_loader_);
How come active_loader_ needs to out live Fail()? Seems this could just be
active_loader_ = nullptr or done after fail?

[hubbe, 2016-05-11 00:06:39]

https://codereview.chromium.org/1958123004/diff/60001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1958123004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:193: {
On 2016/05/10 22:42:09, DaleCurtis_OOO_Until_May_16 wrote:
> Delete extra {} now.

Done.

https://codereview.chromium.org/1958123004/diff/60001/media/blink/resource_mu...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1958123004/diff/60001/media/blink/resource_mu...
media/blink/resource_multibuffer_data_provider.cc:167:
std::unique_ptr<ActiveLoader> active_loader = std::move(active_loader_);
On 2016/05/10 22:42:09, DaleCurtis_OOO_Until_May_16 wrote:
> How come active_loader_ needs to out live Fail()? Seems this could just be
> active_loader_ = nullptr or done after fail?

Good question, I was lazily doing this the same as in some other places. Cleaned
up here and elsewhere.

[DaleCurtis, 2016-05-11 00:08:41]

Is it "this" may be deleted, or "this" will be deleted? If will then no need for
nullptr at all? :)

[hubbe, 2016-05-11 00:13:51]

On 2016/05/11 00:08:41, DaleCurtis_OOO_Until_May_16 wrote:
> Is it "this" may be deleted, or "this" will be deleted? If will then no need
for
> nullptr at all? :)

It's a very fragile "will".
Since all readers go away when we call Fail(), all data providers should too.
But if any readers decide to hang around even after Fail(), or if the code ever
changes in the future...
Which is why I prefer to clear it explicitly, because if we don't, and for some
reason we don't get deleted, we may end up inserting bad data into the
multibuffer later.

[DaleCurtis, 2016-05-11 00:15:04]

sgtm

[ddorwin, 2016-05-11 02:58:53]

Description was changed from

==========
The crosss-origin checks in the multibuffer code are not sufficient, as they
only trigger when a redirect happens. Service-workers do not need redirects to
paste data from diffierent origins together.

This CL fixes that.

BUG=610747
==========

to

==========
The cross-origin checks in the multibuffer code are not sufficient, as they only
trigger when a redirect happens. Service-workers do not need redirects to paste
data from diffierent origins together.

This CL fixes that.

BUG=610747
==========

[hubbe, 2016-05-11 03:31:49]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-05-11 03:31:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1958123004/#ps80001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-05-11 03:32:10]

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

[commit-bot: I haz the power, 2016-05-11 04:29:13]

Description was changed from

==========
The cross-origin checks in the multibuffer code are not sufficient, as they only
trigger when a redirect happens. Service-workers do not need redirects to paste
data from diffierent origins together.

This CL fixes that.

BUG=610747
==========

to

==========
The cross-origin checks in the multibuffer code are not sufficient, as they only
trigger when a redirect happens. Service-workers do not need redirects to paste
data from diffierent origins together.

This CL fixes that.

BUG=610747
==========

[commit-bot: I haz the power, 2016-05-11 04:29:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-11 04:31:20]

Description was changed from

==========
The cross-origin checks in the multibuffer code are not sufficient, as they only
trigger when a redirect happens. Service-workers do not need redirects to paste
data from diffierent origins together.

This CL fixes that.

BUG=610747
==========

to

==========
The cross-origin checks in the multibuffer code are not sufficient, as they only
trigger when a redirect happens. Service-workers do not need redirects to paste
data from diffierent origins together.

This CL fixes that.

BUG=610747

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

[commit-bot: I haz the power, 2016-05-11 04:31:22]

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