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

fix($parse): fix CSP nested property evaluation, and issue that prevente... #5592

Closed
wants to merge 7 commits into from
Closed

fix($parse): fix CSP nested property evaluation, and issue that prevente... #5592

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
339fc96
fix($parse): fix CSP nested property evaluation, and issue that preve…
dozingcat Jan 1, 2014
5177a62
fix($parse): fix CSP nested property evaluation, and issue that preve…
dozingcat Jan 1, 2014
e1d7a5f
fix($parse): fix CSP nested property evaluation, and issue that preve…
dozingcat Jan 2, 2014
3a126a9
fix($parse): fix CSP nested property evaluation, and issue that preve…
dozingcat Jan 2, 2014
6f991b7
fix($parse): fix CSP nested property evaluation, and issue that preve…
dozingcat Jan 2, 2014
c4fd978
tests
dozingcat Jan 2, 2014
ba7fce5
fix($parse): fix CSP nested property evaluation, and issue that preve…
dozingcat Jan 2, 2014
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
13 changes: 11 additions & 2 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,15 +894,19 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
if (pathVal == null) return pathVal;
pathVal = pathVal[key0];

if (!key1) return pathVal;
if (pathVal == null) return key1 ? undefined : pathVal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at this line (897)... Now look at the one below it...

What we're doing here is this: if the path key is not present, and there is another path key after it, we always want to return undefined.

If there are problems with the existing algorithm, we'll need to fix it, but to undo that would be essentially reverting a desirable fix. Lets find a better solution for the csp issue.

I'm fairly sure that this changeset would have broken tests if the test suite for the changeset fixing #5442 / #2249 didn't get shrunk down, with a few test cases removed

Even if it doesn't break tests, we can make this more concise and not breaking the logic mentioned above by combining it into the second if statement

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 think that's a separate issue. As I understand it given scope.a={b: null}, $eval('a.b') should return null and $eval('a.b.c') should return undefined. This changeset doesn't modify that behavior. It addresses the case where cspSafeGetterFn is called with an expression of 'a.b.c', so the key3 and key4 parameters are undefined. pathVal then becomes undefined after evaluating pathVal[key3], unless we check for the key being undefined first.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the very least, it does duplicate functionality, and this could be made much more concise.

An 8 byte change || !key1 would have the same impact, and absolutely avoid breaking logic

Good catch on the csp test fixes, though

pathVal = pathVal[key1];

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

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

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

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

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

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

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

pathVal = pathVal[key4];
Expand Down Expand Up @@ -1218,8 +1226,6 @@ function $ParseProvider() {


this.$get = ['$filter', '$sniffer', '$log', function($filter, $sniffer, $log) {
$parseOptions.csp = $sniffer.csp;

promiseWarning = function promiseWarningFn(fullExp) {
if (!$parseOptions.logPromiseWarnings || promiseWarningCache.hasOwnProperty(fullExp)) return;
promiseWarningCache[fullExp] = true;
Expand All @@ -1237,6 +1243,9 @@ function $ParseProvider() {
return cache[exp];
}

// The csp option has to be set here because in tests the $sniffer service sets its csp
// property after $get has executed.
$parseOptions.csp = $sniffer.csp;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not sure about this though, it seems weird to read this every time $parse is called. If it's the best we can do then that's one thing, but is there not a better solution?

var lexer = new Lexer($parseOptions);
var parser = new Parser(lexer, $filter, $parseOptions);
parsedExpression = parser.parse(exp, false);
Expand Down
19 changes: 19 additions & 0 deletions test/ng/parseSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,25 @@ describe('parser', function() {
expect(scope.$eval("a.b.c.d.e.f.g.h.i.j.k.l.m.n", scope)).toBe('nooo!');
});

forEach([2, 3, 4, 5, 6, 7, 8, 9, 10, 20, 42, 99], function(pathLength) {
it('should resolve nested paths of length ' + pathLength, function() {
// Create a nested object {x2: {x3: {x4: ... {x[n]: 42} ... }}}.
var obj = 42;
for (var i = pathLength; i >= 2; i--) {
var newObj = {}
newObj['x' + i] = obj;
obj = newObj;
}
// Assign to x1 and build path 'x1.x2.x3. ... .x[n]' to access the final value.
scope.x1 = obj;
var path = 'x1';
for (var i = 2; i <= pathLength; i++) {
path += '.x' + i;
}
expect(scope.$eval(path)).toBe(42);
});
});

it('should be forgiving', function() {
scope.a = {b: 23};
expect(scope.$eval('b')).toBeUndefined();
Expand Down