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

dozingcat · 2014-01-01T18:24:51Z

...d its tests from failing

cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This
wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add
test that verifies evaluation of nested properties of multiple lengths.

Fixes #5591

…nted its tests from failing cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add test that verifies evaluation of nested properties of multiple lengths. Fixes <a href="https://github.com/angular/angular.js/issues/5591">#5591</a>

caitp · 2014-01-01T18:34:35Z

src/ng/parse.js

@@ -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;


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

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.

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

…nted its tests from failing cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add test that verifies evaluation of nested properties of multiple lengths. Fixes <a href="https://github.com/angular/angular.js/issues/5591">#5591</a>

dozingcat · 2014-01-01T19:36:37Z

Moved key checks to existing if statements as suggested.

…nted its tests from failing cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add test that verifies evaluation of nested properties of multiple lengths. Fixes <a href="https://github.com/angular/angular.js/issues/5591">#5591</a>

caitp · 2014-01-02T02:42:10Z

src/ng/parse.js

@@ -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;


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

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.

…nted its tests from failing cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add test that verifies evaluation of nested properties of multiple lengths. Fixes <a href="https://github.com/angular/angular.js/issues/5591">#5591</a>

caitp · 2014-01-02T04:46:59Z

src/ng/parse.js

@@ -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;


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?

…nted its tests from failing cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add test that verifies evaluation of nested properties of multiple lengths. Fixes <a href="https://github.com/angular/angular.js/issues/5591">#5591</a>

dozingcat · 2014-01-02T05:50:01Z

How about this? Setting document.securityPolicy.isActive makes the $sniffer service see that CSP is enabled at injection time (via the csp() function in Angular.js). It's a bit ugly to mess with a global object like that, but maybe in a test it's ok. The "proper" solution might be to have a CSP detection service which could be mocked in tests, but that seems like overkill.

Incidentally the promise logging tests were never enabling CSP mode; that's now fixed.

caitp · 2014-01-02T05:53:41Z

Messing with the global variable seems a bit worrisome --- But let's see what other folks say about it... Probably won't hear much this week, but next week is a possibility

…nted its tests from failing cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add test that verifies evaluation of nested properties of multiple lengths. Closes angular#5591 Closes angular#5592

IgorMinar · 2014-01-03T22:06:58Z

I reviewed it and this is a really nice PR. Thanks for all the work @dozingcat

I squashed the changes, added an afterEach to restore the global state and I'm testing this all on the CI right now.

I'll merge it and sync it into google today

…nted its tests from failing cspSafeGetterFn incorrectly returned undefined if any of its key parameters were undefined. This wasn't caught by the $parse unit tests because of a timing problem where $ParseProvider was reading the CSP flag before the tests manually set it, so the CSP property evaluation tests never ran. Add test that verifies evaluation of nested properties of multiple lengths. Closes angular#5591 Closes angular#5592

caitp reviewed Jan 1, 2014
View reviewed changes

caitp reviewed Jan 2, 2014
View reviewed changes

dozingcat added 2 commits January 1, 2014 23:09

caitp reviewed Jan 2, 2014
View reviewed changes

ghost assigned IgorMinar Jan 2, 2014

dozingcat added 2 commits January 2, 2014 00:37

tests

c4fd978

IgorMinar closed this in 3b1a4fe Jan 3, 2014

campersau mentioned this pull request Jan 10, 2014

multiple nested bind template not working on Chrome App #5727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dozingcat commented Jan 1, 2014

caitp Jan 1, 2014

dozingcat Jan 1, 2014

caitp Jan 1, 2014

dozingcat commented Jan 1, 2014

caitp Jan 2, 2014

dozingcat Jan 2, 2014

caitp Jan 2, 2014

dozingcat commented Jan 2, 2014

caitp commented Jan 2, 2014

IgorMinar commented Jan 3, 2014

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

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

Conversation

dozingcat commented Jan 1, 2014

caitp Jan 1, 2014

Choose a reason for hiding this comment

dozingcat Jan 1, 2014

Choose a reason for hiding this comment

caitp Jan 1, 2014

Choose a reason for hiding this comment

dozingcat commented Jan 1, 2014

caitp Jan 2, 2014

Choose a reason for hiding this comment

dozingcat Jan 2, 2014

Choose a reason for hiding this comment

caitp Jan 2, 2014

Choose a reason for hiding this comment

dozingcat commented Jan 2, 2014

caitp commented Jan 2, 2014

IgorMinar commented Jan 3, 2014