Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
fix($compile): disallow interpolations for DOM event handlers
Browse files Browse the repository at this point in the history
BREAKING CHANGE: Interpolations inside DOM event handlers are
    disallowed.  DOM event handlers execute arbitrary Javascript code.
    Using an interpolation for such handlers means that the interpolated
    value is a JS string that is evaluated.  Storing or generating such
    strings is error prone and likely leads to an XSS if you're not
    super careful.  On the other hand, ng-click and such event handlers
    evaluate Angular expressions that are a lot safer (e.g. No direct
    access to global objects - only scope), cleaner and harder to
    exploit.

    To migrate the code follow the example below:

    Before:

        JS:   scope.foo = 'alert(1)';
        HTML: <div onclick="{{foo}}">

    After:

        JS:   scope.foo = function() { alert(1); }
        HTML: <div ng-click="foo()">
  • Loading branch information
chirayuk committed Jun 22, 2013
1 parent 1adf29a commit 39841f2
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 0 deletions.
10 changes: 10 additions & 0 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ function $CompileProvider($provide) {
CLASS_DIRECTIVE_REGEXP = /(([\d\w\-_]+)(?:\:([^;]+))?;?)/,
urlSanitizationWhitelist = /^\s*(https?|ftp|mailto|file):/;

// Ref: http://developers.whatwg.org/webappapis.html#event-handler-idl-attributes
// The assumption is that future DOM event attribute names will begin with
// 'on' and be composed of only English letters.
var EVENT_HANDLER_ATTR_REGEXP = /^(on[a-z]*|formaction)$/;

/**
* @ngdoc function
Expand Down Expand Up @@ -1165,6 +1169,12 @@ function $CompileProvider($provide) {
compile: valueFn(function attrInterpolateLinkFn(scope, element, attr) {
var $$observers = (attr.$$observers || (attr.$$observers = {}));

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.");
}

// we need to interpolate again, in case the attribute value has been updated
// (e.g. by another directive's compile function)
interpolateFn = $interpolate(attr[name], true);
Expand Down
30 changes: 30 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2831,6 +2831,36 @@ describe('$compile', function() {
});
});

describe('interpolation on HTML DOM event handler attributes onclick, onXYZ, formaction', function() {
it('should disallow interpolation on onclick', inject(function($compile, $rootScope) {
// All interpolations are disallowed.
$rootScope.onClickJs = "";
expect(function() {
$compile('<button onclick="{{onClickJs}}"></script>')($rootScope);
}).toThrow(
"[$compile:nodomevents] Interpolations for HTML DOM event attributes are disallowed. " +
"Please use the ng- versions (such as ng-click instead of onclick) instead.");
expect(function() {
$compile('<button ONCLICK="{{onClickJs}}"></script>')($rootScope);
}).toThrow(
"[$compile:nodomevents] Interpolations for HTML DOM event attributes are disallowed. " +
"Please use the ng- versions (such as ng-click instead of onclick) instead.");
expect(function() {
$compile('<button ng-attr-onclick="{{onClickJs}}"></script>')($rootScope);
}).toThrow(
"[$compile:nodomevents] Interpolations for HTML DOM event attributes are disallowed. " +
"Please use the ng- versions (such as ng-click instead of onclick) instead.");
}));

it('should pass through arbitrary values on onXYZ event attributes that contain a hyphen', inject(function($compile, $rootScope) {
element = $compile('<button on-click="{{onClickJs}}"></script>')($rootScope);
$rootScope.onClickJs = 'javascript:doSomething()';
$rootScope.$apply();
expect(element.attr('on-click')).toEqual('javascript:doSomething()');
}));
});


describe('ngAttr* attribute binding', function() {

it('should bind after digest but not before', inject(function($compile, $rootScope) {
Expand Down

2 comments on commit 39841f2

@matsko
Copy link
Contributor

@matsko matsko commented on 39841f2 Jun 24, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@cironunes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Please sign in to comment.