Check the format of an applicationServerKey when used to register a push subscription.

Check the format of an applicationServerKey when used to register a push subscription.

The Web Push specificatoion requires applicationServerKeys to be valid VAPID keys,
but we have not previous enforced that, which has led to issues where web developers
attempt to subscribe with invalid keys. The error eventually returned is not
informative.

This CL adds code in Blink to do some basic sanity checking that the provided value is
syntactically valid. Although the spec only allows p256 keys to be valid, this code
also allows the web developer to subscribe with a numeric sender ID as an argument to
subscribe() instead of specifying it in the manifest file, but checks that the ID provided
is a number.

This CL also adds testing for the new syntax checking.

BUG=636022
Committed: https://crrev.com/3f9e6105d28fbb930d90e38c23d07a9af9b53dbc
Cr-Commit-Position: refs/heads/master@{#425649}

[harkness, 2016-10-11 14:31:27]

Description was changed from

==========
Moved format checking to the provided applicationServerKey.

BUG=636022
==========

to

==========
Check the format of an applicationServerKey when used to register a push
subscription.

Although the only applicationServerKeys which should be valid are either VAPID
keys
or numeric sender ids, we have not previous enforced that. This adds a check in
Blink
to do some basic sanity checking that the provided value is valid. This won't
check
if the provided sender ID exists or is correct, but will at least check syntax.

This also adds testing for the new syntax checking.

BUG=636022
==========

[harkness, 2016-10-11 14:32:30]

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

[harkness, 2016-10-11 14:32:31]

This ended up being completely disjoint code from the original, so I just
created a new review and I'll delete the other one.

[Peter Beverloo, 2016-10-11 15:10:41]

Thanks!

Meta comment: Please don't delete CLs. Just close them.

One comment on the CLs description:

    """Although the only applicationServerKeys which should be valid are
    either VAPID keys or numeric sender ids, we have not previous enforced
    that."""

This could use some clarification. The specification *only* allows VAPID
keys, so the "should" is ambiguous. We support Sender IDs here as well
because the benefit of doing so is significant, as it removes the failure
case where the manifest can't be downloaded.

https://codereview.chromium.org/2411733002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2411733002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:468:
IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribeFailureLongKey) {
The following three tests should be layout tests.

In general, we use layout tests for all Web Exposed behaviour unless we
really can't, for example because it's depending on mocked behaviour.

That's not the case here -- the exception is thrown by Blink, and can be
observed by layout tests directly.

https://codereview.chromium.org/2411733002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp
(right):

https://codereview.chromium.org/2411733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp:49:
WebString inputWebString = WebString::fromLatin1(input, length);
Could we validate the input before converting it to a WebString? It seems to me
that something like the following would do the trick:

=========================
const bool isVapid = length == 65 && input[0] == 0x04;
const bool isSenderId =
    std::find_if_not(input, input + length,
                     &WTF::isASCIIDigit<unsigned char>) == input + length;

if (length > 0 && (isVapid || isSenderId))
  return WebString::fromLatin1(input, length);

// create exception
=========================

That avoids unnecessarily creating the WebString, as well as usage of
std::string and the third copy of the same data you have on line 57. It also
avoids needing the IsNotDigit function.

(We can't use std::string here anyway.)

[harkness, 2016-10-14 13:53:00]

Description was changed from

==========
Check the format of an applicationServerKey when used to register a push
subscription.

Although the only applicationServerKeys which should be valid are either VAPID
keys
or numeric sender ids, we have not previous enforced that. This adds a check in
Blink
to do some basic sanity checking that the provided value is valid. This won't
check
if the provided sender ID exists or is correct, but will at least check syntax.

This also adds testing for the new syntax checking.

BUG=636022
==========

to

==========
Check the format of an applicationServerKey when used to register a push
subscription.

The Web Push specificatoion requires applicationServerKeys to be valid VAPID
keys,
but we have not previous enforced that, which has led to issues where web
developers
attempt to subscribe with invalid keys. The error eventually returned is not
informative.

This CL adds code in Blink to do some basic sanity checking that the provided
value is
syntactically valid. Although the spec only allows p256 keys to be valid, this
code
also allows the web developer to subscribe with a numeric sender ID as an
argument to
subscribe() instead of specifying it in the manifest file, but checks that the
ID provided
is a number.

This CL also adds testing for the new syntax checking.

BUG=636022
==========

[harkness, 2016-10-14 13:53:55]

Re: meta-comment  - previous CL closed, not deleted.

CL description updated. I think this captures the intention.

https://codereview.chromium.org/2411733002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2411733002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:468:
IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribeFailureLongKey) {
On 2016/10/11 15:10:41, Peter Beverloo wrote:
> The following three tests should be layout tests.
> 
> In general, we use layout tests for all Web Exposed behaviour unless we
> really can't, for example because it's depending on mocked behaviour.
> 
> That's not the case here -- the exception is thrown by Blink, and can be
> observed by layout tests directly.

Done.

https://codereview.chromium.org/2411733002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp
(right):

https://codereview.chromium.org/2411733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp:49:
WebString inputWebString = WebString::fromLatin1(input, length);
On 2016/10/11 15:10:41, Peter Beverloo wrote:
> Could we validate the input before converting it to a WebString? It seems to
me
> that something like the following would do the trick:
> 
> =========================
> const bool isVapid = length == 65 && input[0] == 0x04;
> const bool isSenderId =
>     std::find_if_not(input, input + length,
>                      &WTF::isASCIIDigit<unsigned char>) == input + length;
> 
> if (length > 0 && (isVapid || isSenderId))
>   return WebString::fromLatin1(input, length);
> 
> // create exception
> =========================
> 
> That avoids unnecessarily creating the WebString, as well as usage of
> std::string and the third copy of the same data you have on line 57. It also
> avoids needing the IsNotDigit function.
> 
> (We can't use std::string here anyway.)

Done.

[Peter Beverloo, 2016-10-14 14:36:30]

lgtm % the formatting change we discussed

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/push_messaging/application-server-key-format-test.html
(right):

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/push_messaging/application-server-key-format-test.html:40:
e.message, /The provided applicationServerKey is not valid./);
Why don't you use includes() like you use on line 27 here? (Dito on lines 51 and
64.)

assert_true(e.message.includes('The provided applicationServerKey is not
valid.'));

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/push_messaging/resources/test-helpers.js
(right):

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/push_messaging/resources/test-helpers.js:38:
var swRegistration;
no "var"

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp
(right):

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp:45:
const bool isVapid = length == 65 && input && *input == 0x04;
Is checking for |input| required? When would length == 65 and we have a NULL
|input|?

(It seems to me like crashing is a good thing if we ever mess this up :-). Your
test coverage ensures we do.)

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp:47:
(length > 0) && (length < kMaxApplicationServerKeyLength) &&
No parenthesis around the two comparisons on this line.

[harkness, 2016-10-14 15:13:47]

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/push_messaging/application-server-key-format-test.html
(right):

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/push_messaging/application-server-key-format-test.html:40:
e.message, /The provided applicationServerKey is not valid./);
On 2016/10/14 14:36:30, Peter Beverloo wrote:
> Why don't you use includes() like you use on line 27 here? (Dito on lines 51
and
> 64.)
> 
> assert_true(e.message.includes('The provided applicationServerKey is not
> valid.'));

Done.

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/push_messaging/resources/test-helpers.js
(right):

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/push_messaging/resources/test-helpers.js:38:
var swRegistration;
On 2016/10/14 14:36:30, Peter Beverloo wrote:
> no "var"

Done.

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp
(right):

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp:45:
const bool isVapid = length == 65 && input && *input == 0x04;
On 2016/10/14 14:36:30, Peter Beverloo wrote:
> Is checking for |input| required? When would length == 65 and we have a NULL
> |input|?
> 
> (It seems to me like crashing is a good thing if we ever mess this up :-).
Your
> test coverage ensures we do.)

Done.

https://codereview.chromium.org/2411733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.cpp:47:
(length > 0) && (length < kMaxApplicationServerKeyLength) &&
On 2016/10/14 14:36:30, Peter Beverloo wrote:
> No parenthesis around the two comparisons on this line.

Done.

[harkness, 2016-10-17 08:57:48]

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

[commit-bot: I haz the power, 2016-10-17 08:58:09]

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-17 10:20:10]

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

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

