fix($parse): treat falsy values as defined in assignment expressions

[m-amr]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bug, solving Issue #14990

Closes #14990

[lgalfaso]

Please rename to isNull

[lgalfaso]

Looks like variations of the same issue are present at
https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L1069
and
https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L1080

with the expressions foo.bar.baz = 1 and foo.bar['baz'] = 1 where in both cases foo = {bar: ''}

It would be best to fix the three cases in one go.

[m-amr]

@lgalfaso
Thanks for your feedback

I have a problem with the unit test.

 it('should not assign a property to locals properties with falsy value', inject(function($parse) {
      var local = {a:0};
      expect(function() {
        $parse("a.b =1")(local);
      }).toThrow();
  });

I get this error 'Expected function to throw an exception'
but when i call $parse("a.b =1")(local) alone i get
TypeError: Cannot create property 'b' on number '0'

I have no idea why toThrow() doesn't detect that TypeError was thrown from parse function ?

[lgalfaso]

There are at least 2 parts:

The CSP: true version also needs to be fixed, these are
https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L1706
https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L1731
https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L1750

There might be discrepancies on how this is handled in different browsers.

[m-amr]

@lgalfaso
After the fix that i had done.
Here are the results of the three cases
Case 1:

    $scope.a = 0;
    $scope.$eval("a.b=2");

I get TypeError: Cannot create property 'b' on number '0'

Case 2:

    $scope.a = 0;
    $scope.$eval("a['b']=2");

I get TypeError: Cannot create property 'b' on number '0'

Case 3:

    $scope.a = 0;
    $scope.$eval("a={'b':2}");

This shouldn't throw any error because assigning 'a' with another value.

Is that what you mean ?

[lgalfaso]

For some reasons that are not important, the following 3 expressions follow 3 slightly different code paths

foo.bar = 1;
foo.bar.baz = 1;
foo.bar['baz'] = 1;

In this case we want to fix the three of them as they all have the same issue. This is, the following cases should behave in the same way

$parse('foo.bar = 1')({foo: 0});
$parse('foo.bar.baz = 1')({foo: {bar: 0}});
$parse('foo.bar["baz"] = 1')({foo: {bar: 0}});

[m-amr]

After I handled these two cases

  $parse('foo.bar.baz = 1')({foo: {bar: 0}});
  $parse('foo.bar["baz"] = 1')({foo: {bar: 0}});

Some old cases failed

     it('should prevent assigning only in the context of an actual constructor', function() {
      // foo.constructor is not a constructor.
      expect(function() {
        delete scope.foo;
        scope.$eval('foo.constructor[0] = ""', {foo: {constructor: ''}});
      }).not.toThrow();

and

   it('should prevent assigning only in the context of an actual prototype', function() {
      // foo.constructor.prototype is not a constructor prototype.
      expect(function() {
        delete scope.foo;
        scope.$eval('foo.constructor.prototype[0] = ""', {foo: {constructor: {prototype: ''}}});
      }).not.toThrow();

So these Cases are against the the change to handle

    $parse('foo.bar.baz = 1')({foo: {bar: 0}});
    $parse('foo.bar["baz"] = 1')({foo: {bar: 0}});

so what is your suggestion ?

[gkalpak]

The description is misleading. There is no locals object in the test. Use scope instead.

[gkalpak]

The tests you mentioned were affected by the bug you fixed. You could change the initial values to something that can be assigned to, e.g.:

// Before:
{foo: {constructor: ''}}
{foo: {constructor: {prototype: ''}}}

// After:
{foo: {constructor: {}}}
{foo: {constructor: {prototype: {}}}}

[m-amr]

@gkalpak
Could you please check this.
I have no idea why at the Unit test the toThrow function didn't detect any exception throw from parse function
when I write

      var scope = {a:0};
      $parse("a.b =1")(scope);

I get this error.
TypeError: Cannot create property 'b' on number '0'
at fn (eval at compile (/Apache Server Root/repositories/angular.js/src/ng/parse.js:906:15), :4:248)
at Object. (/Apache Server Root/repositories/angular.js/test/ng/parseSpec.js:3916:27)
at Object.invoke (/Apache Server Root/repositories/angular.js/src/auto/injector.js:879:19)
at Object.workFn (/Apache Server Root/repositories/angular.js/src/ngMock/angular-mocks.js:3103:20)
Error: Declaration Location
at window.inject.angular.mock.inject (/Apache Server Root/repositories/angular.js/src/ngMock/angular-mocks.js:3065:25)
at Suite. (/Apache Server Root/repositories/angular.js/test/ng/parseSpec.js:3914:81)
at Suite. (/Apache Server Root/repositories/angular.js/test/ng/parseSpec.js:3867:7)
at /Apache Server Root/repositories/angular.js/test/ng/parseSpec.js:1838:5
Chrome 52.0.2743 (Mac OS X 10.11.5): Executed 5535 of 5535 (2 FAILED) (22.486 secs / 21.484 secs)

but when i use

      var scope = {a:0};
      expect(function() {
        $parse("a.b =1")(scope);
      }).toThrow();

I get
Chrome 52.0.2743 (Mac OS X 10.11.5) parser csp: true locals should not assign a property to scope properties with falsy value FAILED
Expected function to throw an exception.
at Object. (/Apache Server Root/repositories/angular.js/test/ng/parseSpec.js:3919:14)
at Object.invoke (/Apache Server Root/repositories/angular.js/src/auto/injector.js:879:19)
at Object.workFn (/Apache Server Root/repositories/angular.js/src/ngMock/angular-mocks.js:3103:20)

I have no Idea why the test didn't detect the thrown exception, but when i used
$parse("a.b =1")(scope); it throw exception.

Does angular handle these exception in a different way ? or I am missing something.

[gkalpak]

@m-amr, I see you pushed another commit. Were you able to solve the issue you had or should I take a look?

[m-amr]

@gkalpak, No I still have the same problem, Could you please check it.

[m-amr]

@gkalpak
Thanks for your feedback,
I had added all the test cases that you had mentioned and every thing work,
Could you please give it a last look ?

[gkalpak]

Nit: Use /* ... */ (and consistent whitespace).

[m-amr]

Done.

[gkalpak]

LGTM
I'll let @lgalfaso sign it off 😃

[lgalfaso]

LGTM
landed as 4f44e01