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

Fix serverTrust crash #4542

Merged
merged 7 commits into from
Apr 19, 2020
Merged

Fix serverTrust crash #4542

merged 7 commits into from
Apr 19, 2020

Conversation

Kaspik
Copy link
Contributor

@Kaspik Kaspik commented Apr 3, 2020

Issue Link 🔗

Goals ⚽

  • Release of the new version 4.0.1 with no crash.

Implementation Details 🚧

challenge.protectionSpace.serverTrust is nil if the authentication method of the protection space is not server trust

@0xced
Copy link
Contributor

0xced commented Apr 3, 2020

Thanks for the fix! Can you add a unit test that triggers the crash (without your fix) in order to avoid a future potential regression?

@Kaspik Kaspik closed this Apr 3, 2020
@0xced
Copy link
Contributor

0xced commented Apr 4, 2020

@Kaspik May I ask why did you close this pull request?

@Kaspik
Copy link
Contributor Author

Kaspik commented Apr 4, 2020

Hah, I merged my other PR and it closed this one. I'll reopen.

@Kaspik Kaspik reopened this Apr 4, 2020
@0xced
Copy link
Contributor

0xced commented Apr 4, 2020

Are you comfortable to add a unit test that triggers the crash (without your fix) in order to avoid a future potential regression or would you prefer me to add the unit test?

@Kaspik
Copy link
Contributor Author

Kaspik commented Apr 4, 2020

I'm already out from my computer for the weekend, so feel free to do it otherwise I'll try on Monday. :)

@3a4oT
Copy link

3a4oT commented Apr 9, 2020

ping @Kaspik

@Kaspik
Copy link
Contributor Author

Kaspik commented Apr 9, 2020

As static NSError * ServerTrustError(SecTrustRef serverTrust, NSURL *url) was private, I ended up updating it to func that is publicly exposed, then adding test it throws if serverTrust is nil.

Let me know if you have any other idea how to revert this but still test it.

jshier
jshier suggested changes Apr 10, 2020
View reviewed changes
Copy link
Member

@jshier jshier left a comment

Choose a reason for hiding this comment

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

Thanks for the work, I like the solution, but I don't really want to make the helper function public just to test it. Perhaps you can test using the delegate method instead so we only test public API?

AFNetworking/AFURLSessionManager.h
@@ -447,6 +447,8 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (void)setDownloadTaskDidResumeBlock:(nullable void (^)(NSURLSession *session, NSURLSessionDownloadTask *downloadTask, int64_t fileOffset, int64_t expectedTotalBytes))block;

+ (nonnull NSError *)serverTrustErrorWithServerTrust:(nonnull SecTrustRef)serverTrust url:(nonnull NSURL *)url;
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not make this method public. Instead of testing the error method directly, perhaps you can test the delegate method which creates the error? I do like the nullability annotations though, so applying those to the function might be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to "repro" the case where it returns with no server trust - so if anyone has, feel free to give me a hint or final solution.

@ElfSundae ElfSundae mentioned this pull request Apr 18, 2020
Merged
@jshier jshier changed the base branch from master to bug/servertrust-crash April 19, 2020 02:04
@jshier
Copy link
Member

jshier commented Apr 19, 2020

Merging to branch to add tests but will credit this PR with the change.

@jshier jshier merged commit 111352b into AFNetworking:bug/servertrust-crash Apr 19, 2020
@jshier jshier mentioned this pull request Apr 19, 2020
Merged
@jshier jshier added this to the 4.0.1 milestone Apr 20, 2020
jshier added a commit that referenced this pull request Apr 20, 2020
@jshier @Kaspik @ElfSundae
Fix ServerTrustError crash. (#4553)
34b6b1b 
* Fix serverTrust crash (#4542)

* 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

* Fix evaluateServerTrust determination (#4552)

Co-authored-by: Jon Shier <jon@jonshier.com>

* Revert changes making error generation public.

This reverts commit 111352b.

# Conflicts:
#	AFNetworking/AFURLSessionManager.m

* Make static error function a method instead.

* Make trust generatio part of AFTestCase.

* Add test cases for server trust error generation.

* Mark parameters explicitly nullable.

* Fix nullability (#4551)

* Replace instead of appending to the default header (#4550)

* Replace instead of appending to the default header

* Add tests for setting HTTP headers per request

* Fix SPM usage with better publicHeadersPath. (#4554)

Co-authored-by: Jakub Kašpar <kaspikk@gmail.com>
Co-authored-by: Elf Sundae <elf.sundae@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@jshier jshier jshier requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.1
Development

Successfully merging this pull request may close these issues.

Crashed!
4 participants
@Kaspik @0xced @3a4oT @jshier