Harden passkey delegate retention, sign-out cleanup, input validation, and app re-lock - #159
Open
iam-mercy wants to merge 1 commit into
Open
Harden passkey delegate retention, sign-out cleanup, input validation, and app re-lock#159iam-mercy wants to merge 1 commit into
iam-mercy wants to merge 1 commit into
Conversation
…, and app re-lock - PasskeyService: replace the objc_setAssociatedObject delegate-retention hack with instance-level, controller-keyed storage so concurrent performRequest calls (e.g. register() and authenticate() in flight together) each retain their own delegate without clobbering the other's. - RegisterView: validate username length/character-set and trim whitespace client-side before registration, with an inline validation message instead of a generic backend error. - AuthStore.signOut(): clear the stored credential ID, pending local notifications, the scheduled background refresh task, the OfflineCache directory, and local iCloud vault associations — not just the auth token — so no vault data survives sign-out on a shared/resold device. - Add a ScenePhase-driven re-lock timer with a configurable timeout (SettingsView) and a privacy overlay shown whenever the app isn't in the foreground, so balances/beneficiary details require a fresh Face ID check after backgrounding and never appear in the app switcher. Closes ethos-protocol#9, ethos-protocol#10, ethos-protocol#11, ethos-protocol#12
|
@iam-mercy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
Implements four related iOS security/usability issues:
objc_setAssociatedObjectDelegate-Retention Hack #9 — Replace theobjc_setAssociatedObjectdelegate-retention hack inPasskeyService.What changed
PasskeyService.swift(#9)objc_setAssociatedObjecthack with instance-level, controller-keyed storage (activeDelegates: [ObjectIdentifier: PasskeyDelegate]), sinceASAuthorizationController.delegateis a weak reference and something has to retain the delegate until the request completes.register()andauthenticate()) each retain their own delegate instead of one clobbering the other's reference.RegisterView/UsernameValidation(#11)UsernameValidation(inModels.swift): trims whitespace, enforces a 3–30 character length and an alphanumeric +_/-character set, mirroring typical WebAuthn userID/display-name constraints.RegisterViewnow shows a friendly inline validation message instead of only gating onusername.isEmpty, and passes the trimmed username through to registration.AuthStore.signOut()(#10)KeychainService.deleteCredentialID(),NotificationService.removeAllPendingNotifications(),BackgroundRefreshService.cancelScheduledRefresh(),OfflineCache.clearAll(), andICloudSyncService.clearLocalAssociations()(device-local only — the iCloud KV store itself is left alone so other signed-in devices aren't affected).Re-lock on background/foreground (#12)
AuthStoregainedisLockedandhandleScenePhaseChange(_:now:), which starts a timer when the app backgrounds and re-locks (independent of the passkey session) if the app was away for at least the configured timeout.RootViewnow shows aLockScreenView(Face ID re-prompt) onceisLockedis set, and aPrivacyOverlayViewany timescenePhase != .active— the latter covers content immediately on every backgrounding (regardless of the timeout), so the app-switcher snapshot never captures balances.ReLockTimeoutOption(Immediately / 30s / 1m / 5m / 15m / Never, persisted inUserDefaults, default 1 minute) and a picker inSettingsViewto configure or disable it.Verification performed
This sandbox has no Swift/Xcode toolchain and no network access to fetch one, so I could not run
swift build/xcodebuildlocally. I instead:NotificationService,BackgroundRefreshService,OfflineCache,ICloudSyncService,KeychainService) and existing UI tests (AuthUITests) to confirm behavior isn't changed for already-covered paths (e.g. empty username still disables Register).Tests/HostedTests) exactly, placing each new test alongside the existing tests that exercise the same real system API (Keychain /UNUserNotificationCenter) so CI skip conventions stay consistent.Please still run the two CI test jobs (
ios-ci.yml) on this PR — that's the only place a full build/test can actually happen for this project.Test plan
New/changed automated tests (all follow existing naming/skip conventions in the files they're added to):
PasskeyDelegateRetentionTests(bare bundle) — two concurrentperformRequest-style calls each retain and release their own delegate without affecting the other's.UsernameValidationTests(bare bundle) — trimming, min/max length, and character-set rejection/acceptance.AuthStoreReLockTests/ReLockTimeoutOptionTests(bare bundle) — lock fires once the configured timeout elapses, doesn't fire early,.nevernever fires, unauthenticated state is never locked, and the persisted-option default/round-trip.SignOutClearsLocalStateTests(hosted target) — credential ID, iCloud local association, and pending notifications are all gone aftersignOut(), andisLocked/isAuthenticatedreset; plus an offline-cache assertion that previously cached vault data is unreadable after sign-out.KeychainServiceTests,OfflineCacheTests,ICloudSyncServiceTests, andBackgroundRefreshServiceTestsfor the newdeleteCredentialID()/clearAll()/clearLocalAssociations()/cancelScheduledRefresh()methods.Manual QA (once buildable — see "Known pre-existing issues" below):
Known pre-existing issues (out of scope for this batch)
While reviewing the affected files I found that
maincurrently does not compile, from what looks like an incompletely-resolved merge conflict (96b0e1f/09d9e92) predating this branch:ViewModels/Stores.swift:VaultStoredeclaresscheduleReminders()twice (duplicate declaration), the second of which also callsNotificationService.scheduleCheckInReminderwith the wrong argument list.Views/Views.swift:VaultDetailViewdeclaresload2FAStatus()twice, calls an undefinedrefreshTTLPeriodically(), and references several@Stateproperties (showDeposit,showWithdraw,showManageBeneficiary,ttlRemaining) that are never declared.Tests/EthosProtocolTests.swiftreferencesVaultStore.applyUpdate/deposit/withdraw/updateBeneficiaryandVaultDetailView.ttlRefreshInterval, none of which currently exist inSources/.These are unrelated to issues #9–#12 (passkey/auth/sign-out/re-lock), so per the batch scope I left them untouched and did not attempt to fix them here — flagging them since they'll block any build/CI run on
main(and on this branch, which is based on it) until addressed separately.Closes #9
Closes #10
Closes #11
Closes #12