Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FormAnalyzer] Tweak regex to match forgot password attribute #712

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

dbajpeyi
Copy link
Collaborator

@dbajpeyi dbajpeyi commented Nov 29, 2024

Reviewer: @GioSensation
Asana: https://app.asana.com/0/1200930669568058/1208866408334189/f

Description

(sharperimage.com, domain rank: ~47k)
It seems like we don't match a a pretty strong signal of forgotPassword or forgot-password attributes in the reset password form, causing form scores on sharperimages.com to be considered signup. Additionally the attribute text "Password reset request" ends up matching signup, because of the request part. This PR:

  1. Adds support for - in login regexes before the password token
  2. Adds negative lookahead for request so that it's ignored in signup regexes when reset comes before request.

Also note that the "Forgot password" heading are outside of the actual form attribute, so it'd be harder to catch them, since here we have literally a form element correctly encapsulating the fields
Screenshot 2024-11-29 at 17 25 21

Steps to test

  1. Go to sharperimage.com/account/forgot-password
  2. Email field should not show email protection.

@dbajpeyi dbajpeyi force-pushed the dbajpeyi/bug/sharperimages.com branch from 6429e4c to 765fbe2 Compare November 29, 2024 16:13
@@ -416,7 +416,7 @@ const matchingConfiguration = {
},
resetPasswordLink: {
match:
"(forgot(ten)?|reset|don't remember) (your )?password|password forgotten" +
"(forgot(ten)?|reset|don't remember)[-\\s]?(your )?password|password forgotten" +
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows forgot-password to be matched, which is a valid case. Note that this also allows forgotPassword to be matched now, creating the test results to change (see comment below).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, but how about this?

Suggested change
"(forgot(ten)?|reset|don't remember)[-\\s]?(your )?password|password forgotten" +
"(forgot(ten)?|reset|don't remember).?(your )?password|password forgotten" +

This way, it also catches _ and . : or whatever else they choose to use. It's also simpler to read.

@@ -120,7 +120,7 @@
{ "html": "samash_signup.html" },
{ "html": "financialtimes_login.html", "title": "Login" },
{ "html": "containerstore_login_signup.html" },
{ "html": "containerstore_forgot_password.html", "expectedFailures": ["username"] },
{ "html": "containerstore_forgot_password.html", "expectedFailures": ["password.new"] },
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we are breaking the password field now, but on the other hand we're fixing the username field. I think its a net win, given the site in question (sharperimages.com).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unfortunate. I've spent 30 mins looking for a quick win but couldn't find anything obvious.

Note that containerstore has a higher domain rank than what we're fixing now, but in this case we're just changing what breaks on containerstore, so it's not a net regression. FYI, you can check domain rank using https://tranco-list.eu/query.

Copy link
Collaborator Author

@dbajpeyi dbajpeyi Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i've been using that tool for triaging as well - but I also considered this is a neutral change as mentioned in the comment, regardless of the rank.

Actually we fix the "first step" of the process which is arguably better 😄

@dbajpeyi dbajpeyi marked this pull request as ready for review November 29, 2024 16:16
@dbajpeyi dbajpeyi changed the title fix: tweak regex to match the form attribute [FormAnalyzer] Tweak regex to match forgot password attribute Nov 29, 2024
@@ -384,7 +384,7 @@ const matchingConfiguration = {
},
signupRegex: {
match:
'sign(ing)?.?up|join|\\bregist(er|ration)|newsletter|\\bsubscri(be|ption)|contact|create|start|enroll|settings|preferences|profile|update|checkout|purchase|buy|^order|schedule|estimate|request|new.?customer|(confirm|re.?(type|enter)|repeat) password|password confirm' +
'sign(ing)?.?up|join|\\bregist(er|ration)|newsletter|\\bsubscri(be|ption)|contact|create|start|enroll|settings|preferences|profile|update|checkout|purchase|buy|^order|schedule|estimate|(?<!reset\\s)request|new.?customer|(confirm|re.?(type|enter)|repeat) password|password confirm' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a negative lookbehind, not lookahead, and it seems like it's not supported by some versions of Safari (therefore Webkit and the webview we use). On top of that, I tried removing it and the form seems to still be working, therefore I'd nix this to avoid compatibility with older versions of Webkit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I used it to give that extra score, but it's not necessary we keep it. I can remove it, maybe we bring it back when we have full support :)

@@ -416,7 +416,7 @@ const matchingConfiguration = {
},
resetPasswordLink: {
match:
"(forgot(ten)?|reset|don't remember) (your )?password|password forgotten" +
"(forgot(ten)?|reset|don't remember)[-\\s]?(your )?password|password forgotten" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, but how about this?

Suggested change
"(forgot(ten)?|reset|don't remember)[-\\s]?(your )?password|password forgotten" +
"(forgot(ten)?|reset|don't remember).?(your )?password|password forgotten" +

This way, it also catches _ and . : or whatever else they choose to use. It's also simpler to read.

@@ -120,7 +120,7 @@
{ "html": "samash_signup.html" },
{ "html": "financialtimes_login.html", "title": "Login" },
{ "html": "containerstore_login_signup.html" },
{ "html": "containerstore_forgot_password.html", "expectedFailures": ["username"] },
{ "html": "containerstore_forgot_password.html", "expectedFailures": ["password.new"] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unfortunate. I've spent 30 mins looking for a quick win but couldn't find anything obvious.

Note that containerstore has a higher domain rank than what we're fixing now, but in this case we're just changing what breaks on containerstore, so it's not a net regression. FYI, you can check domain rank using https://tranco-list.eu/query.

@dbajpeyi dbajpeyi force-pushed the dbajpeyi/bug/sharperimages.com branch from 0b0ce00 to b60c7da Compare December 5, 2024 11:43
@dbajpeyi dbajpeyi force-pushed the dbajpeyi/bug/sharperimages.com branch from 21be6ff to 0d05868 Compare December 5, 2024 11:44
Copy link
Member

@GioSensation GioSensation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@dbajpeyi dbajpeyi merged commit ca1722c into main Dec 6, 2024
1 check passed
@dbajpeyi dbajpeyi deleted the dbajpeyi/bug/sharperimages.com branch December 6, 2024 09:02
graeme pushed a commit to duckduckgo/BrowserServicesKit that referenced this pull request Dec 9, 2024
Task/Issue URL:
https://app.asana.com/0/1208923931185505/1208923931185505
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0


## Description
Updates Autofill to version
[16.0.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0).

### Autofill 16.0.0 release notes
## What's Changed
This PR introduces breaking changes for Android/Windows causing save
autofill prompt to get triggered in username only fields.
* Upgrade to shared eslint config + Adopt Prettier by @muodov in
duckduckgo/duckduckgo-autofill#695
* Ignore lint PR in git blame by @muodov in
duckduckgo/duckduckgo-autofill#699
* Update password-related json files (2024-11-18) by @daxmobile in
duckduckgo/duckduckgo-autofill#706
* [Form] Always scan shadow elements when categorizing the form inputs
by @dbajpeyi in
duckduckgo/duckduckgo-autofill#703
* Bump ts-to-zod from 3.1.3 to 3.14.0 by @dependabot in
duckduckgo/duckduckgo-autofill#710
* [FormAnalyzer] Fix paperlesspost.com login form by @dbajpeyi in
duckduckgo/duckduckgo-autofill#711
* [FormAnalyzer] Tweak regex to match forgot password attribute by
@dbajpeyi in duckduckgo/duckduckgo-autofill#712
* [Form] Trigger partialSave on username/email only form submit by
@dbajpeyi in duckduckgo/duckduckgo-autofill#702


**Full Changelog**:
duckduckgo/duckduckgo-autofill@15.1.0...16.0.0

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

Co-authored-by: dbajpeyi <[email protected]>
graeme added a commit to duckduckgo/iOS that referenced this pull request Dec 9, 2024
Task/Issue URL:
https://app.asana.com/0/1208923931185505/1208923931185505
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0
BSK PR: duckduckgo/BrowserServicesKit#1122

## Description
Updates Autofill to version
[16.0.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0).

### Autofill 16.0.0 release notes
## What's Changed
This PR introduces breaking changes for Android/Windows causing save
autofill prompt to get triggered in username only fields.
* Upgrade to shared eslint config + Adopt Prettier by @muodov in
duckduckgo/duckduckgo-autofill#695
* Ignore lint PR in git blame by @muodov in
duckduckgo/duckduckgo-autofill#699
* Update password-related json files (2024-11-18) by @daxmobile in
duckduckgo/duckduckgo-autofill#706
* [Form] Always scan shadow elements when categorizing the form inputs
by @dbajpeyi in
duckduckgo/duckduckgo-autofill#703
* Bump ts-to-zod from 3.1.3 to 3.14.0 by @dependabot in
duckduckgo/duckduckgo-autofill#710
* [FormAnalyzer] Fix paperlesspost.com login form by @dbajpeyi in
duckduckgo/duckduckgo-autofill#711
* [FormAnalyzer] Tweak regex to match forgot password attribute by
@dbajpeyi in duckduckgo/duckduckgo-autofill#712
* [Form] Trigger partialSave on username/email only form submit by
@dbajpeyi in duckduckgo/duckduckgo-autofill#702


**Full Changelog**:
duckduckgo/duckduckgo-autofill@15.1.0...16.0.0

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

---------

Co-authored-by: dbajpeyi <[email protected]>
Co-authored-by: Graeme Arthur <[email protected]>
graeme added a commit to duckduckgo/macos-browser that referenced this pull request Dec 9, 2024
Task/Issue URL:
https://app.asana.com/0/1208923931185505/1208923931185505
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0
BSK PR: duckduckgo/BrowserServicesKit#1122

## Description
Updates Autofill to version
[16.0.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0).

### Autofill 16.0.0 release notes
## What's Changed
This PR introduces breaking changes for Android/Windows causing save
autofill prompt to get triggered in username only fields.
* Upgrade to shared eslint config + Adopt Prettier by @muodov in
duckduckgo/duckduckgo-autofill#695
* Ignore lint PR in git blame by @muodov in
duckduckgo/duckduckgo-autofill#699
* Update password-related json files (2024-11-18) by @daxmobile in
duckduckgo/duckduckgo-autofill#706
* [Form] Always scan shadow elements when categorizing the form inputs
by @dbajpeyi in
duckduckgo/duckduckgo-autofill#703
* Bump ts-to-zod from 3.1.3 to 3.14.0 by @dependabot in
duckduckgo/duckduckgo-autofill#710
* [FormAnalyzer] Fix paperlesspost.com login form by @dbajpeyi in
duckduckgo/duckduckgo-autofill#711
* [FormAnalyzer] Tweak regex to match forgot password attribute by
@dbajpeyi in duckduckgo/duckduckgo-autofill#712
* [Form] Trigger partialSave on username/email only form submit by
@dbajpeyi in duckduckgo/duckduckgo-autofill#702


**Full Changelog**:
duckduckgo/duckduckgo-autofill@15.1.0...16.0.0

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

---------

Co-authored-by: dbajpeyi <[email protected]>
Co-authored-by: Graeme Arthur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants