Allow whitelisted bundles to load in Chrome

chrome/common/mac/cfbundle_blocker_unittest.mm

+// Copyright (c) 2011 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/common/mac/cfbundle_blocker.h"
+
+#import <Foundation/Foundation.h>
+
+#include "base/basictypes.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chrome {
+namespace common {
+namespace mac {
+namespace {
+
+struct IsBundleAllowedTestcase {
+  NSString* bundle_id;
+  NSString* version;
+  bool allowed;
+};
+
+TEST(CFBundleBlockerTest, IsBundleAllowed) {
+  const IsBundleAllowedTestcase kTestcases[] = {
+    // Block things without a bundle ID.
+    { nil, nil, false },
+
+    // Block bundle IDs that aren't in the whitelist.
+    { @"org.chromium.Chromium.evil", nil, false },
+
+    // The AllowedBundle structure for Google Authetnicator BT doesn't
+    // require a version, so this should work equally well with any version
+    // including no version at all.
+    { @"com.google.osax.Google_Authenticator_BT", nil, true },
+    { @"com.google.osax.Google_Authenticator_BT", @"0.5.0.0", true },
+
+    // Typos should be blocked.
+    { @"com.google.osax.Google_Authenticator_B", nil, false },
+    { @"com.google.osax.Google_Authenticator_BQ", nil, false },
+    { @"com.google.osax.Google_Authenticator_BTQ", nil, false },
+    { @"com.google.osax", nil, false },
+    { @"com.google", nil, false },
+    { @"com", nil, false },
+    { @"", nil, false },
+
+    // MySpeed requires a version, so make sure that versions below don't work
+    // and versions above do.
+    { @"com.enounce.MySpeed.osax", nil, false },
+    { @"com.enounce.MySpeed.osax", @"", false },
+    { @"com.enounce.MySpeed.osax", @"1200", false },
+    { @"com.enounce.MySpeed.osax", @"1201", true },
+    { @"com.enounce.MySpeed.osax", @"1202", true },
+
+    // DefaultFolderX is whitelisted as com.stclairsoft.DefaultFolderX. Make
+    // sure that "child" IDs such as com.stclairsoft.DefaultFolderX.osax work.
+    // It uses a dotted versioning scheme, so test the version comparator out.
+    { @"com.stclairsoft.DefaultFolderX.osax", nil, false },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"", false },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"3.5.4", false },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.3.4", false },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.4.2", false },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.4.3", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.4.4", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.4.10", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.5", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.5.2", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.10", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"4.10.2", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"5", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"5.3", true },
+    { @"com.stclairsoft.DefaultFolderX.osax", @"5.3.2", true },
+
+    // Other "child" IDs that might want to load.
+    { @"com.stclairsoft.DefaultFolderX.CarbonPatcher", @"4.4.3", true },
+    { @"com.stclairsoft.DefaultFolderX.CocoaPatcher", @"4.4.3", true },
+  };
+
+  for (size_t index = 0; index < arraysize(kTestcases); ++index) {
+    const IsBundleAllowedTestcase& testcase = kTestcases[index];
+    NSString* bundle_id = testcase.bundle_id;
+    NSString* version = testcase.version;
+    NSString* version_print = version ? version : @"(nil)";
+    EXPECT_EQ(testcase.allowed, IsBundleAllowed(bundle_id, version))
+        << "index " << index
+        << ", bundle_id " << [bundle_id UTF8String]
+        << ", version " << [version_print UTF8String];
+  }
+}

[Scott Hess - ex-Googler, 2011/10/13 00:05:54]

Thank you for writing the test!  I think IsBundleAllowed() also became clearer
because it could just exit-early rather than maintaining nested state.

That said, I once got yelled at for writing a data-driven test like this.  I
personally consider it reasonable, but figured I should bring it up.  I think
the theory is whether this is really better than a bunch of:

EXPECT_FALSE(IsBundleAllowed(@"com.stclairsoft.DefaultFolderX.osax", @"3.5.4"));

I can kinda see the point.  I think your logging into failure is appropriate to
allow debuggers to see what's happening.

[Mark Mentovai, 2011/10/13 03:23:15]

shess wrote:
I agree that IsBundleAllowed is cleaner this way, and it’s a benefit that it’s
easily testable.
There’s longstanding precedent for the big array. ’Course, some of the precedent
is stuff that I wrote, but that’s one of the perks of being with the project for
a long time. There’s also non-Mark precedent in Chrome.

The chief advantage of writing a bunch of EXPECT_xxx is that they show you the
input and line number, but including the index and inputs overcomes that. I like
writing as much of the test as possible in a real language (like C++ or
Objective-C) and having the silly macro show up in only one location. Writing
EXPECT_xxx(IsBundleAllowed(…, …)); over and over again feels like a lot of
boilerplate over and over, and one of the cardinal rules of programming is that
if you find yourself doing the same thing more than once, maybe you ought to
refactor. Doing it my way eats up less horizontal space and makes it less likely
that things will need to be wrapped “creatively.”

+
+}  // namespace
+}  // namespace mac
+}  // namespace common
+}  // namespace chrome