Dry run: This issue passed the CQ dry run.

[harkness, 2016-10-17 10:28:17]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-10-17 10:28:17]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2411733002/#ps120001
(title: "More formatting")

[commit-bot: I haz the power, 2016-10-17 10:28:40]

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-17 10:34:47]

Description was changed from

==========
Check the format of an applicationServerKey when used to register a push
subscription.

The Web Push specificatoion requires applicationServerKeys to be valid VAPID
keys,
but we have not previous enforced that, which has led to issues where web
developers
attempt to subscribe with invalid keys. The error eventually returned is not
informative.

This CL adds code in Blink to do some basic sanity checking that the provided
value is
syntactically valid. Although the spec only allows p256 keys to be valid, this
code
also allows the web developer to subscribe with a numeric sender ID as an
argument to
subscribe() instead of specifying it in the manifest file, but checks that the
ID provided
is a number.

This CL also adds testing for the new syntax checking.

BUG=636022
==========

to

==========
Check the format of an applicationServerKey when used to register a push
subscription.

The Web Push specificatoion requires applicationServerKeys to be valid VAPID
keys,
but we have not previous enforced that, which has led to issues where web
developers
attempt to subscribe with invalid keys. The error eventually returned is not
informative.

This CL adds code in Blink to do some basic sanity checking that the provided
value is
syntactically valid. Although the spec only allows p256 keys to be valid, this
code
also allows the web developer to subscribe with a numeric sender ID as an
argument to
subscribe() instead of specifying it in the manifest file, but checks that the
ID provided
is a number.

