Skip to content

Commit

Permalink
feat($compile): add support for arbitrary property and event bindings
Browse files Browse the repository at this point in the history 
Fixes angular#16428
Closes angular#16614
  • Loading branch information
@jbedard
jbedard committed Jul 25, 2018
1 parent f92f819 commit 0a5c935
Show file tree
Hide file tree
Showing 8 changed files with 1,392 additions and 76 deletions.
13 changes: 13 additions & 0 deletions docs/content/error/$compile/ctxoverride.ngdoc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
@ngdoc error
@name $compile:ctxoverride
@fullName DOM Property Security Context Override
@description

This error occurs when the security context for a property is defined via {@link ng.$compileProvider#addPropertySecurityContext addPropertySecurityContext()} multiple times under different security contexts.

For example:

```js
$compileProvider.addPropertySecurityContext("my-element", "src", $sce.MEDIA_URL);
$compileProvider.addPropertySecurityContext("my-element", "src", $sce.RESOURCE_URL); //throws
```
10 changes: 5 additions & 5 deletions docs/content/error/$compile/nodomevents.ngdoc
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
@ngdoc error
@name $compile:nodomevents
@fullName Interpolated Event Attributes
@fullName Event Attribute/Property Binding
@description

This error occurs when one tries to create a binding for event handler attributes like `onclick`, `onload`, `onsubmit`, etc.
This error occurs when one tries to create a binding for event handler attributes or properties like `onclick`, `onload`, `onsubmit`, etc.

There is no practical value in binding to these attributes and doing so only exposes your application to security vulnerabilities like XSS.
For these reasons binding to event handler attributes (all attributes that start with `on` and `formaction` attribute) is not supported.
There is no practical value in binding to these attributes/properties and doing so only exposes your application to security vulnerabilities like XSS.
For these reasons binding to event handler attributes and properties (`formaction` and all starting with `on`) is not supported.


An example code that would allow XSS vulnerability by evaluating user input in the window context could look like this:
Expand All @@ -17,4 +17,4 @@ An example code that would allow XSS vulnerability by evaluating user input in t

Since the `onclick` evaluates the value as JavaScript code in the window context, setting the `username` model to a value like `javascript:alert('PWND')` would result in script injection when the `div` is clicked.


Please use the `ng-*` or `ng-on-*` versions instead (such as `ng-click` or `ng-on-click` rather than `onclick`).
6 changes: 6 additions & 0 deletions src/.eslintrc.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,15 @@
/* ng/q.js */
"markQExceptionHandled": false,

/* sce.js */
"SCE_CONTEXTS": false,

/* ng/directive/directives.js */
"ngDirective": false,

/* ng/directive/ngEventDirs.js */
"createEventDirective": false,

/* ng/directive/input.js */
"VALID_CLASS": false,
"INVALID_CLASS": false,
Expand Down
227 changes: 193 additions & 34 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1586,6 +1586,91 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return cssClassDirectivesEnabledConfig;
};


/**
* The security context of DOM Properties.
* @private
*/
var PROP_CONTEXTS = createMap();

/**
* @ngdoc method
* @name $compileProvider#addPropertySecurityContext
* @description
*
* Defines the security context for DOM properties bound by ng-prop-*.
*
* @param {string} elementName The element name or '*' to match any element.
* @param {string} propertyName The DOM property name.
* @param {string} ctx The {@link $sce} security context in which this value is safe for use, e.g. `$sce.URL`
* @returns {object} `this` for chaining
*/
this.addPropertySecurityContext = function(elementName, propertyName, ctx) {
var key = (elementName.toLowerCase() + '|' + propertyName.toLowerCase());

if (key in PROP_CONTEXTS && PROP_CONTEXTS[key] !== ctx) {
throw $compileMinErr('ctxoverride', 'Property context \'{0}.{1}\' already set to \'{2}\', cannot override to \'{3}\'.', elementName, propertyName, PROP_CONTEXTS[key], ctx);
}

PROP_CONTEXTS[key] = ctx;
return this;
};

/* Default property contexts.
*
* Copy of https://github.com/angular/angular/blob/6.0.6/packages/compiler/src/schema/dom_security_schema.ts#L31-L58
* Changing:
* - SecurityContext.* => SCE_CONTEXTS/$sce.*
* - STYLE => CSS
* - various URL => MEDIA_URL
* - *|formAction, form|action URL => RESOURCE_URL (like the attribute)
*/
(function registerNativePropertyContexts() {
function registerContext(ctx, values) {
forEach(values, function(v) { PROP_CONTEXTS[v.toLowerCase()] = ctx; });
}

registerContext(SCE_CONTEXTS.HTML, [
'iframe|srcdoc',
'*|innerHTML',
'*|outerHTML'
]);
registerContext(SCE_CONTEXTS.CSS, ['*|style']);
registerContext(SCE_CONTEXTS.URL, [
'area|href', 'area|ping',
'a|href', 'a|ping',
'blockquote|cite',
'body|background',
'del|cite',
'input|src',
'ins|cite',
'q|cite'
]);
registerContext(SCE_CONTEXTS.MEDIA_URL, [
'audio|src',
'img|src', 'img|srcset',
'source|src', 'source|srcset',
'track|src',
'video|src', 'video|poster'
]);
registerContext(SCE_CONTEXTS.RESOURCE_URL, [
'*|formAction',
'applet|code', 'applet|codebase',
'base|href',
'embed|src',
'frame|src',
'form|action',
'head|profile',
'html|manifest',
'iframe|src',
'link|href',
'media|src',
'object|codebase', 'object|data',
'script|src'
]);
})();


