fix: handle confirmpassword honeypot better#3529
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce false positives from the confirmPassword honeypot on the user creation flow by discouraging password managers from filling the field and by adding a timing-based signal before restricting accounts.
Changes:
- Added a server-side “fast honeypot” signal based on
_passwordHoneypot+_formStartedAtMs, and return403when honeypot restrictions are triggered. - Updated the user creation client to send
_formStartedAtMsand adjusted how honeypot values are gathered. - Added tests covering restriction behavior and session invalidation when honeypots are triggered.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server/user/api.ts | Adds fast honeypot signal logic and changes response behavior on honeypot trigger. |
| server/user/tests/api.test.ts | Adds tests and mocking around honeypot-triggered restrictions and spam-tag behavior. |
| client/containers/UserCreate/UserCreate.tsx | Tracks form start time and sends honeypot/timing fields in the create-user payload. |
| client/components/Honeypot/Honeypot.tsx | Adds password-manager ignore data-* attributes (currently only in dev render path). |
Comments suppressed due to low confidence (1)
client/components/Honeypot/Honeypot.tsx:55
- The password-manager ignore
data-*attributes were only added in thedevModerender path. In production (the hidden input branch), these attributes are missing, so password managers can still autofill the honeypot and reproduce the original issue. Apply the samedata-*ignore attributes to the non-dev<input>as well.
<input
type="text"
name={name}
autoComplete="off"
style={{ width: 80, fontSize: 11, padding: '1px 4px' }}
data-1p-ignore
data-lpignore="true"
data-bwignore
data-form-type="other"
data-protonpass-ignore
/>
</label>
);
}
return (
<input
type="text"
className="honeypot-input"
name={name}
tabIndex={-1}
autoComplete="off"
aria-hidden="true"
/>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| facebook, | ||
| googleScholar, | ||
| _honeypot: confirmPwHoneypot || websiteHoneypot, | ||
| _honeypot: websiteHoneypot || passwordHoneypot, |
There was a problem hiding this comment.
The client still combines the website and confirmPassword honeypots into _honeypot, but the server’s new logic expects the confirmPassword honeypot in _passwordHoneypot (with _honeypot reserved for the website field). As-is, confirmPassword autofill will continue to trigger the server via _honeypot without the 5s timing check. Send passwordHoneypot in _passwordHoneypot and keep _honeypot for websiteHoneypot only.
| _honeypot: websiteHoneypot || passwordHoneypot, | |
| _honeypot: websiteHoneypot, | |
| _passwordHoneypot: passwordHoneypot, |
server/user/__tests__/api.test.ts
Outdated
| /** biome-ignore-all lint/correctness/noUndeclaredVariables: <explanation> */ | ||
| import { User } from 'server/models'; | ||
| // import { getSpamTagForUser } from 'server/spamTag/userQueries'; | ||
| import { login, modelize, setup, teardown } from 'stubstub'; | ||
|
|
||
| const tonyEmail = `${crypto.randomUUID()}@gmail.com`; | ||
| const autofillEmail = `${crypto.randomUUID()}@gmail.com`; | ||
| const restrictedEmail = `${crypto.randomUUID()}@gmail.com`; | ||
| const delayedHoneypotEmail = `${crypto.randomUUID()}@gmail.com`; | ||
| const spamEmail = `${crypto.randomUUID()}@gmail.com`; |
There was a problem hiding this comment.
The file-wide biome-ignore-all lint/correctness/noUndeclaredVariables (with a placeholder <explanation>) hides real issues and will allow accidental undeclared globals to slip into the test. Prefer importing vi (and any other globals that aren’t part of the configured test environment) instead of disabling the rule for the whole file.
| /** biome-ignore-all lint/correctness/noUndeclaredVariables: <explanation> */ | |
| import { User } from 'server/models'; | |
| // import { getSpamTagForUser } from 'server/spamTag/userQueries'; | |
| import { login, modelize, setup, teardown } from 'stubstub'; | |
| const tonyEmail = `${crypto.randomUUID()}@gmail.com`; | |
| const autofillEmail = `${crypto.randomUUID()}@gmail.com`; | |
| const restrictedEmail = `${crypto.randomUUID()}@gmail.com`; | |
| const delayedHoneypotEmail = `${crypto.randomUUID()}@gmail.com`; | |
| const spamEmail = `${crypto.randomUUID()}@gmail.com`; | |
| import { beforeAll, afterAll, describe, vi } from 'vitest'; | |
| import { randomUUID } from 'crypto'; | |
| import { User } from 'server/models'; | |
| // import { getSpamTagForUser } from 'server/spamTag/userQueries'; | |
| import { login, modelize, setup, teardown } from 'stubstub'; | |
| const tonyEmail = `${randomUUID()}@gmail.com`; | |
| const autofillEmail = `${randomUUID()}@gmail.com`; | |
| const restrictedEmail = `${randomUUID()}@gmail.com`; | |
| const delayedHoneypotEmail = `${randomUUID()}@gmail.com`; | |
| const spamEmail = `${randomUUID()}@gmail.com`; |
server/user/__tests__/api.test.ts
Outdated
| it('restricts and does not authenticate users when website honeypot is filled within 5 seconds', async () => { | ||
| const { restrictedSignup } = models; | ||
| const agent = await login(); | ||
| await agent | ||
| .post('/api/users') | ||
| .send({ | ||
| email: restrictedSignup.email, | ||
| hash: restrictedSignup.hash, | ||
| firstName: 'Bot', | ||
| lastName: 'Account', | ||
| password: 'oh!', | ||
| _passwordHoneypot: 'oh!', | ||
| _formStartedAtMs: Date.now(), | ||
| }) | ||
| .expect(403); | ||
| const createdUser = await User.findOne({ where: { email: restrictedSignup.email } }); | ||
| // if (!createdUser) { | ||
| // throw new Error('Expected user to be created'); | ||
| // } | ||
| expect(createdUser).toBeDefined(); | ||
| const { getSpamTagForUser } = await import('server/spamTag/userQueries'); | ||
| const spamTag = await getSpamTagForUser(createdUser!.id); | ||
| expect(spamTag?.status).toEqual('confirmed-spam'); |
There was a problem hiding this comment.
The test name says “website honeypot” but the payload uses _passwordHoneypot, and there are commented-out assertions left in the test body. Rename the test to match what it’s exercising (confirmPassword timing gate) and remove the commented-out code to keep the suite clear.
| const { altcha, _honeypot, _passwordHoneypot, _formStartedAtMs, ...body } = { ...req.body }; | ||
| const fastHoneypotSignal = getFastHoneypotSignal({ | ||
| honeypot: _passwordHoneypot, | ||
| formStartedAtMs: _formStartedAtMs, | ||
| }); | ||
|
|
||
| const newUser = await createUser(body); | ||
| if (isHoneypotFilled(req.body)) { | ||
| if (fastHoneypotSignal || _honeypot) { | ||
| await handleHoneypotTriggered( | ||
| newUser.id, | ||
| 'create-user', | ||
| req.body.website ?? 'confirmPassword honeypot', | ||
| _honeypot || 'confirmPassword + very fast', | ||
| { |
There was a problem hiding this comment.
The new fast honeypot check only considers _passwordHoneypot, but the handler still treats any non-empty _honeypot as an immediate restriction. Since the client currently sends the confirmPassword honeypot value via _honeypot, legitimate autofill will still be restricted regardless of timing. Align the server logic so _honeypot represents the website honeypot only, and use _passwordHoneypot + _formStartedAtMs for the confirmPassword timing gate (and consider passing the actual fastHoneypotSignal value into handleHoneypotTriggered for logging).
Issue(s) Resolved
The "confirmPassword" honeypot on the create user page was sometimes getting triggered by legit users.
This is fixed by:
data-attributes asking pw managers to ignore this fieldTest Plan
Screenshots (if applicable)
Optional
Notes/Context/Gotchas
Supporting Docs