service worker: Implement NavigationPreloadManager.setHeaderValue

service worker: Implement NavigationPreloadManager.setHeaderValue

BUG=649558
Committed: https://crrev.com/1d15ea07fc2554facd01404b28b35c137a7b24b8
Cr-Commit-Position: refs/heads/master@{#429439}

[falken, 2016-10-27 08:13:23]

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

[commit-bot: I haz the power, 2016-10-27 08:13:45]

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, 2016-10-27 09:49:25]

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

[commit-bot: I haz the power, 2016-10-27 09:49:26]

Dry run: This issue passed the CQ dry run.

[falken, 2016-10-28 01:48:29]

Description was changed from

==========
service worker: Implement NavigationPreloadManager.setHeaderValue

BUG=
==========

to

==========
service worker: Implement NavigationPreloadManager.setHeaderValue

BUG=649558
==========

[falken, 2016-10-28 01:51:53]

falken@chromium.org changed reviewers:
+ horo@chromium.org

[falken, 2016-10-28 01:51:54]

ptal

[falken, 2016-10-28 01:52:04]

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

[commit-bot: I haz the power, 2016-10-28 01:52:37]

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, 2016-10-28 03:13:10]

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

[commit-bot: I haz the power, 2016-10-28 03:13:11]

Dry run: This issue passed the CQ dry run.

[horo, 2016-10-31 03:32:52]

https://codereview.chromium.org/2451373003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.cpp
(right):

https://codereview.chromium.org/2451373003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.cpp:24:
const String& value) {
It is not written in the spec yet, but I think we should block '\r', '\n' and
'\0'.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...

[falken, 2016-10-31 08:32:08]

https://codereview.chromium.org/2451373003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.cpp
(right):

https://codereview.chromium.org/2451373003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.cpp:24:
const String& value) {
On 2016/10/31 03:32:52, horo wrote:
> It is not written in the spec yet, but I think we should block '\r', '\n' and
> '\0'.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...
> 

Done. I used IsValidHttpHeaderValue which looks like is what fetch() uses too.
Unfortunately the check on Chrome side is not exactly the same, but it shouldn't
be so bad. It's probably best to match fetch() at least.

[falken, 2016-10-31 08:33:14]

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

[commit-bot: I haz the power, 2016-10-31 08:33:25]

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, 2016-10-31 10:14:24]

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

[commit-bot: I haz the power, 2016-10-31 10:14:25]

Dry run: This issue passed the CQ dry run.

[horo, 2016-10-31 11:14:41]

lgtm

[falken, 2016-10-31 13:28:32]

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

[commit-bot: I haz the power, 2016-10-31 13:28:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[falken, 2016-10-31 13:30:24]

falken@chromium.org changed reviewers:
+ jwd@chromium.org, nick@chromium.org, tsepez@chromium.org

[falken, 2016-10-31 13:30:25]

thanks. adding owners:
- tsepez: IPC
- nick: bad_messages
- jwd: histograms

[commit-bot: I haz the power, 2016-10-31 17:12:54]

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

[commit-bot: I haz the power, 2016-10-31 17:12:55]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-10-31 18:35:36]

bad_message lgtm with some info

https://codereview.chromium.org/2451373003/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2451373003/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:839:
bad_message::SWDH_SET_NAVIGATION_PRELOAD_HEADER_BAD_REGISTRATION_ID);
FYI: from some recent experience with bad_message::ReceivedBadMessage, I've
noticed that the windows compiler will sometimes emit only one call to
ReceivedBadMessage, shared by multiple places where you call it (the various
call sites will load the enum value into a register and jump to the shared
callsite).

The resulting code is correct, but the stack traces might misattribute which of
the two lines was responsible for the call. The lesson here is: if you see these
happening in practice, trust the numeric code in the magic signature
(corresponding to the bad_message enum value, stored in a crash key) more than
the source view on crash/, to figure out which of the cases you're hitting.

[Tom Sepez, 2016-10-31 20:09:50]

https://codereview.chromium.org/2451373003/diff/160001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2451373003/diff/160001/content/common/service...
content/common/service_worker/service_worker_messages.h:216: std::string /*
header_value */)
Are there any constraints on the values that can be supplied that need to be
validated browser-side? 

For example, what happens if the value contains unexpected punctuation,
newlines, etc. -- can we get in a situation where a compromised renderer might
be able to inject unrelated headers?

[falken, 2016-11-01 00:35:28]

