From 41385cc327cdc55381ee9daf4d2ef21a95267873 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Sat, 10 Oct 2026 16:36:46 -0400 Subject: [PATCH] fix(email): one canonicalization and blocklist for every address path Share invites, share notifications and app feedback checked addresses through EmailClient.clean/validate, a second copy of the rules in util/email.ts that stripped dots on iCloud, stripped `+tags` on every domain, and matched the blocklist case-sensitively. They now use util/email.ts: isBlockedEmail for the blocklist, abuseKey for the per-address notification budget, and cleanEmail for the invite digest key. EmailClient keeps transport and templates only. isBlockedEmail also lowercases the configured suffixes, so a mixed-case blocklist entry still matches. --- src/backend/clients/email/EmailClient.test.ts | 83 ------------- src/backend/clients/email/EmailClient.ts | 113 +----------------- .../feedback/AppFeedbackService.test.ts | 23 ++++ .../services/feedback/AppFeedbackService.ts | 4 +- .../share/ShareNotificationService.ts | 16 +-- .../services/share/ShareService.test.ts | 36 ++++++ src/backend/services/share/ShareService.ts | 3 +- src/backend/services/share/shareEmail.test.ts | 75 ++++++++++++ src/backend/util/email.test.ts | 12 ++ src/backend/util/email.ts | 11 +- 10 files changed, 169 insertions(+), 207 deletions(-) diff --git a/src/backend/clients/email/EmailClient.test.ts b/src/backend/clients/email/EmailClient.test.ts index c4534dec7..cbe80f00d 100644 --- a/src/backend/clients/email/EmailClient.test.ts +++ b/src/backend/clients/email/EmailClient.test.ts @@ -425,86 +425,3 @@ describe('EmailClient — share notification templates', () => { expect(html).toContain('href="https://puter.test"'); }); }); - -describe('EmailClient.clean', () => { - const clean = (email: string) => startClient().clean(email); - - it('strips subaddressing on ordinary domains', () => { - expect(clean('person+tag@example.test')).toBe('person@example.test'); - }); - - it('ignores dots and subaddressing for gmail', () => { - expect(clean('first.last+news@gmail.com')).toBe('firstlast@gmail.com'); - }); - - it('canonicalizes googlemail onto gmail', () => { - expect(clean('first.last@googlemail.com')).toBe('firstlast@gmail.com'); - }); - - it('keeps yahoo subaddressing, which yahoo treats as distinct', () => { - expect(clean('person-tag+x@yahoo.com')).toBe('person-tag+x@yahoo.com'); - }); - - it('ignores dots for icloud aliases', () => { - expect(clean('first.last+tag@me.com')).toBe('firstlast@me.com'); - }); - - it('returns anything that is not an address unchanged', () => { - expect(clean('not-an-email')).toBe('not-an-email'); - expect(clean('@nolocal.test')).toBe('@nolocal.test'); - }); -}); - -describe('EmailClient.validate', () => { - it('waves everything through in a dev environment', async () => { - const client = startClient({ env: 'dev' } as Partial); - await expect(client.validate('anything@blocked.test')).resolves.toBe( - true, - ); - }); - - it('rejects a blocked domain suffix', async () => { - const client = startClient({ - blockedEmailDomains: ['blocked.test'], - } as unknown as Partial); - - await expect(client.validate('person@blocked.test')).resolves.toBe( - false, - ); - await expect(client.validate('person@allowed.test')).resolves.toBe( - true, - ); - }); - - it('matches the blocklist against the cleaned address', async () => { - const client = startClient({ - blockedEmailDomains: ['@gmail.com'], - } as unknown as Partial); - await expect( - client.validate('first.last+tag@googlemail.com'), - ).resolves.toBe(false); - }); - - it('lets a registered validator veto an address', async () => { - const client = startClient(); - const seen: string[] = []; - client.addValidator((email) => { - seen.push(email); - return !email.startsWith('spam'); - }); - - await expect(client.validate('spam+x@example.test')).resolves.toBe( - false, - ); - await expect(client.validate('fine@example.test')).resolves.toBe(true); - // Validators always see the canonical form. - expect(seen).toEqual(['spam@example.test', 'fine@example.test']); - }); - - it('supports asynchronous validators', async () => { - const client = startClient(); - client.addValidator(async (email) => email !== 'no@example.test'); - await expect(client.validate('no@example.test')).resolves.toBe(false); - await expect(client.validate('yes@example.test')).resolves.toBe(true); - }); -}); diff --git a/src/backend/clients/email/EmailClient.ts b/src/backend/clients/email/EmailClient.ts index 75523104c..cc980ee37 100644 --- a/src/backend/clients/email/EmailClient.ts +++ b/src/backend/clients/email/EmailClient.ts @@ -71,54 +71,12 @@ export interface SendMailOptions { headers?: Record; } -export type EmailValidator = (email: string) => Promise | boolean; - interface CompiledTemplate { subject: ReturnType; html: ReturnType; text?: ReturnType; } -// -- Clean-email rules ------------------------------------------------ - -type CleanRule = (parts: { local: string; domain: string }) => { - local: string; - domain: string; -}; - -const CLEAN_RULES: Record = { - dots_dont_matter: ({ local, domain }) => ({ - local: local.replace(/\./g, ''), - domain, - }), - remove_subaddressing: ({ local, domain }) => ({ - local: local.split('+')[0], - domain, - }), -}; - -const PROVIDER_RULES: Record = { - gmail: { apply: ['dots_dont_matter'], skip: [] }, - icloud: { apply: ['dots_dont_matter'], skip: [] }, - yahoo: { apply: [], skip: ['remove_subaddressing'] }, -}; - -const DOMAIN_TO_PROVIDER: Record = { - 'gmail.com': 'gmail', - 'googlemail.com': 'gmail', - 'yahoo.com': 'yahoo', - 'yahoo.co.uk': 'yahoo', - 'yahoo.ca': 'yahoo', - 'yahoo.com.au': 'yahoo', - 'icloud.com': 'icloud', - 'me.com': 'icloud', - 'mac.com': 'icloud', -}; - -const DOMAIN_ALIASES: Record = { - 'googlemail.com': 'gmail.com', -}; - // -- EmailClient ------------------------------------------------------ /** @@ -126,8 +84,8 @@ const DOMAIN_ALIASES: Record = { * * - Template-based outbound mail (via `send`) * - Raw nodemailer passthrough (via `sendRaw`) - * - Canonical-form normalization for dedup (via `clean`) - * - Policy + extensible validation (via `validate`) + * + * Address normalization and the domain blocklist live in `util/email.ts`. */ export class EmailClient extends PuterClient { private transport: ReturnType | null = @@ -135,7 +93,6 @@ export class EmailClient extends PuterClient { private compiledTemplates: Partial< Record > = {}; - private validators: EmailValidator[] = []; constructor(config: IConfig) { super(config); @@ -218,72 +175,6 @@ export class EmailClient extends PuterClient { }); } - // -- Public API: clean / validate --------------------------------- - - /** - * Normalize an email to its canonical form for dedup comparisons. Applies - * provider-specific rules (e.g. Gmail ignores dots in the local part) plus - * generic subaddressing removal. - */ - clean(email: string): string { - let [local, domain] = email.split('@'); - if (!local || !domain) return email; - - if (DOMAIN_ALIASES[domain]) { - domain = DOMAIN_ALIASES[domain]; - } - - // Default: strip subaddressing on everything unless provider skips it - const ruleNames = new Set(['remove_subaddressing']); - const provider = DOMAIN_TO_PROVIDER[domain]; - const rules = provider ? PROVIDER_RULES[provider] : undefined; - - if (rules) { - rules.apply.forEach((r) => ruleNames.add(r)); - rules.skip.forEach((r) => ruleNames.delete(r)); - } - - let parts = { local, domain }; - for (const name of ruleNames) { - parts = CLEAN_RULES[name](parts); - } - - return `${parts.local}@${parts.domain}`; - } - - /** - * Check whether an email is allowed to be used. Checks domain blocklist - * plus any registered validators (services can call `addValidator()` to - * register custom policy hooks). - */ - async validate(email: string): Promise { - if (this.config.env === 'dev') return true; - - const cleaned = this.clean(email); - - const blocked = this.config.blockedEmailDomains; - if (Array.isArray(blocked)) { - for (const suffix of blocked) { - if (cleaned.endsWith(suffix)) return false; - } - } - - for (const validator of this.validators) { - const ok = await validator(cleaned); - if (!ok) return false; - } - - return true; - } - - /** - * Register a custom validation hook. Services can call this during their - * startup to veto specific emails (e.g. a disposable-email service). - */ - addValidator(fn: EmailValidator): void { - this.validators.push(fn); - } - // -- Internals ---------------------------------------------------- private defaultFrom(): string { diff --git a/src/backend/services/feedback/AppFeedbackService.test.ts b/src/backend/services/feedback/AppFeedbackService.test.ts index a2ec3ca2d..ef3cafe2b 100644 --- a/src/backend/services/feedback/AppFeedbackService.test.ts +++ b/src/backend/services/feedback/AppFeedbackService.test.ts @@ -632,6 +632,29 @@ describe('AppFeedbackService owner email', () => { } }); + it('stores without emailing an owner on a blocked domain, whatever its case', async () => { + const send = mockEmailReady(); + const ownerId = await makeDeliverableOwner(); + await server.stores.user.update(ownerId, { + email: `Owner-${ownerId}@Blocked.Example`, + }); + await server.stores.user.invalidateById(ownerId); + const app = await makeApp(ownerId, { feedbackEnabled: true }); + const userId = await makeUser(); + const prev = liveConfig().blockedEmailDomains; + liveConfig().blockedEmailDomains = ['blocked.example']; + try { + await service.submit({ userId, app: app.name, message: 'hi' }); + } finally { + liveConfig().blockedEmailDomains = prev; + } + + expect(send).not.toHaveBeenCalled(); + const rows = await feedbackRows(userId); + expect(rows).toHaveLength(1); + expect(Boolean(rows[0].email_sent)).toBe(false); + }); + it('stores but does not email past the per-app daily email cap', async () => { const send = mockEmailReady(); const ownerId = await makeDeliverableOwner(); diff --git a/src/backend/services/feedback/AppFeedbackService.ts b/src/backend/services/feedback/AppFeedbackService.ts index e15caa5bb..19bf33ee2 100644 --- a/src/backend/services/feedback/AppFeedbackService.ts +++ b/src/backend/services/feedback/AppFeedbackService.ts @@ -18,6 +18,7 @@ */ import { HttpError } from '../../core/http/HttpError.js'; +import { isBlockedEmail } from '../../util/email.js'; import { PuterService } from '../types.js'; /** @@ -299,7 +300,8 @@ export class AppFeedbackService extends PuterService { ) { return; } - if (!(await this.clients.email.validate(owner.email))) return; + if (isBlockedEmail(owner.email, this.config.blockedEmailDomains)) + return; // Claim an email-cap slot *before* sending: flip email_sent, recount // with the claim included, and release the slot if a concurrent diff --git a/src/backend/services/share/ShareNotificationService.ts b/src/backend/services/share/ShareNotificationService.ts index 9d483be0a..1c940d325 100644 --- a/src/backend/services/share/ShareNotificationService.ts +++ b/src/backend/services/share/ShareNotificationService.ts @@ -41,6 +41,7 @@ import { sharedViewLink, } from './shareDeepLink'; import { NOTIFY_FANOUT_CAP } from '../../stores/team/TeamStore'; +import { abuseKey, cleanEmail, isBlockedEmail } from '../../util/email.js'; import { blocksAllShares, type ResolvedShare } from './ShareService'; /** @@ -603,9 +604,8 @@ export class ShareNotificationService extends PuterService { } #addressKey(email: string): string { - const canonical = this.clients.email.clean(email.trim().toLowerCase()); return createHash('sha256') - .update(canonical) + .update(abuseKey(email.trim())) .digest('hex') .slice(0, 32); } @@ -670,8 +670,8 @@ export class ShareNotificationService extends PuterService { skipped('recipient has unsubscribed', { holderId }); return; } - if (!(await this.clients.email.validate(to))) { - skipped('address refused by validate', { holderId }); + if (isBlockedEmail(to, this.config.blockedEmailDomains)) { + skipped('address is on a blocked domain', { holderId }); return; } @@ -1077,13 +1077,15 @@ export class ShareNotificationService extends PuterService { // Each address fails alone — one refused send must not cost the // next invitee their only channel. try { - if (!(await this.clients.email.validate(to))) { - skipped('invite address refused by validate', { to }); + if (isBlockedEmail(to, this.config.blockedEmailDomains)) { + skipped('invite address is on a blocked domain', { to }); continue; } const mayOpen = await this.#claimInviteEmail(issuerId, to); + // Canonical, so a later share typed in another case or alias + // joins this digest instead of losing to the spent budget. await this.#queueDigest( - `invite:${to}`, + `invite:${cleanEmail(to)}`, { kind: 'invite', to }, issuer, count, diff --git a/src/backend/services/share/ShareService.test.ts b/src/backend/services/share/ShareService.test.ts index f67c13bd6..cfc44aae5 100644 --- a/src/backend/services/share/ShareService.test.ts +++ b/src/backend/services/share/ShareService.test.ts @@ -5563,6 +5563,42 @@ describe('ShareService', () => { ), ).toBe(before); }); + + it('refuses an invite to a blocked domain whatever its case', async () => { + const owner = await makeUser(); + const file = await makeFile(owner.user); + const cfg = server.services.share.config as { + blockedEmailDomains?: string[]; + }; + const prev = cfg.blockedEmailDomains; + cfg.blockedEmailDomains = ['mailinator.com']; + try { + for (const email of [ + 'temp@mailinator.com', + 'Temp@Mailinator.com', + 'temp@SUB.MAILINATOR.COM', + ]) { + await expect( + share(owner.actor, { + uid: file.uuid, + recipient: { email }, + mode: 'read', + }), + email, + ).rejects.toMatchObject({ + statusCode: 400, + legacyCode: 'email_not_allowed', + }); + expect( + await server.stores.share.listPendingByEmail( + email.toLowerCase(), + ), + ).toEqual([]); + } + } finally { + cfg.blockedEmailDomains = prev; + } + }); }); describe('claiming is safe against races and duplicates', () => { diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index c2dc0202d..d1ea97f4c 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -32,6 +32,7 @@ import { isUniqueViolation } from '../../util/dbError.js'; import { abuseKey, cleanEmail, + isBlockedEmail, isProviderCanonicalized, isStorableEmail, } from '../../util/email.js'; @@ -3151,7 +3152,7 @@ export class ShareService extends PuterService { // happens to the email. if ( !isStorableEmail(email) || - !(await this.clients.email.validate(email)) + isBlockedEmail(email, this.config.blockedEmailDomains) ) { throw new HttpError(400, 'invalid recipient email address', { legacyCode: 'email_not_allowed', diff --git a/src/backend/services/share/shareEmail.test.ts b/src/backend/services/share/shareEmail.test.ts index f7014de64..0be7305fc 100644 --- a/src/backend/services/share/shareEmail.test.ts +++ b/src/backend/services/share/shareEmail.test.ts @@ -650,6 +650,81 @@ describe('share email', () => { expect(mail.html).not.toContain('?shared='); }); + const mailToInbox = (address: string) => + sent.filter((mail) => mail.to.toLowerCase() === address.toLowerCase()); + + it("keys an invitee's digest and budget case-insensitively", async () => { + const owner = env.users.user; + const invitee = uninvitedAddress(); + const shouted = invitee.toUpperCase(); + const [first, second, third] = [ + await makeFile(owner, 'case-a'), + await makeFile(owner, 'case-b'), + await makeFile(owner, 'case-c'), + ]; + + await withDigestWindow(env, DIGEST_WINDOW_SECONDS, async () => { + await shareWith(owner, invitee, [{ uid: first.uid }]); + await sleep(SETTLE_MS); + await shareWith(owner, shouted, [{ uid: second.uid }]); + }); + + // One inbox, one digest, both files. + const mail = await waitForMail({ to: invitee }); + await sleep(SETTLE_MS); + expect(mailToInbox(invitee)).toHaveLength(1); + expect(mail.html).toContain(first.name); + expect(mail.html).toContain(second.name); + + // The pair's quiet window holds whatever case the next share uses. + const capitalized = `${invitee[0].toUpperCase()}${invitee.slice(1)}`; + await shareWith(owner, capitalized, [{ uid: third.uid }]); + await sleep(SETTLE_MS); + expect(mailToInbox(invitee)).toHaveLength(1); + }, 15_000); + + it('folds gmail spellings of one invitee into one digest', async () => { + const owner = env.users.admin; + const local = `gm${uniqueSuffix()}`; + // Gmail ignores dots and `+tag`: both are `${local}@gmail.com`. + const tagged = `${local.slice(0, 4)}.${local.slice(4)}+news@gmail.com`; + const plain = `${local}@gmail.com`; + const first = await makeFile(owner, 'gmail-a'); + const second = await makeFile(owner, 'gmail-b'); + + await withDigestWindow(env, DIGEST_WINDOW_SECONDS, async () => { + await shareWith(owner, tagged, [{ uid: first.uid }]); + await sleep(SETTLE_MS); + await shareWith(owner, plain, [{ uid: second.uid }]); + }); + + const mail = await waitForMail({ to: tagged }); + await sleep(SETTLE_MS); + expect(mailToInbox(plain)).toHaveLength(0); + expect(mailToInbox(tagged)).toHaveLength(1); + expect(mail.html).toContain(first.name); + expect(mail.html).toContain(second.name); + }, 15_000); + + it('treats dotted and undotted iCloud addresses as two invitees', async () => { + const owner = env.users.admin; + const local = `ic${uniqueSuffix()}`; + // Apple allocates the exact string: these are two mailboxes. + const dotted = `${local.slice(0, 4)}.${local.slice(4)}@icloud.com`; + const undotted = `${local}@icloud.com`; + const first = await makeFile(owner, 'icloud-a'); + const second = await makeFile(owner, 'icloud-b'); + + await shareWith(owner, dotted, [{ uid: first.uid }]); + expect((await waitForMail({ to: dotted })).html).toContain(first.name); + + // Its own budget: the dotted address's window doesn't silence it. + await shareWith(owner, undotted, [{ uid: second.uid }]); + const mail = await waitForMail({ to: undotted }); + expect(mail.html).toContain(second.name); + expect(mail.html).not.toContain(first.name); + }); + it('honors an account-wide unsubscribe, and offers the link to those who have not', async () => { const owner = env.users.user; const recipient = await signUpAndConfirm(uninvitedAddress()); diff --git a/src/backend/util/email.test.ts b/src/backend/util/email.test.ts index f02ffe351..23bc55474 100644 --- a/src/backend/util/email.test.ts +++ b/src/backend/util/email.test.ts @@ -120,6 +120,18 @@ describe('isBlockedEmail', () => { expect( isBlockedEmail('A.B+tag@MAILINATOR.com', ['mailinator.com']), ).toBe(true); + expect( + isBlockedEmail('first.last+tag@googlemail.com', ['@gmail.com']), + ).toBe(true); + }); + + it('ignores case in the configured suffixes too', () => { + expect(isBlockedEmail('a@mailinator.com', ['Mailinator.COM'])).toBe( + true, + ); + expect(isBlockedEmail('a@Mailinator.com', ['Mailinator.com'])).toBe( + true, + ); }); }); diff --git a/src/backend/util/email.ts b/src/backend/util/email.ts index 8dd0406d8..a67b19ea8 100644 --- a/src/backend/util/email.ts +++ b/src/backend/util/email.ts @@ -18,8 +18,8 @@ */ /** - * Email normalization + block-list check. Stateless — used by AuthController - * (signup / change-email / save-account). + * Email normalization + block-list check. Stateless; every path that matches, + * keys or blocks on an address uses these rules. * * CleanEmail('foo.bar+tag@gmail.com') === 'foobar@gmail.com' * isBlockedEmail('temp@mailinator.com', ['mailinator.com']) === true @@ -170,7 +170,8 @@ export function isProviderCanonicalized(email: string): boolean { /** * Returns true when the (cleaned) email matches any of the blocked domain - * suffixes. Suffix-match so `mailinator.com` blocks `foo@bar.mailinator.com`. + * suffixes. Suffix-match so `mailinator.com` blocks `foo@bar.mailinator.com`; + * case-insensitive on both sides. */ export function isBlockedEmail( email: string, @@ -178,5 +179,7 @@ export function isBlockedEmail( ): boolean { if (!blockedDomains || blockedDomains.length === 0) return false; const clean = cleanEmail(email); - return blockedDomains.some((suffix) => clean.endsWith(suffix)); + return blockedDomains.some((suffix) => + clean.endsWith(suffix.toLowerCase()), + ); }