[MOO-2391] Improve SessionCookieStore#53
Closed
MxKevinBeqo wants to merge 5 commits into
Closed
Conversation
dccc1e6 to
69c6314
Compare
Contributor
Author
|
Closing as not required for 11.12. |
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.
Problem
Some users on iOS were seeing a system "Enter password" dialog appear over the splash screen during app startup. The root cause was a combination of two issues:
SessionCookieStoreserialised all no-expiry session cookies into a single keychain item. For users with many or large session cookies the blob could exceed iOS's reasonable-size threshold (~400 KB), producing an item that required user interaction to decrypt.What changed
ios/Modules/NativeCookieModule/SessionCookieStore.swiftChunked keychain storage (
set/get)Instead of writing the entire cookie blob as a single keychain item, blobs larger than 64 KB are now split into sequential 64 KB items. Blobs that fit within 64 KB continue to use the existing single-item format, preserving full backward compatibility with items written by older builds.
A commit marker item (
_chunkcount) is written last, after every chunk has been stored successfully. Its presence is the guarantee that all chunks were committed. If the process is killed mid-write the marker is absent, and the incomplete set is discarded cleanly on the next read.If any chunk write fails, all chunks already written are deleted before returning, so
restore()never reassembles a partial cookie set.On read, the code checks for the commit marker first. If it is present the chunks are reassembled in order. If it is absent the code falls back to reading the legacy bare key, so items written by older builds are transparently consumed on first launch after upgrade.
There is no longer any ceiling that can cause persistence to fail due to blob size.
Self-healing reads (
get)All keychain read queries now carry
kSecUseAuthenticationUIFail. This causes the keychain to returnerrSecInteractionNotAllowedrather than showing an authentication dialog if a stored item would require user interaction.On
errSecInteractionNotAllowedthe item is deleted andnilis returned, breaking any retry loop.Any other unreadable state (including deserialization failure of a corrupt or legacy blob) also deletes the item and returns
nil. This guarantees that a bad item is cleaned up duringrestore()on the first launch after the fix is deployed; the user loses the bridged session once and re-authenticates once.clear()Updated to remove both the legacy single-item key and all chunk keys, so a
clearAll()call from JavaScript leaves no orphaned chunk items in the keychain.persist(completion:)— DEBUG onlyA
#if DEBUG-only overload ofpersistthat accepts a completion closure. The publicpersist()signature is unchanged. The overload exists solely to let the test bridge resolve its Promise after the keychain write finishes rather than racing against it.Covered scenarios:
clearAll: removes cookies and any chunk marker after both small and large persists; does not throw on an empty store.Security notes
The five new methods added to
NativeCookieModuleare protected by#if DEBUGat every layer — Swift, Objective-C bridge, and they are exported under a clearly namedNativeCookieTestHelpersTypeScript object. They are not reachable from a production (Release configuration) binary. The CI harness build uses-configuration Debug, so they are available during test runs.