this.$get = [
'$injector', '$interpolate', '$exceptionHandler', '$templateRequest', '$parse',
'$controller', '$rootScope', '$sce', '$animate',
Expand Down Expand Up @@ -1631,12 +1716,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}


function sanitizeSrcset(value) {
function sanitizeSrcset(value, invokeType) {
if (!value) {
return value;
}
if (!isString(value)) {
throw $compileMinErr('srcset', 'Can\'t pass trusted values to `$set(\'srcset\', value)`: "{0}"', value.toString());
throw $compileMinErr('srcset', 'Can\'t pass trusted values to `{0}`: "{1}"', invokeType, value.toString());
}

// Such values are a bit too complex to handle automatically inside $sce.
Expand Down Expand Up @@ -1916,7 +2001,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
: function denormalizeTemplate(template) {
return template.replace(/\{\{/g, startSymbol).replace(/}}/g, endSymbol);
},
NG_ATTR_BINDING = /^ngAttr[A-Z]/;
NG_PREFIX_BINDING = /^ng(Attr|Prop|On)([A-Z].*)$/;
var MULTI_ELEMENT_DIR_RE = /^(.+)Start$/;

compile.$$addBindingInfo = debugInfoEnabled ? function $$addBindingInfo($element, binding) {
Expand Down Expand Up @@ -2252,43 +2337,66 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
directiveNormalize(nodeName), 'E', maxPriority, ignoreDirective);

// iterate over the attributes
for (var attr, name, nName, ngAttrName, value, isNgAttr, nAttrs = node.attributes,
for (var attr, name, nName, value, ngPrefixMatch, nAttrs = node.attributes,
j = 0, jj = nAttrs && nAttrs.length; j < jj; j++) {
var attrStartName = false;
var attrEndName = false;

var isNgAttr = false, isNgProp = false, isNgEvent = false;
var multiElementMatch;

attr = nAttrs[j];
name = attr.name;
value = attr.value;

// support ngAttr attribute binding
ngAttrName = directiveNormalize(name);
isNgAttr = NG_ATTR_BINDING.test(ngAttrName);
if (isNgAttr) {
nName = directiveNormalize(name.toLowerCase());

// Support ng-attr-*, ng-prop-* and ng-on-*
if ((ngPrefixMatch = nName.match(NG_PREFIX_BINDING))) {
isNgAttr = ngPrefixMatch[1] === 'Attr';
isNgProp = ngPrefixMatch[1] === 'Prop';
isNgEvent = ngPrefixMatch[1] === 'On';

// Normalize the non-prefixed name
name = name.replace(PREFIX_REGEXP, '')
.substr(8).replace(/_(.)/g, function(match, letter) {
.toLowerCase()
.substr(4 + ngPrefixMatch[1].length).replace(/_(.)/g, function(match, letter) {
return letter.toUpperCase();
});
}

var multiElementMatch = ngAttrName.match(MULTI_ELEMENT_DIR_RE);
if (multiElementMatch && directiveIsMultiElement(multiElementMatch[1])) {
// Support *-start / *-end multi element directives
} else if ((multiElementMatch = nName.match(MULTI_ELEMENT_DIR_RE)) && directiveIsMultiElement(multiElementMatch[1])) {
attrStartName = name;
attrEndName = name.substr(0, name.length - 5) + 'end';
name = name.substr(0, name.length - 6);
}

nName = directiveNormalize(name.toLowerCase());
attrsMap[nName] = name;
if (isNgAttr || !attrs.hasOwnProperty(nName)) {
if (isNgProp || isNgEvent) {
attrs[nName] = value;
attrsMap[nName] = attr.name;

if (isNgProp) {
addPropertyDirective(node, directives, nName, name);
} else {
addEventDirective(directives, nName, name);
}
} else {
// Update nName for cases where a prefix was removed
// NOTE: the .toLowerCase() is unnecessary and causes https://github.com/angular/angular.js/issues/16624 for ng-attr-*
nName = directiveNormalize(name.toLowerCase());
attrsMap[nName] = name;

if (isNgAttr || !attrs.hasOwnProperty(nName)) {
attrs[nName] = value;
if (getBooleanAttrName(node, nName)) {
attrs[nName] = true; // presence means true
}
}

addAttrInterpolateDirective(node, directives, value, nName, isNgAttr);
addDirective(directives, nName, 'A', maxPriority, ignoreDirective, attrStartName,
attrEndName);
}
addAttrInterpolateDirective(node, directives, value, nName, isNgAttr);
addDirective(directives, nName, 'A', maxPriority, ignoreDirective, attrStartName,
attrEndName);
}

if (nodeName === 'input' && node.getAttribute('type') === 'hidden') {
Expand Down Expand Up @@ -3325,42 +3433,95 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}


function getTrustedContext(node, attrNormalizedName) {
function getTrustedAttrContext(nodeName, attrNormalizedName) {
if (attrNormalizedName === 'srcdoc') {
return $sce.HTML;
}
var tag = nodeName_(node);
// All tags with src attributes require a RESOURCE_URL value, except for
// img and various html5 media tags, which require the MEDIA_URL context.
// All nodes with src attributes require a RESOURCE_URL value, except for
// img and various html5 media nodes, which require the MEDIA_URL context.
if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') {
if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) {
if (['img', 'video', 'audio', 'source', 'track'].indexOf(nodeName) === -1) {
return $sce.RESOURCE_URL;
}
return $sce.MEDIA_URL;
} else if (attrNormalizedName === 'xlinkHref') {
// Some xlink:href are okay, most aren't
if (tag === 'image') return $sce.MEDIA_URL;
if (tag === 'a') return $sce.URL;
if (nodeName === 'image') return $sce.MEDIA_URL;
if (nodeName === 'a') return $sce.URL;
return $sce.RESOURCE_URL;
} else if (
// Formaction
(tag === 'form' && attrNormalizedName === 'action') ||
(nodeName === 'form' && attrNormalizedName === 'action') ||
// If relative URLs can go where they are not expected to, then
// all sorts of trust issues can arise.
(tag === 'base' && attrNormalizedName === 'href') ||
(nodeName === 'base' && attrNormalizedName === 'href') ||
// links can be stylesheets or imports, which can run script in the current origin
(tag === 'link' && attrNormalizedName === 'href')
(nodeName === 'link' && attrNormalizedName === 'href')
) {
return $sce.RESOURCE_URL;
} else if (tag === 'a' && (attrNormalizedName === 'href' ||
} else if (nodeName === 'a' && (attrNormalizedName === 'href' ||
attrNormalizedName === 'ngHref')) {
return $sce.URL;
}
}

function getTrustedPropContext(nodeName, propNormalizedName) {
var prop = propNormalizedName.toLowerCase();
return PROP_CONTEXTS[nodeName + '|' + prop] || PROP_CONTEXTS['*|' + prop];
}

function sanitizeSrcsetPropertyValue(value) {
return sanitizeSrcset($sce.valueOf(value), 'ng-prop-srcset');
}
function addPropertyDirective(node, directives, attrName, propName) {
if (EVENT_HANDLER_ATTR_REGEXP.test(propName)) {
throw $compileMinErr('nodomevents', 'Property bindings for HTML DOM event properties are disallowed');
}

var nodeName = nodeName_(node);
var trustedContext = getTrustedPropContext(nodeName, propName);

var sanitizer = identity;
// Sanitize img[srcset] + source[srcset] values.
if (propName === 'srcset' && (nodeName === 'img' || nodeName === 'source')) {
sanitizer = sanitizeSrcsetPropertyValue;
} else if (trustedContext) {
sanitizer = $sce.getTrusted.bind($sce, trustedContext);
}

directives.push({
priority: 100,
compile: function ngPropCompileFn(_, attr) {
var ngPropGetter = $parse(attr[attrName]);
var ngPropWatch = $parse(attr[attrName], function sceValueOf(val) {
// Unwrap the value to compare the actual inner safe value, not the wrapper object.
return $sce.valueOf(val);
});

return {
pre: function ngPropPreLinkFn(scope, $element) {
function applyPropValue() {
var propValue = ngPropGetter(scope);
$element.prop(propName, sanitizer(propValue));
}

applyPropValue();
scope.$watch(ngPropWatch, applyPropValue);
}
};
}
});
}

function addEventDirective(directives, attrName, eventName) {
directives.push(
createEventDirective($parse, $rootScope, $exceptionHandler, attrName, eventName, /*forceAsync=*/false)
);
}

function addAttrInterpolateDirective(node, directives, value, name, isNgAttr) {
var trustedContext = getTrustedContext(node, name);
var nodeName = nodeName_(node);
var trustedContext = getTrustedAttrContext(nodeName, name);
var mustHaveExpression = !isNgAttr;
var allOrNothing = ALL_OR_NOTHING_ATTRS[name] || isNgAttr;

Expand All @@ -3369,16 +3530,14 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
// no interpolation found -> ignore
if (!interpolateFn) return;

if (name === 'multiple' && nodeName_(node) === 'select') {
if (name === 'multiple' && nodeName === 'select') {
throw $compileMinErr('selmulti',
'Binding to the \'multiple\' attribute is not supported. Element: {0}',
startingTag(node));
}

if (EVENT_HANDLER_ATTR_REGEXP.test(name)) {
throw $compileMinErr('nodomevents',
'Interpolations for HTML DOM event attributes are disallowed. Please use the ' +
'ng- versions (such as ng-click instead of onclick) instead.');
throw $compileMinErr('nodomevents', 'Interpolations for HTML DOM event attributes are disallowed');
}

directives.push({
Expand Down
Loading

0 comments on commit 0a5c935

Please sign in to comment.