fix(email): scrub gallery passwords from the sent-mail archive (#1340)
* fix(email): scrub gallery passwords from the sent-mail archive
The email queue kept every gallery password and client PIN in clear
text in email_data and rendered_html after the mail was sent, and the
Messages reading pane handed them back to any admin with the messaging
flag. A password hash in the events table bought nothing while the
plaintext sat next to it.
Once a mail is out, or its retries are exhausted, the processor now
masks secret-looking variables (password, passcode, pin) in email_data
and replaces their values in the rendered body, plain and HTML-escaped.
The reading pane applies the same masking to rows archived before this
change. Pending rows keep the real values so a retry still sends them.
Relates to issue 1271
* fix(email): keep a quoted ">" from cutting an attribute value out of redaction
The tag splitter stopped at the first ">", so a template attribute such as
title="{{gallery_password}} > details" left the password unmasked in the
archived HTML while email_data was already masked. The tokenizer is now
quote-aware; a tag with an unbalanced quote falls through as text and is
scrubbed there.
* fix(email): scrub secrets inside HTML comments in the archived body
A comment such as <!-- PIN: {{client_password}} --> was split off as a tag
and its body, which has no attribute, was never scrubbed. Comments are now
one segment and their content is masked whole.
---------
Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
8017370271
commit
69754f8a2c
@@ -0,0 +1,56 @@
|
||||
/**
|
||||
* The Messages reading pane must not serve passwords (see
|
||||
* utils/emailSecretRedaction.js). Rows sent before the processor learned to
|
||||
* scrub still carry the gallery password in email_data and rendered_html;
|
||||
* the route redacts them on read.
|
||||
*/
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
|
||||
process.env.NODE_ENV = 'test';
|
||||
process.env.TEST_DATABASE_PATH = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-paneredact-')), 'db.sqlite');
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'paneredact-test-secret';
|
||||
process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-paneredact-storage-'));
|
||||
|
||||
const request = require('supertest');
|
||||
const { bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken, buildRouteApp } = require('../integration/helpers/crmDb');
|
||||
const { invalidateFeatureFlagCache } = require('../../src/middleware/requireFeatureFlag');
|
||||
const { MASK } = require('../../src/utils/emailSecretRedaction');
|
||||
|
||||
describe('GET /admin/email/queue/:id redacts secrets from legacy rows', () => {
|
||||
let db; let cleanup; let app; let token; let rowId;
|
||||
const PASSWORD = 'Sunset-42!'; const PIN = 'Tom & Ada\'s 7788';
|
||||
|
||||
beforeAll(async () => {
|
||||
({ db, cleanup } = await bootCrmDb());
|
||||
const { adminId } = await seedMinimal(db);
|
||||
await assignAdminRole(db, adminId, 'super_admin');
|
||||
token = mintAdminToken(adminId);
|
||||
await db('feature_flags').insert({ key: 'messaging', value: true }).onConflict('key').merge({ value: true });
|
||||
invalidateFeatureFlagCache();
|
||||
const ins = await db('email_queue').insert({
|
||||
recipient_email: '[email protected]', email_type: 'gallery_created', status: 'sent',
|
||||
created_at: new Date().toISOString(), sent_at: new Date().toISOString(), retry_count: 0,
|
||||
email_data: JSON.stringify({ customer_name: 'Ada', gallery_password: PASSWORD, client_password: PIN, cc: ['[email protected]'] }),
|
||||
rendered_html: `<ul><li>Password: ${PASSWORD}</li><li>PIN: Tom & Ada's 7788</li></ul>`,
|
||||
}).returning('id');
|
||||
rowId = ins[0]?.id ?? ins[0];
|
||||
app = buildRouteApp('/api/admin/email', require('../../src/routes/adminEmail'));
|
||||
}, 120000);
|
||||
afterAll(async () => { if (cleanup) await cleanup(); });
|
||||
|
||||
it('masks the password and the PIN in the rendered body, keeps the rest', async () => {
|
||||
const res = await request(app).get(`/api/admin/email/queue/${rowId}`).set('Authorization', `Bearer ${token}`);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.renderedHtml).not.toContain(PASSWORD);
|
||||
expect(res.body.renderedHtml).not.toContain('Ada's 7788');
|
||||
expect(res.body.renderedHtml).toContain(`Password: ${MASK}`);
|
||||
expect(res.body.renderedHtml).toContain(`PIN: ${MASK}`);
|
||||
expect(res.body.cc).toBe('[email protected]');
|
||||
expect(JSON.stringify(res.body)).not.toContain(PASSWORD);
|
||||
// the stored row is untouched by a read
|
||||
const row = await db('email_queue').where('id', rowId).first();
|
||||
expect(row.rendered_html).toContain(PASSWORD);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,121 @@
|
||||
/**
|
||||
* Passwords leave the email archive once a row is final (see
|
||||
* utils/emailSecretRedaction.js). The gallery-created email carries the
|
||||
* gallery password and the client PIN; after the mail is out — or after the
|
||||
* row is out of retries — neither survives in email_data or rendered_html.
|
||||
*/
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
|
||||
process.env.NODE_ENV = 'test';
|
||||
process.env.TEST_DATABASE_PATH = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-mailredact-')), 'db.sqlite');
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'mailredact-test-secret';
|
||||
process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-mailredact-storage-'));
|
||||
|
||||
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
|
||||
const { MASK } = require('../../src/utils/emailSecretRedaction');
|
||||
|
||||
function stubWebhookTransport(impl) {
|
||||
const transport = require('../../src/services/emailWebhookTransport');
|
||||
const savedFrom = process.env.EMAIL_FROM;
|
||||
process.env.EMAIL_FROM = '[email protected]';
|
||||
const mails = [];
|
||||
const enabled = jest.spyOn(transport, 'isEnabled').mockReturnValue(true);
|
||||
// mockRestore() wipes mock.calls, so keep our own copy of what went out
|
||||
const send = jest.spyOn(transport, 'send').mockImplementation(async (mail) => { mails.push(mail); return impl(mail); });
|
||||
return { mails, restore() { enabled.mockRestore(); send.mockRestore(); if (savedFrom === undefined) delete process.env.EMAIL_FROM; else process.env.EMAIL_FROM = savedFrom; } };
|
||||
}
|
||||
|
||||
describe('email archive redaction', () => {
|
||||
let db; let cleanup; let eventId;
|
||||
const PASSWORD = 'Sunset-42!'; const PIN = '7788';
|
||||
const queue = (extra = {}) => db('email_queue').insert({
|
||||
event_id: eventId, recipient_email: '[email protected]', email_type: 'gallery_created',
|
||||
email_data: JSON.stringify({
|
||||
customer_name: 'Ada', host_name: 'Ada', event_name: 'Redaction Wedding', event_date: '2026-09-07',
|
||||
gallery_link: 'https://photos.example/gallery/redaction-wedding/tok', gallery_password: PASSWORD,
|
||||
client_link: 'https://photos.example/gallery/redaction-wedding/client-access?token=abc', client_password: PIN,
|
||||
expiry_date: null, welcome_message: '',
|
||||
}),
|
||||
status: 'pending', created_at: new Date().toISOString(), scheduled_at: new Date().toISOString(), retry_count: 0,
|
||||
...extra,
|
||||
}).returning('id').then((r) => r[0]?.id ?? r[0]);
|
||||
|
||||
beforeAll(async () => {
|
||||
({ db, cleanup } = await bootCrmDb());
|
||||
await seedMinimal(db);
|
||||
const ins = await db('events').insert({
|
||||
slug: 'redaction-wedding', event_type: 'wedding', event_name: 'Redaction Wedding', event_date: '2026-09-07',
|
||||
customer_email: '[email protected]', customer_name: 'Ada', password_hash: 'x', share_link: '/gallery/redaction-wedding/tok',
|
||||
share_token: 'tok', expires_at: new Date(Date.now() + 86400000).toISOString(), is_active: true, created_at: new Date().toISOString(),
|
||||
}).returning('id');
|
||||
eventId = ins[0]?.id ?? ins[0];
|
||||
}, 120000);
|
||||
afterAll(async () => { if (cleanup) await cleanup(); });
|
||||
|
||||
it('scrubs the variables and the rendered body once the mail is out', async () => {
|
||||
const id = await queue();
|
||||
const stub = stubWebhookTransport(async () => ({ messageId: 'sent-1' }));
|
||||
try {
|
||||
const { processEmailQueue } = require('../../src/services/emailProcessor');
|
||||
await processEmailQueue({ ignoreSchedule: true, onlyId: id });
|
||||
} finally { stub.restore(); }
|
||||
const row = await db('email_queue').where('id', id).first();
|
||||
expect(row.status).toBe('sent');
|
||||
const data = JSON.parse(row.email_data);
|
||||
expect(data.gallery_password).toBe(MASK);
|
||||
expect(data.client_password).toBe(MASK);
|
||||
expect(data.customer_name).toBe('Ada');
|
||||
expect(row.rendered_html).toBeTruthy();
|
||||
expect(row.rendered_html).not.toContain(PASSWORD);
|
||||
expect(row.rendered_html).not.toContain(PIN);
|
||||
expect(row.rendered_html).toContain(MASK);
|
||||
// the mail itself went out with the real password; only the archive lost it
|
||||
expect(stub.mails).toHaveLength(1);
|
||||
expect(String(stub.mails[0].html)).toContain(PASSWORD);
|
||||
expect(String(stub.mails[0].html)).not.toContain(MASK);
|
||||
|
||||
// Messages "resend" copies the archived variables into a new pending
|
||||
// row and "retry" re-queues the row itself: both mails must say the
|
||||
// password is not shown rather than print the mask (or the password).
|
||||
const { resendEmail, retryEmail } = require('../../src/services/projectService');
|
||||
const resent = await resendEmail(id);
|
||||
await retryEmail(id);
|
||||
for (const rowId of [resent.id, id]) {
|
||||
const stub2 = stubWebhookTransport(async () => ({ messageId: `sent-again-${rowId}` }));
|
||||
try {
|
||||
const { processEmailQueue } = require('../../src/services/emailProcessor');
|
||||
await processEmailQueue({ ignoreSchedule: true, onlyId: rowId });
|
||||
} finally { stub2.restore(); }
|
||||
expect(stub2.mails).toHaveLength(1);
|
||||
const html = String(stub2.mails[0].html);
|
||||
expect(html).not.toContain(MASK);
|
||||
expect(html).not.toContain(PASSWORD);
|
||||
expect(html).not.toContain(PIN);
|
||||
expect(html).not.toContain('{{password_security_message}}');
|
||||
expect(html).toContain('security');
|
||||
// archived again without the password (mask or sentinel, never the value)
|
||||
expect(JSON.parse((await db('email_queue').where('id', rowId).first()).email_data).gallery_password).not.toBe(PASSWORD);
|
||||
}
|
||||
});
|
||||
|
||||
it('keeps the variables in the clear while the row can still be retried', async () => {
|
||||
const id = await queue({ retry_count: 1 });
|
||||
const stub = stubWebhookTransport(async () => { throw new Error('transport down'); });
|
||||
try {
|
||||
const { processEmailQueue } = require('../../src/services/emailProcessor');
|
||||
await processEmailQueue({ ignoreSchedule: true, onlyId: id });
|
||||
let row = await db('email_queue').where('id', id).first();
|
||||
expect(row.retry_count).toBe(2);
|
||||
expect(JSON.parse(row.email_data).gallery_password).toBe(PASSWORD);
|
||||
// out of retries — a Messages "retry" resets the counter and this row
|
||||
// must still be able to send the real password
|
||||
await processEmailQueue({ ignoreSchedule: true, onlyId: id });
|
||||
row = await db('email_queue').where('id', id).first();
|
||||
expect(row.retry_count).toBe(3);
|
||||
expect(row.status).not.toBe('sent');
|
||||
expect(JSON.parse(row.email_data).gallery_password).toBe(PASSWORD);
|
||||
} finally { stub.restore(); }
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,90 @@
|
||||
/**
|
||||
* Passwords must not survive in the email archive (see the module header of
|
||||
* utils/emailSecretRedaction.js).
|
||||
*/
|
||||
const { secretValues, redactEmailData, redactRenderedHtml, replaceMaskedSecrets, parseEmailData, MASK } = require('../../src/utils/emailSecretRedaction');
|
||||
|
||||
describe('email secret redaction', () => {
|
||||
const data = {
|
||||
customer_name: 'Ada', gallery_link: 'https://p.example/gallery/x/tok',
|
||||
gallery_password: 'Sunset-42!', client_password: '1234', welcome_message: 'hi',
|
||||
attachments: [{ filename: 'a.pdf', password: 'zip-secret' }],
|
||||
};
|
||||
|
||||
it('finds the secrets by key name, nested included, and skips sentinels', () => {
|
||||
expect(secretValues(data).sort()).toEqual(['1234', 'Sunset-42!', 'zip-secret']);
|
||||
expect(secretValues({ gallery_password: '{{password_security_message}}' })).toEqual([]);
|
||||
expect(secretValues({ gallery_password: 'No password required' })).toEqual([]);
|
||||
expect(secretValues({ gallery_password: '(set at creation)' })).toEqual([]);
|
||||
expect(secretValues({ gallery_password: '' })).toEqual([]);
|
||||
});
|
||||
|
||||
it('masks the secrets in the variables and leaves everything else alone', () => {
|
||||
const redacted = redactEmailData(data);
|
||||
expect(redacted.gallery_password).toBe(MASK);
|
||||
expect(redacted.client_password).toBe(MASK);
|
||||
expect(redacted.attachments[0].password).toBe(MASK);
|
||||
expect(redacted.attachments[0].filename).toBe('a.pdf');
|
||||
expect(redacted.customer_name).toBe('Ada');
|
||||
expect(redacted.gallery_link).toBe(data.gallery_link);
|
||||
// sentinels stay readable
|
||||
expect(redactEmailData({ gallery_password: 'No password required' }).gallery_password).toBe('No password required');
|
||||
// the input is not mutated
|
||||
expect(data.gallery_password).toBe('Sunset-42!');
|
||||
});
|
||||
|
||||
it('strips the secrets from the rendered HTML, plain and HTML-escaped', () => {
|
||||
const html = '<li>Password: Sunset-42!</li><li>PIN: 1234</li><p>Tom & Ada's day</p>';
|
||||
const out = redactRenderedHtml(html, secretValues({ ...data, client_password: 'Tom & Ada\'s day' }));
|
||||
expect(out).toContain(`Password: ${MASK}`);
|
||||
expect(out).not.toContain('Sunset-42!');
|
||||
expect(out).toContain(`<p>${MASK}</p>`);
|
||||
expect(redactRenderedHtml(html, [])).toBe(html);
|
||||
expect(redactRenderedHtml(null, ['x'])).toBeNull();
|
||||
});
|
||||
|
||||
it('parses stored email_data leniently', () => {
|
||||
expect(parseEmailData('{"a":1}')).toEqual({ a: 1 });
|
||||
expect(parseEmailData({ a: 1 })).toEqual({ a: 1 });
|
||||
expect(parseEmailData('not json')).toEqual({});
|
||||
expect(parseEmailData(null)).toEqual({});
|
||||
});
|
||||
|
||||
it('ignores the pipeline sentinels but not a brace-wrapped real password', () => {
|
||||
expect(secretValues({ gallery_password: '(set at creation)', client_password: 'No password required' })).toEqual([]);
|
||||
expect(secretValues({ gallery_password: '{{Sunset-42!}}' })).toEqual(['{{Sunset-42!}}']);
|
||||
});
|
||||
|
||||
it('replaceMaskedSecrets swaps archive masks for the security sentinel, leaves the rest', () => {
|
||||
const out = replaceMaskedSecrets({ customer_name: 'Ada', gallery_password: MASK, client_password: MASK, nested: { pin: MASK, note: MASK } });
|
||||
expect(out).toEqual({
|
||||
customer_name: 'Ada',
|
||||
gallery_password: '{{password_security_message}}',
|
||||
client_password: '{{password_security_message}}',
|
||||
nested: { pin: '{{password_security_message}}', note: MASK },
|
||||
});
|
||||
});
|
||||
|
||||
it('masks a raw secret that the template turned into markup', () => {
|
||||
const html = '<p>PIN: Se<cr3t>Pin42! and Se<cr3t>Pin42!</p>';
|
||||
expect(redactRenderedHtml(html, ['Se<cr3t>Pin42!'])).toBe(`<p>PIN: ${MASK} and ${MASK}</p>`);
|
||||
});
|
||||
|
||||
it('masks a secret in a quoted attribute value that contains ">"', () => {
|
||||
const html = '<a title="Sunset42 > details" href="/x">Sunset42</a>';
|
||||
expect(redactRenderedHtml(html, ['Sunset42'])).toBe(`<a title="${MASK} > details" href="/x">${MASK}</a>`);
|
||||
// unbalanced quote: the broken tag is treated as text, still scrubbed
|
||||
expect(redactRenderedHtml('<a title="Sunset42>Sunset42</a>', ['Sunset42'])).toBe(`<a title="${MASK}>${MASK}</a>`);
|
||||
});
|
||||
|
||||
it('masks a secret inside an HTML comment, ">" included', () => {
|
||||
const html = '<!-- PIN: 7788 --><p>x</p><!-- a > 7788 -->';
|
||||
expect(redactRenderedHtml(html, ['7788'])).toBe(`<!-- PIN: ${MASK} --><p>x</p><!-- a > ${MASK} -->`);
|
||||
});
|
||||
|
||||
it('masks overlapping secrets completely and leaves markup alone', () => {
|
||||
const html = '<p>Password: Sunset-42! PIN: Sunset-42!7788</p><a href="https://x.example/?p=Sunset-42!" style="color:red" title=7788>href</a>';
|
||||
const out = redactRenderedHtml(html, ['Sunset-42!', 'Sunset-42!7788', 'href', 'style', '7788']);
|
||||
expect(out).toBe(`<p>Password: ${MASK} PIN: ${MASK}</p><a href="https://x.example/?p=${MASK}" style="color:red" title=${MASK}>${MASK}</a>`);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user