Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bitwarden can't handle ShadowDOM #713

Closed
quthla opened this issue Sep 5, 2018 · 40 comments · Fixed by #4119
Closed

Bitwarden can't handle ShadowDOM #713

quthla opened this issue Sep 5, 2018 · 40 comments · Fixed by #4119
Labels
browser Browser Extension

Comments

@quthla
Copy link

quthla commented Sep 5, 2018

It won't find the login fields. 1Password supports this already in beta and Bitwarden should adapt to it too.

You can use this site for testing: https://vaadin.com/

@kspearrin
Copy link
Member

Seems to work fine with autofilling for me

@quthla
Copy link
Author

quthla commented Sep 5, 2018

In Chrome? Or FF?

@kspearrin
Copy link
Member

I tried Firefox.

@quthla
Copy link
Author

quthla commented Sep 5, 2018

Oh probably version 61. You must manually enable custom elements and shadow dom there in about:config or else it will be polyfilled and work as usual. It will be activated in 63 by default

@kspearrin
Copy link
Member

kspearrin commented Sep 5, 2018

Not sure I understand the issue.

@quthla
Copy link
Author

quthla commented Sep 5, 2018

Navigate to about:config in Firefox.

Enable dom.webcomponents.customelements.enabled and dom.webcomponents.shadowdom.enabled

In Chrome this is enabled by default already so maybe you can test there

@kspearrin
Copy link
Member

I can see the issue in Chrome. Thanks.

I don't really know much about shadow dom or why this would be occurring. Any ideas?

@quthla
Copy link
Author

quthla commented Sep 5, 2018

I'm not sure what the best practice is but you could maybe check 1Password code to see how they do it.

https://developer.mozilla.org/en-US/docs/Web/API/Element/shadowRoot

This will get you all the child nodes of that element which might contain other shadow dom nodes so maybe you need to traverse through every single node.

@fouram
Copy link

fouram commented Feb 13, 2020

Hi, is this issue getting any attention? This is still a problem for I'd assume many applications. HomeAssistant and Firefox Sync logins are two examples I can think of right off the top of my head, but any application that uses Shadow DOM with login fields could be affected. Now that all major browsers have implemented it, it's becoming more and more popular.

@lightmaster
Copy link

Is this an issue with Bitwarden or the websites in particular? The example website https://vaadin.com/ in the OP does let me autofill, but Home Assistant login page does not let me autofill.

@hukoeth
Copy link

hukoeth commented Mar 25, 2020

Home Assistant login doesn't work for me either. If you inspect the Home Assitant login page you can see that it uses ShadowDOM as well.

@sorryusernameisalreadytaken

I ca confirm HomeAssistant Login-Page. Any plans for this?

@hukoeth
Copy link

hukoeth commented May 21, 2020

I looked a bit closer at Home Assistant and I think for this particular login page it needs to be solved by the home assistant team.
There are several topics open for this:
https://community.home-assistant.io/search?q=password%20manager%20category%3A13

@lordgoata
Copy link

And the Home Assistant team says its needs to be fixed by the password managers...

home-assistant/frontend#1622 (comment)

shrug

@lukenorman
Copy link

We are seeing issues with bitwarden and our product. Login is a component, bitwarden isn't handling forms in the shadowDOM properly.

@ilium007
Copy link

Its not just BitWarden. Still can't fill auth page 3 years later.

@rgarrigue
Copy link

Go the issue with my bank's login page https://mon.cmso.com/auth/login, login & password input are under #shadow-root.

@BenjaminMaxdoro
Copy link

We enabled ShadowDOM for out web app and now the auto fill isn't working anymore. Still an issue.

@bitwarden-bot
Copy link

Hi @quthla,
We're cleaning up our repositories in preparation for a major reorganization. Issues from last year will be marked as stale and closed after two weeks. If you still need help, comment to let us know and we'll look into it.
Thanks!

@ilium007
Copy link

I’d say this is still a major problem

@lukenorman
Copy link

lukenorman commented Apr 12, 2022 via email

@rgarrigue
Copy link

Still have the issue.

I wish like there's Ctrl Shift L for autofill there was a shortcut to copy password (and one for user maybe). Since my bank like most website remember my username, I could shortcut + Ctrl V, would be good enough.

