Fixes NSSecureCoding (breaks backward compatibly)

[S2Ler]

I found quite interesting bug in OIDServiceDiscovery NSSecureCoding implementation:
if discoveryDictionary contains NSArrays as values, secure unarchive fails with exception: value for key 'NS.objects' was of unexpected class NSArray. Allowed classes are {( OIDServiceDiscovery )}.

I found a way to fix this, but this breaks all current users archives.

The question is what can we do about this? I can think of the following variants:

• Keep code as is. But this breaks contract, it says that NSSecureCoding is supported, while it is not.
• Ask all users to migrate archives. Not an option in my opinion.
• Modify initWithCoder: in a way that will be able to decode old archives, but the new ones will be created with new secure coding implementation.

Below is possible variant of 3rd option:

- (nullable instancetype)initWithCoder:(NSCoder *)aDecoder {
  NSError *error;
  __block NSDictionary *dictionary;
  void (^oldImpl)(void) = ^{
    dictionary = [[NSDictionary alloc] initWithCoder:aDecoder];
  };

  @try {
    dictionary = [aDecoder decodeObjectOfClasses:[NSSet setWithObjects:[NSArray class], [NSDictionary class], nil] forKey:kDiscoveryDictionaryKey];
    if (!dictionary) {
      oldImpl();
    }
  }
  @catch (NSException *exception) {
    oldImpl();
  }

  self = [self initWithDictionary:dictionary error:&error];
  if (error) {
    return nil;
  }
  return self;
}

[codecov-io]

Codecov Report

Merging #315 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   74.05%   74.13%   +0.08%     
==========================================
  Files          58       58              
  Lines        5076     5084       +8     
==========================================
+ Hits         3759     3769      +10     
+ Misses       1317     1315       -2

Impacted Files	Coverage Δ
Source/OIDServiceDiscovery.m	94.73% <100%> (+1.31%)	⬆️
UnitTests/OIDServiceDiscoveryTests.m	84.94% <100%> (+0.44%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3599c0...67586fa. Read the comment docs.

[WilliamDenniss]

Good catch.

Can you sign the CLA so we can review & merge this?

[S2Ler]

Good catch.

Can you sign the CLA so we can review & merge this?

Sure.

[WilliamDenniss]

Good catch. What a pain! I think the third option is the only choice, it's one thing to break API compatibility, but it's another to break all the existing serialized data.

Can you add that backwards-compatibility logic that you proposed into the PR?

[S2Ler]

Will do. I will also add tests to make sure the fix works.

[S2Ler]

Unfortunately, I didn't found a way to accomplish what I wanted with backward compatibility. @WilliamDenniss up to you to decide what to do with this issue. For my code, I removed secure coding when using AppAuth.

[WilliamDenniss]

@StevenEWright any thoughts on this?

[StevenEWright]

Can we avoid catching exceptions and still maintain backwards compatibility by checking like:

if ([aDecoder containsValueForKey:kDiscoveryDictionaryKey]) {
    dictionary = [aDecoder decodeObjectOfClasses:[NSSet setWithObjects:[NSArray class], [NSDictionary class], nil] forKey:kDiscoveryDictionaryKey];
} else {
    dictionary = [[NSDictionary alloc] initWithCoder:aDecoder];
}

@WilliamDenniss

[StevenEWright]

OK... we are talking about this offline, and I just wanted to update the thread with more info.

Here is where the problem starts:

AppAuth-iOS/Source/OIDServiceConfiguration.m

Line 161 in 65ef95c

OIDServiceDiscovery *discoveryDocument = [aDecoder decodeObjectOfClass:[OIDServiceDiscovery class]

This is where we decode the OIDServiceDiscovery. Unfortunately, the way it is currently working, the decoder is simply passed along to NSDictionary's ``initWithCoder:... but the decoder is specifically meant for an OIDServiceDiscovery`, not an `NSDictionary`.

TL;DR: I don't believe any current serialized OIDServiceConfiguration will decode properly.

This may change the calculus of "backwards compatibility". If no current serialized data is decodable - then maintaining "backwards compatibility" is a moot point.

We need to understand if there are other cases where this wouldn't be a problem (like maybe persisting OIDServiceDiscovery instances directly?) I'm just ruminating here... I don't know... we need to look.

But, if there are no current use cases where this does actually work, then we should just fix it properly and not worry about the "backwards compatibility" issue.

Still looking....

[WilliamDenniss]

Steve and I spent a bit of time debugging this. Our conclusion is that the root object of your discovery doc JSON was likely an array, and not a dictionary.

I'm remiss for not asking for that data in the first place, but we reverse-engineered your code to assume that your doc must be an array.

    dictionary = [aDecoder decodeObjectOfClasses:[NSSet setWithObjects:[NSArray class], [NSDictionary class], nil] forKey:kDiscoveryDictionaryKey];

This line means that you'll accept an NSArray OR an NSDictionary, NOT an NSDictionary that can contain an array. In fact, the discovery doc can only be an NSDictionary and not an NSArray (although it can, and in fact typically will, include NSArrays).

The fact that your discovery doc failed to decode, but then "succeeded" with your fix, we believe indicates it was an NSArray (else why would adding NSArray fix it?). To catch this issue earlier (i.e. before deserialization), the OIDServiceDiscovery constructor now verifies that the root JSON object is an NSDictionary #399

The official samples serialize and deserialize OIDServiceDiscovery objects, and when tested against Google do not show any secure coding exceptions.