Skip to content

fix: replace deprecated NSKeyedArchiver/NSKeyedUnarchiver APIs #1680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
garvitkaushik-123 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: replace deprecated NSKeyedArchiver/NSKeyedUnarchiver APIs #1680

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions iOS_SDK/OneSignalSDK/OneSignalCore/Source/OneSignalUserDefaults.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,15 +151,22 @@
[self.userDefaults synchronize];
}

- (id _Nullable)getSavedCodeableDataForKey:(NSString * _Nonnull)key defaultValue:(id _Nullable)value {
if ([self keyExists:key])
return [NSKeyedUnarchiver unarchiveObjectWithData:[self.userDefaults objectForKey:key]];

if ([self keyExists:key]) {
NSData *data = [self.userDefaults objectForKey:key];
NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingFromData:data error:nil];
unarchiver.requiresSecureCoding = NO;
id result = [unarchiver decodeTopLevelObjectAndReturnError:nil];
[unarchiver finishDecoding];
return result;
}

Check warning on line 163 in iOS_SDK/OneSignalSDK/OneSignalCore/Source/OneSignalUserDefaults.m

View check run for this annotation

Claude / Claude Code Review

Migration @catch no longer triggers on corrupt archives 

Minor behavior change to flag: the old `+[NSKeyedUnarchiver unarchiveObjectWithData:]` threw `NSException` on decode failures, which `migrateIAMRedisplayCache` in `OSInAppMessageMigrationController.m:50-68` catches to trigger corrupt-cache recovery. The new `initForReadingFromData:error:` + `decodeTopLevelObjectAndReturnError:` pair is documented to convert exceptions to `NSError` and return nil for some failure modes, so the recovery `@catch` may no longer fire for genuinely-corrupt archive byt
Comment on lines 154 to +163

@claude claude Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Minor behavior change to flag: the old +[NSKeyedUnarchiver unarchiveObjectWithData:] threw NSException on decode failures, which migrateIAMRedisplayCache in OSInAppMessageMigrationController.m:50-68 catches to trigger corrupt-cache recovery. The new initForReadingFromData:error: + decodeTopLevelObjectAndReturnError: pair is documented to convert exceptions to NSError and return nil for some failure modes, so the recovery @catch may no longer fire for genuinely-corrupt archive bytes. Impact is bounded (only devices upgrading from SDK < 3.2.03, no crash — corrupt entry just stays stale until the sibling migrateToOSInAppMessageInternal clears it), but worth a defensive if (!result && data) { @throw ... } if you want to preserve the original migration contract.

Extended reasoning...

What changed

The PR replaces +[NSKeyedUnarchiver unarchiveObjectWithData:] with initForReadingFromData:error: + decodeTopLevelObjectAndReturnError:. Beyond the deprecation-warning fix, this changes the failure contract: the old API raised NSException on any decode failure (invalid archive bytes, missing class, wrong-type input), while the new API pair is explicitly designed to convert exceptions into NSError return values.

Where this matters

In OSInAppMessageMigrationController.m:50-68, migrateIAMRedisplayCache wraps getSavedCodeableDataForKey: in @try / @catch (NSException *) specifically to detect legacy corrupt cache data. The comment on line 41-43 states the intent: "Devices could potentially have bad data in the OS_IAM_REDISPLAY_DICTIONARY that was saved as a dictionary and not CodeableData. Try to detect if that is the case…". When the @catch fires, the code re-reads the value as an NSDictionary and re-saves it as archived data.

Step-by-step trace (corrupt-NSData subcase)

  1. Device has SDK version < 30203 in NSUserDefaults and a corrupt NSData blob stored under OS_IAM_REDISPLAY_DICTIONARY (e.g., partial write, unknown archive class).
  2. getSavedCodeableDataForKey: runs — [[NSKeyedUnarchiver alloc] initForReadingFromData:data error:nil] returns nil (per Apple docs for malformed archive headers).
  3. unarchiver.requiresSecureCoding = NO is a no-op (message to nil).
  4. [unarchiver decodeTopLevelObjectAndReturnError:nil] returns nil (message to nil).
  5. getSavedCodeableDataForKey: returns nil — note it returns result (nil), not value (the default).
  6. Back in migrateIAMRedisplayCache: [[NSMutableDictionary alloc] initWithDictionary:nil] on modern Foundation returns an empty mutable dict without throwing.
  7. @catch never fires. Recovery path is skipped.

Addressing the refutations

The strongest refutation argues (a) decodingFailurePolicy defaults to RaiseException and (b) wrong-type inputs (NSDictionary passed where NSData is expected) still raise NSInvalidArgumentException via unrecognized selector, so the @catch still fires for the specific "saved-as-dictionary" case the migration was built for. These points have merit — decodeTopLevelObjectAndReturnError: is documented to catch internal exceptions and convert to NSError, and messaging -bytes/-length on an NSDictionary really would throw. So for the primary target case named in the migration comment (NSDictionary stored via saveDictionaryForKey: earlier), the @catch likely still triggers.

However, the failure surface is narrower than before. Cases that previously all threw and now silently return nil include: (a) NSData with corrupt archive bytes, (b) archive referencing an unmapped class (relevant because migrateIAMRedisplayCache runs before migrateToOSInAppMessageInternal registers the OSInAppMessage → OSInAppMessageInternal class mapping on line 77), and (c) any decode failure that Apple has converted to error-return over the years. That is a real, documented API contract change on a code path whose whole reason for existing is to catch decode failures.

Severity rationale

Marking this a nit rather than blocking. Reasons: the migration is version-gated to sdkVersion < 30203 (SDK 3.02.03, ~2019); the affected user population — devices upgrading from that old SDK with genuinely corrupt cache — is small; the failure mode is silent (no crash, callers see empty state); and migrateToOSInAppMessageInternal runs immediately after and clears the key if the result is empty, so the stale data eventually gets wiped even if not migrated to the new format. The behavior change is worth flagging so the author can decide whether to preserve the original contract, but is not a merge blocker.

Suggested fix

If preserving the original migration contract is desired, capture the error and rethrow, e.g.:

- (id _Nullable)getSavedCodeableDataForKey:(NSString * _Nonnull)key defaultValue:(id _Nullable)value {
    if ([self keyExists:key]) {
        NSData *data = [self.userDefaults objectForKey:key];
        NSError *error = nil;
        NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingFromData:data error:&error];
        if (!unarchiver) {
            @throw [NSException exceptionWithName:NSInvalidUnarchiveOperationException reason:error.localizedDescription userInfo:nil];
        }
        unarchiver.requiresSecureCoding = NO;
        id result = [unarchiver decodeTopLevelObjectAndReturnError:&error];
        [unarchiver finishDecoding];
        if (!result && error) {
            @throw [NSException exceptionWithName:NSInvalidUnarchiveOperationException reason:error.localizedDescription userInfo:nil];
        }
        return result;
    }
    return value;
}

Alternatively, restructure migrateIAMRedisplayCache to trigger the recovery path when getSavedCodeableDataForKey: returns nil while the raw NSUserDefaults value is non-nil.

return value;
}

- (void)saveCodeableDataForKey:(NSString * _Nonnull)key withValue:(id _Nullable)value {
[self.userDefaults setObject:[NSKeyedArchiver archivedDataWithRootObject:value] forKey:key];
NSData *data = [NSKeyedArchiver archivedDataWithRootObject:value requiringSecureCoding:NO error:nil];
[self.userDefaults setObject:data forKey:key];
[self.userDefaults synchronize];
}

Expand Down
Loading