feat($parse): secure expressions by hiding "private" properties

[chirayuk]

BREAKING CHANGE:

This commit introduces the notion of "private" properties (properties
whose names begin and/or end with an underscore) on the scope chain.
These properties will not be available to Angular expressions (i.e. {{
}} interpolation in templates and strings passed to $parse) They are
freely available to JavaScript code (as before).

Motivation

Angular expressions execute in a limited context.  They do not have
direct access to the global scope, Window, Document or the Function
constructor.  However, they have direct access to names/properties on
the scope chain.  It has been a long standing best practice to keep
sensitive APIs outside of the scope chain (in a closure or your
controller.)  That's easier said that done for two reasons: (1)
JavaScript does not have a notion of private properties so if you need
someone on the scope chain for JavaScript use, you also expose it to
Angular expressions, and (2) the new "controller as" syntax that's now
in increased usage exposes the entire controller on the scope chain
greatly increaing the exposed surface.  Though Angular expressions are
written and controlled by the developer, they (1) typically deal with
user input and (2) don't get the kind of test coverage that JavaScript
code would.  This commit provides a way, via a naming convention, to
allow publishing/restricting properties from controllers/scopes to
Angular expressions enabling one to only expose those properties that
are actually needed by the expressions.

[mary-poppins]

Oh golly!  Another PR from a favorite contributor.  I'm giddy with joy!

[chirayuk]

@IgorMinar: I have another idea for this that I want to run by you.  I'm considering allowing access to "private" fields if they resolve to a non-function object.  One could still use it to perform mischief depending on the app (e.g. if some controller published a function looked something like function foo() { this.dispatchMap[this._state](…); } and the attacker was able to update _state to some unsafe string.  And this is just with strings).  So my thought was to only allow read/getter access to such fields and only when using the [] index operator syntax if the resulting value was not a function.  Thoughts?  If we do that, I think we should do it as a follow up commit – possible even after the 1.2 release.

[petebacondarwin]

I am not convinced by this change.

In the first place, the only real pattern that this allows is using scope to share private properties between controllers. Other aspects of private properties can be secured by placing them in the closure of the controller and exposing secure getters and setters on the scope.

Using scope to share private data between controllers seems like a bad idea to me. This would be better done using services that are responsible for holding this shared state, which is injected into the closure of the controller when it is instantiated.

Also, security issues like this are hard for most programmers to comprehend and I am not convinced that, even with this change in place, programmers would use it correctly to secure their code.

Finally, it makes me feel even more strongly that the "controller as" syntax is not a great pattern. Controllers were originally an excellent place to put helper functions, aspects of which can be published, securely, to the scope. By allowing the template designer to attach the entire controller object to the scope, you basically open up this security issue.

[vojtajina]

We can't I set "constructor", when I can get it?

[vojtajina]

Discussed with @chirayuk in person. LGTM, just do the little style fixes.

[vojtajina]

style: remove the else

[vojtajina]

@petebacondarwin This change does not make sharing code between controllers through scope inheritance (which we do not recommend) any simpler.

I agree with you, that sharing stuff between controllers through scope inheritance is not a good thing, but I don't think this change promotes this style in any way. Please explain more, if you think it does...

You can achieve "private" state for controllers even with "controller as" syntax:

var MyCtrl = function() {
  var privateMethod = function() {};
  this.publicMethod = function() {};
};

I think this change is mostly for people using the "prototype" style (eg. Google internal projects).

[krotscheck]

You just broke the apps of everyone who is building with CouchDB. The convention there is that document ID's and revisions are prefixed with underscores.

[btford]

@krotscheck this only "breaks" them in the sense that you cannot directly bind to these properties.

You can easily:

1. alias them
2. write a method to return the id given a document

Developers should have to explicitly choose to expose these kinds of properties in their app. This is consistent with the intention of the change

[anfedorov]

MongoDB also has primary keys stored on an _id field, iirc.

[krotscheck]

@btford That may be the case, however javascript already provides the ability to make variables private via scope isolation. Why enforce this convention? Furthermore, why fix something that isn't broken?

angular.module('mymodule').controller('myController', function() {
      var privateVariable = 'this.variable.is.private';

      return {
            getPrivateVariable: function () { return privateVariable; }
      }
});

Note, I'm using "Scope isolation" in the context of "Scope inside of the function/lambda", not "AngularJS scope"

