Mangopay / cardregistration-js-kit

Mangopay V2 JS resources for card registration front-end workflow
MIT License
38 stars 34 forks source link

False positive when detecting antivirus blockage? #27

Closed nclavaud closed 7 years ago

nclavaud commented 8 years ago

Commit https://github.com/Mangopay/cardregistration-js-kit/commit/f056f5d017f200a9ff7120bb872a4195b16480f1 has added support for antivirus blockage detection and this is great!

However, some of our users are experiencing issues with this feature, and we feel like the antivirus blockage result code 001596 is returned from time to time for another (unknown) reason.

Is it possible that xmlhttp.status is not specific enough to target only antivirus blockages? Could it lead to false positives?

hobailey commented 8 years ago

Hey, thanks for the feedback. Would you be able to add some debugging for when this ResultCode is triggered (perhaps logging xmlhttp)? From the specs:

Before the request is complete, the value of status will be 0. It is worth noting that browsers report a status of 0 in case of XMLHttpRequest errors too. But a "normal" XMLHttpRequest error should have been caught by the if (result) line in theory.

nclavaud commented 8 years ago

We have managed to trigger the antivirus blockage detection in sandbox by simulating a network partition:

We got the same behavior with Chrome and Firefox.

screenshot from 2016-11-15 16-22-26

hobailey commented 8 years ago

Nice, thanks for the info @nclavaud :-)

Sinewyk commented 8 years ago

Colleague of @nclavaud over here ...

So we tried various things, and as it happens, a XMLHttpRequest (and a request in general) with a status of 0 can mean a lot of things, but can be boiled down to "You did not receive valid http headers":

What we can do about it ?

Take a look at the server logs looking for strange errors. Any "hard" error; not sending a proper status = 50x, will generate a network error.

Deprecate the "antivirus error" for a more general "connectivity error", and clean up a bit the errorHandling code of the library, there's a status == "0" check in the tokenizeCard but not finishRegistration.

For now we are retrying the entire registerCard procedure if we have a status == 0 on either tokenizeCard or finishRegistration and it seems to do the trick.

Hope it helps.

javiercr commented 8 years ago

For now we are retrying the entire registerCard procedure if we have a status == 0 on either tokenizeCard or finishRegistration and it seems to do the trick.

@Sinewyk does the retry work for antivirus blocking connections? 🤔

PS: thanks for sharing your test results!

Sinewyk commented 8 years ago

Nah, it does not, but we have a maximum number of retries in case it just won't work.

We chose a retry every 3 seconds for a maximum of 10 retries, which means that after 30+ seconds, if there's really a problem, we show an error saying like "Fix your antivirus, or make sure you are well connected if you are on your phone/tablet" or something like that.

hobailey commented 8 years ago

This is really useful, thanks so much for sharing @Sinewyk and @nclavaud :-) So have you made changes to how this is triggered then? Ie do you think that it's too over-sensitive as it currently is? (Or at least the error message too precise?)

Sinewyk commented 8 years ago

So have you made changes to how this is triggered then

No, but we de-specify the error triggered by the library in our error handler.

If we receive a err && err.xmlhttp && err.xmlhttp.status === 0 || #yourAntivirusError then we retry at most 10 times. In case of an error at the end, we show a connectivity error page, with one of the possible issues being an antivirus badly configured.

And yes, the second check is actually superfluous ... for now ^^.

Or at least the error message too precise ?

In my opinion, it is not over-sensitive, but it is too precise yes.

hobailey commented 7 years ago

Hey @Sinewyk, sorry for the delay getting back to you, and thanks for your reply. So are you just saying that this error mesage needs to be more generic? And I'm not sure what you mean by #yourAntivirusError and

the second check is actually superfluous (ie you only really need err && err.xmlhttp && err.xmlhttp.status === 0, right?)

Sinewyk commented 7 years ago

Yes, that error message needs to be more generic.

And yes, we only need the left part for now but that means internal knowledge of this library and I'm not the only dev, so I didn't want to remove some clue about what this test was for.

hobailey commented 7 years ago

OK, great - thanks again for all your help and feedback with this :-)