fix: scope Keychain items by an explicit owner attribute

CHANGELOG.md

@@ -11,6 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **The Keychain backend could see another CLI's credentials** (#55). App scoping was a
+  dot-prefix match on the service string, and since the service is `{app}.{provider}` and
+  provider names may contain dots, app `com.acme.cli` could not distinguish its own
+  `pro.Adobe` item from app `com.acme.cli.pro`'s `Adobe` item. The neighbour could list,
+  export **and delete** credentials it did not own; reverse-DNS identifiers nest by
+  convention, so the collision is plausible rather than contrived. Items now record their
+  owning app in `kSecAttrComment` and every query filters on it exactly. Items written before
+  this release carry no owner and fall back to the old prefix test, so nothing already stored
+  becomes invisible — meaning **legacy items stay ambiguous until rewritten**, and only items
+  written from this release on are scoped exactly.
+
 - **`RestoreCredentialAsync_Preserves…` flaked on the OS-native backends** (#61). The test
   retried until one credential was visible and then asserted `IsSelected`, which comes from a
   *separate* store item written moments earlier — so the credential could become visible

src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs

@@ -71,6 +71,7 @@ public Task<string> AddCredentialAsync(string providerName, string accountName,
                 Label = accountName,
                 Description = environment,
                 Data = Encoding.UTF8.GetBytes(credentialData),
+                OwnerAppIdentifier = _appIdentifier,
             };
 
             AddItem(attrs);
@@ -91,7 +92,7 @@ public Task<string> AddCredentialAsync(string providerName, string accountName,
         /// Best-effort: returns after a bounded wait even if the item never
         /// appears, leaving any genuine failure to the caller's own lookup.
         /// </summary>
-        private static void ConfirmItemVisible(string service, string account)
+        private void ConfirmItemVisible(string service, string account)
         {
             const int maxAttempts = 20;
             for (var attempt = 1; attempt <= maxAttempts; attempt++)
@@ -344,6 +345,7 @@ public Task RestoreCredentialAsync(CredentialExport credential)
                 Label = credential.AccountName,
                 Description = credential.Environment,
                 Data = Encoding.UTF8.GetBytes(credential.CredentialData),
+                OwnerAppIdentifier = _appIdentifier,
             });
 
             if (credential.IsSelected)
@@ -391,6 +393,33 @@ private string ResolveStoredProviderName(string providerName)
             return providerName;
         }
 
+        /// <summary>
+        /// Whether <paramref name="item"/> belongs to this manager's app.
+        /// </summary>
+        /// <remarks>
+        /// Items written by this version carry the owning app identifier in
+        /// <c>kSecAttrGeneric</c> and are matched on it exactly. Items written before that
+        /// attribute existed carry nothing, so they fall back to the old service-prefix
+        /// check — without which every previously stored credential would vanish.
+        /// <para>
+        /// The fallback is not a complete fix and is not presented as one: a legacy item
+        /// belonging to an app whose identifier is a dot-prefix of another's stays
+        /// ambiguous until it is rewritten. Everything written from now on is scoped
+        /// exactly (#55).
+        /// </para>
+        /// </remarks>
+        private bool IsOwnedByThisApp(KeychainItem item)
+        {
+            if (!string.IsNullOrEmpty(item.OwnerAppIdentifier))
+            {
+                return string.Equals(item.OwnerAppIdentifier, _appIdentifier, StringComparison.Ordinal);
+            }
+
+            // Legacy item: no owner recorded, so fall back to the ambiguous prefix test.
+            return item.Service.StartsWith(_appIdentifier + ".", StringComparison.Ordinal)
+                || string.Equals(item.Service, SelectionsService, StringComparison.Ordinal);
+        }
+
         private string ServiceFor(string providerName) => $"{_appIdentifier}.{providerName}";
 
         private string SelectionsService => $"{_appIdentifier}{SelectionsServiceSuffix}";
@@ -431,6 +460,7 @@ private void WriteSelection(string providerName, string accountId)
                     Label = $"{_appIdentifier} active credential ({providerName})",
                     Description = string.Empty,
                     Data = bytes,
+                    OwnerAppIdentifier = _appIdentifier,
                 });
             }
             else
@@ -497,6 +527,12 @@ private sealed class KeychainItem
             public string? Description { get; init; }
             public DateTime? CreatedAt { get; init; }
             public byte[]? Data { get; init; }
+
+            /// <summary>
+            /// Owning app identifier, stored in <c>kSecAttrComment</c>. Null for items
+            /// written before this attribute existed — see <see cref="IsOwnedByThisApp"/>.
+            /// </summary>
+            public string? OwnerAppIdentifier { get; init; }
         }
 
         private static void AddItem(KeychainItem item)
@@ -510,13 +546,21 @@ private static void AddItem(KeychainItem item)
                 var descCf = Track(handles, NewCfString(item.Description ?? string.Empty));
                 var dataCf = Track(handles, NewCfData(item.Data ?? []));
 
+                // kSecAttrComment records which app owns the item, so scoping does not have
+                // to be inferred from the service string. That inference was ambiguous:
+                // the service is "{app}.{provider}" and provider names may contain dots, so
+                // app "com.acme.cli" could not tell its own "pro.Adobe" item from app
+                // "com.acme.cli.pro"'s "Adobe" item (#55).
+                var ownerCf = Track(handles, NewCfString(item.OwnerAppIdentifier ?? string.Empty));
+
                 var pairs = new List<(IntPtr, IntPtr)>
                 {
                     (Constants.KSecClass, Constants.KSecClassGenericPassword),
                     (Constants.KSecAttrService, serviceCf),
                     (Constants.KSecAttrAccount, accountCf),
                     (Constants.KSecAttrLabel, labelCf),
                     (Constants.KSecAttrDescription, descCf),
+                    (Constants.KSecAttrComment, ownerCf),
                     (Constants.KSecValueData, dataCf),
                 };
 
@@ -588,7 +632,12 @@ private static void DeleteItem(string service, string account)
             }
         }
 
-        private static KeychainItem? QuerySingleItem(string service, string account, bool includeData)
+        /// <summary>
+        /// Fetches one item by service + account, or null if it does not exist or is not
+        /// ours. See <see cref="IsOwnedByThisApp"/> — an exact service query is not on its
+        /// own proof of ownership, because the service string is ambiguous (#55).
+        /// </summary>
+        private KeychainItem? QuerySingleItem(string service, string account, bool includeData)
         {
             var handles = new List<IntPtr>();
             try
@@ -615,7 +664,11 @@ private static void DeleteItem(string service, string account)
 
                 try
                 {
-                    return DecodeItem(result);
+                    var decoded = DecodeItem(result);
+
+                    // Present but owned by another app: treat as not found rather than
+                    // handing back someone else's credential (#55).
+                    return decoded is not null && IsOwnedByThisApp(decoded) ? decoded : null;
                 }
                 finally
                 {
@@ -631,7 +684,7 @@ private static void DeleteItem(string service, string account)
             }
         }
 
-        private static List<KeychainItem> QueryItems(string service, bool includeData)
+        private List<KeychainItem> QueryItems(string service, bool includeData)
         {
             // Bulk query: request attributes only. Combining kSecReturnAttributes
             // + kSecReturnData + kSecMatchLimitAll in a single SecItemCopyMatching
@@ -661,7 +714,9 @@ private static List<KeychainItem> QueryItems(string service, bool includeData)
 
                 try
                 {
-                    stubs = DecodeArray(result);
+                    // The service string alone does not establish ownership, so filter even
+                    // though this query pinned an exact service (#55).
+                    stubs = [.. DecodeArray(result).Where(IsOwnedByThisApp)];
                 }
                 finally
                 {
@@ -732,9 +787,7 @@ private List<KeychainItem> QueryAllItemsForApp(bool includeData)
                 try
                 {
                     var items = DecodeArray(result);
-                    stubs = [.. items
-                        .Where(i => i.Service.StartsWith(_appIdentifier + ".", StringComparison.Ordinal)
-                            || string.Equals(i.Service, SelectionsService, StringComparison.Ordinal))];
+                    stubs = [.. items.Where(IsOwnedByThisApp)];
                 }
                 finally
                 {
@@ -832,6 +885,7 @@ private static List<KeychainItem> DecodeArray(IntPtr arrayOrDict)
                 Description = ReadCfStringAt(dict, Constants.KSecAttrDescription),
                 CreatedAt = ReadCfDateAt(dict, Constants.KSecAttrCreationDate),
                 Data = ReadCfDataAt(dict, Constants.KSecValueData),
+                OwnerAppIdentifier = ReadCfStringAt(dict, Constants.KSecAttrComment),
             };
         }
 

src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs

@@ -153,6 +153,18 @@ internal static class Constants
             internal static readonly IntPtr KSecAttrLabel = LoadSymbol(Security, "kSecAttrLabel");
             internal static readonly IntPtr KSecAttrDescription = LoadSymbol(Security, "kSecAttrDescription");
             internal static readonly IntPtr KSecAttrGeneric = LoadSymbol(Security, "kSecAttrGeneric");
+
+            /// <summary>
+            /// <c>kSecAttrComment</c>. Used to record which app owns an item.
+            /// </summary>
+            /// <remarks>
+            /// Deliberately not <c>kSecAttrGeneric</c>, which participates in the primary
+            /// key for generic-password items on some macOS versions — adding it to items
+            /// that previously lacked it would change uniqueness semantics and could let a
+            /// legacy item and a new one coexist for the same service + account.
+            /// <c>kSecAttrComment</c> is plain metadata and carries no such risk.
+            /// </remarks>
+            internal static readonly IntPtr KSecAttrComment = LoadSymbol(Security, "kSecAttrComment");
             internal static readonly IntPtr KSecAttrCreationDate = LoadSymbol(Security, "kSecAttrCreationDate");
             internal static readonly IntPtr KSecValueData = LoadSymbol(Security, "kSecValueData");
 

tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs

@@ -455,6 +455,62 @@ public async Task SelectCredentialAsync_ReplacingAnExistingSelection_LeavesExact
             Assert.False(listed.Single(c => c.AccountId == prod).IsSelected);
         }
 
