Fill fields inside shadow roots

[raxod502]

Fairly straightforward. I just introduce a function queryShadowRoots that converts a single parent element into a list that contains the parent plus all shadow roots amongst its descendants. Then queryAllVisible invokes it and iterates through the results, applying querySelectorAll to each individual shadow root (and the original parent).

I tested this and it seemed to work right away, allowing autofill to succeed on one of my sites where the login form is inside a shadow root.

I recommend viewing the diff with whitespace changes disabled.

Closes #73

[erayd]

Interrogating every single element on the page just to find the roots can be quite slow, and is a notable performance issue on pages with a large number of elements. I would suggest instead collating a list of shadow roots as they are created, or marking them in some way that will allow querySelect() to locate them later. This is vastly quicker.

This can be done by intercepting the attachShadow function via a content script in the page context at document_start.

For example:

var _attachShadow;
if (!_attachShadow) {
    _attachShadow = Element.prototype.attachShadow;
    Element.prototype.attachShadow = function (options) {
        this.setAttribute("is-shadow", "");
        return _attachShadow.call(this, options);
    };
}

Which allows quickly getting the roots later using document.querySelectorAll("[is-shadow]"). This approach also works well with recursion, if you want to support nested shadow roots.

[raxod502]

Hmm, not a bad idea, the thing about that though is we have to start injecting code into every webpage at startup, whereas the current architecture only has us injecting any logic once the extension menu icon is interacted with and a password entry is selected.

I would be somewhat leery of going to a model where the extension has to have access to all website data even when not actively being used, maybe it could be opt in though?

[erayd]

...the thing about that though is we have to start injecting code into every webpage at startup, whereas the current architecture only has us injecting any logic once the extension menu icon is interacted with and a password entry is selected.

This is true. However, the injected code would be extremely lightweight, and is essentially a noop where shadow roots aren't used.

I would be somewhat leery of going to a model where the extension has to have access to all website data even when not actively being used, maybe it could be opt in though?

It's not a move to that model; Browserpass already holds (and uses) the all-sites permission, and therefore already has access to all site data:

browserpass-extension/src/manifest-chromium.json

Line 41 in f4fffcd

"host_permissions": ["http://*/*", "https://*/*"],

[raxod502]

Browserpass already holds (and uses) the all-sites permission, and therefore already has access to all site data

Mmm, I think the way the host permission is handled is a bit different than the other ones, the manifest doesn't say anything about it being optional, but check out how it renders in the Firefox preferences:

So at the minimum we'd have to implement in such a way that we only use the new approach when the permission has been granted.

[erayd]

Mmm, I think the way the host permission is handled is a bit different than the other ones, the manifest doesn't say anything about it being optional, but check out how it renders in the Firefox preferences:

I think this is just Firefox's way of making it revocable by the user. Chrome has a similar thing on the context menu for it. However, we hold the permission by default - it's not something we need to ask for, and unless the user has manually removed it later, we should already hold it on all existing installs.

So at the minimum we'd have to implement in such a way that we only use the new approach when the permission has been granted.

Easy enough. Just have the injected script drop an identifier in the DOM when it runs (e.g. an attribute on the document element), and if that identifier is missing when we go to fill credentials, then gracefully fall back to the slow / expensive check-all-the-elements approach.

[raxod502]

Fair enough. Will wait to hear back from @max-baz before doing more work though, as I'd like an indication that this will be merged at some point.

[max-baz]

@erayd designed and developed a big portion of this extension, I trust his judgment and happy to get this merged as soon as you two reach the stage where you both are happy with the implementation 👍

[raxod502]

Oh, okay! I'll fix this up as soon as I have a chance, then.

[raxod502]

I'm sorry this took so long. But I had some hours to spend on this today.

It is way more complicated than you would think to do the shadow root tracking approach. You can look at the last few commits (for which I pushed a revert for now, to keep the PR head working) to see some of the many issues I ran into.

I'll continue working on this, though.

[erayd]

@raxod502 Would a reference implementation help? I have a working system that you could look at if that's helpful. Not currently public (although will be later), but I can add you to the repo.

[erayd]

Also, looking at your commits: you don't need the staged injection stuff (early-inject-injector.js), or any injected script elements etc. Just set "world": "MAIN" for the shadow-tracking script in manifest.json to make it run in the same execution context as the page.

[raxod502]

Would a reference implementation help? I have a working system that you could look at if that's helpful

That would be quite helpful, yes!

[erayd]

Would a reference implementation help? I have a working system that you could look at if that's helpful

That would be quite helpful, yes!

@raxod502 Done - see https://github.com/parcel-pm/parcel. Feel free to open an issue on that repo if you need anything about it explained. You're also welcome to directly copy code from it for use in Browserpass if you want.

[raxod502]

So a problem with the implementation in Parcel is that, per https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-conformance,

When authoring custom element constructors, authors are bound by [...] The element must not gain any attributes or children

Which means that if a custom element tries to call attachShadow in its constructor, then the setAttribute call in the instrumented attachShadow will cause an error. Even worse, it doesn't cause an error right away, but rather the browser finds out about the extra attribute after the constructor is finished, and causes the entire element construction to fail, and the custom element to fail to render. Thus adding a try clause does not help the situation.

An example of a webpage which triggers this behavior: https://raxod502.github.io/browserpass-test-website/index.html (it's just a publicly hosted version of the test code originally shared in #73 (comment))

It's a pretty bad issue, because we are having the new script be loaded in every webpage, meaning that some page elements will fail out even if the user doesn't try to interact with Browserpass.

Is there a way you're working around this in Parcel?

[erayd]

Is there a way you're working around this in Parcel?

I'm daily-driving Parcel, and I can't say I've noticed any issues like this at all - but this is definitely a real problem that you have found, and I appreciate you highlighting it.

I've now added a workaround in Parcel by deferring the hook behind a setTimeout(), which takes it out of the execution context of the constructor (parcel-pm/parcel#36). Is there any scenario you are aware of where this solution will not work?

"use strict";

// Track shadow roots as they are created so we don't have to search for them later
var _attachShadow;
if (!_attachShadow) {
    _attachShadow = Element.prototype.attachShadow;
    Element.prototype.attachShadow = function (options) {
        const root = _attachShadow.call(this, options);
        setTimeout(() => {
            // move the hook logic outside of the attachShadow call to avoid it from running inside anybody's custom element constructor
            const hostUUID = crypto.randomUUID();
            this.setAttribute("is-shadow", "");
            this.setAttribute("parcel-shadow-host", hostUUID);
            root.addEventListener("click", (ev) => {
                const evUUID = crypto.randomUUID();
                ev.target.setAttribute("parcel-shadow-event", evUUID);
                document.dispatchEvent(new CustomEvent("parcel-shadow-click", { detail: { host: hostUUID, target: evUUID } }));
            });
        }, 0);
        return root;
    };
}

[raxod502]

Alrighty! Thank you for all the help, I rewrote my code and everything seems to work nicely now. I tested https://raxod502.github.io/browserpass-test-website/index.html as well as https://reddit.com/login/ on both Firefox and Chromium, and logging statements indicated that the shadow root detection is being detected at form filling time, and is successfully being used to find shadow roots.

[max-baz]

I confirmed with @erayd that he is happy with your change, and so am I - thank you very much for your contribution!