I have noticed the newly added state parameter for requests is not working properly:
currently it is set at "", cf SmartcarAuth.swift:45
it somehow gets dropped while in SFSafariViewController
when resumeAuthorizationFlow(with url: URL) is called the given url doesn't contain the state parameter anymore which causes a crash at let state = query?[1].value
I also tried setting it to " " but it prevents a valid URL from being created because it is not escaped properly, which causes a crash in:
let safariVC = SFSafariViewController(url: URL(string: authorizationURL)!)
Setting it to "%20" fixes the crash above, but the test in resumeAuthorizationFlow(with url: URL) doesn't handle escaping right and we end end comparing " " and "%20".
I set it to a random UUID string which seems to have fixes the problem.
I would highly recommend using a valid default value, or manage the case where the value is empty. To prevent crashes I would also suggest refactoring resumeAuthorizationFlow(with url: URL) as follows :
public func resumeAuthorizationFlow(with url: URL) throws -> String {
let urlComp = URLComponents(url: url, resolvingAgainstBaseURL: false)
guard let query = urlComp?.queryItems else {
// declare a new error maybe?
throw AuthorizationError.missingURL
}
guard let code = query.filter({ $0.name == "code" }).first?.value else {
throw AuthorizationError.missingURL
}
guard let state = query.filter({ $0.name == "state" }).first?.value else {
throw AuthorizationError.missingState
}
if state != self.request.state {
throw AuthorizationError.invalidState
}
return code
}
Using query parameter names instead of probable indexes guarantees to find the right element if it exists, and this will prevent further crashes when accessing index out of bounds.
Hello,
I have noticed the newly added
state
parameter for requests is not working properly:""
, cfSmartcarAuth.swift:45
SFSafariViewController
resumeAuthorizationFlow(with url: URL)
is called the given url doesn't contain thestate
parameter anymore which causes a crash atlet state = query?[1].value
I also tried setting it to
" "
but it prevents a valid URL from being created because it is not escaped properly, which causes a crash in:Setting it to
"%20"
fixes the crash above, but the test inresumeAuthorizationFlow(with url: URL)
doesn't handle escaping right and we end end comparing" "
and"%20"
.I set it to a random UUID string which seems to have fixes the problem.
I would highly recommend using a valid default value, or manage the case where the value is empty. To prevent crashes I would also suggest refactoring
resumeAuthorizationFlow(with url: URL)
as follows :Using query parameter names instead of probable indexes guarantees to find the right element if it exists, and this will prevent further crashes when accessing index out of bounds.
Thanks for considering,
Stan