Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fix ServerTrustError crash. #4553

Merged
merged 10 commits into from
Apr 20, 2020
Merged

Fix ServerTrustError crash. #4553

merged 10 commits into from
Apr 20, 2020

Conversation

jshier
Copy link
Member

@jshier jshier commented Apr 19, 2020 •
edited
Loading

Issue Link 🔗

#4541

Goals ⚽

This PR builds on #4542 and #4552 to fix the crash seen when attempting to create an error for a nil SecTrustRef. It combines both approaches:

  • Ensure we only attempt to check the trusts when [challenge.protectionSpace.authenticationMethod isEqualToString:NSURLAuthenticationMethodServerTrust] is YES.
  • When creating an error, ensure proper nullability checks when inserting values into the userInfo dictionary, as even in the NSURLAuthenticationMethodServerTrust case, the values can be nil.

Implementation Details 🚧

This PR makes the previous

static NSError * ServerTrustError(SecTrustRef serverTrust, NSURL *url)

convenience function into an internal method on AFURLSessionManager

- (nonnull NSError *)serverTrustErrorForServerTrust:(SecTrustRef)serverTrust url:(NSURL *)url`

to enable testing and adds additional checking internally to ensure no nil values are inserted into the dictionary.

Testing Details 🔍

Tests were added for each permutation of parameters to the new method. To enable this, a SecTrust creation function was moved into AFTestCase to enable its use in other tests.

Kaspik and others added 6 commits April 18, 2020 22:05
@Kaspik
Fix serverTrust crash (#4542)
111352b 
* Update readme, add stale bot, add issue template, add pull request template

* Update copy from Alamofire

* Fix crash on nil serverTrust

* Expose server trust error

* add test for error throws

* Fix nullability
@ElfSundae @jshier
Fix evaluateServerTrust determination (#4552)
a5fe977 
Co-authored-by: Jon Shier <jon@jonshier.com>
@jshier
Revert changes making error generation public.
08c6a7e 
This reverts commit 111352b.

# Conflicts:
#	AFNetworking/AFURLSessionManager.m
@jshier
Make static error function a method instead.
992b742
@jshier
Make trust generatio part of AFTestCase.
c34196a
@jshier
Add test cases for server trust error generation.
ca15d83
@jshier jshier requested a review from 0xced April 19, 2020 03:43
@jshier
Copy link
Member Author

jshier commented Apr 19, 2020

@0xced Since you commented on the other PR, would you mind taking a look?

@Kaspik @ElfSundae This combines your previous PRs. Please take a look when you get a chance to ensure it matches your expectation.

Kaspik
Kaspik approved these changes Apr 19, 2020
View reviewed changes
Copy link
Contributor

@Kaspik Kaspik left a comment

Choose a reason for hiding this comment

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

🎉

Tests/Tests/AFHTTPSessionManagerTests.m
@@ -28,6 +28,12 @@ @interface AFHTTPSessionManagerTests : AFTestCase
@property (readwrite, nonatomic, strong) AFHTTPSessionManager *sessionManager;
@end

@interface AFURLSessionManager (Testing)

- (nonnull NSError *)serverTrustErrorForServerTrust:(SecTrustRef)serverTrust url:(NSURL *)url;
Copy link
Contributor

Choose a reason for hiding this comment

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

ServerTrust and url could have nullability too. :)

@ElfSundae
Copy link
Contributor

I think this may be a bit over-processing.

  • serverTrust may be nil even in the server trust authentication? I have not met this issue yet.

    According to the Apple doc:

    This value is nil if the authentication method of the protection space is not server trust.

  • How the task URL is nil inside didReceiveChallenge callback?

  • minor: nonnull could be removed to follow our style :)

@jshier
Copy link
Member Author

jshier commented Apr 19, 2020

Unfortunately those are not API level guarantees, so they can't form the basis of our implementation. Both the serverTrust and the task.currentRequest are nullable in the API, so we have to treat them as such at every point.

I'll double check the nullability.

@jshier
Copy link
Member Author

jshier commented Apr 20, 2020 •
edited
Loading

Doesn't look like NS_ASSUME_NONNULL is active in the .m file, so nullable is the default, and we have to mark nonnull explicitly.

Edit: Appears you need to mark it explicitly for it to show up.

@jshier
Copy link
Member Author

jshier commented Apr 20, 2020

Merging this for release, but still feel free to review, @0xced.

@jshier jshier merged commit 34b6b1b into master Apr 20, 2020
@jshier jshier deleted the bug/servertrust-crash branch April 20, 2020 02:00
@jshier jshier added this to the 4.0.1 milestone Apr 20, 2020
parkboo added a commit to parkboo/AFNetworking that referenced this pull request Sep 22, 2020
@parkboo
Merge commit '77ef5fed64d98107acd177a90182163a20ba4567'
f6f71be 
* commit '77ef5fed64d98107acd177a90182163a20ba4567': (31 commits)
  Use low priority queue instead of background for SCNetworkReachabilityGetFlags() (AFNetworking#4587)
  Exempt "needs investigation" from stalebot.
  Only mark unlabeled issues as stale.
  Loop over only changed keypaths in request serializer (AFNetworking#4581)
  Disable 'Zombie Objects' diagnostics for tvOS scheme (AFNetworking#4572)
  Fix docblock for setAuthenticationChallengeHandler (AFNetworking#4574)
  Fix type in CHANGELOG. (AFNetworking#4562)
  Prepare 4.0.1 Release (AFNetworking#4555)
  Fix ServerTrustError crash. (AFNetworking#4553)
  Fix SPM usage with better publicHeadersPath. (AFNetworking#4554)
  Replace instead of appending to the default header (AFNetworking#4550)
  Fix nullability (AFNetworking#4551)
  Update CHANGELOG.md (AFNetworking#4537)
  Fix UIKit+AFNetworking.h (AFNetworking#4536)
  Remove unused UIImage+AFNetworking.h (AFNetworking#4535)
  Separate bundle identifier for watchOS target (AFNetworking#4533)
  Fix MobileCoreServices renamed warning, close #4520 (AFNetworking#4532)
  Add FOUNDATION_EXPORT for AFJSONObjectByRemovingKeysWithNullValues (AFNetworking#4529)
  (Infra) Make infra changes to cleanup project and simplify its maintenance (AFNetworking#4531)
  Improve podspec (AFNetworking#4528)
  ...

# Conflicts:
#	Framework/AFNetworking.h
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Kaspik Kaspik Kaspik approved these changes

@0xced 0xced Awaiting requested review from 0xced

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.1
Development

Successfully merging this pull request may close these issues.
3 participants
@jshier @ElfSundae @Kaspik