Verify that the notification response contains sensible data

Verify that the notification response contains sensible data

BUG=571056
Committed: https://crrev.com/4ca5df8a61616c5332a9b016f7c2fcf8a8561797
Cr-Commit-Position: refs/heads/master@{#403691}

[Miguel Garcia, 2016-06-28 17:22:08]

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

[Peter Beverloo, 2016-06-28 23:35:52]

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.h (right):

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.h:43: // Returns
true if all the checks pass, false otherwise.
There is a contrast between "some sanity checks" and "real production checks".
How about generalizing as:

  // Validates contents of the |response| dictionary as received from the system
  // when a notification gets activated.

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.h:44: static bool
VerifyNotificationData(NSDictionary* response);
Please annotate this with WARN_UNUSED_RESULT from compiler_specific.h since it's
security-sensitive.

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:195:
std::string notificationOrigin = base::SysNSStringToUTF8(
unused?

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:202: if
(buttonIndex.intValue < -1 ||
I think we're missing `nil` protection here and on line 209?

If the |response| dictionary does not contain the `objectForKey` key it returns
`nil`. (I.e. buttonIndex = nil.) That's fine by itself, but:

  (1) Is reading `nil`.intValue and `nil`.unsignedIntValue safe?
  (2) If we follow the logic of a `nil` NSString's `length` property defaulting
to zero, would these default to zero too, thus passing the checks for a missing
property?

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:212: << " Does
not correspond to a valid operation.";
nit: Does -> does

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
(right):

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:33: } 
// namespace
nit: the first and final line of the anonymous namespace should be blank lines.

https://codereview.chromium.org/2105863002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:64: }
Add tests for missing `operation` and `buttonIndex` fields?

[Miguel Garcia, 2016-06-30 10:42:00]

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
File chrome/browser/notifications/notification_platform_bridge_mac.h (right):

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.h:43: // Returns
true if all the checks pass, false otherwise.
On 2016/06/28 23:35:52, Peter Beverloo wrote:
> There is a contrast between "some sanity checks" and "real production checks".
> How about generalizing as:
> 
>   // Validates contents of the |response| dictionary as received from the
system
>   // when a notification gets activated.

Done.

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.h:44: static bool
VerifyNotificationData(NSDictionary* response);
On 2016/06/28 23:35:52, Peter Beverloo wrote:
> Please annotate this with WARN_UNUSED_RESULT from compiler_specific.h since
it's
> security-sensitive.

Done.

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:195:
std::string notificationOrigin = base::SysNSStringToUTF8(
On 2016/06/28 23:35:52, Peter Beverloo wrote:
> unused?

Done.

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:202: if
(buttonIndex.intValue < -1 ||
Yeah I think the right way to do it is ensure that the keys exist in the first
place. I will add those checks.

While at it I added a check to ensure that the origin is valid if it's present.

On 2016/06/28 23:35:52, Peter Beverloo wrote:
> I think we're missing `nil` protection here and on line 209?
> 
> If the |response| dictionary does not contain the `objectForKey` key it
returns
> `nil`. (I.e. buttonIndex = nil.) That's fine by itself, but:
> 
>   (1) Is reading `nil`.intValue and `nil`.unsignedIntValue safe?
>   (2) If we follow the logic of a `nil` NSString's `length` property
defaulting
> to zero, would these default to zero too, thus passing the checks for a
missing
> property?

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:212: << " Does
not correspond to a valid operation.";
On 2016/06/28 23:35:52, Peter Beverloo wrote:
> nit: Does -> does

Done.

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
(right):

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:33: } 
// namespace
On 2016/06/28 23:35:52, Peter Beverloo wrote:
> nit: the first and final line of the anonymous namespace should be blank
lines.

Done.

https://chromiumcodereview.appspot.com/2105863002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:64: }
On 2016/06/28 23:35:52, Peter Beverloo wrote:
> Add tests for missing `operation` and `buttonIndex` fields?

Done.

[Peter Beverloo, 2016-07-01 13:31:43]

rs lgtm, but you probably want Robert to take a look at this too since using
data received from the system won't be rare

https://codereview.chromium.org/2105863002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2105863002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:194: ![response
objectForKey:notification_constants::kNotificationProfileId]) {
+kNotificationIncognito

https://codereview.chromium.org/2105863002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:222: LOG(ERROR)
<< "NotificationId not provided";
"not provided" -> "is empty" perhaps, since we now verify that it's included in
the dictionary? Dito on line 227

[Miguel Garcia, 2016-07-01 13:37:34]

miguelg@chromium.org changed reviewers:
+ rsesek@chromium.org

[Miguel Garcia, 2016-07-01 13:37:34]

Of course, I was going to add him anyway, I am not intending to modify a single
line of cocoa code without him saying it's ok :)

[Robert Sesek, 2016-07-01 15:35:57]

https://codereview.chromium.org/2105863002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2105863002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:195: LOG(ERROR)
<< "Missing required key";
Should we ship this log information, or should it be DLOG/VLOG ?

https://codereview.chromium.org/2105863002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:199: NSNumber*
buttonIndex =
naming: under_scores since this is in C++

[Miguel Garcia, 2016-07-04 15:18:56]

Given that Peter LGTMed and Robert didn't have any blocking comments plus the US
holidays I am going to land this but please do reach back if you think there are
extra comments to work on.

https://chromiumcodereview.appspot.com/2105863002/diff/40001/chrome/browser/n...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://chromiumcodereview.appspot.com/2105863002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:194: ![response
objectForKey:notification_constants::kNotificationProfileId]) {
On 2016/07/01 13:31:43, Peter Beverloo wrote:
> +kNotificationIncognito

Done.

https://chromiumcodereview.appspot.com/2105863002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:195: LOG(ERROR)
<< "Missing required key";
On 2016/07/01 15:35:57, Robert Sesek wrote:
> Should we ship this log information, or should it be DLOG/VLOG ?

Yeah I think we should ship it, it's a pretty bad issue if we find these kind of
problems so better ship as much information as possible.

https://chromiumcodereview.appspot.com/2105863002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:199: NSNumber*
buttonIndex =
On 2016/07/01 15:35:57, Robert Sesek wrote:
> naming: under_scores since this is in C++

Done.

https://chromiumcodereview.appspot.com/2105863002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:222: LOG(ERROR)
<< "NotificationId not provided";
On 2016/07/01 13:31:43, Peter Beverloo wrote:
> "not provided" -> "is empty" perhaps, since we now verify that it's included
in
> the dictionary? Dito on line 227

Done.

[Miguel Garcia, 2016-07-04 15:19:43]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-07-04 15:19:43]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2105863002/#ps60001
(title: "review")

[commit-bot: I haz the power, 2016-07-04 15:19:56]

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-07-04 16:08:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-04 16:10:41]

Description was changed from

==========
Verify that the notification response contains sensible data

BUG=571056
==========

to

==========
Verify that the notification response contains sensible data

BUG=571056

Committed: https://crrev.com/4ca5df8a61616c5332a9b016f7c2fcf8a8561797
Cr-Commit-Position: refs/heads/master@{#403691}
==========

[commit-bot: I haz the power, 2016-07-04 16:10:42]

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

[Robert Sesek, 2016-07-06 16:51:39]

lgtm