This CL also adds testing for the new syntax checking.

BUG=636022
==========

[commit-bot: I haz the power, 2016-10-17 10:34:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-17 10:38:08]

Description was changed from

==========
Check the format of an applicationServerKey when used to register a push
subscription.

The Web Push specificatoion requires applicationServerKeys to be valid VAPID
keys,
but we have not previous enforced that, which has led to issues where web
developers
attempt to subscribe with invalid keys. The error eventually returned is not
informative.

This CL adds code in Blink to do some basic sanity checking that the provided
value is
syntactically valid. Although the spec only allows p256 keys to be valid, this
code
also allows the web developer to subscribe with a numeric sender ID as an
argument to
subscribe() instead of specifying it in the manifest file, but checks that the
ID provided
is a number.

This CL also adds testing for the new syntax checking.

BUG=636022
==========

to

==========
Check the format of an applicationServerKey when used to register a push
subscription.

The Web Push specificatoion requires applicationServerKeys to be valid VAPID
keys,
but we have not previous enforced that, which has led to issues where web
developers
attempt to subscribe with invalid keys. The error eventually returned is not
informative.

This CL adds code in Blink to do some basic sanity checking that the provided
value is
syntactically valid. Although the spec only allows p256 keys to be valid, this
code
also allows the web developer to subscribe with a numeric sender ID as an
argument to
subscribe() instead of specifying it in the manifest file, but checks that the
ID provided
is a number.

This CL also adds testing for the new syntax checking.

BUG=636022

Committed: https://crrev.com/3f9e6105d28fbb930d90e38c23d07a9af9b53dbc
Cr-Commit-Position: refs/heads/master@{#425649}
==========

[commit-bot: I haz the power, 2016-10-17 10:38:09]

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