Skip to content

coinbase login form#31

Open
audreyality wants to merge 4 commits into
mainfrom
autofill/pm-39001/coinbase
Open

coinbase login form#31
audreyality wants to merge 4 commits into
mainfrom
autofill/pm-39001/coinbase

Conversation

@audreyality

Copy link
Copy Markdown
Member

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-39001

📔 Objective

Add a map for login.coinbase.com.

@audreyality audreyality requested a review from a team as a code owner June 25, 2026 14:13
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds a single Forms Map entry for login.coinbase.com (maps/forms/forms.jsonc). The entry is schema-valid — email, password, and username are valid field keys and next/submit are valid action keys — and all selectors carry tag anchors as the selector philosophy requires. No security, correctness, or breaking-change concerns. Two convention/intent points are noted below for the author's consideration.

Code Review Details
  • ♻️ : username duplicates the email selector and contradicts the README authentication guidance ("use email, not username")
    • maps/forms/forms.jsonc:25-27
  • ❓ : Single account-login form may be conflating a multi-step / 2FA flow (both next and submit actions; div#two-factor container) — consider whether separate forms entries fit better
    • maps/forms/forms.jsonc:23-32

Comment thread maps/forms/forms.jsonc Outdated
Comment on lines +25 to +27
"email": ["input[name='email-input']"],
"password": ["input#current-password"],
"username": ["input[name='email-input']"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

♻️ DEBT: username duplicates the email selector and contradicts the documented authentication guidance.

Details and fix

Both email and username map to the same selector input[name='email-input']. The Fields → Authentication section of maps/forms/README.md states:

In cases where an email is used for authentication, the email field key should be used, not username.

Since the dedicated email key is already present here, the username entry is redundant and runs counter to that guidance. Consider removing the username key:

"fields": {
  "email": ["input[name='email-input']"],
  "password": ["input#current-password"]
},

(The tiktok.com entry maps both keys, but its element is literally named username; here the element is an email input, so the email-only mapping is the documented choice.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is intentional. It is the username used to log into the website and only allows email addresses.

@jprusik jprusik Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

mapping email to username is a convention of the bitwarden browser client, not the forms map. We should drop the username entry, but in the interest of our short-term goals, I'd be fine with a FIXME comment to drop it later.

See also for context:

#19
#18 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm confused. Tiktok also allows emails to be usernames, and you gave the opposite feedback there.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, true; I also gave conflicting advice here, which I called out, so I don't blame you for the confusion (and why I attempted to clarify the new position in the docs).

I think the right thing is to mark it as something to revisit but leave it as is for now. We can reconvene on it as a team offline.

@audreyality audreyality Jun 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm... Ok, after thinking about it a bit I see there's a difference between these cases. Tiktok allows either kind of input in a single field. Coinbase only allows emails. So in this case, we should only use the email fill.

Addressed in a840e39.

Comment thread maps/forms/forms.jsonc Outdated
Comment on lines +23 to +32
"container": ["div#two-factor > div > form"],
"fields": {
"email": ["input[name='email-input']"],
"password": ["input#current-password"],
"username": ["input[name='email-input']"]
},
"actions": {
"next": ["button[name='email-submit-button']"],
"submit": ["button[name='password-submit-button']"]
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

QUESTION: Is this single account-login form correctly modeling what looks like a multi-step (and possibly 2FA) flow?

Details

Two signals suggest this entry conflates more than one screen into a single form:

  1. The actions include both next (email-submit-button) and submit (password-submit-button), which is the shape of a multi-step login where the email step and password step render separately. The email and password inputs would then be mutually exclusive on screen. The README's Selector Arrays note states mutually-exclusive inputs should be represented as independent forms array entries (see the tiktok.com precedent).
  2. The container is div#two-factor > div > form. A container named two-factor is unexpected for an email/password login form and may indicate the selector was captured from the 2FA/OTP screen rather than the credential screen.

Could you confirm the container and field selectors were all verified on the same rendered step, and whether splitting the email and password steps into separate forms entries better matches the page?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The email and password fields are on a multi-step form, but the page's URL doesn't change. It reveals the steps in place. I do not know what second factors are supported. That would require setting up a coinbase account.

Signed-off-by: ✨ Audrey ✨ <ajensen@bitwarden.com>
Comment thread maps/forms/forms.jsonc Outdated
Co-authored-by: Jonathan Prusik <jprusik@users.noreply.github.com>
Signed-off-by: ✨ Audrey ✨ <audrey@audreyality.com>

@jprusik jprusik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only big concern is adding the form selector context ahead of the field and action selectors

Comment thread maps/forms/forms.jsonc
Comment on lines +26 to +27
"email": ["input[name='email-input']"],
"password": ["input#current-password"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"email": ["input[name='email-input']"],
"password": ["input#current-password"]
"email": ["div#two-factor form input[name='email-input']"],
"password": ["div#two-factor form input#current-password"]

Comment thread maps/forms/forms.jsonc
Comment on lines +30 to +31
"next": ["button[name='email-submit-button']"],
"submit": ["button[name='password-submit-button']"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"next": ["button[name='email-submit-button']"],
"submit": ["button[name='password-submit-button']"]
"next": ["div#two-factor form button[name='email-submit-button']"],
"submit": ["div#two-factor form button[name='password-submit-button']"]

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