New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(security): Add an HTML sanitizer #8453

Merged
merged 4 commits into from May 9, 2016

Conversation

Projects
None yet
4 participants
@mprobst
Contributor

mprobst commented May 4, 2016

No description provided.

import {sanitizeHtml} from '../../src/security/html_sanitizer';
import {browserDetection} from '@angular/platform-browser/testing';
export function main() {

This comment has been minimized.

@IgorMinar

IgorMinar May 4, 2016

Member

there are many more tests at https://github.com/angular/angular.js/blob/master/test/ngSanitize/sanitizeSpec.js did you intentionally port only a few?

This comment has been minimized.

@mprobst

mprobst May 4, 2016

Contributor

This is intentional.

Most of the tests in the sanitize spec assert that the parser behaves in a particular way, but this code doesn't actually implement a parser. Effectively we're testing the DOM parser. I presume this was done because ngSanitize at some point implemented its own parser, but it doesn't apply to this code.

merge(VOID_ELEMENTS, BLOCK_ELEMENTS, INLINE_ELEMENTS, OPTIONAL_END_TAG_ELEMENTS);
// Attributes that have href and hence need to be sanitized
const URI_ATTRS = tagSet("background,cite,href,longdesc,src,xlink:href");

This comment has been minimized.

@anjmao

anjmao May 4, 2016

Code style: can we use both single and double quotes?

This comment has been minimized.

@mprobst

mprobst May 4, 2016

Contributor

Ah sorry, I'm used to the newer clang-format version automatically single-quoting strings. Fixed.

do {
if (mXSSAttempts === 0) {
throw new Error('Failed to sanitize html because the input is unstable');

This comment has been minimized.

@anjmao

anjmao May 4, 2016

Untested code brach, but I'm not sure if it possible to test with unit test.

This comment has been minimized.

@mprobst

mprobst May 4, 2016

Contributor

Yeah, we didn't have a test for this earlier either. We could try to mock the entire interaction with the DOM, but then we'd end up with a pure change detector test (i.e. we only assert that there is a for loop counting down from 5, which is not a good test).

@rjamet do you have an example exploit that'd trigger five instable DOM parses in a row?

@ocombe ocombe referenced this pull request May 4, 2016

Closed

added simple directive #31

@mprobst mprobst referenced this pull request May 6, 2016

Closed

Angular 2 Security Tracking Issue #8511

17 of 17 tasks complete
@mprobst

This comment has been minimized.

Contributor

mprobst commented May 9, 2016

mprobst added some commits May 1, 2016

feat(security): add an HTML sanitizer.
This is based on Angular 1's implementation, parsing an HTML document
into an inert DOM Document implementation, and then serializing only
specifically whitelisted elements.

It currently does not support SVG sanitization, all SVG elements are
rejected.

If available, the sanitizer uses the `<template>` HTML element as an
inert container.

Sanitization works client and server-side.

Reviewers: rjamet, tbosch , molnarg , koto

Differential Revision: https://reviews.angular.io/D108

@mprobst mprobst merged commit 7a524e3 into angular:master May 9, 2016

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
cla/google All necessary CLAs are signed

@mprobst mprobst deleted the mprobst:html-sanitizer branch May 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment