Repository: electron/electron
Pull request: fix: implement keychain item deletion in MAS safeStorage migration

Backport of #49815 to 40-x-y, plus an additional fix for a bug that was already resolved on main.

## Summary

Fixes three bugs in the MAS safeStorage keychain migration patch:

1. **`AddRandomPasswordToKeychain` used the unsuffixed account name** — On a clean first launch, the code created the keychain entry with `GetAccountName()` (e.g. `"MyApp"`) instead of `suffixed_account_name` (`"MyApp Key"`). This caused every first launch to create the wrong entry, then unnecessarily migrate it on the next launch.

2. **`FindGenericPassword` never populated the `SecKeychainItemRef` output parameter** — The SecItem API (`SecItemCopyMatching`) returns `CFTypeRef`, not `SecKeychainItemRef`. The parameter was silently ignored.

3. **`ItemDelete` was a no-op** — Always returned `noErr` without calling any deletion API.

## Changes

- Fixed `AddRandomPasswordToKeychain` to use `suffixed_account_name`
- Removed the `AppleSecKeychainItemRef` typedef and unused `item` parameter from `FindGenericPassword`
- Changed `ItemDelete` to accept `service_name` + `account_name` and implemented it using `SecItemDelete` with the modern SecItem API
- Fixed `#endif` comment mismatches and inconsistent preprocessor guards in the mock

## Disclosure

This PR was authored with Claude and manually tested/verified.

Notes: Fixed `safeStorage` failing to clean up legacy keychain entries when migrating to suffixed account names on macOS, and fixed first launch creating entries with the wrong (unsuffixed) account name.

Changed files:
filename: patches/chromium/feat_ensure_mas_builds_of_the_same_application_can_use_safestorage.patch
patch: @@ -12,7 +12,7 @@ We attempt to migrate the safe storage key from the old account, if that migrati
 Existing apps that aren't built for the app store should be unimpacted, there is one edge case where a user uses BOTH an AppStore and a darwin build of the same app only one will keep it's access to the safestorage key as during the migration we delete the old account.  This is an acceptable edge case as no one should be actively using two versions of the same app.
 
 diff --git a/components/os_crypt/common/keychain_password_mac.mm b/components/os_crypt/common/keychain_password_mac.mm
-index f19628cc0cdba39b232f55935e8eee9786b02a77..036b50f53e78bc21ed1e1d6dd876b50ab1e8f05d 100644
+index f19628cc0cdba39b232f55935e8eee9786b02a77..ab6ce9b1cf85029050a7497cd53813a03a46d408 100644
 --- a/components/os_crypt/common/keychain_password_mac.mm
 +++ b/components/os_crypt/common/keychain_password_mac.mm
 @@ -27,6 +27,12 @@
@@ -28,14 +28,13 @@ index f19628cc0cdba39b232f55935e8eee9786b02a77..036b50f53e78bc21ed1e1d6dd876b50a
  namespace {
  
  // These two strings ARE indeed user facing.  But they are used to access
-@@ -96,11 +102,51 @@
+@@ -96,18 +102,56 @@
                                    uma_result);
    };
  
 +  const std::string account_name_suffix = kAccountNameSuffix;
 +  const std::string suffixed_account_name = GetAccountName() + account_name_suffix;
    auto password =
--      keychain_->FindGenericPassword(GetServiceName(), GetAccountName());
 +      keychain_->FindGenericPassword(GetServiceName(), suffixed_account_name);
 +
 +  if (password.has_value()) {
@@ -53,10 +52,8 @@ index f19628cc0cdba39b232f55935e8eee9786b02a77..036b50f53e78bc21ed1e1d6dd876b50a
 +
 +  // If the suffixed account didn't exist, we should check if the legacy non-suffixed account
 +  // exists. If it does we can use that key and migrate it to the new account
-+  base::apple::ScopedCFTypeRef<SecKeychainItemRef> item_ref;
 +  password =
-+      keychain_->FindGenericPassword(GetServiceName(), GetAccountName(),
-+                                     item_ref.InitializeInto());
+       keychain_->FindGenericPassword(GetServiceName(), GetAccountName());
  
    if (password.has_value()) {
      uma_result = FindGenericPasswordResult::kPasswordFound;
@@ -70,7 +67,7 @@ index f19628cc0cdba39b232f55935e8eee9786b02a77..036b50f53e78bc21ed1e1d6dd876b50a
 +      // If we successfully made the suffixed account we can delete the old
 +      // account to ensure new apps don't try to use it and run into IAM
 +      // issues
-+      error = keychain_->ItemDelete(item_ref.get());
++      error = keychain_->ItemDelete(GetServiceName(), GetAccountName());
 +      if (error != noErr) {
 +        OSSTATUS_DLOG(ERROR, error) << "Keychain delete for legacy key failed";
 +      }
@@ -81,141 +78,97 @@ index f19628cc0cdba39b232f55935e8eee9786b02a77..036b50f53e78bc21ed1e1d6dd876b50a
      return std::string(base::as_string_view(*password));
    }
  
+   if (password.error() == errSecItemNotFound) {
+     uma_result = FindGenericPasswordResult::kPasswordNotFound;
+     return AddRandomPasswordToKeychain(*keychain_, GetServiceName(),
+-                                       GetAccountName());
++                                       suffixed_account_name);
+   }
+ 
+   OSSTATUS_LOG(ERROR, password.error()) << "Keychain lookup failed";
 diff --git a/crypto/apple/keychain.h b/crypto/apple/keychain.h
