Skip to content

Commit

Permalink
Use a separate triple Mustache allowlist for email (#37189)
Browse files Browse the repository at this point in the history
This allowlist of triple Mustache tags was shared with the email
format. triple-mustache is a pretty security sensitive feature in
the email format, and adding non-formatting tags like amp-img to
the allowlist requires a security review.

For some email clients (like Gmail) the Mustache is actually rendered
on the server side, and the server uses a different triple-mustache
allowlist. However, for consistency with email clients who don't
server-side render the template we should keep the client-side
sanitization consistent.

Using a separate list for email so we don't accidentally modify the
email format when there are requests to add new tags for the website
format (e.g., #32039). We can still add to the email allowlist on a
case-by-case basis.
  • Loading branch information
zhangsu committed Dec 16, 2021
1 parent 5438c97 commit 2bfd164
Show file tree
Hide file tree
Showing 7 changed files with 207 additions and 80 deletions.
4 changes: 3 additions & 1 deletion extensions/amp-mustache/0.1/amp-mustache.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ export class AmpMustache extends BaseTemplate {
super(element, win);

// Unescaped templating (triple mustache) has a special, strict sanitizer.
mustache.setUnescapedSanitizer(sanitizeTagsForTripleMustache);
mustache.setUnescapedSanitizer((html) =>
sanitizeTagsForTripleMustache(html, this.win.document)
);

user().warn(
TAG,
Expand Down
14 changes: 13 additions & 1 deletion extensions/amp-mustache/amp-mustache.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,19 @@ that among other things, you can't use `amp-mustache` to:
- Calculate tag name. E.g. `<{{tagName}}>` is not allowed.
- Calculate attribute name. E.g. `<div {{attrName}}=something>` is not allowed.

The output of "triple-mustache" is sanitized to only allow the following tags: `a`, `amp-img`, `article`, `aside`, `b`, `blockquote`, `br`, `caption`, `code`, `col`, `colgroup`, `dd`, `del`, `details`, `div`, `dl`, `dt`, `em`, `figcaption`, `figure`, `footer`, `h1`, `h2`, `h3`, `header`, `hr`, `i`, `ins`, `li`, `main`, `mark`, `nav`, `ol`, `p`, `pre`, `q`, `s`, `section`, `small`, `span`, `strong`, `sub`, `summary`, `sup`, `table`, `tbody`, `td`, `tfoot`, `th`, `thead`, `time`, `tr`, `u`, `ul`.
The output of "triple-mustache" is sanitized to only allow the following tags:

[filter formats="websites, ads"]

`a`, `amp-img`, `article`, `aside`, `b`, `blockquote`, `br`, `caption`, `code`, `col`, `colgroup`, `dd`, `del`, `details`, `div`, `dl`, `dt`, `em`, `figcaption`, `figure`, `footer`, `h1`, `h2`, `h3`, `header`, `hr`, `i`, `ins`, `li`, `main`, `mark`, `nav`, `ol`, `p`, `pre`, `q`, `s`, `section`, `small`, `span`, `strong`, `sub`, `summary`, `sup`, `table`, `tbody`, `td`, `tfoot`, `th`, `thead`, `time`, `tr`, `u`, `ul`.

[/filter]<!-- formats="websites, ads" -->

[filter formats="email"]

`a`, `article`, `aside`, `b`, `blockquote`, `br`, `caption`, `code`, `col`, `colgroup`, `dd`, `del`, `details`, `div`, `dl`, `dt`, `em`, `figcaption`, `figure`, `footer`, `h1`, `h2`, `h3`, `header`, `hr`, `i`, `ins`, `li`, `main`, `mark`, `nav`, `ol`, `p`, `pre`, `q`, `s`, `section`, `small`, `span`, `strong`, `sub`, `summary`, `sup`, `table`, `tbody`, `td`, `tfoot`, `th`, `thead`, `time`, `tr`, `u`, `ul`.

[/filter]<!-- formats="email" -->

### Sanitization

Expand Down
5 changes: 4 additions & 1 deletion src/purifier/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
BIND_PREFIX,
DENYLISTED_TAGS,
EMAIL_ALLOWLISTED_AMP_TAGS,
EMAIL_TRIPLE_MUSTACHE_ALLOWLISTED_TAGS,
TRIPLE_MUSTACHE_ALLOWLISTED_TAGS,
isValidAttr,
markElementForDiffing,
Expand Down Expand Up @@ -103,7 +104,9 @@ export class Purifier {
// RETURN_DOM_FRAGMENT to keep the <template> and FORCE_BODY to prevent
// reparenting. See https://github.com/cure53/DOMPurify/issues/285#issuecomment-397810671
const fragment = this.domPurifyTriple_.sanitize(dirty, {
'ALLOWED_TAGS': TRIPLE_MUSTACHE_ALLOWLISTED_TAGS,
'ALLOWED_TAGS': isAmp4Email(this.doc_)
? EMAIL_TRIPLE_MUSTACHE_ALLOWLISTED_TAGS
: TRIPLE_MUSTACHE_ALLOWLISTED_TAGS,
'FORCE_BODY': true,
'RETURN_DOM_FRAGMENT': true,
});
Expand Down
65 changes: 64 additions & 1 deletion src/purifier/sanitation.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ export const EMAIL_ALLOWLISTED_AMP_TAGS = {
};

/**
* Allowlist of tags allowed in triple mustache e.g. {{{name}}}.
* Allowlist of tags allowed in triple mustache in non-email format, e.g.
* {{{name}}}.
* Very restrictive by design since the triple mustache renders unescaped HTML
* which, unlike double mustache, won't be processed by the AMP Validator.
* @const {!Array<string>}
Expand Down Expand Up @@ -158,6 +159,68 @@ export const TRIPLE_MUSTACHE_ALLOWLISTED_TAGS = [
'ul',
];

/**
* Same as `TRIPLE_MUSTACHE_ALLOWLISTED_TAGS` except for the email format. The
* email format has a different threat model and needs to evolve the allowlist
* independently from other formats, as certain tags can be considered safe in
* other formats but not in the email format.
* @const {!Array<string>}
*/
export const EMAIL_TRIPLE_MUSTACHE_ALLOWLISTED_TAGS = [
'a',
'article',
'aside',
'b',
'blockquote',
'br',
'caption',
'code',
'col',
'colgroup',
'dd',
'del',
'details',
'div',
'dl',
'dt',
'em',
'figcaption',
'figure',
'footer',
'h1',
'h2',
'h3',
'header',
'hr',
'i',
'ins',
'li',
'main',
'mark',
'nav',
'ol',
'p',
'pre',
'q',
's',
'section',
'small',
'span',
'strong',
'sub',
'summary',
'sup',
'table',
'tbody',
'td',
'tfoot',
'th',
'thead',
'time',
'tr',
'u',
'ul',
];
/**
* Tag-agnostic attribute allowlisted used by both Caja and DOMPurify.
* @const {!Array<string>}
Expand Down
17 changes: 13 additions & 4 deletions src/sanitizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
BIND_PREFIX,
DENYLISTED_TAGS,
EMAIL_ALLOWLISTED_AMP_TAGS,
EMAIL_TRIPLE_MUSTACHE_ALLOWLISTED_TAGS,
TRIPLE_MUSTACHE_ALLOWLISTED_TAGS,
isValidAttr,
} from '#purifier/sanitation';
Expand Down Expand Up @@ -243,19 +244,23 @@ export function sanitizeHtml(html, doc) {
* We do so in sanitizeHtml which occurs after this initial sanitizing.
*
* @param {string} html
* @param {!Document} doc
* @return {string}
*/
export function sanitizeTagsForTripleMustache(html) {
return htmlSanitizer.sanitizeWithPolicy(html, tripleMustacheTagPolicy);
export function sanitizeTagsForTripleMustache(html, doc) {
return htmlSanitizer.sanitizeWithPolicy(html, (tagName, attribs) =>
tripleMustacheTagPolicy(tagName, attribs, doc)
);
}

/**
* Tag policy for handling what is valid html in templates.
* @param {string} tagName
* @param {!Array<string>} attribs
* @param {!Document} doc
* @return {?{tagName: string, attribs: !Array<string>}}
*/
function tripleMustacheTagPolicy(tagName, attribs) {
function tripleMustacheTagPolicy(tagName, attribs, doc) {
if (tagName == 'template') {
for (let i = 0; i < attribs.length; i += 2) {
if (attribs[i] == 'type' && attribs[i + 1] == 'amp-mustache') {
Expand All @@ -266,7 +271,11 @@ function tripleMustacheTagPolicy(tagName, attribs) {
}
}
}
if (!TRIPLE_MUSTACHE_ALLOWLISTED_TAGS.includes(tagName)) {
if (isAmp4Email(doc)) {
if (!EMAIL_TRIPLE_MUSTACHE_ALLOWLISTED_TAGS.includes(tagName)) {
return null;
}
} else if (!TRIPLE_MUSTACHE_ALLOWLISTED_TAGS.includes(tagName)) {
return null;
}
return {
Expand Down
132 changes: 79 additions & 53 deletions test/unit/test-purifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,25 @@ describes.sandboxed
.configure()
.skipFirefox()
.run('DOMPurify-based', {}, (env) => {
let html;
let purify;
let purifyTripleMustache;
let rewriteAttributeValueSpy;

beforeEach(() => {
html = document.createElement('html');
const documentEl = {
documentElement: html,
createElement: (tagName) => document.createElement(tagName),
};

rewriteAttributeValueSpy = env.sandbox.spy(
urlRewrite,
'rewriteAttributeValue'
);

const purifier = new Purifier(
document,
documentEl,
{},
urlRewrite.rewriteAttributeValue
);
Expand Down Expand Up @@ -488,63 +495,82 @@ describes.sandboxed
expect(purifyTripleMustache(html)).to.be.equal(html);
});

it('should allowlist formatting related elements', () => {
const nonAllowlistedTag = '<img>';
const allowlistedFormattingTags =
'<b>abc</b><div>def</div>' +
'<br><code></code><del></del><em></em>' +
'<i></i><ins></ins><mark></mark><s></s>' +
'<small></small><strong></strong><sub></sub>' +
'<sup></sup><time></time><u></u><hr>';
const html = `${allowlistedFormattingTags}${nonAllowlistedTag}`;
// Expect the purifier to unescape the allowlisted tags and to sanitize
// and remove the img tag.
expect(purifyTripleMustache(html)).to.be.equal(
allowlistedFormattingTags
);
});
['amp', 'amp4email'].forEach((format) => {
describe(`with ${format} format`, () => {
beforeEach(() => {
html.setAttribute(format, '');
});

it('should allowlist h1, h2, h3 and amp-img elements', () => {
const html =
'<h1>Heading 1</h1>' +
'<h2>Heading 2</h2>' +
'<h3>Heading 3</h3>' +
'<amp-img></amp-img>';
expect(purifyTripleMustache(html)).to.be.equal(html);
it('should allowlist formatting related elements', () => {
const nonAllowlistedTag = '<img>';
const allowlistedFormattingTags =
'<b>abc</b><div>def</div>' +
'<br><code></code><del></del><em></em>' +
'<i></i><ins></ins><mark></mark><s></s>' +
'<small></small><strong></strong><sub></sub>' +
'<sup></sup><time></time><u></u><hr>';
const html = `${allowlistedFormattingTags}${nonAllowlistedTag}`;
// Expect the purifier to unescape the allowlisted tags and to
// sanitize and remove the img tag.
expect(purifyTripleMustache(html)).to.be.equal(
allowlistedFormattingTags
);
});

it('should allowlist h1, h2 and h3 elements', () => {
const html =
'<h1>Heading 1</h1>' +
'<h2>Heading 2</h2>' +
'<h3>Heading 3</h3>';
expect(purifyTripleMustache(html)).to.be.equal(html);
});

it('should allowlist table related elements and anchor tags', () => {
const html =
'<table class="valid-class">' +
'<colgroup><col><col></colgroup>' +
'<caption>caption</caption>' +
'<thead><tr><th colspan="2">header</th></tr></thead>' +
'<tbody><tr><td>' +
'<a href="http://www.google.com">google</a>' +
'</td></tr></tbody>' +
'<tfoot><tr>' +
'<td colspan="2"><span>footer</span></td>' +
'</tr></tfoot>' +
'</table>';
expect(purifyTripleMustache(html)).to.be.equal(html);
});

it('should allowlist container elements', () => {
const html =
'<article>Article</article>' +
'<aside></aside>' +
'<blockquote>A quote</blockquote>' +
'<details></details>' +
'<figcaption></figcaption>' +
'<figure></figure>' +
'<footer>Footer</footer>' +
'<header></header>' +
'<main class="content"></main>' +
'<nav></nav>' +
'<pre></pre>' +
'<section id="sec"></section>' +
'<summary></summary>';
expect(purifyTripleMustache(html)).to.be.equal(html);
});
});
});

it('should allowlist table related elements and anchor tags', () => {
const html =
'<table class="valid-class">' +
'<colgroup><col><col></colgroup>' +
'<caption>caption</caption>' +
'<thead><tr><th colspan="2">header</th></tr></thead>' +
'<tbody><tr><td>' +
'<a href="http://www.google.com">google</a>' +
'</td></tr></tbody>' +
'<tfoot><tr>' +
'<td colspan="2"><span>footer</span></td>' +
'</tr></tfoot>' +
'</table>';
expect(purifyTripleMustache(html)).to.be.equal(html);
it('should allowlist amp-img element', () => {
html.setAttribute('amp', '');
const markup = '<amp-img></amp-img>';
expect(purifyTripleMustache(markup)).to.be.equal(markup);
});

it('should allowlist container elements', () => {
const html =
'<article>Article</article>' +
'<aside></aside>' +
'<blockquote>A quote</blockquote>' +
'<details></details>' +
'<figcaption></figcaption>' +
'<figure></figure>' +
'<footer>Footer</footer>' +
'<header></header>' +
'<main class="content"></main>' +
'<nav></nav>' +
'<pre></pre>' +
'<section id="sec"></section>' +
'<summary></summary>';
expect(purifyTripleMustache(html)).to.be.equal(html);
it('should not allowlist amp-img element for AMP4Email', () => {
html.setAttribute('amp4email', '');
const markup = '<amp-img></amp-img>';
expect(purifyTripleMustache(markup)).to.be.empty;
});

it('should sanitize tags, removing unsafe attributes', () => {
Expand Down

0 comments on commit 2bfd164

Please sign in to comment.