+        [Fact]
+        [SupportedOSPlatform("macos")]
+        public async Task Items_AreInvisibleToAnAppWhoseIdentifierIsADotPrefix()
+        {
+            Assert.SkipWhen(_skip, SkipReason);
+
+            // "…cli" is a dot-prefix of "…cli.pro", and the service string is
+            // "{app}.{provider}" — so the neighbour's prefix test matched pro's item and it
+            // could list, export and delete another CLI's credentials (#55).
+            var neighbourId = $"test.nextiteration.sca.{Guid.NewGuid():N}";
+            var neighbour = new KeychainCredentialManager(neighbourId);
+            var pro = new KeychainCredentialManager(neighbourId + ".pro");
+
+            var id = await pro.AddCredentialAsync("Adobe", "prod", "Production", "{\"apiKey\":\"PRO\"}");
+            try
+            {
+                // pro's service is "{neighbourId}.pro.Adobe": it still starts with
+                // "{neighbourId}." so the old check matched, but the owner attribute does not.
+                Assert.Empty(await neighbour.GetProviderNamesAsync());
+                Assert.Empty(await neighbour.ExportCredentialsAsync());
+                Assert.Empty(await neighbour.ListCredentialsAsync("pro.Adobe"));
+
+                // And the owner still sees its own item.
+                Assert.Single(await RetryHelper.UntilAsync(() => pro.ExportCredentialsAsync(), r => r.Count == 1));
+            }
+            finally
+            {
+                _ = await pro.DeleteCredentialAsync(id);
+            }
+        }
+
+        [Fact]
+        [SupportedOSPlatform("macos")]
+        public async Task NeighbourCannotDeleteAnotherAppsCredential()
+        {
+            Assert.SkipWhen(_skip, SkipReason);
+
+            var neighbourId = $"test.nextiteration.sca.{Guid.NewGuid():N}";
+            var neighbour = new KeychainCredentialManager(neighbourId);
+            var pro = new KeychainCredentialManager(neighbourId + ".pro");
+
+            var id = await pro.AddCredentialAsync("Adobe", "prod", "Production", "{\"apiKey\":\"PRO\"}");
+            try
+            {
+                // The destructive half of the same bug: delete resolves the item through the
+                // same app-scoped lookup, so an unfixed prefix test let a neighbour remove
+                // credentials it does not own.
+                Assert.False(await neighbour.DeleteCredentialAsync(id));
+                Assert.Single(await RetryHelper.UntilAsync(() => pro.ExportCredentialsAsync(), r => r.Count == 1));
+            }
+            finally
+            {
+                _ = await pro.DeleteCredentialAsync(id);
+            }
+        }
+
         private sealed class FakeAdobeSummaryProvider : ICredentialSummaryProvider
         {
             public string ProviderName => "Adobe";