fix(security): re-check inline CSS after template substitution
The fourth bypass found in this review, and the one no lexer fix
reaches: sanitizing runs on the stored body, but safeTemplateReplace
rewrites it afterwards, so the string that was validated is not the
string that is sent.
A conditional inside a style attribute can delete the very quoting that
made a url() inert:
style="--x:x{{#if company_name}}'{{/if}};background:url(https://evil…)"
At write time the url() genuinely sits inside a CSS string and is
correctly left alone. Expanding the conditional for a recipient with no
company name removes both quotes and the background goes live —
confirmed end to end against the real functions.
The style-attribute pass now runs again on the substituted output.
Substitution cannot introduce a `"` (values are HTML-escaped), so the
attribute match still holds. body_css is not substituted, so the
<style> block cannot be rewritten after its check and needs nothing.
This is the case the removed newsletter pass had been covering. Rather
than reinstating a second definition of "disallowed", the one definition
now runs at both points where the content changes.
Refs #1264
This commit is contained in:
@@ -12,7 +12,9 @@ jest.mock('../../src/database/db', () => ({ db: jest.fn(), logActivity: jest.fn(
|
||||
|
||||
const {
|
||||
sanitizeCampaignBody, sanitizeCampaignCss, MAX_BODY_BYTES,
|
||||
sanitizeInlineStylesAfterSubstitution,
|
||||
} = require('../../src/services/newsletterService');
|
||||
const { safeTemplateReplace } = require('../../src/services/emailProcessor');
|
||||
|
||||
describe('sanitizeCampaignBody', () => {
|
||||
it('returns empty string for empty input', () => {
|
||||
@@ -188,3 +190,46 @@ describe('sanitizeCampaignCss', () => {
|
||||
expect(css).not.toContain('</style>');
|
||||
});
|
||||
});
|
||||
|
||||
describe('inline CSS is re-checked after template substitution', () => {
|
||||
// The stored body is sanitized, but safeTemplateReplace rewrites it
|
||||
// afterwards — so the string that was validated is not the string that is
|
||||
// sent. A conditional inside a style attribute can delete the quoting that
|
||||
// made a url() inert, which no amount of lexer correctness can catch.
|
||||
const payload =
|
||||
`<p style="--x:x{{#if company_name}}'{{/if}};`
|
||||
+ `background:url(https://evil.example/p.gif);`
|
||||
+ `--y:x{{#if company_name}}'{{/if}}">hi</p>`;
|
||||
|
||||
it('neutralises a url() that substitution would activate', () => {
|
||||
const stored = sanitizeCampaignBody(payload);
|
||||
// Correctly left alone at write time: the url() really is inside a CSS
|
||||
// string while the conditionals are still in place.
|
||||
expect(stored).toContain('evil.example');
|
||||
|
||||
const substituted = safeTemplateReplace(stored, { company_name: '' }, { escapeHtml: true });
|
||||
// Expansion removed the quotes, so without the recheck this ships live.
|
||||
expect(substituted).toMatch(/background:url\(https:\/\/evil\.example/);
|
||||
|
||||
const rendered = sanitizeInlineStylesAfterSubstitution(substituted);
|
||||
expect(rendered).not.toMatch(/url\(\s*['"]?https:\/\/evil\.example/);
|
||||
expect(rendered).toContain('background:none');
|
||||
});
|
||||
|
||||
it('leaves a body without style attributes untouched', () => {
|
||||
const html = '<p>Hello {{first_name}}</p>';
|
||||
expect(sanitizeInlineStylesAfterSubstitution(html)).toBe(html);
|
||||
});
|
||||
|
||||
it('keeps legitimate inline styles through the recheck', () => {
|
||||
const html = '<p style="color:red;font-size:14px">hi</p>';
|
||||
const out = sanitizeInlineStylesAfterSubstitution(html);
|
||||
expect(out).toContain('color:red');
|
||||
expect(out).toContain('font-size:14px');
|
||||
});
|
||||
|
||||
it('is safe on empty and nullish input', () => {
|
||||
expect(sanitizeInlineStylesAfterSubstitution('')).toBe('');
|
||||
expect(sanitizeInlineStylesAfterSubstitution(null)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user