"Spell-Jacking" mitigation ~ prevent sensitive data leak …

[dlehammer]

… from spell checker.

Background: Chrome & Edge Enhanced Spellcheck Features Expose PII, Even Your Passwords

[williamdes]

Thank you for raising awareness about this vulnerability, I committed this fix for the next phpMyAdmin version.

[BlackDex]

@dlehammer, is there a reason you have this marked as a WIP/Draft PR?

[jjlin]

Just to be clear (since the cited article doesn't mention it until almost the end, when it should be one of the first things mentioned), this issue likely has very limited impact since it relies on a non-default Enhanced spell check option to be set in the browser.

This option clearly discloses that it

Uses the same spell checker that’s used in Google search. Text you type in the browser is sent to Google.

so I doubt it would appeal to many users.

[BlackDex]

so I doubt it would appeal to many users.

That doesn't mean we shouldn't add this attribute. And, if I'm correct, I thought that MS Edge has that feature enabled by default.

[jjlin]

I didn't say I had a problem with the PR itself, just with how the article was written. As such, I was simply making the observation that this issue has much less impact than might be immediately apparent.

[dlehammer]

Hi @BlackDex ,

Regarding

.. is there a reason you have this marked as a WIP/Draft PR?

It's intentional as I'm unfamiliar with this code-base (~ but a happy user), and the draft status is intended to encourage the maintainers to chime in regarding any missing changes before the PR covers all sensitive inputs.
Especially dynamically generated input fields, if present, is hard to spot for a newbie :)

[dlehammer]

Hi @jjlin,

Regarding

.. I was simply making the observation that this issue has much less impact than might be immediately apparent.

While that might be true as generalization, vaultwarden primarily handles sensitive information, and I as user would expect a security related tool to address all known avenues of information leaks when possible.
Especially this type of autonomous background leak, that can be addressed via a small attribute addition (for now) :)

[RealOrangeOne]

Spellcheck isn't enabled on type="password" inputs. This issue only affects inputs which use a "Show password" toggle to change the input to type="text". I don't believe the admin inputs do this, so there isn't a problem here.

See also wagtail/wagtail#9855 and https://www.otto-js.com/news/article/chrome-and-edge-enhanced-spellcheck-features-expose-pii-even-your-passwords

[tessus]

@dlehammer

and the draft status is intended to encourage the maintainers to chime in regarding any missing changes

the problem is that the PR cannot be merged. Even if you think there is more to be done, it could already be ready. But people have to get back to you and ask you to move it out of the Draft state.

To but it bluntly: A draft PR is only used, if

• the code is not intended to be merged - ever
• if the project has additional code owner and reviewer rules, which would minimize the notifications
• if it is a huge PR that truly is in its first stages and you don't even want others to look at it yet

Well, this is at least my opinion on this subject.

[dlehammer]

Hi @RealOrangeOne,

Spellcheck isn't enabled on type="password" inputs.

Sounds sensible, I've adjusted accordingly 🤞 (9b20dec)

[dlehammer]

Moved to ready for review, as advised by @tessus 🤓