-index 1d2264a5229206f45d1a9bcb009d47180efa6a8b..1dcf2b1d09831012c7f5768a5c6193d529efc821 100644
+index 1d2264a5229206f45d1a9bcb009d47180efa6a8b..4472e5b605e09659bd75cd4797f073775fe4b354 100644
 --- a/crypto/apple/keychain.h
 +++ b/crypto/apple/keychain.h
-@@ -17,6 +17,14 @@
- 
- namespace crypto::apple {
- 
-+// TODO(smaddock): Migrate to SecItem* as part of
-+// https://issues.chromium.org/issues/40233280
-+#if BUILDFLAG(IS_IOS)
-+using AppleSecKeychainItemRef = void*;
-+#else
-+using AppleSecKeychainItemRef = SecKeychainItemRef;
-+#endif
-+
- // Wraps the KeychainServices API in a very thin layer, to allow it to be
- // mocked out for testing.
- 
-@@ -44,13 +52,18 @@ class CRYPTO_EXPORT Keychain {
-   // std::vector<uint8_t> arm is populated instead.
-   virtual base::expected<std::vector<uint8_t>, OSStatus> FindGenericPassword(
-       std::string_view service_name,
--      std::string_view account_name) const = 0;
-+      std::string_view account_name,
-+      AppleSecKeychainItemRef* item = nullptr) const = 0;
- 
-   virtual OSStatus AddGenericPassword(
-       std::string_view service_name,
+@@ -51,6 +51,11 @@ class CRYPTO_EXPORT Keychain {
        std::string_view account_name,
        base::span<const uint8_t> password) const = 0;
  
 +#if BUILDFLAG(IS_MAC)
-+  virtual OSStatus ItemDelete(AppleSecKeychainItemRef item) const = 0;
-+#endif  // !BUILDFLAG(IS_MAC)
++  virtual OSStatus ItemDelete(std::string_view service_name,
++                              std::string_view account_name) const = 0;
++#endif  // BUILDFLAG(IS_MAC)
 +
   protected:
    Keychain();
  };
 diff --git a/crypto/apple/keychain_secitem.h b/crypto/apple/keychain_secitem.h
-index eb74282adaba24ebd667f0ab3fc34dbe4cd8b527..7b91eb27489cece38eca719986255c5ec01c4bac 100644
+index eb74282adaba24ebd667f0ab3fc34dbe4cd8b527..0d25e49e2fa1b374d6867b8c602f7685a7f9498d 100644
 --- a/crypto/apple/keychain_secitem.h
 +++ b/crypto/apple/keychain_secitem.h
-@@ -17,12 +17,17 @@ class CRYPTO_EXPORT KeychainSecItem : public Keychain {
- 
-   base::expected<std::vector<uint8_t>, OSStatus> FindGenericPassword(
-       std::string_view service_name,
--      std::string_view account_name) const override;
-+      std::string_view account_name,
-+      AppleSecKeychainItemRef* item) const override;
- 
-   OSStatus AddGenericPassword(
+@@ -23,6 +23,11 @@ class CRYPTO_EXPORT KeychainSecItem : public Keychain {
        std::string_view service_name,
        std::string_view account_name,
        base::span<const uint8_t> password) const override;
 +
 +#if BUILDFLAG(IS_MAC)
-+  OSStatus ItemDelete(AppleSecKeychainItemRef item) const override;
-+#endif  // !BUILDFLAG(IS_MAC)
++  OSStatus ItemDelete(std::string_view service_name,
++                      std::string_view account_name) const override;
++#endif  // BUILDFLAG(IS_MAC)
  };
  
  }  // namespace crypto::apple
 diff --git a/crypto/apple/keychain_secitem.mm b/crypto/apple/keychain_secitem.mm
-index a8d50dd27db52526b0635c2b97f076df1994a6aa..e45f0d1079c8acfae55cf873e66ab3d9a10ad8ee 100644
+index a8d50dd27db52526b0635c2b97f076df1994a6aa..464c17909b9a554b269a70ea08771da6ec7ac011 100644
 --- a/crypto/apple/keychain_secitem.mm
 +++ b/crypto/apple/keychain_secitem.mm
