GH-292 android: SSL errors handling in Android

[nicolashenry]

Platforms affected

Android

What does this PR do?

Fix #292

What testing has been done on this change?

I tested on my company website which currently have a chaining certificate issue (it should be fixed soon).
Maybe it should be done on all platforms but our certificate issue only occurs on Android (iOS does not care about this problem).

Checklist

• Reported an issue in the JIRA database
• Commit message follows the format: "CB-3232: (android) SSL errors handling in Android", where CB-xxxx is the JIRA ID & "android" is the platform affected.
• Added automated test coverage as appropriate for this change.

[janpio]

Ok I see this adds some handling of the error. What happens with the message then?

[nicolashenry]

Before the change, the events came like this :

{type: "loadstart", url: "https://ourdomain.com/"}

{type: "loadstop", url: "https://ourdomain.com/"}

After :

{type: "loadstart", url: "https://ourdomain.com/"}

{type: "loaderror", url: "https://ourdomain.com/", code: 0, sslerror: 3, message: "The certificate authority is not trusted"}

{type: "loadstop", url: "https://ourdomain.com/"}

The handler.cancel(); is just here to ensure that the connection will not be processed (the default behaviour is to cancel it anyway so the 'super' call is probably already doing it).

I am not sure about what should the loaderror should have as message, I just copied description from the constants here : https://developer.android.com/reference/android/net/http/SslError

[janpio]

Ah ok, so the PR makes sure the error is not just swallowed but bubbles up to the web app and can be handled there?

[nicolashenry]

Yes, exactly.

[nicolashenry]

I don't know why travis build is failing, the tests seem ok when I run them myself :

[janpio]

Very possible this is unrelated, someone will look into this (either by fixing the flaky tests in general or by giving your PR the thumbs-up anyway) in the future. I am not sure enough to do so myself, so we'll have to wait for another maintainer.

[nicolashenry]

@janpio I noticed that onReceivedError is not called on http error with Android SDK >= 23 (so loaderror is never triggered for errors which are not ssl error) because onReceivedHttpError should be used instead. Should I create another issue/PR for that?

[janpio]

Yes please @nicolashenry - sounds not directly related, so another PR would make it much easier to review. Thanks!

[mtsibulski]

I would really like to see this merged please. I am trying to upload an app to Google and they sent me an email about a SSL Error Handler vulnerability and a deadline to fix it. Here is a help center article they provided: https://support.google.com/faqs/answer/7071387
There's a chance that Google may be warning or rejecting more apps going forward :/

[breautek]

I would really like to see this merged please. I am trying to upload an app to Google and they sent me an email about a SSL Error Handler vulnerability and a deadline to fix it. Here is a help center article they provided: https://support.google.com/faqs/answer/7071387
There's a chance that Google may be warning or rejecting more apps going forward :/

Thanks for bringing attention to this issue. I'll review/test this tonight.

@janpio I know it's been awhile since you approved this PR. Do you think you're approval still stands?

[jclarkAtTillster]

Looking forward to getting this update in.

[timbru31]

We've had this bug, too, due to a mis-configured Apache webserver - so the PR definitely solves an issue. 👍

@breautek for retesting: Have a webserver with an invalid/incomplete chain.

[breautek]

Looks good to me, but I do think we should remove the deprecated constant that @timbru31 pointed out.

[nicolashenry]

I removed the deprecated constant 👍