[cburgdorf]

I try to imagine a use case where this actually safes us from anything? I can't see the real benefit.

[RobertLowe]

@krotscheck Agreed. I don't understand why you can just use javascript scopes to hide state like this. If this feature is needed, it shouldn't be done by detecting an underscore. It could be done by declaring that variable is private, perhaps a helper method.

[boneskull]

For a breaking change this is of very small benefit and probably even to a smaller number of users. I like this idea, but as a compromise: let us toggle this behavior.

[krotscheck]

@btford Bindings are not the only ones impacted. More investigation suggests that the introduced bug exists in ngResource, where resource variable interpolation triggers ensureSafeMemberName().

return $resource(myBaseUri + '/resource/:_id', {_id: '@_id'});

[tomjakubowski]

This benefits a prototype-style controller using the "controller as" syntax:

function WidgetCtrl() {
}

WidgetCtrl.prototype._doSomethingPrivate = function () {
};

WidgetCtrl.prototype.doSomethingPublic = function () {
  // ...
};

angular.module('myApp').controller('WidgetCtrl', WidgetCtrl);

<div ng-controller="WidgetCtrl as widget">
  <button ng-click="widget.doSomethingPublic()">do something</button>
</div>

where _-prefixed members on the controller prototype are hidden from the scope.

[btford]

of very small benefit and probably even to a smaller number of users

In a constructive conversation, I'd avoid making these types of assumptions. A feature that is of "small benefit" to you might be important to others. This makes security audits much easier for large applications using controllers with methods defined on their prototype.

I understand that this is annoying issue, and the implications @krotscheck pointed out in ngResource seem undesirable.

let us toggle this behavior

A flag that disables this feature seems reasonable. @chirayuk, what do you think? Would anyone like to prepare a PR?

[cburgdorf]

@btford just out of curiosity, why exactly does it make security audits easier? What exactly makes it more secure that such method can't be accessed from within expressions? Since we all know that true privacy is not possible when using a prototype based programming style in JavaScript we have long lived by the rule to just accept things as private when they were communicated as such.

[cburgdorf]

@btford or by "secure" do you mean "securing" that developers don't abuse the code base? I'm just curious if we mean the same thing with "security" in this context.

[Mparaiso]

I quote :

Hi thanks for the opportunity to discuss that "feature".

JavaScript does not have a notion of private properties

Yet you want to do that ? I understand the point, but convention over configuration is a bad,thing.Want that ? let developpers configure filtered methods through an Angular API. _ for "private" members is not a good practice in any language. it's basically HUNGARIAN NOTATION and not understanding the nature of javascript;

Some mark their private with private prefix, some other use @@ or other symbols so members can only be called when they are quoted ( myobject['@@mymember'] ) , Angular owns the $$ namespace . It will make things confusing if you go that route.

Regards.

[cburgdorf]

@Mparaiso good point with $$ being used internally by Angular to mark private stuff.

[boneskull]

@btford Fair enough. As @vojtajina commented, this is important to devs writing internal apps for Google.

I'll look at writing a PR later tonight if nobody has beaten me to it. I'll hazard a guess the code would be similar to a simple toggle like parseProvider.unwrapPromises() provides.

[tortlieb]

I'd rather see it removed than being a flag, but if it's a flag, please make the default that underscores are allowed, as this was the 1.2.0-rc.3 behavior.

On another note, this should have been added to one of the release candidates instead of being shoved into the actual release, so the full impact of the code could be assessed. Is it too late to call this rc4?

[kzar]

@tortlieb agreed, I know it's not really my place to say but I feel bringing in breaking changes like this between rc3 and the actual release is crazy. Personally I'd like 1.2.0 to be a really solid version that works as expected and that is documented well.

[btford]

The _id issue is obviously a nuisance for databases that have this convention. We're investigating options, but suggestions (aside from outright reverting the change :P ) are welcome.

I'm going to try to address some of the questions below.

why exactly does it make security audits easier?

The short answer is "because Angular now guarantees that foo._private is not available to expression bindings."

It's hard to think of a simple, clear, obvious example off the top of my head, because most exploits are none of these things.

function Ctrl () {}
Ctrl.prototype._deleteUser = function () {};
Ctrl.prototype.promptToDeleteUser = function () {
  // bring up dialog box, do some sanity checks, then _delete
};

