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

fix($parse): correctly escape unsafe identifier characters #14942

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Jul 22, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.

What is the current behavior? (You can also link to an open issue here)
While it is possible to use arbitrary identifier characters in Angular expressions, the RegExp that detect unsafe identifiers (in order to escape them in the generated JS code), was failing to detect them in some cases.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

Other information:
This PR also includes some minor clean-up changes and fixes the setLiteral() tests to run in CSP mode too.

@gkalpak
Copy link
Member Author

gkalpak commented Jul 22, 2016

/cc @lgalfaso

@lgalfaso
Copy link
Contributor

LGTM

@@ -1260,7 +1261,7 @@ ASTCompiler.prototype = {
},

nonComputedMember: function(left, right) {
var SAFE_IDENTIFIER = /[$_a-zA-Z][$_a-zA-Z0-9]*/;
var SAFE_IDENTIFIER = /^[$_a-zA-Z][$_a-zA-Z0-9]$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need to keep the *? Otherwise almost everything will use the ['...'] format...

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I must have accidentally removed it 😇

@gkalpak gkalpak force-pushed the fix-parse-escape-unsafe-identifiers branch from d8658e9 to 4754b21 Compare July 25, 2016 16:46
@gkalpak gkalpak closed this in 8ddfa2a Jul 26, 2016
gkalpak added a commit that referenced this pull request Jul 26, 2016
This commit also adds a couple of tests for `$parseProvider.setIdentifierFns()`.

Closes #14942
@gkalpak gkalpak deleted the fix-parse-escape-unsafe-identifiers branch July 26, 2016 07:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants