Focus vulnerab fix #319
Focus vulnerab fix #319
Conversation
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
| const moved = new CustomEvent('mouseMove', {detail: {x: x + 50, y: y + 15}}) | ||
| window.dispatchEvent(moved) | ||
| }, {x, y}) | ||
| } |
There was a problem hiding this comment.
The hover state is passed by the native layer, so now we need to send it manually here in the tests.
| previousY = event.detail.y | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the meat of the PR. Basically, it avoids setting the .currentFocus class if the mouse has not moved.
| @@ -62,8 +62,10 @@ ${css} | |||
| this.autofillButtons.forEach((btn) => { | |||
| this.registerClickableButton(btn, () => { | |||
| // Fire only if the cursor is hovering the button | |||
| if (btn.matches('.tooltip__button:hover, .currentFocus')) { | |||
| if (btn.matches('.wrapper:not(.top-autofill) button:hover, .currentFocus')) { | |||
There was a problem hiding this comment.
In these we check that the button does have the hover state, either the browser's native or our own .currentFocus.
| .tooltip__button:hover .label, | ||
| .tooltip__button.currentFocus .label, | ||
| .tooltip__button:hover .label { | ||
| .wrapper:not(.top-autofill) .tooltip__button:hover .label { |
There was a problem hiding this comment.
These were unnecessary duplicates.
| @@ -106,7 +106,7 @@ | |||
| border-radius: 6px; | |||
| } | |||
| .tooltip__button.currentFocus, | |||
| .tooltip__button:hover { | |||
| .wrapper:not(.top-autofill) .tooltip__button:hover { | |||
There was a problem hiding this comment.
The hover state is only present in the in-page tooltips. If it's a top-autofill (the native webview) we use .currentFocus.
| safeExecute(this.activeButton, handler, { | ||
| checkVisibility: this.options.checkVisibility | ||
| }) | ||
| if (this.activeButton.matches('.wrapper:not(.top-autofill) *:hover, .currentFocus')) { |
There was a problem hiding this comment.
❓ Is there a reason we use *:hover here and button:hover for a similar check in src/UI/DataHTMLTooltip.js?
There was a problem hiding this comment.
Yes, it's because the in-context signup also has an anchor tag. I could have used the :is() selector but I'm not sure all our supported versions will work there.
Reviewer: @alistairjcbrown
Asana: https://app.asana.com/0/0/1204595906186359/f
Description
This addresses a potential clickjacking using the focus event. More info on Asana.
Steps to test
More info on Asana.