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.

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

Already on GitHub? Sign in to your account

Remove unsafe URLCredential optional fields force unwrapping #2184

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@achernoprudov
Contributor

achernoprudov commented Jun 27, 2017

Issue Link 馃敆
No registered issue.

Goals 鈿斤笍
Alamofire crashes if log is enabled and request use client authentication through .pfx certificate. Log cant fetch user and password fields and crash by force unwrapping.

Implementation Details 馃毀
Removed unsafe wrapping for user and password field.

Testing Details 馃攳

@jshier

This comment has been minimized.

Show comment
Hide comment
@jshier

jshier Jul 22, 2017

Contributor

Thanks for the PR! However, before we merge, it would be a good idea to make this encoding a little more elegant, as empty username or password fields aren't usually the best behavior in some scenarios. Can you look into how cURL handles various types of auth and how URLCredential conveys them and see if you can come up with a more general solution?

Contributor

jshier commented Jul 22, 2017

Thanks for the PR! However, before we merge, it would be a good idea to make this encoding a little more elegant, as empty username or password fields aren't usually the best behavior in some scenarios. Can you look into how cURL handles various types of auth and how URLCredential conveys them and see if you can come up with a more general solution?

@jshier jshier requested review from jshier and cnoon Jul 22, 2017

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Aug 22, 2017

Member

I agree @jshier. I remember I looked into this a while back and it's much more complicated than this PR makes it. Certain types of auth support passwords and not usernames, etc. We need to make sure the cURL representation works in all the various cases.

As a temporary solution to the crash issue, I'd suggest we add a guard to ensure both the user and password exist on the credential, otherwise skip it. That is essentially all we support correctly today, so that change would just make it safe.

Member

cnoon commented Aug 22, 2017

I agree @jshier. I remember I looked into this a while back and it's much more complicated than this PR makes it. Certain types of auth support passwords and not usernames, etc. We need to make sure the cURL representation works in all the various cases.

As a temporary solution to the crash issue, I'd suggest we add a guard to ensure both the user and password exist on the credential, otherwise skip it. That is essentially all we support correctly today, so that change would just make it safe.

components.append("-u \(credential.user!):\(credential.password!)")
let user = credential.user ?? ""
let password = credential.password ?? ""
components.append("-u \(user):\(password)")

This comment has been minimized.

@cnoon

cnoon Aug 22, 2017

Member

Could we change both of these to guard or continue?

guard let user = credential.user, let password = credential.password else { continue }
components.append("-u \(user):\(password)")

Then we'll circle back on this change in a future release to add better support for other types of auth.

@cnoon

cnoon Aug 22, 2017

Member

Could we change both of these to guard or continue?

guard let user = credential.user, let password = credential.password else { continue }
components.append("-u \(user):\(password)")

Then we'll circle back on this change in a future release to add better support for other types of auth.

cnoon added a commit that referenced this pull request Aug 22, 2017

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Aug 22, 2017

Member

Okay @achernoprudov, I ended up just making these changes myself and pushed them up into master in 0d2348d while maintaining your attribution. We'll still need to circle back to this and add better support for credentials other than basic auth, but this is certainly an improvement.

Thanks again! 馃嵒

Member

cnoon commented Aug 22, 2017

Okay @achernoprudov, I ended up just making these changes myself and pushed them up into master in 0d2348d while maintaining your attribution. We'll still need to circle back to this and add better support for credentials other than basic auth, but this is certainly an improvement.

Thanks again! 馃嵒

@cnoon cnoon closed this Aug 22, 2017

@cnoon cnoon added this to the 4.6.0 milestone Aug 22, 2017

@cnoon cnoon added the request label Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment