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 3 commits
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
21 changes: 11 additions & 10 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 (pathVal == null) return key1 ? undefined : pathVal;
if (pathVal == null || !key1) 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.

Hmm, on the other hand.. Maybe something more like

if (!key1) return pathVal;
else if (pathVal == null) return undefined;

might be cleaner / easier to read, and possibly save a comparison. Although it still technically violates the style guide.

I contradict myself a lot, you'll get used to it ;) I just want to avoid duplicating code, and try to keep it readable and straight forward

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 like that better too, the conditions are basically independent. If I drop the "else" I don't think it violates the style guide, since it references Google's JS style guide, which "follow[s] the C++ formatting rules in spirit", which says "Short conditional statements may be written on one line if this enhances readability." (Yay indirection). And there are several other occurrences of "if (...) return ..." in the Angular source.

Of course the most readable and concise implementation would be to iterate over the array of path components directly, but playing with jsperf showed about a 35% performance hit, which is presumably why it's unrolled into keyN parameters.

pathVal = pathVal[key1];

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

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

if (pathVal == null) return key4 ? undefined : pathVal;
if (pathVal == null || !key4) 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 (pathVal == null) return key1 ? undefined : pathVal;
if (pathVal == null || !key1) 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 (pathVal == null) return key2 ? undefined : pathVal;
if (pathVal == null || !key2) 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 (pathVal == null) return key3 ? undefined : pathVal;
if (pathVal == null || !key3) 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 (pathVal == null) return key4 ? undefined : pathVal;
if (pathVal == null || !key4) return key4 ? undefined : pathVal;

pathVal = pathVal[key4];
if (pathVal && pathVal.then) {
Expand Down Expand Up @@ -1218,8 +1218,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 +1235,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