AFHTTPSessionManager
now throws exception if SSL pinning mode is set for non https sessions
#3687
Conversation
Current coverage is 86.75% (diff: 100%)@@ master #3687 diff @@
==========================================
Files 44 44
Lines 6067 6110 +43
Methods 1079 1088 +9
Messages 0 0
Branches 407 408 +1
==========================================
+ Hits 5256 5301 +45
+ Misses 808 806 -2
Partials 3 3
|
7c49173
to
6fc3e37
Compare
@@ -98,6 +98,22 @@ - (void)setResponseSerializer:(AFHTTPResponseSerializer <AFURLResponseSerializat | |||
[super setResponseSerializer:responseSerializer]; | |||
} | |||
|
|||
- (void)setSecurityPolicy:(AFSecurityPolicy *)securityPolicy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a note to the documentation declaring this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be a good idea. The only issue is that this behaviour only applies at the AFHTTPSessionManager
level (as AFURLSessionManager
has no base URL). So, should we redefine the securityPolicy
property on AFHTTPSessionManager
for the sole purpose of augmenting its documentation? Or is it going to be more confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be a good idea, since that behavior only affects AFHTTPSessionManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I just rebased on master and force-pushed with documentation.
e4d3f1b
to
181d633
Compare
After much struggling, all tests finally pass! |
@0xced awesome! I think it would make sense to cherry-pick the tests stability into its on PR, so we can have a direct reference to that and it doesn't get lost in this changeset. Would you be able to do that? If not, I can take a stab at it. |
Yes, I will do it. |
424fd01
to
e698f45
Compare
Can you rebase this with master to get Xcode 8 coverage? |
…URLs ### Before this commit Setting a security policy configured with `AFSSLPinningModeCertificate` or `AFSSLPinningModePublicKey` on a AFHTTPSessionManager instance configured with an insecure `http` base URL was valid. Requests made with this manager would always succeed since the `-[AFURLSessionManager URLSession:didReceiveChallenge:completionHandler:]` would never be called and thus the security policy would never be evaluated. ### After this commit Setting a security policy configured with `AFSSLPinningModeCertificate` or `AFSSLPinningModePublicKey` on a AFHTTPSessionManager instance configured with an insecure `http` base URL will throw an exception. This will force the manager to be configured with a secure `https` URL. Note that properly configuring App Transport Security (ATS) would also solve this issue since insecure connections would fail anyway, but this is a *belt and suspenders* solution.
e698f45
to
f9ed2d2
Compare
I rebased, but I think you should be able to rebase and merge from the GitHub interface now. |
I wanted to see the green check before merging! |
I realized right after posting. 😄 |
AFHTTPSessionManager
now throws exception if SSL pinning mode is set for non https sessions
Before this commit
Setting a security policy configured with
AFSSLPinningModeCertificate
orAFSSLPinningModePublicKey
on a AFHTTPSessionManager instance configured with an insecurehttp
base URL was valid. Requests made with this manager would always succeed since the-[AFURLSessionManager URLSession:didReceiveChallenge:completionHandler:]
would never be called and thus the security policy would never be evaluated.After this commit
Setting a security policy configured with
AFSSLPinningModeCertificate
orAFSSLPinningModePublicKey
on a AFHTTPSessionManager instance configured with an insecurehttp
base URL will throw an exception. This will force the manager to be configured with a securehttps
URL.Note that properly configuring App Transport Security (ATS) would also solve this issue since insecure connections would fail anyway, but this is a belt and suspenders solution.