https://codereview.chromium.org/2451373003/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2451373003/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:839:
bad_message::SWDH_SET_NAVIGATION_PRELOAD_HEADER_BAD_REGISTRATION_ID);
On 2016/10/31 18:35:36, ncarter wrote:
> FYI: from some recent experience with bad_message::ReceivedBadMessage, I've
> noticed that the windows compiler will sometimes emit only one call to
> ReceivedBadMessage, shared by multiple places where you call it (the various
> call sites will load the enum value into a register and jump to the shared
> callsite).
> 
> The resulting code is correct, but the stack traces might misattribute which
of
> the two lines was responsible for the call. The lesson here is: if you see
these
> happening in practice, trust the numeric code in the magic signature
> (corresponding to the bad_message enum value, stored in a crash key) more than
> the source view on crash/, to figure out which of the cases you're hitting.

Thanks, that's good to know.

https://codereview.chromium.org/2451373003/diff/160001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2451373003/diff/160001/content/common/service...
content/common/service_worker/service_worker_messages.h:216: std::string /*
header_value */)
On 2016/10/31 20:09:49, Tom Sepez wrote:
> Are there any constraints on the values that can be supplied that need to be
> validated browser-side? 
> 
> For example, what happens if the value contains unexpected punctuation,
> newlines, etc. -- can we get in a situation where a compromised renderer might
> be able to inject unrelated headers?

Good question. Yes, the current patchset calls net::HttpUtil::IsValidHeaderValue
in ServiceWorkerDispatcherHost to reject strings that contain CR or LF or \0,
which is important since HTTP header fields are delimited by CRLF. As noted in
the comment, this is more lenient than the Blink-side check of
blink::isValidHTTPHeaderValue, which further restricts the string to
latin1-only. Ideally the checks would match perfectly but rolling our own
isValidHTTPHeaderValue just for this seems like overkill since there's already
one in net::HttpUtil, and filtering out CRLF and \0 seems enough to stop
injection of bad headers.

Actually I think renderers can already send arbitrary headers via
content::ResourceRequest, and these are also validated by
net::HttpUtil::IsValidHeaderValue via ResourceDispatcherHostImpl::BeginRequest
calling HttpRequestHeaders::AddHeadersFromString.

[haraken, 2016-11-01 02:16:26]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-11-01 02:16:27]

drive-by

https://codereview.chromium.org/2451373003/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadCallbacks.h
(right):

https://codereview.chromium.org/2451373003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadCallbacks.h:51:
SetNavigationPreloadHeaderCallbacks(ScriptPromiseResolver*);

Add explicit.

[falken, 2016-11-01 06:46:15]

Updated. Current review list:
- tsepez: messages
- jwd: histograms

https://codereview.chromium.org/2451373003/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadCallbacks.h
(right):

https://codereview.chromium.org/2451373003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadCallbacks.h:51:
SetNavigationPreloadHeaderCallbacks(ScriptPromiseResolver*);
On 2016/11/01 02:16:27, haraken wrote:
> 
> Add explicit.

Done, thanks.

[falken, 2016-11-02 03:28:20]

falken@chromium.org changed reviewers:
+ rkaplow@chromium.org
- jwd@chromium.org

[falken, 2016-11-02 03:28:21]

- tsepez: ping?
- timing out jwd (ooo?) adding rkaplow for metrics

[falken, 2016-11-02 08:00:27]

Patchset #12 (id:220001) has been deleted

[falken, 2016-11-02 08:04:18]

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

[commit-bot: I haz the power, 2016-11-02 08:04:37]

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, 2016-11-02 10:17:22]

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

[commit-bot: I haz the power, 2016-11-02 10:17:23]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2016-11-02 17:56:51]

lgtm

[Tom Sepez, 2016-11-02 19:12:16]

Thanks for the explanation, LGTM.

[falken, 2016-11-02 22:50:37]

Thank you reviewers.

[falken, 2016-11-02 22:50:46]

The CQ bit was checked by falken@chromium.org

[falken, 2016-11-02 22:50:46]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2451373003/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-02 22:51:47]

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-11-02 22:58:46]

Description was changed from

==========
service worker: Implement NavigationPreloadManager.setHeaderValue

BUG=649558
==========

to

==========
service worker: Implement NavigationPreloadManager.setHeaderValue

BUG=649558
==========

[commit-bot: I haz the power, 2016-11-02 22:58:48]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-11-02 23:02:43]

Description was changed from

==========
service worker: Implement NavigationPreloadManager.setHeaderValue

BUG=649558
==========

to

==========
service worker: Implement NavigationPreloadManager.setHeaderValue

BUG=649558

Committed: https://crrev.com/1d15ea07fc2554facd01404b28b35c137a7b24b8
Cr-Commit-Position: refs/heads/master@{#429439}
==========

[commit-bot: I haz the power, 2016-11-02 23:02:44]

Patchset 12 (id:??) landed as
https://crrev.com/1d15ea07fc2554facd01404b28b35c137a7b24b8
Cr-Commit-Position: refs/heads/master@{#429439}