<div ng-controller="Ctrl as ctrl">
  <select ng-model="action">
    <option value="promptToDeleteUser"></option>
  </select>
  <button ng-click="ctrl[action]()">
</div>

Consider in AngularJS pre-1.2.0: you'd think that because there is no <option value="_deleteUser">, the ngClick couldn't call it. But an attacker, through some elaborate method, might inject a script that changes <option value="promptToDeleteUser"> to <option value="_deleteUser">.

If you use the prefix, it makes it more obvious to verify that your app is safe: you know that ngClick cannot call _deleteUser, so you know that only promptToDeleteUser needs to perform these sanity checks. This means an auditor can focus their efforts into testing just this one method rather than several. This seems small, but in a large app with hundreds of controllers and thousands of methods, the ability to really focus your security efforts on the places that matter is huge.

Why not just defensively program and do these checks in all methods? There are cases where for performance implications, it would be slow to always do a full set of checks/sanitization. This offers developers a pragmatic choice.

Why not just use closures? This means you get a bound method for each instance of a controller. With prototypes, you only have one. This potentially saves memory on large apps, or for apps with more constraints (like ones targeting mobile). It's also easier to use prototypes with tools like Google Closure Compiler. We think developers should have the choice to use either in a way that is safe.

You can never be completely certain with security, but offering developers this guarantee makes it easier to cull some of the possible attack vectors. This, in turn means that security reviewers can focus on the un-prefixed parts of the app that are exposed to users/attackers.

I don't think that security alone is the only benefit from this change. I've switched to this style for a few apps and I find that it helps communicate intentions better. If you intend for some API to be "private" Angular now binds you to use this. This is consistent with the principle of least surprise; if you refactor some method names that are "private," you know your template does not rely on these APIs.

this was the 1.2.0-rc.3 behavior.

Our policy is that RCs can have breaking changes. Yes we could keep doing RCs until everything "stabilizes," but we found that doing that in the past meant slower releases. Remember that no one is forcing you to update to 1.2.0. You're free to keep using 1.2.0-rc.3 and update at your own pace.

If you want to know about breaking changes earlier, you can subscribe to AngularJS's Github notifications. All major changes like this are discussed publicly via PR, just like this one.

[ryn101]

One simple feature which breaks with this new change is the ng-repeat 'track by' functionality when using MongoDb.

<li ng-repeat="obj in objects track by obj._id"></li>

This is the most common use case for me and unfortunately will not work with the final version without resorting to unnecessary work around's/applying new naming conventions.

[Eduardo-M-Cavalcanti]

Just need to comment out lines 9097 thru 9101 (5 lines) of angular.js 1.2.0.

  if (name.charAt(0) === '_' || name.charAt(name.length-1) === '_') {
    throw $parseMinErr('isecprv',
        'Referencing private fields in Angular expressions is disallowed! Expression: {0}',
        fullExpression);
  }

Or this snippet on the minified version:

if("_"===b.charAt(0)||"_"===b.charAt(b.length-1))throw pa("isecprv",a)

Someone gave the tip here: http://docs.angularjs.org/error/$parse:isecprv
Regards.

[goya]

i shall be forking and making a bower package that does this for you.

[iammerrick]

It just needs to be taken out. I don't understand the rationale for this.

[iammerrick]

Can we start a petition on whitehouse.gov?

[ocombe]

Why the hell would you put something like that from a RC to a release ?!
You just broke everyones apps who use mongoDB, couchDB, elasticSearch, etc...
As you stated it's a convention, but we don't have to follow yours if we don't want.
Make this optionnal or remove it asap.
Thanks.

[petebacondarwin]

See #4926

[kzar]

@petebacondarwin So what version will this be disabled by default from?

[spiderpug]

@kzar Just check the milestone of the PR: 1.2.1

[vojtajina]

Guys, sorry for the troubles this change caused. This "feature" was mostly for people using Closure compiler and Google JS style, but I didn't realize that many people were relying on accessing _* properties in templates. For now we are reverting that change (4ab16aa).

We gonna release 1.2.1 (which will contain this "fix") within next days.

The main outcome of this issue is: we should not put any breaking changes into RC.
Lesson learned.

[mlandalv]

Thanks @vojtajina! 👍

[robfletcher]

