Skip to content

Commit d1b1e59

Browse files
authored
fix: handle confirmpassword honeypot better (#3529)
1 parent 66cd981 commit d1b1e59

4 files changed

Lines changed: 179 additions & 8 deletions

File tree

client/components/Honeypot/Honeypot.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,11 @@ const Honeypot = (props: HoneypotProps) => {
3434
name={name}
3535
autoComplete="off"
3636
style={{ width: 80, fontSize: 11, padding: '1px 4px' }}
37+
data-1p-ignore
38+
data-lpignore="true"
39+
data-bwignore
40+
data-form-type="other"
41+
data-protonpass-ignore
3742
/>
3843
</label>
3944
);

client/containers/UserCreate/UserCreate.tsx

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ type Props = {
1717
const UserCreate = (props: Props) => {
1818
const { signupData } = props;
1919
const altchaRef = useRef<import('components').AltchaRef>(null);
20+
const formStartedAtMsRef = useRef(Date.now());
2021
const [postUserIsLoading, setPostUserIsLoading] = useState(false);
2122
const [postUserError, setPostUserError] = useState<string | undefined>(undefined);
2223
const [subscribed, setSubscribed] = useState(false);
@@ -44,8 +45,8 @@ const UserCreate = (props: Props) => {
4445
evt.preventDefault();
4546
if (!acceptTerms) return;
4647
const formData = new FormData(evt.currentTarget);
47-
const confirmPwHoneypot = formData.get('confirmPassword') as string;
4848
const websiteHoneypot = formData.get('website') as string;
49+
const passwordHoneypot = formData.get('confirmPassword') as string;
4950
setPostUserIsLoading(true);
5051
setPostUserError(undefined);
5152
try {
@@ -61,14 +62,14 @@ const UserCreate = (props: Props) => {
6162
title,
6263
bio,
6364
location,
64-
// useful to check
65-
website: websiteHoneypot,
6665
orcid,
6766
github,
6867
twitter,
6968
facebook,
7069
googleScholar,
71-
_honeypot: confirmPwHoneypot || websiteHoneypot,
70+
_honeypot: websiteHoneypot,
71+
_passwordHoneypot: passwordHoneypot,
72+
_formStartedAtMs: formStartedAtMsRef.current,
7273
altcha: altchaPayload,
7374
gdprConsent: gdprCookiePersistsSignup() ? getGdprConsentElection() : null,
7475
};

server/user/__tests__/api.test.ts

Lines changed: 133 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
1+
import { vitest } from 'vitest';
2+
13
import { User } from 'server/models';
24
import { login, modelize, setup, teardown } from 'stubstub';
35

46
const tonyEmail = `${crypto.randomUUID()}@gmail.com`;
7+
const autofillEmail = `${crypto.randomUUID()}@gmail.com`;
8+
const restrictedEmail = `${crypto.randomUUID()}@gmail.com`;
9+
const delayedHoneypotEmail = `${crypto.randomUUID()}@gmail.com`;
10+
const spamEmail = `${crypto.randomUUID()}@gmail.com`;
511

612
const models = modelize`
713
User user {}
@@ -11,10 +17,58 @@ const models = modelize`
1117
completed: false
1218
count: 1
1319
}
20+
Signup autofillSignup {
21+
email: ${autofillEmail}
22+
hash: "hash-autofill"
23+
completed: false
24+
count: 1
25+
}
26+
Signup restrictedSignup {
27+
email: ${restrictedEmail}
28+
hash: "hash-restricted"
29+
completed: false
30+
count: 1
31+
}
32+
Signup delayedHoneypotSignup {
33+
email: ${delayedHoneypotEmail}
34+
hash: "hash-delayed-honeypot"
35+
completed: false
36+
count: 1
37+
}
38+
Signup spamSignup {
39+
email: ${spamEmail}
40+
hash: "hash-spam"
41+
completed: false
42+
count: 1
43+
}
1444
User suggestionUser {}
1545
`;
1646

17-
setup(beforeAll, models.resolve);
47+
const { deleteSessionsForUser } = vitest.hoisted(() => {
48+
return {
49+
deleteSessionsForUser: vitest.fn().mockResolvedValue(undefined),
50+
};
51+
});
52+
53+
vitest.mock(import('server/utils/session'), async (importOriginal) => {
54+
const og = await importOriginal();
55+
return {
56+
...og,
57+
deleteSessionsForUser: deleteSessionsForUser,
58+
};
59+
});
60+
vitest.mock(import('server/spamTag/notifications'), async (importOriginal) => {
61+
const og = await importOriginal();
62+
return {
63+
...og,
64+
notify: vitest.fn().mockResolvedValue(undefined),
65+
};
66+
});
67+
68+
setup(beforeAll, async () => {
69+
await models.resolve();
70+
});
71+
1872
teardown(afterAll);
1973

2074
describe('/api/users', () => {
@@ -44,6 +98,84 @@ describe('/api/users', () => {
4498
expect(userNow?.isSuperAdmin).toEqual(false);
4599
});
46100

101+
it('immediately restricts users when website honeypot is filled', async () => {
102+
const { spamSignup } = models;
103+
const agent = await login();
104+
await agent
105+
.post('/api/users')
106+
.send({
107+
email: spamSignup.email,
108+
hash: spamSignup.hash,
109+
firstName: 'Slow',
110+
lastName: 'Fill',
111+
password: 'oh!',
112+
_honeypot: 'oh!',
113+
_formStartedAtMs: Date.now() - 6000,
114+
})
115+
.expect(403);
116+
const createdUser = await User.findOne({ where: { email: spamSignup.email } });
117+
expect(createdUser).toBeDefined();
118+
const { getSpamTagForUser } = await import('server/spamTag/userQueries');
119+
const spamTag = await getSpamTagForUser(createdUser!.id);
120+
expect(spamTag?.status).toBe('confirmed-spam');
121+
await agent
122+
.post('/api/login')
123+
.send({ email: createdUser!.email, password: 'oh!' })
124+
.expect(403);
125+
});
126+
127+
it('does not restrict users when honeypot is filled after 5 seconds', async () => {
128+
const { delayedHoneypotSignup } = models;
129+
const agent = await login();
130+
await agent
131+
.post('/api/users')
132+
.send({
133+
email: delayedHoneypotSignup.email,
134+
hash: delayedHoneypotSignup.hash,
135+
firstName: 'Slow',
136+
lastName: 'Fill',
137+
password: 'oh!',
138+
_passwordHoneypot: 'oh!',
139+
_formStartedAtMs: Date.now() - 6000,
140+
})
141+
.expect(201);
142+
const createdUser = await User.findOne({ where: { email: delayedHoneypotSignup.email } });
143+
if (!createdUser) {
144+
throw new Error('Expected user to be created');
145+
}
146+
const { getSpamTagForUser } = await import('server/spamTag/userQueries');
147+
const spamTag = await getSpamTagForUser(createdUser.id);
148+
expect(spamTag).toBeNull();
149+
await agent
150+
.put('/api/users')
151+
.send({ userId: createdUser.id, firstName: 'Still', lastName: 'Allowed' })
152+
.expect(201);
153+
});
154+
155+
it('restricts and does not authenticate users when password honeypot is filled within 5 seconds', async () => {
156+
const { restrictedSignup } = models;
157+
const agent = await login();
158+
await agent
159+
.post('/api/users')
160+
.send({
161+
email: restrictedSignup.email,
162+
hash: restrictedSignup.hash,
163+
firstName: 'Bot',
164+
lastName: 'Account',
165+
password: 'oh!',
166+
_passwordHoneypot: 'oh!',
167+
_formStartedAtMs: Date.now(),
168+
})
169+
.expect(403);
170+
const createdUser = await User.findOne({ where: { email: restrictedSignup.email } });
171+
expect(createdUser).toBeDefined();
172+
const { getSpamTagForUser } = await import('server/spamTag/userQueries');
173+
const spamTag = await getSpamTagForUser(createdUser!.id);
174+
expect(spamTag?.status).toEqual('confirmed-spam');
175+
176+
expect(deleteSessionsForUser).toHaveBeenCalledWith(createdUser!.email);
177+
});
178+
47179
it('allows an exisiting user to read another users profile info', async () => {
48180
const { user, suggestionUser } = models;
49181
const agent = await login(user);

server/user/api.ts

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,33 @@ import { createUser, getSuggestedEditsUserInfo, updateUser } from './queries';
1414

1515
export const router = Router();
1616

17+
const ACCOUNT_RESTRICTED_MESSAGE =
18+
'Your account has been restricted. If you believe this is an error, please contact hello@pubpub.org.';
19+
const FAST_HONEYPOT_WINDOW_MS = 5_000;
20+
21+
const getFastHoneypotSignal = ({
22+
honeypot,
23+
formStartedAtMs,
24+
nowMs = Date.now(),
25+
}: {
26+
honeypot: unknown;
27+
formStartedAtMs: unknown;
28+
nowMs?: number;
29+
}): string | null => {
30+
const honeypotValue = typeof honeypot === 'string' ? honeypot.trim() : '';
31+
32+
if (honeypotValue.length === 0) return null;
33+
34+
const parsedFormStartedAtMs = Number(formStartedAtMs);
35+
36+
if (!Number.isFinite(parsedFormStartedAtMs)) return null;
37+
38+
const elapsedMs = nowMs - parsedFormStartedAtMs;
39+
if (elapsedMs < 0 || elapsedMs > FAST_HONEYPOT_WINDOW_MS) return null;
40+
41+
return honeypotValue;
42+
};
43+
1744
const getRequestIds = (req) => {
1845
const user = req.user || {};
1946
return {
@@ -35,18 +62,24 @@ router.post('/api/users', async (req, res) => {
3562
if (!ok) {
3663
return res.status(400).json('Please complete the verification and try again.');
3764
}
38-
const { altcha, _honeypot, website, ...body } = { ...req.body };
65+
const { altcha, _honeypot, _passwordHoneypot, _formStartedAtMs, ...body } = { ...req.body };
66+
const fastHoneypotSignal = getFastHoneypotSignal({
67+
honeypot: _passwordHoneypot,
68+
formStartedAtMs: _formStartedAtMs,
69+
});
70+
3971
const newUser = await createUser(body);
40-
if (isHoneypotFilled(req.body)) {
72+
if (fastHoneypotSignal || _honeypot) {
4173
await handleHoneypotTriggered(
4274
newUser.id,
4375
'create-user',
44-
req.body.website ?? 'confirmPassword honeypot',
76+
_honeypot || 'confirmPassword + very fast',
4577
{
4678
communitySubdomain: req.headers.host?.split('.')[0],
4779
content: req.body.fullName ? `name: ${req.body.fullName}` : undefined,
4880
},
4981
);
82+
return res.status(403).json(ACCOUNT_RESTRICTED_MESSAGE);
5083
}
5184
passport.authenticate('local')(req, res, () => {
5285
const hashedUserId = getHashedUserId(newUser);

0 commit comments

Comments
 (0)