@dwbit dwbit removed the stale label Apr 19, 2022
@Hinton Hinton added the browser Browser Extension label May 5, 2022
addisonbeck pushed a commit that referenced this issue May 5, 2022
Hinton added a commit that referenced this issue Jun 3, 2022
* Move access logic to org model (#713)

* [feature] Allow for top level groupings to be collapsed (#712)

* [End User Vault Refresh] Refactor route permission checking (#727)

* Update admin access logic

* Centralize route permission handling

* Add permission check for disabled orgs

* [EndUserVaultRefresh] Add base routing guard (#732)

* Add a base class for Angular routing guards

* Update Guard naming convention

* Bump node-forge to 1.2.1 (#722)

* Remove Internet Explorer logic (#723)

* Username generator (#734)

* add support for username generation

* remove unused Router

* pr feedback

* Bump electron and related dependencies (#736)

* PS-91 make isMacAppStore return true/false (#735)

* return false if undefined from isMacAppStore

* PS-91 use strict equality instead of null coalescing

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

* [bug] Fix Safari CSV importer for URL and Notes (#730)

* Fix import path for safari importer (#740)

* Force updates to be silent (#739)

* support for username gen website setting (#738)

* Fix jslibModule forms (#742)

* Add DatePipe provider to JslibModule (#741)

* Feature/move to jest (#744)

* Switch to jest

* Fix jslib-angular package name

* Make angular test project

* Split up tests by jslib project

* Remove obsolete node test script

* Use legacy deps with jest-preset-angular

* Move web tests to common

* Remove build from pipeline

This was only being used because we were not using ts runners.
We are now, so build is unnecessary

* Remove the VerifyMasterPasswordComponent from jslib module (#747)

* Add ellipsis pipe to jslib module (#746)

* add ellipsis pipe to jslib module

* Add ellipsis pipe to exports

* Add ColorPasswordCountPipe to JslibModule (#751)

* Generator cleanup (#753)

* type is null by default

* rename generator component

* remove showWebsiteOption

* shorthand if check

* EC-134 Fix api token refresh (#749)

* Fix apikey token refresh

* Refactor: use class for TokenRequestTwoFactor

* Remove keytar and biometric logic (#706)

* [bug] CL - fix default button display and callout header class (#756)

* [EC-142] Fix error during import of 1pux containing new email field format (#758)

* Add support for complex email field type

* Ensure complex email field type gets imported on identities

* [euvr] Separate Billing Payment/History APIs (#750)

* [euvr] Separate Billing Payment/History APIs

* Updated to new accounts billing API

* Removed getUserBilling as it will become obsolete once merged

* [end user vault refresh] Base Changes For Vault Filters (#737)

* [dependency] Update icons

* Avoid duplicate fullSync api calls (#716)

* Tweak component library slightly (#715)

* Check runtime name vs mangled name (#724)

* Add Chromatic (#719)

* Update SECURITY.md (#725)

* Update SECURITY.md

Add link to our HackerOne program for submitting potential security issues.

* Revise language on SECURITY.md

* Remove error Response type check (#731)

* Remove error Response type check

Minimization is impacting type checking in a non-consistent way.
The previous type check works locally,
but not from build artifacts 🤷. We only set `captchaRequired` on
our errors when we want a resubmit with captcha included, so we're safe
keying off that

* linter

* [JslibModule] Add JslibModule (#733)

* Add ellipsis pipe (#728)

* add ellipsis pipe

* run prettier

* Account for ellipsis length in returned string

* Fix complete words case

* Fix another complete words issue

* fix for if there are not spaces in long value

* extract length check to beginning of method

* condense if statements

* remove log

* [refactor] Add optional folders param to folderService.getAllNested()

This will be used later for use cases where the vault filters service needs to build a list of nested folders that have been filtered by organization

* [feature] Add organization filters

This is an MVP implementation of the changes needed for the vault refresh. This includes collapsable top level groupings, and organization based filters that dynamically adjust folders and collections.

* [refactor] Break down vault filter into several components

These changes rename and rewrite the GroupingsComponent into a VaultFiltersModule. The module follows typical angular patterns for structure and purpose, and contain components for each filter type. The mostly communicate via Input and Output, and depend on a VaultFilterService for sending and recieving data from other parts of the product.

* [bug] Add missing events for folder add/edit

* [refactor] Dont directly change activeFilter in VaultFilterComponent

* [refactor] Move DisplayMode to a dedicated file

Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
Co-authored-by: Oscar Hinton <oscar@oscarhinton.com>
Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>

* [CL-16 Component Library] Menu Dropdown (#761)

* [bug] Add missing null check in vault filters (#769)

* [bug] Add @Injectable to VaultFilterService (#781)

* [fix] Ran prettier

* [fix] Fix merge issue

I used createUrlTree when merging guards because I knew that was the angular standard, didn't notice that redirect was a helper method from us

* Remove BaseGuard (#791)

Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
Co-authored-by: Thomas Rittson <trittson@bitwarden.com>
Co-authored-by: Oscar Hinton <oscar@oscarhinton.com>
Co-authored-by: Kyle Spearrin <kspearrin@users.noreply.github.com>
Co-authored-by: Jake Fink <jfink@bitwarden.com>
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
Co-authored-by: David Frankel <42774874+frankeld@users.noreply.github.com>
Co-authored-by: Daniel James Smith <djsmith85@users.noreply.github.com>
Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>
Co-authored-by: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com>
Co-authored-by: Vincent Salucci <vincesalucci21@gmail.com>
Hinton added a commit that referenced this issue Jun 3, 2022
* Billing Sync Api Keys / Free Bitwarden Families Page updates (#767)

* Work on billing sync api key maintenance

* Add sync status call

* Work on sync status copy

* Return actual model

* Update api calls/models

* Fix linting

* Run linting

* Add in notAllowedValueAsync.validator.ts (#774)

* Add in notAllowedValueAsync.validator.ts

* Fix lint error

* Run prettier

* [PS-248] Feature/manage billing sync connection (#770)

* Define org connection request and responses

* Add organization connection API CRUD

* Linter fixes

* Handle create vs update in component

* PR feedback

* Remove unused import

* Linter fixes

* Use self hosted endpoints for f4e (#779)

* Use self hosted endpoints for f4e

* Call the method

* Chore/merge/self hosted families for enterprise (#778)

* Remove keytar and biometric logic (#706)

* [bug] CL - fix default button display and callout header class (#756)

* [EC-142] Fix error during import of 1pux containing new email field format (#758)

* Add support for complex email field type

* Ensure complex email field type gets imported on identities

* Update introduction for CL (#729)

* Add jsdoc comments for user verification, password reprompt, and appApiAction (#754)

* Rename and add comments to clarify password reprompt classes

* Add comment for appApiAction

* copy default options (#764)

* Update jest configs to remove roots (#766)

* Remove support for alreadyEncrypted (#762)

* Add tests for domain models (#768)

* Fix language always defaulting to english (#765)

* Rename Export DTOs (#763)

* [BEEEP] Allow linking to ciphers (#760)

* Remove userId from data models (#771)

* Add reorganization notice (#776)

* Add reorganization notice

* [BEEEP] Add banner component (#759)

Co-authored-by: Oscar Hinton <oscar@oscarhinton.com>
Co-authored-by: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com>
Co-authored-by: Daniel James Smith <djsmith85@users.noreply.github.com>
Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
Co-authored-by: Kyle Spearrin <kspearrin@users.noreply.github.com>

* Chore/merge/self hosted families for enterprise (#796)

* Remove keytar and biometric logic (#706)

* [bug] CL - fix default button display and callout header class (#756)

* [EC-142] Fix error during import of 1pux containing new email field format (#758)

* Add support for complex email field type

* Ensure complex email field type gets imported on identities

* Update introduction for CL (#729)

* Add jsdoc comments for user verification, password reprompt, and appApiAction (#754)

* Rename and add comments to clarify password reprompt classes

* Add comment for appApiAction

* copy default options (#764)

* Update jest configs to remove roots (#766)

* Remove support for alreadyEncrypted (#762)

* Add tests for domain models (#768)

* Fix language always defaulting to english (#765)

* Rename Export DTOs (#763)

* [BEEEP] Allow linking to ciphers (#760)

* Remove userId from data models (#771)

* Add reorganization notice (#776)

* Add reorganization notice

* [BEEEP] Add banner component (#759)

* [EC-159] [BEEEP] Remove factory providers in Angular DI (#775)

* Forwarded email alias generation (#772)

* generate forwarded alias with SL and AD

* added forwarded email to type list

* add ApiService dep

* ApiServiceAbstraction

* use proper status codes

* only generate on button press

* reset username to `-`

* reset username when forwarded

* Authorization header for anonaddy

* use proper anonaddy json path

* firefox relay support

* update description for firefox

* log username generation errors

* PS-302 Added DeviceId to the 2FA email request and set it when calling the endpoint that's needed to see if it's a 2FA email because of a new device (#782)

* [EC-154] [BEEEP] Add token for localesDirectory (#783)

* Add token for localesDirectory

* Add token for SYSTEM_LANGUAGE

* [PS-74] Fix user authentication state checks (#721)

* Create authService.authStatus, refactor isLocked checks

* Rename authStatus -> getAuthStatus

* [CP-30] Added creditCardNumber pipe for viewing saved card numbers properly (#590)

Co-authored-by: Hinton <oscar@oscarhinton.com>

* Fix linting (#789)

* fix default forwardedService property name (#788)

* Stop clearing list on every reload (#784)

* [EC-151] Hide Subscription/Billing information for Provider-managed organizations (#777)

* add canManageBilling permission and hasProvider helper method

* [feat] End User Vault Refresh (#790)

* Move access logic to org model (#713)

* [feature] Allow for top level groupings to be collapsed (#712)

* [End User Vault Refresh] Refactor route permission checking (#727)

* Update admin access logic

* Centralize route permission handling

* Add permission check for disabled orgs

* [EndUserVaultRefresh] Add base routing guard (#732)

* Add a base class for Angular routing guards

* Update Guard naming convention

* Bump node-forge to 1.2.1 (#722)

* Remove Internet Explorer logic (#723)

* Username generator (#734)

* add support for username generation

* remove unused Router

* pr feedback

* Bump electron and related dependencies (#736)

* PS-91 make isMacAppStore return true/false (#735)

* return false if undefined from isMacAppStore

* PS-91 use strict equality instead of null coalescing

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

* [bug] Fix Safari CSV importer for URL and Notes (#730)

* Fix import path for safari importer (#740)

* Force updates to be silent (#739)

* support for username gen website setting (#738)

* Fix jslibModule forms (#742)

* Add DatePipe provider to JslibModule (#741)

* Feature/move to jest (#744)

* Switch to jest

* Fix jslib-angular package name

* Make angular test project

* Split up tests by jslib project

* Remove obsolete node test script

* Use legacy deps with jest-preset-angular

* Move web tests to common

* Remove build from pipeline

This was only being used because we were not using ts runners.
We are now, so build is unnecessary

* Remove the VerifyMasterPasswordComponent from jslib module (#747)

* Add ellipsis pipe to jslib module (#746)

* add ellipsis pipe to jslib module

* Add ellipsis pipe to exports

* Add ColorPasswordCountPipe to JslibModule (#751)

* Generator cleanup (#753)

* type is null by default

* rename generator component

* remove showWebsiteOption

* shorthand if check

* EC-134 Fix api token refresh (#749)

* Fix apikey token refresh

* Refactor: use class for TokenRequestTwoFactor

* Remove keytar and biometric logic (#706)

* [bug] CL - fix default button display and callout header class (#756)

* [EC-142] Fix error during import of 1pux containing new email field format (#758)

* Add support for complex email field type

* Ensure complex email field type gets imported on identities

* [euvr] Separate Billing Payment/History APIs (#750)

* [euvr] Separate Billing Payment/History APIs

* Updated to new accounts billing API

* Removed getUserBilling as it will become obsolete once merged

* [end user vault refresh] Base Changes For Vault Filters (#737)

* [dependency] Update icons

* Avoid duplicate fullSync api calls (#716)

* Tweak component library slightly (#715)

* Check runtime name vs mangled name (#724)

* Add Chromatic (#719)

* Update SECURITY.md (#725)

* Update SECURITY.md

Add link to our HackerOne program for submitting potential security issues.

* Revise language on SECURITY.md

* Remove error Response type check (#731)

* Remove error Response type check

Minimization is impacting type checking in a non-consistent way.
The previous type check works locally,
but not from build artifacts 🤷. We only set `captchaRequired` on
our errors when we want a resubmit with captcha included, so we're safe
keying off that

* linter

* [JslibModule] Add JslibModule (#733)

* Add ellipsis pipe (#728)

* add ellipsis pipe

* run prettier

* Account for ellipsis length in returned string

* Fix complete words case

* Fix another complete words issue

* fix for if there are not spaces in long value

* extract length check to beginning of method

* condense if statements

* remove log

* [refactor] Add optional folders param to folderService.getAllNested()

This will be used later for use cases where the vault filters service needs to build a list of nested folders that have been filtered by organization

* [feature] Add organization filters

This is an MVP implementation of the changes needed for the vault refresh. This includes collapsable top level groupings, and organization based filters that dynamically adjust folders and collections.

* [refactor] Break down vault filter into several components

These changes rename and rewrite the GroupingsComponent into a VaultFiltersModule. The module follows typical angular patterns for structure and purpose, and contain components for each filter type. The mostly communicate via Input and Output, and depend on a VaultFilterService for sending and recieving data from other parts of the product.

* [bug] Add missing events for folder add/edit

* [refactor] Dont directly change activeFilter in VaultFilterComponent

* [refactor] Move DisplayMode to a dedicated file

Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
Co-authored-by: Oscar Hinton <oscar@oscarhinton.com>
Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>

* [CL-16 Component Library] Menu Dropdown (#761)

* [bug] Add missing null check in vault filters (#769)

* [bug] Add @Injectable to VaultFilterService (#781)

* [fix] Ran prettier

* [fix] Fix merge issue

I used createUrlTree when merging guards because I knew that was the angular standard, didn't notice that redirect was a helper method from us

* Remove BaseGuard (#791)

Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
Co-authored-by: Thomas Rittson <trittson@bitwarden.com>
Co-authored-by: Oscar Hinton <oscar@oscarhinton.com>
Co-authored-by: Kyle Spearrin <kspearrin@users.noreply.github.com>
Co-authored-by: Jake Fink <jfink@bitwarden.com>
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
Co-authored-by: David Frankel <42774874+frankeld@users.noreply.github.com>
Co-authored-by: Daniel James Smith <djsmith85@users.noreply.github.com>
Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>
Co-authored-by: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com>
Co-authored-by: Vincent Salucci <vincesalucci21@gmail.com>

* [EC-192] Use ts-jest instead of deprecated ts-jest/utils (#792)

* [SG-230] “All Items” and “Trash” missing from Organization Vault (#795)

Co-authored-by: Oscar Hinton <oscar@oscarhinton.com>
Co-authored-by: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com>
Co-authored-by: Daniel James Smith <djsmith85@users.noreply.github.com>
Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
Co-authored-by: Kyle Spearrin <kspearrin@users.noreply.github.com>
Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>
Co-authored-by: Anthony Garera <gareraanthony@gmail.com>
Co-authored-by: Jake Fink <jfink@bitwarden.com>
Co-authored-by: Addison Beck <addisonbeck1@gmail.com>
Co-authored-by: Thomas Rittson <trittson@bitwarden.com>
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
Co-authored-by: David Frankel <42774874+frankeld@users.noreply.github.com>
Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>
Co-authored-by: Vincent Salucci <vincesalucci21@gmail.com>

Co-authored-by: Justin Baur <admin@justinbaur.com>
Co-authored-by: Justin Baur <136baur@gmail.com>
Co-authored-by: Oscar Hinton <oscar@oscarhinton.com>
Co-authored-by: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com>
Co-authored-by: Daniel James Smith <djsmith85@users.noreply.github.com>
Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
Co-authored-by: Kyle Spearrin <kspearrin@users.noreply.github.com>
Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>
Co-authored-by: Anthony Garera <gareraanthony@gmail.com>
Co-authored-by: Jake Fink <jfink@bitwarden.com>
Co-authored-by: Addison Beck <addisonbeck1@gmail.com>
Co-authored-by: Thomas Rittson <trittson@bitwarden.com>
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
Co-authored-by: David Frankel <42774874+frankeld@users.noreply.github.com>
Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>
Co-authored-by: Vincent Salucci <vincesalucci21@gmail.com>
@murilosr
Copy link

I'm having the same issue in Chrome too:

Google Chrome - Version 105.0.5195.125 (Official Build) (64-bit) at PopOS 22 (Linux)
Bitwarden v2022.9.1

The url is public and can be tested:
https://login.clear.com.br/

It doesn't fill the username and password and also doesn't accept custom field. When I try to copy the field name, this string is copied:

Unable to identify a valid form element. Try inspecting the HTML instead.

@larena1
Copy link

larena1 commented Sep 28, 2022

#2283

There's a PR aimed towards fixing this issue. Can that be merged in case it works? Was created in January but stale since then despite it being a relatively simple fix.

@eliykat @Hinton @coltonhurst @shane-melton

@eliykat
Copy link
Member

eliykat commented Sep 28, 2022

@larena1 I'll be reviewing that PR in the current sprint (next couple of weeks).

@larena1
Copy link

larena1 commented Nov 21, 2022

@eliykat is there a problem with the PR? It's nearly been two months when you told us a few weeks

@chadm-sq
Copy link
Contributor

chadm-sq commented Nov 21, 2022

@eliykat #2283 is my PR, and I think I found it doesn't work, or doesn't always work. I think the idea of recursive search is right, but perhaps this try is wrong.

@chadm-sq
Copy link
Contributor

@murilosr

Unable to identify a valid form element. Try inspecting the HTML instead.

This is a distinct, different bug. Please file that as a separate ticket.

@larena1
Copy link

larena1 commented Nov 21, 2022

1Password implemented it years ago. Maybe worth looking how they did it?

@chadm-sq
Copy link
Contributor

@larena1 , careful, you and others. Looking at a different copyrighted implementation would/may poison you for direct contributions on the matter, outside of very general descriptions.

@larena1
Copy link

larena1 commented Nov 21, 2022

In the end, there are most likely not too many ways to implement this anyway and reverse engineering is a common practice also when it comes to proprietary drivers (Nouveau, Panfrost) and stuff and is not forbidden either.

And by looking obviously I didn't mean just copying what they did but rather get some ideas how it can be done and maybe it'll even shed some light on why your implementation isn't always working.

I didn't test your code but recursively getting all nodes seems like a good approach. Any idea in which cases exactly it would fail and why?

@larena1
Copy link

larena1 commented Nov 22, 2022

https://gist.github.com/heyMP/8ef3912847dcc93304652a412981caca

Maybe this could also be of help?

@eliykat
Copy link
Member

eliykat commented Nov 23, 2022

Sorry for the lack of communication on this one, I've been busy with other initiatives and admit I lost track of time. I'll bump it up my list.

The approach in #2283 makes sense in principle, but unfortunately we don't have any automated testing in this area to verify. I need to take a closer look at it myself and do some manual testing.

@chadm-sq:

@eliykat #2283 is my PR, and I think I found it doesn't work, or doesn't always work. I think the idea of recursive search is right, but perhaps this try is wrong.

Does it work consistently for a particular website, but not all websites (that use ShadowDOM)? Or does it work inconsistently when repeatedly testing it on one website?

What site(s) are you testing it on?

Any idea why it's failing?

@chadm-sq
Copy link
Contributor

I don't know any of that, @eliykat. If I were testing or debugging now, I would have just fixed it. It's been months since then.

I gave it an hour. I found there was more to it. I left it to those who don't loathe javascript.

@larena1
Copy link

larena1 commented Nov 23, 2022

@chadm-sq I can absolutely understand you. It's a pity that your PR hasn't been reviewed long ago. This issue was also opened more than 4 years ago and many people have commented since then but nothing ever happened. Unfortunately this goes for all the Bitwarden sub projects. You hardly get any response at all. Seems they rather focus on enterprise features and support and not proper core functionality.

Nice job anyway, Chad!

@chadm-sq
Copy link
Contributor

@larena1, thanks for giving this your attention. It sounds like the test suite needs test data. You can do something useful by preparing a few test files that exercise this problem, so they can be added to the test suite. I think the three kinds are

  • an open shadow root with a form within, inside a normal html doc,
  • a closed shadow root with a form within, inside a normal html doc, and
  • a form within an open shadow root within a open shadow root within a normal document.

@larena1
Copy link

larena1 commented Nov 23, 2022

@chadm-sq you seem to know more about this than I do so it'd be great if you could compile the tests and maybe report to @eliykat what works and what still needs to be looked at.

@RafaelKr
Copy link
Contributor

RafaelKr commented Nov 24, 2022

I worked on autofill to also traverse into open and closed ShadowRoots. See #4119

CC @chadm-sq @larena1 @eliykat

I built a little PoC (proof-of-concept) which finds elements in the ShadowDOM. Didn't get it to a working implementation, but should be usable as a base.
See #2283 (comment) and #2283 (comment) and RafaelKr@8528a74

@eliykat
Copy link
Member

eliykat commented Nov 28, 2022

I've reviewed both #2283 and #4119, thanks again to the contributors. Let's continue the conversation on those PRs.

@eliykat
Copy link
Member

eliykat commented Feb 23, 2023

#4119 has been merged and this issue will be fixed in the March release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser Browser Extension
Projects
None yet