I think the problem is not so much with templates where the change is not unreasonable (I'm not sure I still think it's worth it but that's kind of beside the point). The big deal is that use of _id in $resource declarations is extremely common for anyone interacting with APIs using Mongo or Couch, especially "direct" APIs like MongoHQ.

I have some use of _id in templates but nothing that couldn't be refactored into a function in a controller (and probably be better off for it). Changing every $resource declaration is more problematic and confusing to anyone coming along later to understand.

The main outcome of this issue is: we should not put any breaking changes into RC.
Lesson learned.

I don't think a breaking change in an RC is necessarily a problem. A breaking change in the final release that wasn't in any RC is.

[cburgdorf]

@robfletcher actually it's best to completely avoid putting breaking features even between RCs. A release candidate actually means "Hey, folks, we think this should be the final version if we don't find any new unknown bugs. Please drop it into your apps and if we don't find any new bugs, it's going to be the final version"

[guilbep]

@robfletcher got a point 👍 / I'm moving back to rc-3 so I can play with $interval and use _id along // long live mongodb.

[robfletcher]

@cburgdorf I agree but I think the screaming would have been a notch lower in volume if this had been done in an RC4 rather than final.

[cburgdorf]

@robfletcher absolutely :)

[iammerrick]

Thank you! This made dynamic binding impossible (if a user entered something with an _ which was then used to look up something in a map later.)

[boneskull]

@vojtajina This sounds like the right thing to do considering the "shitstorm" of angry users. Thank you for making this decision!

[splurgy-doug]

Thank you! I will wait for 1.2.1. There was just too much overhead to adjust for such a change. I had to change the entire Angular codebase, and use a function to return the private object. I couldn't do that in Angular expressions, so the next thing I would have had to change was all the APIs and traverse through every object just to remove the underscore in "_id".

[goya]

excellent news! awesome.

[gabrieledarrigo]

Thank you very much!

[Valorum]

Thanks for the fix. I see the 1.2.1 release, but bower does not yet have it? Am I too impatient?

[gustavohenke]

@Valorum it should work if the tag is there

[Valorum]

Let me start by apologizing for not understanding the inner workings completely. When I ask bower about angular, this is what I get (after clearing Bower's cache):

$ bower info angular
bower not-cached    git://github.com/angular/bower-angular.git#*
bower resolve       git://github.com/angular/bower-angular.git#*
bower download      https://github.com/angular/bower-angular/archive/v1.2.0.tar.gz
bower extract       angular#* archive.tar.gz
bower resolved      git://github.com/angular/bower-angular.git#1.2.0

{
  name: 'angular',
  version: '1.2.0',
  main: './angular.js',
  dependencies: {},
  homepage: 'https://github.com/angular/bower-angular'
}

Available versions:
  - 1.2.0
  - 1.2.0-rc.3
  - 1.2.0-rc.2
  - 1.2.0-rc.1
  - 1.0.8
  - 1.0.7
  - 1.0.6
  - 1.0.5
  - 1.0.4
  - 1.0.3

You can request info for a specific version with 'bower info angular#<version>'

https://github.com/angular/bower-angular/releases does not list 1.2.1. I do not know how this normally finds its way into Bower. I assume somebody needs to do something for this to happen?

[laurelnaiad]

1.2.1 hasn't been deployed yet, AFAIK.

[dagumak]

I am not very clear if 1.2.1 is out yet. I see the version reflected in the documentation, but I don't see it available for download. The inconsistency is very confusing.

[petebacondarwin]

It's getting there. Patience is a virtue :-) Wait for the blog posting.

On 15 November 2013 19:23, Douglas Mak [email protected] wrote:

I am not very clear if 1.2.1 is out yet. I see the version reflected in
the documentation, but I don't see it available for download. The
inconsistency is very confusing.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4509#issuecomment-28596155
.

[lucsky]

@vojtajina Thank you Vojta, much appreciated.

[mgcrea]

Thanks for listening to the community. I'm using MongoDB for several apps and this was a pain.

Theses multiple RC's had quite shaken my confidence as I've wasted a ton of time fighting broken changes between theses releases (ngAnimate, $parse). In my opinion, that should never ever happen during RC stage, especially if you use an unstable branch: RC means ready for production to me, like it's time to try to port my apps to it. If it's not ready, it should be called an alpha/beta.

I'm looking forward to finally work with this new branch ;-). Kudos for the hard work.