fix($parse): add .assign() function to settable expressions in CSP mode
#9051
Conversation
| @@ -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) { | |||
There was a problem hiding this comment.
not strictly needed for the fix, but if we want to get rid of anonymous functions..
caitp
changed the title
.assign() function to settable expressions in CSP mode
| beforeEach(module(provideLog)); | ||
| beforeEach(module(function($provide) { | ||
| $provide.decorator('$sniffer', function($delegate) { | ||
| $delegate.csp = function() { return cspEnabled; }; |
There was a problem hiding this comment.
This could just be
$delegate.csp = cspEnabled;No?
caitp
force-pushed
the
issue-9048
branch
2 times, most recently
from
192a0b8 to
64112e1
Compare
|
Without actually running this code it LGTM |
|
the unit tests (in the second commit) pass on travis (there are 11 unit test failures in the first commit due to the regression) --- so, I think it's good to land, might wait for @IgorMinar or @tbosch to say it looks good to them though |
|
@IgorMinar could you give this a quick review? it should be good to check in (2nd commit message just needs some cleanup) |
Previously, the test suite was not actually taking CSP-mode paths when we were expecting it to. Numerous CSP-mode tests are failing, working on fixing these.
Fixes regression where the `assign()` method was not added to chains of identifiers in CSP mode, introduced originally in b3b476d. Closes angular#9048
|
If it's possible to write an e2e csp mode test, I'm all for it --- but I think it would depend on having a backend set the CSP headers when serving the test fixture, and this seems flakey as I'm imagining it in my head. It's probably worth adding some way of real-world testing CSP mode, but I don't think it needs to live in this CL |
|
You are right. Ok, here is a separate issue: #9059. I will give it a shot, lets see... |
|
Ok, thanks! Does this look good to you? It would be good to check in today |
|
@caitp I would squash this into 1 commit, as only the tests show that the change is really needed. |
|
Otherwise LGTM. |
|
Alright --- will merge |
|
I think we want the first commit from this CL in v1.2.x too --- the second one probably isn't needed |
|
Why not? |
|
The regression was not checked into v1.2.x as far as I know --- however I'm having trouble getting the test suite to run on 1.2 currently, so it's hard to verify --- it looks like this was not the only part of the test suite which uses csp mode, so there will need to be another change to make sure the right code paths are taken for the other ones... |
|
Landed in d13b4bd |
|
Oops, I should have noticed that one... |
|
Reviewers didn't notice it, so it's cool =) it was hard to see since the tests weren't failing |
Due to changes to CSP-mode detection and the CSP api in general, the test suite was not able to
report regressions related to CSP.
This CL corrects the regression introduced where assignable expressions would not have an
assignmember function in CSP mode.Closes #9048