Upstream/bitwarden-browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser.git synced 2024-09-19 02:51:14 +02:00
Code Issues Projects Releases Wiki Activity

[PM-9342] Inline menu does not show on username field for a form that has a password field with an invalid autocomplete value (#9860)

Browse Source 
* [PM-9342] Inline menu does not show on username field for a form that has a password field with an invalid autocomplete value

* [PM-9342] Incorporating logic to handle multiple autocomplete values within a captured set of page details

* [PM-9342] Incorporating logic to handle multiple autocomplete values within a captured set of page details

* [PM-9342] Changing logic for how we identify new password fields to reflect a more assertive qualification

* [PM-9342] Adding feedback from code review
This commit is contained in:
Cesar Gonzalez 2024-06-28 14:57:32 -05:00 committed by GitHub
parent a613d9c268
commit 4e3fb99b9f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 151 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

82
apps/browser/src/autofill/services/inline-menu-field-qualification.service.spec.ts
View File

@ -34,10 +34,23 @@ describe("InlineMenuFieldQualificationService", () => {
);
});
it("has a keyword value that indicates the field is for a create account form", () => {
const field = mock<AutofillField>({
type: "password",
placeholder: "create account password",
autoCompleteType: "",
});
expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
false,
);
});
it("has a type that is an excluded type", () => {
AutoFillConstants.ExcludedAutofillLoginTypes.forEach((excludedType) => {
const field = mock<AutofillField>({
type: excludedType,
autoCompleteType: "",
});
expect(
@ -53,6 +66,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: index === 0 ? attribute : "",
htmlName: index === 1 ? attribute : "",
placeholder: index > 1 ? attribute : "",
autoCompleteType: "",
});
expect(
@ -67,6 +81,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "not-password",
htmlName: "not-password",
placeholder: "not-password",
autoCompleteType: "",
});
expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
@ -80,6 +95,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "something-else",
htmlName: "something-else",
placeholder: "something-else",
autoCompleteType: "",
});
expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
@ -93,6 +109,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "search",
htmlName: "something-else",
placeholder: "something-else",
autoCompleteType: "",
});
expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
@ -112,12 +129,14 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "",
autoCompleteType: "",
});
const secondField = mock<AutofillField>({
type: "password",
htmlID: "some-other-password",
htmlName: "some-other-password",
placeholder: "some-other-password",
autoCompleteType: "",
});
pageDetails.fields = [field, secondField];
@ -133,18 +152,21 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "",
autoCompleteType: "",
});
const usernameField = mock<AutofillField>({
type: "text",
htmlID: "user-username",
htmlName: "user-username",
placeholder: "user-username",
autoCompleteType: "",
});
const secondUsernameField = mock<AutofillField>({
type: "text",
htmlID: "some-other-user-username",
htmlName: "some-other-user-username",
placeholder: "some-other-user-username",
autoCompleteType: "",
});
pageDetails.fields = [field, usernameField, secondUsernameField];
@ -186,6 +208,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "validFormId",
autoCompleteType: "",
});
const secondField = mock<AutofillField>({
type: "password",
@ -193,6 +216,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "some-other-password",
placeholder: "some-other-password",
form: "validFormId",
autoCompleteType: "",
});
pageDetails.fields = [field, secondField];
@ -218,12 +242,35 @@ describe("InlineMenuFieldQualificationService", () => {
);
});
it("is structured on a page with a single set of username and password fields", () => {
const field = mock<AutofillField>({
type: "password",
htmlID: "user-password",
htmlName: "user-password",
placeholder: "user-password",
autoCompleteType: "",
});
const usernameField = mock<AutofillField>({
type: "text",
htmlID: "user-username",
htmlName: "user-username",
placeholder: "user-username",
autoCompleteType: "",
});
pageDetails.fields = [field, usernameField];
expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
true,
);
});
it("has a type of `text` with an attribute that indicates the field is a password field", () => {
const field = mock<AutofillField>({
type: "text",
htmlID: null,
htmlName: "user-password",
placeholder: "user-password",
autoCompleteType: "",
});
expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
@ -247,6 +294,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "user-username",
htmlName: "user-username",
placeholder: "user-username",
autoCompleteType: "",
});
pageDetails.fields = [field, usernameField];
@ -273,6 +321,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "validFormId",
autoCompleteType: "",
});
const secondPasswordField = mock<AutofillField>({
type: "password",
@ -280,6 +329,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "some-other-password",
placeholder: "some-other-password",
form: "anotherFormId",
autoCompleteType: "",
});
const usernameField = mock<AutofillField>({
type: "text",
@ -287,6 +337,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-username",
placeholder: "user-username",
form: "validFormId",
autoCompleteType: "",
});
pageDetails.fields = [field, secondPasswordField, usernameField];
@ -310,6 +361,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "some-other-password",
placeholder: "some-other-password",
form: "anotherFormId",
autoCompleteType: "",
});
pageDetails.fields = [field, secondPasswordField];
@ -347,21 +399,23 @@ describe("InlineMenuFieldQualificationService", () => {
});
});
["new", "change", "neue", "ändern"].forEach((keyword) => {
it(`has a keyword of ${keyword} that indicates a 'new or changed' username is being filled`, () => {
const field = mock<AutofillField>({
type: "text",
autoCompleteType: "",
htmlID: "user-username",
htmlName: "user-username",
placeholder: `${keyword} username`,
});
["new", "change", "neue", "ändern", "register", "create", "registration"].forEach(
(keyword) => {
it(`has a keyword of ${keyword} that indicates a 'new or changed' username is being filled`, () => {
const field = mock<AutofillField>({
type: "text",
autoCompleteType: "",
htmlID: "user-username",
htmlName: "user-username",
placeholder: `${keyword} username`,
});
expect(
inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails),
).toBe(false);
});
});
expect(
inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails),
).toBe(false);
});
},
);
describe("does not have a parent form element", () => {
beforeEach(() => {

93
apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts
View File

@ -14,9 +14,18 @@ export class InlineMenuFieldQualificationService
private usernameAutocompleteValues = new Set(["username", "email"]);
private fieldIgnoreListString = AutoFillConstants.FieldIgnoreList.join(",");
private passwordFieldExcludeListString = AutoFillConstants.PasswordFieldExcludeList.join(",");
private currentPasswordAutocompleteValues = new Set(["current-password"]);
private newPasswordAutoCompleteValues = new Set(["new-password"]);
private autofillFieldKeywordsMap: WeakMap<AutofillField, string> = new WeakMap();
private autocompleteDisabledValues = new Set(["off", "false"]);
private newFieldKeywords = new Set(["new", "change", "neue", "ändern"]);
private accountCreationFieldKeywords = new Set([
"register",
"registration",
"create",
"confirm",
...this.newFieldKeywords,
]);
private inlineMenuFieldQualificationFlagSet = false;
constructor() {
@ -62,7 +71,12 @@ export class InlineMenuFieldQualificationService
): boolean {
// If the provided field is set with an autocomplete value of "current-password", we should assume that
// the page developer intends for this field to be interpreted as a password field for a login form.
if (field.autoCompleteType === "current-password") {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.currentPasswordAutocompleteValues,
)
) {
return true;
}
@ -95,7 +109,11 @@ export class InlineMenuFieldQualificationService
// If a single username field or less is present on the page, then we can assume that the
// provided field is for a login form. This will only be the case if the field does not
// explicitly have its autocomplete attribute set to "off" or "false".
return !this.autocompleteDisabledValues.has(field.autoCompleteType);
return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}
// If the field has a form parent and there are multiple visible password fields
@ -117,7 +135,10 @@ export class InlineMenuFieldQualificationService
// If the field has a form parent and no username field exists and the field has an
// autocomplete attribute set to "off" or "false", this is not a password field
return !this.autocompleteDisabledValues.has(field.autoCompleteType);
return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}
/**
@ -132,7 +153,9 @@ export class InlineMenuFieldQualificationService
): boolean {
// If the provided field is set with an autocomplete of "username", we should assume that
// the page developer intends for this field to be interpreted as a username field.
if (this.usernameAutocompleteValues.has(field.autoCompleteType)) {
if (
this.fieldContainsAutocompleteValues(field.autoCompleteType, this.usernameAutocompleteValues)
) {
const newPasswordFieldsInPageDetails = pageDetails.fields.filter(this.isNewPasswordField);
return newPasswordFieldsInPageDetails.length === 0;
}
@ -175,7 +198,10 @@ export class InlineMenuFieldQualificationService
// If the page does not contain any password fields, it might be part of a multistep login form.
// That will only be the case if the field does not explicitly have its autocomplete attribute
// set to "off" or "false".
return !this.autocompleteDisabledValues.has(field.autoCompleteType);
return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}
// If the field is structured within a form, but no password fields are present in the form,
@ -183,7 +209,12 @@ export class InlineMenuFieldQualificationService
if (passwordFieldsInPageDetails.length === 0) {
// If the field's autocomplete is set to a disabled value, we should assume that the field is
// not part of a login form.
if (this.autocompleteDisabledValues.has(field.autoCompleteType)) {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
)
) {
return false;
}
@ -212,7 +243,10 @@ export class InlineMenuFieldQualificationService
// If no visible password fields are found, this field might be part of a multipart form.
// Check for an invalid autocompleteType to determine if the field is part of a login form.
return !this.autocompleteDisabledValues.has(field.autoCompleteType);
return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}
/**
@ -237,7 +271,13 @@ export class InlineMenuFieldQualificationService
* @param field - The field to validate
*/
private isCurrentPasswordField = (field: AutofillField): boolean => {
if (field.autoCompleteType === "new-password") {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.newPasswordAutoCompleteValues,
) ||
this.keywordsFoundInFieldData(field, [...this.accountCreationFieldKeywords])
) {
return false;
}
@ -250,11 +290,19 @@ export class InlineMenuFieldQualificationService
* @param field - The field to validate
*/
private isNewPasswordField = (field: AutofillField): boolean => {
if (field.autoCompleteType === "current-password") {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.currentPasswordAutocompleteValues,
)
) {
return false;
}
return this.isPasswordField(field);
return (
this.isPasswordField(field) &&
this.keywordsFoundInFieldData(field, [...this.accountCreationFieldKeywords])
);
};
/**
@ -422,6 +470,31 @@ export class InlineMenuFieldQualificationService
return keywordValues;
}
/**
* Separates the provided field data into space-separated values and checks if any
* of the values are present in the provided set of autocomplete values.
*
* @param fieldAutocompleteValue - The field autocomplete value to validate
* @param compareValues - The set of autocomplete values to check against
*/
private fieldContainsAutocompleteValues(
fieldAutocompleteValue: string,
compareValues: Set<string>,
) {
if (!fieldAutocompleteValue) {
return false;
}
const autocompleteValueParts = fieldAutocompleteValue.split(" ");
for (let index = 0; index < autocompleteValueParts.length; index++) {
if (compareValues.has(autocompleteValueParts[index])) {
return true;
}
}
return false;
}
/**
* This method represents the previous rudimentary approach to qualifying fields for login forms.
*