processHTML being called too late

chrisleversuch commented 5 years ago

We've noticed that in the last few days (we think since Chrome 71 has started rolling out to Android devices), the processHTML method is being called too late so it's running against the HTML from the request to www.google.com rather than the HTML form that it should be running against. The result is that onAuthorizationCompleted is called with empty strings for md and pares.

This PR uses onPageFinished to capture the HTML from each request (except the one to www.google.com) so that the reqex can be run against the correct HTML.

sandyscoffable commented 5 years ago

Thank you so much for this ❤️

I've been scrabbling around for the last few hours trying to figure out why 3-D Secure randomly stopped working on Thursday last week on Android (after which we turned it off). We could see the empty md and pares but were unsure why. Next step for me was to run in debug mode and capture the HTML it was processing against (which would have highlighted the bug), but you've already solved it. Awesome!

I'll give this a whirl in the next few days.

sandyscoffable commented 5 years ago

Replicating issue on master version

Testing using release 1.1.2:

Webviews using Chrome 70 work fine
Webviews using Chrome 71 don't work

👍 Seems like this is the problem.

Testing this branch

Webviews using Chrome 70 don't seem to work (blank md / pares again - the exact same issue FWICS) 😱
Webviews using Chrome 71 work fine 👍

@chrisleversuch - Can you replicate these same problems on Chrome 70 using this branch?

Test environment

My Samsung Android phone has Chrome 70 installed by default. To get version 71 I had to install Chrome Beta.

The choice of webview to use is buried in the Developer Options on Android.

I tested using our commercial app (development version) and used the payments test platform provided by our PSP.

sandyscoffable commented 5 years ago

I've been testing a bit more ... on this branch, on Chrome 70 it maybe works 1 in 5 times, which seems to indicate some sort of race condition.

chrisleversuch commented 5 years ago

Yeah I'm getting the same problem, not sure how I didn't see it earlier. I'd guess onPageStarted for the Google request is being called before onPageFinished for the previous request. I'll see what I can do

chrisleversuch commented 5 years ago

FYI I think I'll try moving the processing to onPageFinished - as soon as there's an html page with MD and PaRes inputs, trigger the completion handler. That's the html we're interested in and I don't think there's any reason to wait for the form to submit itself

sandyscoffable commented 5 years ago

Cool. I'm now in a bit of a quandary as to what to do. Ideally I'd like to have this fixed this week, with a gradual app rollout over the next 7 days, in time for the Christmas period (I really don't want to be deploying new versions of apps while having Christmas dinner etc).

You are much more familiar with this code than I am and have already made good progress to solving the issue, however I've got a work related deadline. So, if this isn't a high priority for you / you stop work on it, then give me a shout and I'll have a crack at it (no guarantees that I'll actually manage to fix it though). Conversely, if you do manage to fabricate a fix, give me a shout and I'll give it another test to validate it, after which we can try to get the attention of the repository maintainer to deploy a new release (as otherwise this library is dead in the water imminently).

sandyscoffable commented 5 years ago

FYI I think I'll try moving the processing to onPageFinished - as soon as there's an html page with MD and PaRes inputs, trigger the completion handler. That's the html we're interested in and I don't think there's any reason to wait for the form to submit itself

I've been looking at how this library works under the hood. What you propose makes sense, however it seems a pretty fairly fundamental change to this library. I'm worried about breaking the spaghetti code around "stacked mode" which I'm not using (and I've no idea what that is - There is only a cryptic comment to go on and the commit history for those lines aren't very useful). I found https://github.com/LivotovLabs/3DSView/commit/71de5aa3b60136ce2a3a8ff359e851331cbcb0c2 but I still have little idea why this was introduced. Otherwise, your proposed changes look fairly straightforward.

I'm pretty sure this library could be a lot simpler than it currently is at the moment. I guess I/we could put together a fix in the manner that you suggest and ditch stacked mode, and perhaps someone else who is using that can figure out how to fix it, if it's important to them?

FYI I'm merely using the basic D3SView - no frills.

chrisleversuch commented 5 years ago

I've updated the code with another attempt. I've tried it on Chrome 70 and I think Chrome 71 (I installed the beta and changed the WebView setting in Developer Options). I have the same concern as you with stacked mode, I've tried to keep the changes as minimal as possible. It now runs the processing in both onPageStarted and onPageFinished, using a boolean to track when it's been done so the completion handler is only called once.

sandyscoffable commented 5 years ago

Right, I've tested this 10 times on both Chrome 70 and 71 and it works. Good job. 👍

The only niggle I have is a 4-5 second delay before I can enter information on the ACS form (The ACS form is shown, for ages, I can see it, but I can't press on the text entry fields for what feels like ages). The issue seems to be on both Chrome 70 and 71.

I rolled back to 1.1.2, and I can press on text fields immediately (i.e feels rapid in comparison) on both versions of Chrome (albeit 71 fails with the issue we are trying to fix here).

sandyscoffable commented 5 years ago

Video of responsiveness issue using this branch: https://drive.google.com/file/d/1I8ImPRULPQAyyhwCzmhzRi-FxgUul8ez/view?usp=sharing

chrisleversuch commented 5 years ago

I'm not sure what would cause the delay and I don't think I'm seeing the same issue - normally I just tap submit when the form loads but I can interact with the text entry and drop down (I assume the form I see is different to yours - I'm not sure who the payment provider is in this project, we're just using our client's API)

sandyscoffable commented 5 years ago

Video of 1.1.2 https://drive.google.com/open?id=1Rb0UZtMlytsl_6nARul3J9OnHvQ_5oRc

I've rebuilt and tested both versions multiple times with same consistent behaviour (1.1.2 = fast, this branch = slow, same ACS etc) ... not sure what's going on.

sandyscoffable commented 5 years ago

I notice there is some other stuff on master since 1.1.2 was released.

What I might try doing is branching from 1.1.2 and cherry-picking your changes on top, to see if it's your changes, or the other stuff that's causing the issue.

sandyscoffable commented 5 years ago

Actually, I'll just build master, that's easier 🤦‍♂️

sandyscoffable commented 5 years ago

Ok ... master version doesn't appear to exhibit the issue, so it must be caused by these changes.

sandyscoffable commented 5 years ago

👍

sandyscoffable commented 5 years ago

@livotov - would you be able to review this PR? It fixes this library on the latest version of Chrome (71)

rcpassos commented 5 years ago

I need this fix

1240 commented 5 years ago

hey, what are we waiting for?

sandyscoffable commented 5 years ago

@chrisleversuch I've ran into an issue, I'm not 100% sure if it's related to these changes however a few of our customers have ran into the following error:

We have a 3-D Secure success rate of 91-92% on Android, and on our other platforms they are all solidly around 95% (this is on a volume of around 20,000 transactions since I made the change in the Android app).

I suspect it might be an issue confined to certain banks. The cards affected seem to be Co-op, Nationwide, Clydesdale and perhaps Capital One 🙀

Interestingly, some of the IINs involved were processed successfully before I chucked in this branch update. Might just be coincidence though.

Here are a few IINs that seem to be affected: 498824, 557351, 454313

I'll update if I manage to replicate the issue.

ghost commented 5 years ago

Hello, @sandyscoffable have you found a solution yet? I am also experiencing the same issue with capital one cards.

I have had a successful transaction with 454313 btw.

sandyscoffable commented 5 years ago

@rihantvx - 454313 (Nationwide) is possibly a red-herring ... it's quite difficult to get a definitive list as consumers can legitimately abandon the payment process at that point.

Clutching at straws, but I wonder if the issue is with the regex this library uses? That's typically the cause of issues and looks like the weak point in this library.

I've not got a card to hand that I can try so I've not managed to replicate the issue myself yet (I've been embroiled in other stuff and the employee that has a co-op card has gone home). I've got a branch of this that spits out the resulting HTML for inspection, I just need a card that I can test with 🥃

Pretty close to just signing up to a co-op bank account just to test this TBH!

I hate 3-D Secure ... it's a nightmare to test as the HTML returned by these ACS providers possibly differs in some way. Such a cludge.

markomeara-whitbread commented 5 years ago

We also experienced continuing issues even after adopting this change, about 30% of payments still did not complete successfully from 3DSecure screen. We're now moving to a more elaborate solution involving back-end callbacks and no longer using this library.

sandyscoffable commented 5 years ago

@markomeara-whitbread - I assume you mean you have a dedicated endpoint in your server side stack that the consumer is redirected to and the PaRes is extracted? Makes sense I guess.. I could totally do that too..

sandyscoffable commented 5 years ago

I'm going to continue to try and debug / fix this issue.

sandyscoffable commented 5 years ago

I've been able to reproduce the issue here. When the library loads it seems to fairly quickly show the above Google page in the screenshot (without any sort of intervention required).

The code that extracts the PaRes from the HTML doesn't appear to be called (as I had debug code to spit out the HTML if that was hit). There are the following cryptic log lines though which might offer a clue:

2019-01-11 12:55:07.033 10790-12869/com.scoffable W/chromium: [WARNING:spdy_session.cc(2876)] Received RST for invalid stream1
2019-01-11 12:55:07.079 10790-10790/com.scoffable I/chromium: [INFO:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'innerHTML' of undefined", source: https://www.google.com/ (1)

So, still not sure why this is happening, but it's definitely an issue. The employee who has the affected card is now off until Monday, so I can't reproduce again until then.

sandyscoffable commented 5 years ago

Ok, so the issue is clearly with this line:

view.loadUrl(String.format("javascript:window.%s.processHTML(document.getElementsByTagName('html')[0].innerHTML);", JavaScriptNS));

Hopefully I'll get a better idea of why on Monday.

ghost commented 5 years ago

@sandyscoffable Did you manage to reproduce this with Co-op card?

I am yet to find someone in our company who has a co-op card :-(

livotov commented 5 years ago

Thank you guys for awesome discussion and PR! Sorry for delayed merge, was quite hard and busy year, especially end of it.

sandyscoffable commented 5 years ago

@rihantvx - Sorry I've been embroiled in other high priority projects. I do have this on my list to pick up in the near future.

sandyscoffable commented 5 years ago

I've created https://github.com/LivotovLabs/3DSView/issues/23 to track the issue

LivotovLabs / 3DSView