-@@ -138,7 +138,8 @@
- 
- base::expected<std::vector<uint8_t>, OSStatus>
- KeychainSecItem::FindGenericPassword(std::string_view service_name,
--                                     std::string_view account_name) const {
-+                                     std::string_view account_name,
-+                                     AppleSecKeychainItemRef* item) const {
-   base::apple::ScopedCFTypeRef<CFDictionaryRef> query =
-       MakeGenericPasswordQuery(service_name, account_name);
- 
-@@ -165,4 +166,13 @@
+@@ -165,4 +165,18 @@
    return base::ToVector(base::apple::CFDataToSpan(password_data));
  }
  
 +#if BUILDFLAG(IS_MAC)
-+OSStatus KeychainSecItem::ItemDelete(AppleSecKeychainItemRef item) const {
-+  // TODO(smaddock): AppleSecKeychainItemRef aliases the deprecated
-+  // SecKeychainItemRef. Need to update this to accept a CFDictionary in the
-+  // case of SecItemDelete.
-+  return noErr;
++OSStatus KeychainSecItem::ItemDelete(std::string_view service_name,
++                                     std::string_view account_name) const {
++  NSDictionary* query = @{
++    CFToNSPtrCast(kSecClass) : CFToNSPtrCast(kSecClassGenericPassword),
++    CFToNSPtrCast(kSecAttrService) : base::SysUTF8ToNSString(service_name),
++    CFToNSPtrCast(kSecAttrAccount) : base::SysUTF8ToNSString(account_name),
++  };
++  base::apple::ScopedCFTypeRef<CFDictionaryRef> cf_query(
++      NSToCFOwnershipCast(query));
++  return SecItemDelete(cf_query.get());
 +}
 +#endif
 +
  }  // namespace crypto::apple
 diff --git a/crypto/apple/mock_keychain.cc b/crypto/apple/mock_keychain.cc
-index 080806aaf3fc10548b160850ad36ef3519ea2b6f..21f04059d67ba41118face6ee9327aa05e854362 100644
+index 080806aaf3fc10548b160850ad36ef3519ea2b6f..98625524b668b86c857d5a8910bfb53b3ab40575 100644
 --- a/crypto/apple/mock_keychain.cc
 +++ b/crypto/apple/mock_keychain.cc
-@@ -32,7 +32,8 @@ MockKeychain::~MockKeychain() = default;
- 
- base::expected<std::vector<uint8_t>, OSStatus>
- MockKeychain::FindGenericPassword(std::string_view service_name,
--                                  std::string_view account_name) const {
-+                                  std::string_view account_name,
-+                                  AppleSecKeychainItemRef* item) const {
-   IncrementKeychainAccessHistogram();
- 
-   // When simulating |noErr|, return canned |passwordData| and
-@@ -56,6 +57,10 @@ OSStatus MockKeychain::AddGenericPassword(
+@@ -56,6 +56,11 @@ OSStatus MockKeychain::AddGenericPassword(
    return noErr;
  }
  
-+OSStatus MockKeychain::ItemDelete(SecKeychainItemRef itemRef) const {
++OSStatus MockKeychain::ItemDelete(std::string_view service_name,
++                                  std::string_view account_name) const {
 +  return noErr;
 +}
 +
  std::string MockKeychain::GetEncryptionPassword() const {
    IncrementKeychainAccessHistogram();
    return kPassword;
 diff --git a/crypto/apple/mock_keychain.h b/crypto/apple/mock_keychain.h
-index 680efe0312c81449e069c19d9c6ef146da7834db..b49c2ba5f639344ab57e9f14c098effc38729d1f 100644
+index 680efe0312c81449e069c19d9c6ef146da7834db..102db6013b505fed32db176a90f5176118f62773 100644
 --- a/crypto/apple/mock_keychain.h
 +++ b/crypto/apple/mock_keychain.h
-@@ -36,13 +36,18 @@ class CRYPTO_EXPORT MockKeychain : public Keychain {
-   // Keychain implementation.
-   base::expected<std::vector<uint8_t>, OSStatus> FindGenericPassword(
-       std::string_view service_name,
--      std::string_view account_name) const override;
-+      std::string_view account_name,
-+      AppleSecKeychainItemRef* item) const override;
- 
-   OSStatus AddGenericPassword(
-       std::string_view service_name,
+@@ -43,6 +43,11 @@ class CRYPTO_EXPORT MockKeychain : public Keychain {
        std::string_view account_name,
        base::span<const uint8_t> password) const override;
  
-+#if !BUILDFLAG(IS_IOS)
-+  OSStatus ItemDelete(SecKeychainItemRef itemRef) const override;
-+#endif  // !BUILDFLAG(IS_IOS)
++#if BUILDFLAG(IS_MAC)
++  OSStatus ItemDelete(std::string_view service_name,
++                      std::string_view account_name) const override;
++#endif  // BUILDFLAG(IS_MAC)
 +
    // Returns the password that OSCrypt uses to generate its encryption key.
    std::string GetEncryptionPassword() const;