Verify that the notification response contains sensible data

chrome/browser/notifications/notification_platform_bridge_mac.mm

Index: chrome/browser/notifications/notification_platform_bridge_mac.mm
diff --git a/chrome/browser/notifications/notification_platform_bridge_mac.mm b/chrome/browser/notifications/notification_platform_bridge_mac.mm
index b7d52802ecff3ac8fa67e90d9671709d495e5453..c06efc86f3607f7132993ebba9ec8ba41305b670 100644
--- a/chrome/browser/notifications/notification_platform_bridge_mac.mm
+++ b/chrome/browser/notifications/notification_platform_bridge_mac.mm
@@ -184,6 +184,48 @@ bool NotificationPlatformBridgeMac::SupportsNotificationCenter() const {
   return true;
 }
 
+// static
+bool NotificationPlatformBridgeMac::VerifyNotificationData(
+    NSDictionary* response) {
+  NSNumber* buttonIndex =
+      [response objectForKey:notification_constants::kNotificationButtonIndex];
+  NSNumber* operation =
+      [response objectForKey:notification_constants::kNotificationOperation];
+
+  std::string notificationOrigin = base::SysNSStringToUTF8(

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

unused?

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

Done.

+      [response objectForKey:notification_constants::kNotificationOrigin]);
+  NSString* notificationId =
+      [response objectForKey:notification_constants::kNotificationId];
+  NSString* profileId =
+      [response objectForKey:notification_constants::kNotificationProfileId];
+
+  if (buttonIndex.intValue < -1 ||

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

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?

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

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:

+      buttonIndex.intValue >=
+          static_cast<int>(blink::kWebNotificationMaxActions)) {
+    LOG(ERROR) << "Invalid number of buttons supplied " << buttonIndex.intValue;
+    return false;
+  }
+
+  if (operation.unsignedIntValue >
+      notification_operation_common::NOTIFICATION_OPERATION_MAX) {
+    LOG(ERROR) << operation.unsignedIntValue
+               << " Does not correspond to a valid operation.";

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

nit: Does -> does

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

Done.

+    return false;
+  }
+
+  if (notificationId.length <= 0) {
+    LOG(ERROR) << "NotificationId not provided";
+    return false;
+  }
+
+  if (profileId.length <= 0) {
+    LOG(ERROR) << "ProfileId not provided";
+    return false;
+  }
+
+  return true;
+}
+
 // /////////////////////////////////////////////////////////////////////////////
 
 @implementation NotificationCenterDelegate
@@ -191,6 +233,8 @@ bool NotificationPlatformBridgeMac::SupportsNotificationCenter() const {
        didActivateNotification:(NSUserNotification*)notification {
   NSDictionary* response =
       [NotificationResponseBuilder buildDictionary:notification];
+  if (!NotificationPlatformBridgeMac::VerifyNotificationData(response))
+    return;
 
   NSNumber* buttonIndex =
       [response objectForKey:notification_constants::kNotificationButtonIndex];
@@ -199,8 +243,8 @@ bool NotificationPlatformBridgeMac::SupportsNotificationCenter() const {
 
   std::string notificationOrigin = base::SysNSStringToUTF8(
       [response objectForKey:notification_constants::kNotificationOrigin]);
-  NSString* notificationId = [notification.userInfo
-      objectForKey:notification_constants::kNotificationId];
+  NSString* notificationId =
+      [response objectForKey:notification_constants::kNotificationId];
   std::string persistentNotificationId =
       base::SysNSStringToUTF8(notificationId);
   int64_t persistentId;