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

fix($parse): add .assign() function to settable expressions in CSP mode #9051

Closed
wants to merge 2 commits into from
Closed

fix($parse): add .assign() function to settable expressions in CSP mode #9051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d04f316
test($parse): ensure CSP code paths are used when testing
caitp Sep 12, 2014
f2f390d
fix($parse): ensure CSP assignable expressions have `assign()`
caitp Sep 12, 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
8 changes: 4 additions & 4 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,7 @@ function getterFn(path, options, fullExp) {
if (pathKeysLength < 6) {
fn = cspSafeGetterFn(pathKeys[0], pathKeys[1], pathKeys[2], pathKeys[3], pathKeys[4], fullExp);
} else {
fn = function(scope, locals) {
fn = function cspSafeGetter(scope, locals) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not strictly needed for the fix, but if we want to get rid of anonymous functions..

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

var i = 0, val;
do {
val = cspSafeGetterFn(pathKeys[i++], pathKeys[i++], pathKeys[i++], pathKeys[i++],
Expand Down Expand Up @@ -926,14 +926,14 @@ function getterFn(path, options, fullExp) {
var evaledFnGetter = new Function('s', 'l', code); // s=scope, l=locals
/* jshint +W054 */
evaledFnGetter.toString = valueFn(code);
evaledFnGetter.assign = function(self, value) {
return setter(self, path, value, 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.

factored out of this part of the code due to being shared by both versions

Copy link
Contributor

Choose a reason for hiding this comment

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

You have only deleted this from one place. Where is the other place it is defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line 934

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that you say your are factoring it out as it is shared. Are you saying that it should have been shared but wasn't (i.e. there was a bug) and rather than doubling up the code you have factored it out?

At the moment there is only one removal and one addition. I expected to see two removals and one addition. Am I making sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code was in one branch and not the other, thus the bug --- adding it to both branches doesn't make sense since it's the same code, thus it is factored out

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. That's what I thought

fn = evaledFnGetter;
}

fn.sharedGetter = true;
fn.assign = function(self, value) {
return setter(self, path, value, path);
};
getterFnCache[path] = fn;
return fn;
}
Expand Down
19 changes: 6 additions & 13 deletions test/ng/parseSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,19 +210,12 @@ describe('parser', function() {
forEach([true, false], function(cspEnabled) {
describe('csp: ' + cspEnabled, function() {

var originalSecurityPolicy;


beforeEach(function() {
originalSecurityPolicy = window.document.securityPolicy;
window.document.securityPolicy = {isActive : cspEnabled};
});

afterEach(function() {
window.document.securityPolicy = originalSecurityPolicy;
});

beforeEach(module(provideLog));
beforeEach(module(function($provide) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix the test suite to use the CSP path when testing CSP mode

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you then remove the old patching code that enabled CSP above window.document.securityPolicy = ...

$provide.decorator('$sniffer', function($delegate) {
$delegate.csp = cspEnabled;
return $delegate;
});
}, provideLog));

beforeEach(inject(function ($rootScope) {
scope = $rootScope;
Expand Down