iOS: security hardening — Keychain migration, concurrency fixes, attack surface reduction#31979
iOS: security hardening — Keychain migration, concurrency fixes, attack surface reduction#31979Rocuts wants to merge 9 commits intoopenclaw:mainfrom
Conversation
… cap WebSocket message size
Co-Authored-By: Claude Opus 4.6 <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 673d732dc6
ℹ️ 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".
Greptile SummaryThis PR comprehensively addresses 10 security and concurrency findings from a 2026 security audit, implementing critical hardening measures across the iOS app. Key improvements:
Migration safety: The UserDefaults-to-Keychain migration logic is idempotent and runs on first access. Both Test coverage: Updated tests validate Keychain round-trips, migration paths, and connection metadata persistence. The camera and TLS probe concurrency fixes eliminate critical race conditions that could cause crashes. Confidence Score: 4/5
Last reviewed commit: ea47df9 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| port: kind == .manual ? port : nil) | ||
| guard self.saveLastGatewayConnectionData(payload) else { return } |
There was a problem hiding this comment.
migration could lose data if saveLastGatewayConnectionData() fails (discarded return value) but removeLastGatewayDefaults() proceeds - consider checking save success before cleanup
| port: kind == .manual ? port : nil) | |
| guard self.saveLastGatewayConnectionData(payload) else { return } | |
| if self.saveLastGatewayConnectionData(payload) { | |
| self.removeLastGatewayDefaults(defaults) | |
| } |
(requires changing saveLastGatewayConnectionData to return Bool)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/Sources/Gateway/GatewaySettingsStore.swift
Line: 262-263
Comment:
migration could lose data if `saveLastGatewayConnectionData()` fails (discarded return value) but `removeLastGatewayDefaults()` proceeds - consider checking save success before cleanup
```suggestion
if self.saveLastGatewayConnectionData(payload) {
self.removeLastGatewayDefaults(defaults)
}
```
(requires changing `saveLastGatewayConnectionData` to return `Bool`)
How can I resolve this? If you propose a fix, please make it concise.| if self.keychainLoad(account: stableID) == nil { | ||
| guard self.keychainSave(existing, account: stableID) else { return } | ||
| } | ||
| defaults.removeObject(forKey: legacyKey) |
There was a problem hiding this comment.
same data loss risk - if keychainSave() fails (returns false), the legacy entry is removed anyway, permanently losing the TLS fingerprint - consider checking save success before cleanup
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/shared/OpenClawKit/Sources/OpenClawKit/GatewayTLSPinning.swift
Line: 48-51
Comment:
same data loss risk - if `keychainSave()` fails (returns false), the legacy entry is removed anyway, permanently losing the TLS fingerprint - consider checking save success before cleanup
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea47df92fa
ℹ️ 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".
| let task = self.webSocketTask(with: url) | ||
| // Avoid "Message too long" receive errors for large snapshots / history payloads. | ||
| task.maximumMessageSize = 16 * 1024 * 1024 // 16 MB | ||
| task.maximumMessageSize = 4 * 1024 * 1024 // 4 MB |
There was a problem hiding this comment.
Reducing task.maximumMessageSize to 4 MB here (and similarly in apps/shared/OpenClawKit/Sources/OpenClawKit/GatewayTLSPinning.swift) introduces a hard mismatch with the gateway’s chat-history response budget, which is still 6 MB (src/gateway/server-constants.ts:6) and is actively enforced when building chat.history payloads (src/gateway/server-methods/chat.ts:553-560). Any history response between 4 MB and 6 MB is now valid server output but will be rejected by these clients with a "Message too long" receive failure, causing reconnect/fetch failures for larger sessions.
Useful? React with 👍 / 👎.
|
The failing test This PR only touches iOS/Swift files under Could a maintainer re-run CI to confirm? Thanks! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e301d720b9
ℹ️ 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".
| SecItemDelete(query as CFDictionary) | ||
|
|
||
| var insert = query |
There was a problem hiding this comment.
saveString deletes the current keychain item before attempting to add the replacement, so any SecItemAdd failure turns a failed update into data loss: the old value is already gone. This impacts all callers persisting gateway tokens/passwords and stable IDs, and can leave users without recoverable credentials after a transient keychain write error; the write path should keep the prior item until the new value is durably stored.
Useful? React with 👍 / 👎.
| SecItemDelete(query as CFDictionary) | ||
| var insert = query | ||
| insert[kSecValueData as String] = data | ||
| insert[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly | ||
| return SecItemAdd(insert as CFDictionary, nil) == errSecSuccess |
There was a problem hiding this comment.
The TLS pin store also uses delete-then-add, which means a failed SecItemAdd erases an existing fingerprint instead of leaving the prior pin in place. In TOFU flows (GatewayTLSPinningSession saves fingerprints during successful handshakes), this can silently drop trust continuity and force future connections to behave as unpinned until a new pin is stored.
Useful? React with 👍 / 👎.
|
Thanks for the effort here. I’m going to ask for a split before we review this. This PR is too broad: it bundles multiple independent changes (keychain migration, TLS pinning storage, concurrency fixes, rate-limit/message-size tweaks, diagnostics path/protection changes, and Also, please don’t include the large generated audit docs in this PR. Files like:
add a lot of noise and make review harder. Keep this PR to code + tests needed for the actual fix set, and move docs to separate targeted PR(s) only if they’re needed. Please re-open as a series of focused PRs (one concern per PR), and we can review quickly. |
|
I'll cherry pick, merge and give attribution as needed. Thanks @Rocuts |
3 participants
Summary
kSecAttrAccessibleAfterFirstUnlockThisDeviceOnlyon every write via delete-then-add pattern (KeychainStore, GatewayTLSPinning)objc_sync_enter/exitand bareBoolflags withOSAllocatedUnfairLock<T>in TLS probe, camera delegates, and diagnostics log counter#if DEBUG, tighten deep link rate limit (1s → 5s), cap WebSocket message size (16 MB → 4 MB)FileProtectionType.completeUntilFirstUserAuthenticationFindings addressed
Test plan
GatewaySettingsStoreTests— Keychain round-trip, migration, overwrite, provider API keyGatewayConnectionControllerTests— Keychain snapshot/restore for connection load tests🤖 Generated with Claude Code