browserpass / browserpass-legacy

Legacy Browserpass repo, development is now happening at:
https://github.com/browserpass/browserpass-extension
MIT License
998 stars 87 forks source link

browserpassFillForm - support sites that override Event such as dell.com #309

Closed bufke closed 5 years ago

bufke commented 5 years ago

General information

Using inject.js directly injected via dev console.


Exact steps to reproduce the problem

  1. Go to dell.com
  2. Click log in
  3. Attempt to autofill

What should happen?

Autofill successfully.

What happened instead?

Autofill does not work, instead we get Failed to execute 'dispatchEvent' on 'EventTarget': parameter 1 is not of type 'Event'.

The problem

The issue appears to be that some sites define their own Event. We can see easily by typing in Event in a browser dev console on dell.com's login and again on any other website. Event should be native code, but in dell.com (and presumably others using this questionable technique) it is not. A search for the error finds others doing the same thing.

Suggested Fix

The click and focus events are not strictly required and only help in getting sites to revalidate the forms that were autofilled instead of typed by a person. As such, I recommend simply ignoring errors. This fix makes browserpass work on dell.com

     try{
        el.dispatchEvent(new Event(eventName, { bubbles: true }));
      }catch(e) {
        console.warn('Unable to dispatchEvent ' + eventName, e)
      }

This must be applied to both el.dispatchEvent lines. I'd be happy to submit a pull request but wanted to run this by someone first, as ignoring errors outright is maybe not the best solution.

Tangent

I found this while comparing the performance of browserpass's inject.js against other open source password managers. I compared Mitro's, which I'm using myself on Passit, and 1password (notable because bitwarden uses theirs). inject.js actually holds up very well despite being a much smaller and easier to read code base. I'd be happy to collaborate further if you'd like and set up a shared package to autofill login forms. This would make finding and fixing sites that do not work easier to do. In any case you may be interested to know I'm evaluating replacing my current solution (Mitro autofill, but with jquery stripped out) with yours. I'm also interested in trying to reproduce edge cases like these as unit tests.

maximbaz commented 5 years ago

Nice catch, agree with the approach, but wait with the PR for now — releasing v3 is the next milestone, I want to focus on that right now instead of releasing v2 patches.

Having inject.js shared is theoretically interesting, but in practice I haven't actually heard from other password manager authors, but also inject.js does change from time to time and I'd like to keep having an easy control of it. By the way, if you are curious, check out current version of inject.js from v3.

Having unit tests is interesting, but I'm not sure how difficult that would be to implement in practice, if you decide to investigate this, I would love to hear what you find.

bufke commented 5 years ago

I have only 2 tests right now here. It's not easy to write when the problem is interfering Javascript. My idea is to identify why form fill fails for any site that it has trouble with, then write a test as an approximation of what caused it to fail.

Thanks for pointing out v3, I didn't notice that. For now I'll continue to report any bugs I find (and fixes). That will also let me rebase my code off of it easily. If you'd like to contribute to testing, I'd be happy to set up a repo and CI that runs the tests.

Here are my test sites btw. Passit 1.11 should be good to go with browerpass's implementation. :smile:

maximbaz commented 5 years ago

Cool, and those 2 tests look quite interesting 👍 I'll get back to you when things clarify with v3 a bit...

By the way, just tested both browserpass v2 and v3 on these websites, and it is able to fill credentials everywhere just fine...

Not sure what's wrong with your way of testing on dell.com, but with chase.com, browserpass is injecting the script in each iframe, that's how it manages to fill the form.

bufke commented 5 years ago

I understand the iframe issue (I was copying in the script manually thus not affecting every frame). I'm surprised version 2 is working for you on dell though. I'll look more into it later.

bufke commented 5 years ago

I can confirm it works from the actual extension but not when I copy it to console. I have no idea why. It very clearly errors with a TypeError when injected manually. I copied inject.js into the console. Then run window.browserpassFillForm({u: "username", p: "pass"}, false). Up to you if you want to just close this or look into why this happens.

maximbaz commented 5 years ago

Here's the answer 😉 https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Content_scripts#Content_script_environment

Content scripts can access and modify the page's DOM, just like normal page scripts can. They can also see any changes that were made to the DOM by page scripts.

However, content scripts get a "clean view of the DOM". This means:

  • content scripts cannot see JavaScript variables defined by page scripts
  • if a page script redefines a built-in DOM property, the content script will see the original version of the property, not the redefined version.