Skip to content

feat: Swift and Kotlin credential storage tests#271

Open
lukejmann wants to merge 15 commits intomainfrom
swift-and-kotlin-foreign-tests-v2
Open

feat: Swift and Kotlin credential storage tests#271
lukejmann wants to merge 15 commits intomainfrom
swift-and-kotlin-foreign-tests-v2

Conversation

@lukejmann
Copy link
Copy Markdown
Contributor

@lukejmann lukejmann commented Mar 5, 2026

Note

High Risk
Introduces new encryption/key-management and on-disk blob storage code for Android and iOS, which is security- and data-integrity sensitive. Incorrect error handling or atomic-write semantics could lead to lost/undecryptable credentials.

Overview
Adds end-to-end storage coverage for WalletKit across Kotlin and Swift.

On Android, introduces AndroidDeviceKeystore (AES-GCM via AndroidKeyStore), AndroidAtomicBlobStore (atomic temp-file writes), and AndroidStorageProvider/WalletKitStorage.defaultProvider to wire paths, keystore, and blob store under a worldid directory.

On iOS/Swift, adds IOSDeviceKeystore (AES-GCM with Keychain-backed symmetric key plus entitlement-missing fallback), IOSAtomicBlobStore (atomic file writes), and IOSStorageProvider/WalletKitStorage.makeDefaultProvider, along with new Swift and Kotlin test suites covering blob read/write/delete, credential store init/persist/reopen + Merkle cache TTL behavior, and renaming/polishing of logging tests (including more robust Swift polling).

Written by Cursor Bugbot for commit c1bc228. This will update automatically on new commits. Configure here.

…ign-tests-v2

Made-with: Cursor

# Conflicts:
#	swift/tests/WalletKitTests/LoggingTests.swift
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@paolodamico
Copy link
Copy Markdown
Contributor

@codex review

}
}

public func openSealed(associatedData: Data, ciphertext: Data) throws -> Data {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@forceunwrap @NnnOooPppEee could you help double check this is how we should do things?

override fun seal(associatedData: ByteArray, plaintext: ByteArray): ByteArray {
try {
val key = getOrCreateKey()
val cipher = Cipher.getInstance("AES/GCM/NoPadding")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Guardiola31337 @NnnOooPppEee this warrants particular attention

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1bc22854b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +36 to +37
if (!temp.renameTo(file)) {
throw StorageException.BlobStore("failed to atomically replace existing file")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle overwrite when atomic write target already exists

writeAtomic writes to a temp file and then throws if temp.renameTo(file) returns false, but File.renameTo is allowed to fail when the destination already exists. In environments/filesystems where that happens, any rewrite of an existing blob path will fail, so callers cannot safely update previously written data even though writeAtomic is expected to replace atomically. Use a replace-capable move (REPLACE_EXISTING/ATOMIC_MOVE where available) or an explicit overwrite fallback.

Useful? React with 👍 / 👎.

Comment on lines +80 to +83
if addStatus == errSecMissingEntitlement {
Self.setFallbackKey(id: fallbackKeyId(), data: keyData)
return SymmetricKey(data: keyData)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid persisting data behind in-memory entitlement fallback

When SecItemAdd returns errSecMissingEntitlement, the code stores the key only in static in-memory state and still proceeds, which allows writing encrypted blobs that cannot be reopened after process restart (the fallback map is empty on next launch). In entitlement-missing environments this causes silent creation of unrecoverable on-disk credential data; this path should fail fast or use a persistable key mechanism instead of continuing.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants