search

kee-org / browser-addon

Kee adds free, secure and easy password management features to your browser which save time and keep your private data more secure.
https://www.kee.pm
Other
453 stars 41 forks source link

Fix extension error when form contains elements with name set to "id" or "name" #302

Closed burtek closed 3 years ago

burtek commented 3 years ago

Fixes extension error when form contains elements with name attribute set to "id" or "name". See https://github.com/kee-org/browser-addon/issues/297#issuecomment-772832415, https://forum.kee.pm/t/kee-browser-extension-bug-using-form-name-rather-than-form-getattribute-name/3503/3 and https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/name

Bug introduced in https://github.com/kee-org/browser-addon/commit/21efb9d5baccc90f9cce11cdfca24161a08867e2#diff-28be9268beaee14e36d782ac5832cab19593348724c0caf74b2d79c0d349257eR454-R455

Fixes #297

luckyrat commented 3 years ago

Thanks for the PR.

Unfortunately if we use getAttribute we can miss legitimate values for name and id that are set by Javascript code.

Therefore, I think we should use that as a fallback only if form.id or form.name are not strings.

Realistically, I'm not even sure if any websites will actually set the form name attribute if they use a field with name=name but we need to be sure we don't introduce any regression while fixing this issue.

Would you be able to update your PR as suggested above?

Don't worry too much about the commit descriptions - I'll squash and rebase onto master when you're done anyway.

burtek commented 3 years ago

Sure, will change that in about an hour

czw., 4 lut 2021, 17:25 użytkownik Chris Tomlinson notifications@github.com napisał:

Thanks for the PR.

Unfortunately if we use getAttribute we can miss legitimate values for name and id that are set by Javascript code.

Therefore, I think we should use that as a fallback only if form.id or form.name are not strings.

Realistically, I'm not even sure if any websites will actually set the form name attribute if they use a field with name=name but we need to be sure we don't introduce any regression while fixing this issue.

Would you be able to update your PR as suggested above?

Don't worry too much about the commit descriptions - I'll squash and rebase onto master when you're done anyway.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/kee-org/browser-addon/pull/302#issuecomment-773434015, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWOAHR4IEBJ7DQIJQGE6NDS5LC7TANCNFSM4XBTN62Q .

burtek commented 3 years ago

@luckyrat done. Sorry about those commits, had not yet configured my IDE fully on new PC and was kinda editing it in scratchpad