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

fix($parse): return 'undefined' if a middle key's value is null #5480

Closed
wants to merge 1 commit into from
Closed

fix($parse): return 'undefined' if a middle key's value is null #5480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,16 +894,16 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
if (pathVal == null) return pathVal;
pathVal = pathVal[key0];

if (!key1 || pathVal == null) return pathVal;
if (pathVal == null) return key1 ? undefined : pathVal;
pathVal = pathVal[key1];

if (!key2 || pathVal == null) return pathVal;
if (pathVal == null) return key2 ? undefined : pathVal;
pathVal = pathVal[key2];

if (!key3 || pathVal == null) return pathVal;
if (pathVal == null) return key3 ? undefined : pathVal;
pathVal = pathVal[key3];

if (!key4 || pathVal == null) return pathVal;
if (pathVal == null) return key4 ? undefined : pathVal;
pathVal = pathVal[key4];

return pathVal;
Expand All @@ -924,7 +924,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
}
pathVal = pathVal.$$v;
}
if (!key1 || pathVal == null) return pathVal;
if (pathVal == null) return key1 ? undefined : pathVal;

pathVal = pathVal[key1];
if (pathVal && pathVal.then) {
Expand All @@ -936,7 +936,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
}
pathVal = pathVal.$$v;
}
if (!key2 || pathVal == null) return pathVal;
if (pathVal == null) return key2 ? undefined : pathVal;

pathVal = pathVal[key2];
if (pathVal && pathVal.then) {
Expand All @@ -948,7 +948,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
}
pathVal = pathVal.$$v;
}
if (!key3 || pathVal == null) return pathVal;
if (pathVal == null) return key3 ? undefined : pathVal;

pathVal = pathVal[key3];
if (pathVal && pathVal.then) {
Expand All @@ -960,7 +960,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
}
pathVal = pathVal.$$v;
}
if (!key4 || pathVal == null) return pathVal;
if (pathVal == null) return key4 ? undefined : pathVal;

pathVal = pathVal[key4];
if (pathVal && pathVal.then) {
Expand All @@ -980,7 +980,7 @@ function simpleGetterFn1(key0, fullExp) {
ensureSafeMemberName(key0, fullExp);

return function simpleGetterFn1(scope, locals) {
if (scope == null) return scope;
if (scope == null) return undefined;
return ((locals && locals.hasOwnProperty(key0)) ? locals : scope)[key0];
};
}
Expand All @@ -990,10 +990,9 @@ function simpleGetterFn2(key0, key1, fullExp) {
ensureSafeMemberName(key1, fullExp);

return function simpleGetterFn2(scope, locals) {
if (scope == null) return scope;
if (scope == null) return undefined;
scope = ((locals && locals.hasOwnProperty(key0)) ? locals : scope)[key0];

return scope == null ? scope : scope[key1];
return scope == null ? undefined : scope[key1];
Copy link
Contributor

Choose a reason for hiding this comment

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

this is correct. scope variable is at this moment being reused for scope[key0], which if "null-thy" should result in undefined being returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure I understand, this is okay because scope[key0] is a middle segment, so this change doesn't need to be reverted

};
}

Expand Down Expand Up @@ -1036,7 +1035,7 @@ function getterFn(path, options, fullExp) {
var code = 'var p;\n';
forEach(pathKeys, function(key, index) {
ensureSafeMemberName(key, fullExp);
code += 'if(s == null) return s;\n' +
code += 'if(s == null) return undefined;\n' +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I should check if this is the last index or not

Copy link
Contributor

Choose a reason for hiding this comment

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

right. we want to return undefined only if we are not at the last segment of the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it turned out not to be necessary, for whichever reason (istanbul would probably explain why)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's perfect like that.
The return of the last property is actually outside that forEach, on it's own
So return undefined makes perfect sense, since all these += 'ifs' target properties that are in the middle

The only real consequence of this is that when csp is off, if the scope passed to $parse is null, it will return undefined

//csp off
$parse('a.b.c')(null) === undefined

//csp on (and off before this change)
$parse('a.b.c')(null) === null

So really, if we care about that, the code should be

code += 'if(s == null) return ' + (!index ? 's' : 'undefined') + ';\n' +

making us avoid breaking changes (if you're curious about the code when csp is on, there you go)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the answer --- but given the unrolled test cases, I think each of these situations should be covered (well. not quite actually, but it would be an extra 6 lines to fix)

... Yeah, so even if the s == null && !index, it returns the expected result, so I think it's probably okay without a change. not worth fixing until it's known to be broken right? edit after re-reading, I see what you're saying... hmm. I don't know if it's really worth it, but I'll wait to hear back from someone about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine

's='+ (index
// we simply dereference 's' on any .dot notation
? 's'
Expand Down
49 changes: 49 additions & 0 deletions test/ng/parseSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,55 @@ describe('parser', function() {
expect($parse('"name" + id').constant).toBe(false);
}));
});

describe('nulls in expressions', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay unrolled these, there's a tiny bit less coverage but it's not that bad maybe.

Copy link
Contributor

Choose a reason for hiding this comment

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

much better!

// simpleGetterFn1
it('should return null for `a` where `a` is null', inject(function($rootScope) {
$rootScope.a = null;
expect($rootScope.$eval('a')).toBe(null);
}));

it('should return undefined for `a` where `a` is undefined', inject(function($rootScope) {
expect($rootScope.$eval('a')).toBeUndefined();
}));

// simpleGetterFn2
it('should return undefined for properties of `null` constant', inject(function($rootScope) {
expect($rootScope.$eval('null.a')).toBeUndefined();
}));

it('should return undefined for properties of `null` values', inject(function($rootScope) {
$rootScope.a = null;
expect($rootScope.$eval('a.b')).toBeUndefined();
}));

it('should return null for `a.b` where `b` is null', inject(function($rootScope) {
$rootScope.a = { b: null };
expect($rootScope.$eval('a.b')).toBe(null);
}));

// cspSafeGetter && pathKeys.length < 6 || pathKeys.length > 2
it('should return null for `a.b.c.d.e` where `e` is null', inject(function($rootScope) {
$rootScope.a = { b: { c: { d: { e: null } } } };
expect($rootScope.$eval('a.b.c.d.e')).toBe(null);
}));

it('should return undefined for `a.b.c.d.e` where `d` is null', inject(function($rootScope) {
$rootScope.a = { b: { c: { d: null } } };
expect($rootScope.$eval('a.b.c.d.e')).toBeUndefined();
}));

// cspSafeGetter || pathKeys.length > 6
it('should return null for `a.b.c.d.e.f.g` where `g` is null', inject(function($rootScope) {
$rootScope.a = { b: { c: { d: { e: { f: { g: null } } } } } };
expect($rootScope.$eval('a.b.c.d.e.f.g')).toBe(null);
}));

it('should return undefined for `a.b.c.d.e.f.g` where `f` is null', inject(function($rootScope) {
$rootScope.a = { b: { c: { d: { e: { f: null } } } } };
expect($rootScope.$eval('a.b.c.d.e.f.g')).toBeUndefined();
}));
});
});
});
});
Expand Down