clients
clients copied to clipboard
bitwarden
Improve form submission detection
This is related to my post on the community forums. I decided to separate it into two different PRs as for the pop up icons there's still a lot of work to be done, let me know if I should keep it all here.
To sum up this PR, there are basically two kinds of forms, HTML ones and JavaScript ones. The latter is usually the problem with bitwaden's current detection code, as they're submitted in the background without emitting the usual 'submit' event. To combat this, it tries to look for and listen to submission buttons, which while it sometimes works, it's very unreliable, for example, during my initial attempt at trying to get it to work with Google login, it kept detecting the 'Sign in' title as a button. It will most likely also not work if the page is in any language but English. I decided to investigate how other extensions where detection these forms, and implemented my own version of it. The core of how it works is by proxying method calls to fetch and XMLHttpRequest, and when an event that could lead to a form submit is detected, such as clicking a button or pressing the Enter key, it starts listening for form data in these calls, and considers the form as submitted if any is found.
The data is then sent to the background script, which tries to distinguish the type of form and finally calls one of the saving methods. A cache is used for forms where only the username is detected, as support for login flows with more than one page. Please let me know if you find any pages where this breaks, and I'll do my best to rule out any edge cases.
@djsmith85
@djsmith85 maybe you could look at this as part of your work on this feature?
Hi @fuwwy thank you for the contribution. This looks very interesting. Definitely keep the PRs separate as those are two different features. It would also bloat the PR and would take longer to get merged. Less work for both of us 😉
I'll definetly look into it and give you feedback. I am going on vacation though as of Saturday and this will take some time to review and test. In the meantime could you please check that the linter is happy and reduce if-nesting where possible/sensible.
Hey @djsmith85, thanks for the quick reply. I'll keep the work on the pop-ups on a separate branch based on this one, there's only one method that is shared so there shouldn't be much to worry about conflicts. As for the linter, it doesn't give me any warnings, but I noticed some issues with indentation shortly after committing. I'll look into that and the if-nesting soon. Wish you the best on your vacation 🙂
Hi @fuwwy I'm sorry it's taken me so long to come back to this. I hadn't forgotten about it, just busy with other things in regards to the notificationBar. The thing that I haven't gotten around to, is a way to test this properly (at best automatically). I'd like to compare the current detection with your changes and be sure it at least behaves the same way. Also identifying sites which are not handled by our current detection but work with your changes.
Would you still be up to contributing and if so, would you be able to update your branch with the current changes in master? You'll have merge conflicts but they should be easy to solve. I refactored the notification stuff out of runtime.background.ts and moved it into notification.background.ts.
Hey @djsmith85, sorry for the long wait. I've merged the changes from master on both of my branches, but I don't quite remember what work was left to be done. I think this branch is mostly ready, and might just need a few changes to fix any unsupported websites that arise.
Over at features/popup-overlays the current roadblock is that the popup feature currently doesn’t work in Firefox as it doesn’t allow usage of getBackgroundPage in iframes. This might mean that the cipher listing and password generator need to be reimplemented outside of main angular app, which I’d like to avoid. If you have any hint on how to go about this let me know.
Just wanted to note that the issue #713 also affects this area. querySelector and querySelectorAll won't traverse into ShadowDOM elements. I got a workaround for that here: ~~https://github.com/bitwarden/clients/pull/2283#issuecomment-1326273156~~ PR #4119
Probably it's also a good idea to eventually merge the logic for autofill and form submission detection, so the DOM only needs to be traversed once to find all elements related to both of these "tasks".
Another note: The merged logic for form submission and autofill should probably be based on a shared service which listens for all DOM changes via MutationObserver (also inside ShadowDOM) and keep track of all interesting elements (forms, inputs, ...).
The form submission and autofill services can then subscribe to that shared service to receive all these elements and decide what to do with them. First initially after page load and then also on every mutation.
This should lead to clean code and a decent performance, because the DOM is only traversed and watched by a single source.