fix: one gate for an address we are about to store

`validator.isEmail` accepts a quoted local part, so an address whose local part
is an arbitrary string passes it. Five call sites relied on that check alone
before writing the address to `user.email`, and the invite path's own shape
regex was equally permissive.

`isStorableEmail` is now the single gate: the library check, plus a refusal of
`<`, `>` and `"`, which are legal in an address only inside the quoted form.
Nothing deliverable uses that form, so the rule costs no real address --
apostrophes and ampersands in a local part still pass, and the suite covers
them.

One function rather than a check each caller has to remember, because the next
place that stores an address will not remember.
This commit is contained in:
Juan Castro committed 2026-10-06 16:47:15 -04:00
1 parent a1eca46a80
commit 73698be964
6 files changed
+76 -23

No files matched your search

@@ -22,7 +22,6 @@ import type { Request, RequestHandler, Response } from 'express';
import crypto from 'node:crypto';
import { posix as pathPosix } from 'node:path';
import { v4 as uuidv4, validate as validateUuid } from 'uuid';
import validator from 'validator';
import { Controller, Get, Post } from '../../core/http/decorators.js';
import type { HttpErrorOptions } from '../../core/http/HttpError.js';
import { HttpError } from '../../core/http/HttpError.js';
@@ -77,7 +76,11 @@ import {
phoneAttemptsKey,
} from '../../util/cardFallback.js';
import { sessionCookieFlags } from '../../util/cookieFlags.js';
import { cleanEmail, isBlockedEmail } from '../../util/email.js';
import {
cleanEmail,
isBlockedEmail,
isStorableEmail,
} from '../../util/email.js';
import { isGodmodeApp } from '../../util/godmodeApps.js';
import { generate_identifier } from '../../util/identifier.js';
import { parsePhone } from '../../util/phone.js';
@@ -802,7 +805,7 @@ export class AuthController extends PuterController {
throw new HttpError(400, 'email must be a string.', {
legacyCode: 'bad_request',
});
if (!validator.isEmail(body.email))
if (!isStorableEmail(body.email))
throw new HttpError(
400,
'Please enter a valid email address.',
@@ -2414,7 +2417,7 @@ export class AuthController extends PuterController {
if (username) {
user = await this.stores.user.getByUsername(username);
} else {
if (!validator.isEmail(email))
if (!isStorableEmail(email))
throw new HttpError(400, 'Invalid email.', {
legacyCode: 'bad_request',
});
@@ -2808,7 +2811,7 @@ export class AuthController extends PuterController {
legacyCode: 'bad_request',
});
}
if (!validator.isEmail(new_email)) {
if (!isStorableEmail(new_email)) {
throw new HttpError(400, 'Please enter a valid email address.', {
legacyCode: 'bad_request',
});
@@ -3053,7 +3056,7 @@ export class AuthController extends PuterController {
legacyCode: 'username_already_in_use',
});
}
if (!email || !validator.isEmail(email)) {
if (!email || !isStorableEmail(email)) {
throw new HttpError(400, 'Please enter a valid email address.', {
legacyCode: 'bad_request',
});
+2 -1
View File
@@ -280,7 +280,8 @@ export const DEFAULT_DAILY_SHARE_LIMIT = 200;
* Deliverability is the inbox's business, but `a@b` or a pasted sentence must
* not become a permanent pending share that spent quota.
*/
const EMAIL_SHAPE = /^[^\s@]+@[^\s@]+\.[^\s@]+$/u;
// `"<>` excluded too: legal only in a quoted local part nothing uses.
const EMAIL_SHAPE = /^[^\s@"<>]+@[^\s@"<>]+\.[^\s@"<>]+$/u;
/**
* Where "refuse shares from everyone" lives on the user row.
@@ -95,6 +95,25 @@ describe('team account provisioning: home path conflicts', () => {
).resolves.toBeNull();
});
it('refuses a seat email that could render as markup', async () => {
const owner = await makeUser();
const team = await makeTeam(owner.id);
const username = `tp_${Math.random().toString(36).slice(2, 10)}`;
// A quoted local part passes `validator.isEmail`; it must not store.
await expect(
service.provisionAccount(team.uid, owner.id, {
username,
email: '"<img/src=x/onerror=alert(1)>"@example.com',
}),
).rejects.toMatchObject({ statusCode: 400 });
// Refused before any write, so the name is still free.
await expect(
server.stores.user.getByUsername(username, { force: true }),
).resolves.toBeNull();
});
it('does not suggest a name whose home path is occupied', async () => {
const owner = await makeUser();
await makeTeam(owner.id);
+12 -15
View File
@@ -18,7 +18,6 @@
*/
import bcrypt from 'bcrypt';
import validator from 'validator';
import { v4 as uuidv4 } from 'uuid';
import {
USERNAME_MAX_LENGTH,
@@ -56,7 +55,7 @@ import {
type PageResult,
} from '../../util/pagination.js';
import type { UserRow } from '../../stores/user/UserStore';
import { cleanEmail } from '../../util/email.js';
import { cleanEmail, isStorableEmail } from '../../util/email.js';
import {
generateTemporaryPassword,
temporaryPasswordExpiry,
@@ -820,18 +819,16 @@ export class TeamService extends PuterService {
id === null ? null : (users.get(id)?.username ?? null);
return {
items: page.items.map(
(row): MemberActivityEntry => ({
action: row.action,
reason: row.reason,
created_at: epochSeconds(row.created_at),
username: name(row.user_id_keep),
actor_username: name(row.actor_user_id),
// Only a sign-in carries these; the shape stays uniform.
ip: null,
user_agent: null,
}),
),
items: page.items.map((row): MemberActivityEntry => ({
action: row.action,
reason: row.reason,
created_at: epochSeconds(row.created_at),
username: name(row.user_id_keep),
actor_username: name(row.actor_user_id),
// Only a sign-in carries these; the shape stays uniform.
ip: null,
user_agent: null,
})),
...(page.cursor ? { cursor: page.cursor } : {}),
};
}
@@ -917,7 +914,7 @@ export class TeamService extends PuterService {
this.#assertUsableUsername(input.username);
const email = typeof input.email === 'string' ? input.email.trim() : '';
if (email && !validator.isEmail(email)) {
if (email && !isStorableEmail(email)) {
throw new HttpError(400, 'Invalid email', {
legacyCode: 'bad_request',
});
+27 -1
View File
@@ -18,7 +18,8 @@
*/
import { describe, expect, it } from 'vitest';
import { cleanEmail, isBlockedEmail } from './email.ts';
import validator from 'validator';
import { cleanEmail, isBlockedEmail, isStorableEmail } from './email.ts';
describe('cleanEmail', () => {
it('lowercases the whole address', () => {
@@ -94,3 +95,28 @@ describe('isBlockedEmail', () => {
).toBe(true);
});
});
describe('isStorableEmail', () => {
const markup = '"<img/src=x/onerror=alert(1)>"@example.com';
it('refuses a quoted local part that carries markup', () => {
// The reason this gate exists: the library on its own says yes.
expect(validator.isEmail(markup)).toBe(true);
expect(isStorableEmail(markup)).toBe(false);
});
it('refuses the bare metacharacters too', () => {
for (const bad of ['a<b@example.com', 'a>b@example.com', 'a"b@example.com'])
expect(isStorableEmail(bad)).toBe(false);
});
it('still accepts addresses people actually have', () => {
for (const good of [
"o'brien@example.com",
'a&b@example.com',
'foo.bar+tag@gmail.com',
'user_name-1@sub.example.co.uk',
])
expect(isStorableEmail(good)).toBe(true);
});
});
+7
View File
@@ -25,6 +25,8 @@
* isBlockedEmail('temp@mailinator.com', ['mailinator.com']) === true
*/
import validator from 'validator';
type RuleName = 'dots_dont_matter' | 'remove_subaddressing';
interface Parts {
@@ -103,6 +105,11 @@ export function cleanEmail(email: string): string {
return `${parts.local}@${parts.domain}`;
}
/** Storage gate; `isEmail` alone passes quoted local parts carrying markup. */
export function isStorableEmail(email: string): boolean {
return !/["<>]/u.test(email) && validator.isEmail(email);
}
/** Strips `+` on any domain. For abuse decisions only, never for identity. */
export function abuseKey(email: string): string {
const [local, domain] = cleanEmail(email).split('@');