fix(auth)!: fix macOS Keychain consent prompt and storage contract - #1208
Merged
Conversation
A missing item threw .itemNotFound instead of returning nil, contradicting AuthLocalStorage's documented contract. Every fresh launch with no session logged an error, and try? at the call sites collapsed genuine Keychain failures into the same nil as 'no session'. Delete is now idempotent.
Adds a useDataProtectionKeychain flag threaded through every query, and stops sending kSecAttrAccessible on macOS unless it is enabled — the file-based Keychain ignores that attribute, so today it is inert.
Items were namespaced under a fixed 'supabase.gotrue.swift' service shared by every app embedding the SDK. The service now defaults to the host app's bundle identifier, matching KeychainAccess and SimpleKeychain. Existing sessions migrate on first read.
Review found the Keychain migration entry overstated what the prior research established: it asserted the file-based Keychain prompt still displays the literal old "supabase.gotrue.swift" string, but the service-name-drives-dialog-text claim was only ever community-sourced, not Apple-documented (per the design doc's Non-goals section, which is also why a kSecAttrLabel change was dropped from this work). Reword to describe only the documented mechanism instead: the prompt is tied to the app's designated requirement/ACL, governed by code-signing identity rather than kSecAttrService, per Apple TN3137.
remove(key:) previously exited before running the legacy-cleanup loop whenever the primary delete threw, leaving a stale legacy session in place. The next retrieve would migrate it back, signing a signed-out user back in. remove now always attempts every legacy location and re-throws the primary error afterward. retrieve(key:) previously discarded a successfully-read legacy value if the migration write to the primary location failed, surfacing the error and making the user appear signed out despite having a valid stored session. The write and the legacy delete are now sequenced so a failed write leaves the legacy copy in place (for the next read to retry) while still returning the value that was read. Also documents the best-effort semantics in the DocC comments, scopes an overclaim in V3_MIGRATION.md about cross-app Keychain collisions to the platforms where it actually applies, and extends FakeKeychain with writeError/deleteError to cover both fixes with new tests.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Sequence Diagram(s)sequenceDiagram
participant Demo
participant SupabaseClient
participant KeychainLocalStorage
participant Keychain
Demo->>SupabaseClient: Sign in or read session
SupabaseClient->>KeychainLocalStorage: Retrieve session
KeychainLocalStorage->>Keychain: Read primary location
Keychain-->>KeychainLocalStorage: Value or nil
KeychainLocalStorage->>Keychain: Probe legacy location
Keychain-->>KeychainLocalStorage: Legacy value
KeychainLocalStorage->>Keychain: Store migrated value
KeychainLocalStorage-->>SupabaseClient: Return session
SupabaseClient-->>Demo: Report session and Keychain state
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 31825654703Coverage increased (+0.5%) to 86.84%Details
Uncovered Changes
Coverage Regressions4 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Contributor
|
The following capabilities are marked
These may have been renamed, removed, or never registered. Please update the capability matrix. |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Apple-platform Auth storage to improve macOS Keychain behavior and align missing-item semantics.
Changes:
- Adds opt-in data-protection Keychain support.
- Uses bundle-scoped services with legacy migration.
- Makes missing reads and deletes non-errors, with tests and migration guidance.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
V3_MIGRATION.md |
Documents storage and Keychain changes. |
Sources/Auth/Internal/Keychain.swift |
Adds query options and status mapping. |
Sources/Auth/Storage/KeychainLocalStorage.swift |
Implements configuration and legacy migration. |
Tests/AuthTests/KeychainTests.swift |
Tests Keychain queries and status handling. |
Tests/AuthTests/KeychainLocalStorageTests.swift |
Tests migration and failure behavior. |
dictionary.txt |
Adds migration-document terminology. |
Suppressed comments (2)
Sources/Auth/Storage/KeychainLocalStorage.swift:127
- Swallowing a legacy deletion error lets
removereport success while an old credential remains. The nextretrievecan migrate that credential back into the primary location, resurrecting a signed-out session. Attempt every deletion, but retain and throw the first primary or legacy error after the loop, with a regression test for legacy-delete failure.
for legacy in legacyKeychains {
try? legacy.deleteItem(forKey: key)
Sources/Auth/Storage/KeychainLocalStorage.swift:163
- When data protection is enabled, this orders the pre-v3 hardcoded service before the immediately previous bundle-scoped file-based location appended below. Because migration intentionally tolerates failure to delete the old copy, a stale pre-v3 session can coexist with a newer bundle-scoped session; enabling the flag then migrates and returns the stale value without examining the newer one. Probe the same-service file-based location first and update the ordering test.
var candidates: [KeychainConfiguration] = [
KeychainConfiguration(
service: legacyKeychainService,
accessGroup: primary.accessGroup,
useDataProtectionKeychain: false
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address PR review feedback. The `try?` on the legacy probe was justified by "a failing probe must not break a fresh install", but that rationale went stale once `data(forKey:)` started mapping errSecItemNotFound to nil. An absent legacy item now reads as nil, so anything thrown from the probe is a genuine failure — a locked Keychain, a denied ACL prompt — and swallowing it reports "no session" for what is really an error. That is the exact failure this PR set out to fix, reintroduced one layer down. The `try?` on the two legacy *delete* sites is kept: best-effort cleanup of a legacy copy should not fail a sign-out or an otherwise successful migration, and both are documented as such. Also corrects two migration-guide overclaims: - The missing-key contract entry was written as if it fixed AuthLocalStorage protocol-wide. It fixes the Apple implementation only; WinCredLocalStorage still throws on ERROR_NOT_FOUND. Scoped the heading and body, and noted the Windows implementation is being dropped in v3 separately. - Enabling the data-protection Keychain does not avoid the consent prompt on the upgrade read: the migration deliberately probes the old file-based location, which can show the prompt one last time before the value moves.
Tr00d
approved these changes
Aug 14, 2026
Resolves a conflict in V3_MIGRATION.md. The conflict was purely positional: both sides appended new `##` sections at the end of the same region. main added the swift-log migration entry (#1210); this branch added three Keychain entries. Neither touches the other's subject matter, so both are kept — main's Logging section first, then the Keychain ones, so this branch still reads as "appended at the end". Checked for real interactions and found none: the Keychain entries make no reference to SupabaseLogger or the logging API that #1210 removed, and this branch changes no Codable conformance, so it does not intersect #1209's narrowing pass. dictionary.txt merged cleanly, keeping both sides' terms.
grdsdev
force-pushed
the
worktree/quiet-river-c505
branch
from
August 14, 2026 17:43
5142f40 to
47be30f
Compare
…c505 # Conflicts: # V3_MIGRATION.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes SDK-164.
The reported problem
macOS users of apps embedding this SDK see a Keychain consent prompt naming
supabase.gotrue.swift— a string with no visible connection to the host app. Users find it suspicious and some deny access (discussions#39584).Why the originally-filed fix would not have worked
SDK-164 proposed omitting the
servicevalue to "use the default application keychain". Research before implementing showed that does not fix the problem, and is unsafe:SecItemtargets the legacy file-based Keychain by default. Per TN3137: "The SecItem API can target either implementation. It defaults to targeting the file-based keychain." That is the Keychain with ACLs — and ACLs are what produce consent dialogs.kSecAttrServiceplays no part in that decision.KeychainSwiftnever setskSecAttrServiceat all, and its users hit the same dialog (keychain-swift#117).kSecAttrServiceis stored as the empty string, not as absent. Since the generic-password primary key isaccount + service + access group, we would share a namespace with any other library that also omits it — anddeleteItem/SecItemUpdateboth build frombaseQuery, so those queries could match and destroy items this SDK never wrote.What this PR does instead
1. Opt-in
useDataProtectionKeychain— the change that actually removes the dialog. The data-protection Keychain has no ACL model, so it shows no consent prompt. It is off by default deliberately: setting it unconditionally is exactly what caused the-34018 errSecMissingEntitlementregression (#455 → #516 → reverted in #574). Auth0'sSimpleKeychainindependently reaches the same conclusion — it never sets the attribute and exposes it only as a consumer opt-in.2. Default Keychain service is now the host app's bundle identifier. Previously every app embedding this SDK shared one hardcoded service. This is what "use the default app keychain" idiomatically means, and what both
KeychainAccess(the dependency dropped in #403) andSimpleKeychaindo. Falls back to the legacy constant when there is no bundle identifier, avoiding the force-unwrap bug SimpleKeychain still carries (#336). Existing sessions migrate automatically on first read — users are not signed out.3.
kSecAttrAccessibleis now guarded on macOS. It does not apply to the file-based Keychain, so today it is inert; it is now sent only when data protection is enabled. MatchesSimpleKeychain, which guards this and pins it with tests.4.
AuthLocalStorage.retrievenow returnsnilfor a missing key instead of throwing — a contract violation found along the way. The protocol always documented "returns nil if absent", but bothKeychainLocalStorageandWinCredLocalStoragethrew.itemNotFound. Consequences that are now fixed:error "Failed to retrieve session"— a normal empty state reported as a failure.CodeVerifierStorage's "Code verifier not found" debug branch was unreachable.try?at the call sites collapsed a locked Keychain, an ACL denial, or an entitlement failure into the samenilas "no session" — turning a real error into a silent sign-out.remove(key:)is also idempotent now.Migration safety
retrievereads the current location first and, only on a miss, probes legacy locations, moving whatever it finds. Migration is write-then-delete, so there is no data-loss window. The explicitinit(service:)never migrates, so a caller who deliberately chose a service does not inherit data from the old hardcoded one. Two failure modes were closed during review: a failed primary delete no longer skips legacy cleanup (which could resurrect a signed-out session), and a failed primary write no longer discards a successfully-read legacy session.Testing
1128 tests pass. Query construction and migration logic are covered by unit tests and an injected fake keychain.
Known limitation, stated plainly: there are no round-trip tests against the real Keychain, and there cannot be. An SPM test bundle has no provisioning profile, so
kSecUseDataProtectionKeychain = truefails with the very-34018this design works around. That is why the internalKeychainProtocoland fake exist. Verifying that the consent prompt is actually gone requires a signed macOS sample app and remains manual — this should be confirmed before anyone is advised to enable the flag.Breaking changes
Three entries added to
V3_MIGRATION.mdcovering the service default, theretrieve/removecontract, and the new opt-in flag with its entitlement hazard.Follow-ups (not in this PR)
WinCredLocalStoragelooks substantially broken:retrievebuilds its target as"\(service)\\\(key))"with a stray)thatstorelacks, so reads cannot match writes; it escapes a pointer out ofwithCString's closure; andstorecallswithUnsafeMutableBytes(of:)on aDatavalue, capturing the struct's bytes rather than its contents.origin/grdsdev/auth-biometrics-keychainadds a secondSecItemimplementation that duplicates this logic; it should rebase onto this and adoptKeychainProtocol.StorageMigrationpasses re-read storage on every sessionget.scripts/test-docs.shmasks hardxcodebuildfailures via|| true.