Browser: Ctrl-Shift-L does not cycle through ALL matching logins reliably/consistently

[kriswilk]

Describe the Bug

I was happy to hear that Ctrl-Shift-L was enabled to cycle though matching logins for an open site. However my experience so far hasn't worked correctly on several sites. On many, the cycle gets stuck alternating between two (not always the same two!) logins.

Steps To Reproduce

1. Go to a site with multiple logins (my example video uses nintendo.com)
2. Cycle through matching logins with Ctrl-Shift-L

Expected Result

Bitwarden should cycle through all the matching logins one by one as Ctrl-Shift-L is pressed.

Actual Result

Often only 2 matching logins are cycled, ignoring other matching logins.

Screenshots or Videos

I have 6 logins for "nintendo.com" but in the linked video you can see that the keyboard shortcut only cycles through 2 of them.

VIDEO LINK: https://photos.app.goo.gl/xSZEAzjyj3s64CTD9

Environment

• Operating system: Windows 10
• Browser: Chrome 83.0.4103.116 (Official Build) (64-bit)
• Build Version (go to "Settings" → "About" in the app): 1.46.1

Additional Context

Before submitting this issue I was careful to check that all the affected logins have the EXACT SAME URL and matching preferences. So they should be seen as equally good matches for the target sites. Other than Nintendo, I know this problem happens at "rogers.com" and can report others as I find/remember them but there were more...

[AriZoNaiCe]

I will second this -- I was eagerly awaiting this change, but it seems to be somewhat picky about how/when it works. I have a few sites (chase.com, for example) where it doesn't cycle at all and other sites where it only cycles through two of my many logins (google.com, for example).

Additionally, on t-mobile.com, it cycles through my two logins on first use, but after I logout and go to log back in, it will not cycle at all.

The results are different but similar across Chrome and Firefox in my limited testing. In both cases, I am using 1.46.1.
Chrome: 85.0.4183.102 (Official Build) (64-bit)
Firefox: 80.0.1 (64-bit)

Hope this helps to move this feature forward. It was a tremendous idea, and I love that we have a good start in place!

[cscharf]

@xusoo, I would appreciate some help taking a look at this.

[Haxthausen]

I also noticed this issue on google.com. I have 12 logins, but it will only cycle through 5 or 6 of them.

However, if I keep the keyboard shortcut pressed instead of releasing each time, I can see that it cycles through all of them, but just real quick and I'm not able to stop at a specific one. Once I release the keys and press each time then it only cycles through 5 or 6 logins again, even if I loop around multiple times.

Thank you!

[RayBB]

I am poking around trying to understand the issue.

One thing that I'm seeing is that case 'autofill_cmd' is being reached twice even though processMessage is only being called once each time the hotkey is being used.
So it seems like it's going through two logins at a time.

[AriZoNaiCe]

I did consider that it was processing two logins at a time, but then I would have assumed it would never work if you had a site with two logins (always processing back to 0 index). On united.com, it does correctly rotate between each of my two logins correctly. It seems very hit and miss, which is strange.

[RayBB]

@sabattle is also looking at this. I noticed that this problem doesn't occur with 6 logins on https://app.monicahq.com/login but it does on reddit and gmail.

So it seems to be something about the sites that is triggering odd behavior in the extension.

[xusoo]

I think I've found the issue. Seems to be with sites with iframes (especially analytics and ads frames, which I had blocked 🤦‍♂️). The command executes once for each frame (including main tab), and then Bitwarden checks the one that actually needs to be filled. Problem is, prior to checking the iframe, I've already obtained the cipher, therefore moving one position forward in the sorted array, so this means it will skip a login for each iframe checked, hence the "random" cycling.

So at first, I've thought of just checking if the frame URL was the same as the tab URL, and then discarding all the wrong incoming commands, because I thought the login form was always in the main frame, but it is not: for instance, new Reddit loads the login form inside an iframe. (@kspearrin I think you check this here, but it's not really working because pageDetails.tab always hold the current tab, not the frame (which is in pageDetails.details). Or maybe this was meant for other purpose, I don't know.)

Anyway, I need to find a clean way to obtain the cipher without moving forward in the array, check if the frame is "fillable" and if it is, then make the move.

Any suggestions are welcome.

[xusoo]

Ok, so first working draft:

I've created a new class for wrapping the cipher and the function that will update the index:

export class SortedCipher {
    cipher: CipherView;
    updateLastUsedIndex?: () => void;
}

So this is what getNext() will return:

getNext(): SortedCipher {
    const nextIndex = (this.lastUsedIndex + 1) % this.ciphers.length;
    return {
        cipher: this.ciphers[nextIndex],
        updateLastUsedIndex: () => {
            this.lastUsedIndex = nextIndex;
        },
    };
}

And then, back in the autofill.service, the doAutoFillActiveTab looks like this:

let sortedCipher: SortedCipher;
if (fromCommand) {
    sortedCipher = await this.cipherService.getNextCipherForUrl(tab.url);
} else {
    sortedCipher = await this.cipherService.getLastUsedForUrl(tab.url);
}

const autoFillResponse: string = await this.doAutoFill({
    cipher: sortedCipher.cipher,
    pageDetails: [pageDetails],
    skipTotp: !fromCommand,
    skipLastUsed: !fromCommand,
    skipUsernameOnlyFill: !fromCommand,
    onlyEmptyFields: !fromCommand,
    onlyVisibleFields: !fromCommand,
});

// Only update last used index if doAutoFill didn't throw an exception
if (fromCommand) {
    sortedCipher.updateLastUsedIndex();
}

return autoFillResponse;

I don't love it, because it's a bit messy and you carry the function all the way back to the autofill service.

Another idea I had, was to do it it the other way around: instead of having the service return the cipher with getNextCipherForUrl, use a method like autoFillNextCipherForUrl(url, autofillCallback), so it would call the autofilling function itself and then only update the index if it doesn't throw an exception.

this.cipherService.autoFillNextCipherForUrl(tab.url, (cipher) => {
    return await this.doAutoFill({ ... });
});

But you also need to carry a callback all the way down to the ciphers cache class, so it's also not pretty. The only advantage is that you don't need to deal with updating the index, but doesn't look clean either.

I don't know, it's been a long working day and maybe I'm not thinking straight 😄 Let me know what you guys think and if you can come up with a better solution. Let's try to fix this as soon as possible 🙂

@kspearrin @cscharf

[xusoo]

Any comment on this? @kspearrin 🙂

[kspearrin]

@xusoo I'd like to avoid creating a new class again. Can you just create a function in the cipher service that you pass the CipherView into to set the lastUsedIndex?