Verify that the notification response contains sensible data

chrome/browser/notifications/notification_platform_bridge_mac.mm

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/notifications/notification_platform_bridge_mac.h"
 
 #include <utility>
 
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_util.h"
@@ skipping 166 matching lines @@
           objectForKey:notification_constants::kNotificationId]));
     }
   }
   return true;
 }
 
 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
 - (void)userNotificationCenter:(NSUserNotificationCenter*)center
        didActivateNotification:(NSUserNotification*)notification {
   NSDictionary* response =
       [NotificationResponseBuilder buildDictionary:notification];
+  if (!NotificationPlatformBridgeMac::VerifyNotificationData(response))
+    return;
 
   NSNumber* buttonIndex =
       [response objectForKey:notification_constants::kNotificationButtonIndex];
   NSNumber* operation =
       [response objectForKey:notification_constants::kNotificationOperation];
 
   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;
   if (!base::StringToInt64(persistentNotificationId, &persistentId)) {
     LOG(ERROR) << "Unable to convert notification ID: "
                << persistentNotificationId << " to integer.";
     return;
   }
   std::string profileId = base::SysNSStringToUTF8(
       [response objectForKey:notification_constants::kNotificationProfileId]);
@@ skipping 10 matching lines @@
           buttonIndex.intValue);
 }
 
 - (BOOL)userNotificationCenter:(NSUserNotificationCenter*)center
      shouldPresentNotification:(NSUserNotification*)nsNotification {
   // Always display notifications, regardless of whether the app is foreground.
   return YES;
 }
 
 @end