fix($compile): sanitize special chars in directive name #16314

curlydevil · 2017-11-01T13:31:37Z

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

What is the current behavior? (You can also link to an open issue here)
In HTML markup only directive name's x- and data- prefixes are being sanitized. (#16278)

What is the new behavior (if this is a feature change)?
Also special chars are being sanitized, e.g. <div _t></div> will match t directive once again

Does this PR introduce a breaking change?
Should not

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:
@petebacondarwin suggested to update the regex, and I hope that I did it in a way he was thinking about it.

googlebot · 2017-11-01T13:31:39Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-11-01T13:31:39Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-11-01T13:36:15Z

CLAs look good, thanks!

googlebot · 2017-11-01T13:36:15Z

CLAs look good, thanks!

Narretz · 2017-11-01T15:32:41Z

There some test failures with jquery: https://travis-ci.org/angular/angular.js/jobs/295785053

curlydevil · 2017-11-01T15:50:14Z

@Narretz yes, I'm trying to understand why I have them passing locally at 4 browsers but not in CI...

Narretz · 2017-11-01T16:19:48Z

Are you sure you are running tests:jquery?

curlydevil · 2017-11-01T16:43:20Z

I was running grunt autotest to check in several browsers, and whole grunt test before push...
I know what fails but I dunno why it fails in CI... the failing part is <_t></_t> test case... without it everything else passes in CI.

petebacondarwin · 2017-11-01T18:35:07Z

I wonder if it is trying to convert _t to T when it should be t or vice versa?

curlydevil · 2017-11-02T07:11:03Z

Checked how <_t></_t> behaves with 1.5.x - looks like it did not work this way, so I've dropped corresponding test, just leaving it as an A restriction check.

curlydevil · 2017-11-02T08:47:23Z

Doh... Error: Error updating Sauce pass/fail status: 'Could not send request: connect ETIMEDOUT 162.222.75.227:443'
Is there a possibility to restart single stage or the branch build?

this fixes bug introduced in 1.6.x when directive name with preceeding special char in HTML markup does not match the registered name

Narretz · 2017-11-02T09:58:32Z

Checked how <_t></_t> behaves with 1.5.x - looks like it did not work this way, so I've dropped corresponding test, just leaving it as an A restriction check.

Aren't we creating an inconsistent and undocumented behavior? Because afaik there's nothing about the handling of special characters at the start of a directive name in the docs.
Imo this looks like it was never explicitly supported, and if we support it it should be consistent and documented.

The docs:

The normalization process is as follows:
Strip x- and data- from the front of the element/attributes.
Convert the :, -, or _-delimited name to camelCase.

It doesn't say that :, -, or _ are stripped from the front.

gkalpak · 2017-11-02T17:59:31Z

src/ng/compile.js

@@ -3637,7 +3637,7 @@ function SimpleChange(previous, current) {
 SimpleChange.prototype.isFirstChange = function() { return this.previousValue === _UNINITIALIZED_VALUE; };


-var PREFIX_REGEXP = /^((?:x|data)[:\-_])/i;
+var PREFIX_REGEXP = /^((?:x|data)*[:\-_])/i;


The * is wrong. Use ? instead.

Also, PREFIX_REGEXP is used to process ng-attr- attributes (here) and this change affects that too.
IMO, this PR should restore the behavior of directiveNormalize() pre- 73050cd and not affect anything else.

curlydevil · 2017-11-06T08:40:08Z

@gkalpak looking through usage of new function, its implementation is actually correct for CSS, so changing it back might break the desired behaviour there... should I add yet another camelcasing method?

gkalpak · 2017-11-06T10:53:34Z

src/ng/compile.js

+    .replace(SPECIAL_CHARS_REGEXP, function(_, separator, letter, offset) {
+      return offset ? letter.toUpperCase() : letter;
+    })
+    .replace(MOZ_HACK_REGEXP, 'Moz$1');


AFAICT, this was only necessary for CSS, so we don't need it here any more.

gkalpak · 2017-11-06T10:54:42Z

src/ng/compile.js

+ * Also there is special case for Moz prefix starting with upper case letter.
+ * @param name Name to normalize
+ */
+function camelCase(name) {


Since this function is only used in directiveNormalize() it makes sense to inline the code there.

gkalpak · 2017-11-06T10:56:40Z

should I add yet another camelcasing method?

More or less. I.e. you just need to update directiveNormalize() to have the desired behavior (i.e. the bahavior it had for normalizing directive names before 73050cd.

googlebot · 2017-11-08T18:07:49Z

CLAs look good, thanks!

googlebot · 2017-11-08T18:07:49Z

CLAs look good, thanks!

Narretz · 2017-11-09T11:11:41Z

test/ng/compileSpec.js

@@ -294,6 +294,26 @@ describe('$compile', function() {
      inject(function($compile) {});
    });

+    it('should omit special chars before processing directive name', function() {


If this applies only to directives with restrict: 'A', then the test description should reflect that. Maybe even link to this issue in a comment.

curlydevil · 2017-11-13T15:12:57Z

@Narretz @gkalpak @petebacondarwin any other ideas for improvement?

gkalpak · 2017-11-13T15:24:08Z

test/ng/compileSpec.js

+        });
+      });
+      inject(function($compile, $rootScope, log) {
+        element = $compile('<div _t></div>')($rootScope);


The element = parts are redundant.

gkalpak · 2017-11-13T15:26:00Z

test/ng/compileSpec.js

@@ -294,6 +294,27 @@ describe('$compile', function() {
      inject(function($compile) {});
    });

+    it('should omit special chars before processing attribute directive name', function() {


I would change "omit" to "ignore", because I think it bettr describes the behavior.

This fixes regression bug when directive name with preceeding special char in HTML markup does not match the registered name. (introduced in 73050cd) Closes #16314 Closes #16278

googlebot added the cla: no label Nov 1, 2017

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from f45484a to 3ba25d6 Compare November 1, 2017 13:36

googlebot added cla: yes and removed cla: no labels Nov 1, 2017

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch 3 times, most recently from c42eccd to 7c2e14b Compare November 1, 2017 14:48

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from 7c2e14b to 5e859dd Compare November 2, 2017 07:07

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from 5e859dd to 18226ee Compare November 2, 2017 07:12

fix($compile): special chars are sanitized from directive name in markup

6a20b35

this fixes bug introduced in 1.6.x when directive name with preceeding special char in HTML markup does not match the registered name

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from 18226ee to 6a20b35 Compare November 2, 2017 09:50

gkalpak suggested changes Nov 2, 2017

View reviewed changes

Narretz added this to the 1.6.7 milestone Nov 3, 2017

Narretz added the severity: regression label Nov 3, 2017

fix($compile): revert directiveNormalize behaviour to pre-1.6.x

7209817

gkalpak reviewed Nov 6, 2017

View reviewed changes

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from b6fdcde to e126f72 Compare November 8, 2017 18:07

googlebot added cla: yes and removed cla: no labels Nov 8, 2017

fix($compile): regex fix

e6d2439

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from e126f72 to e6d2439 Compare November 9, 2017 10:08

Narretz reviewed Nov 9, 2017

View reviewed changes

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch 6 times, most recently from 273021a to ca546c8 Compare November 10, 2017 11:30

fix($compile): added comment to test with link to issue

a3a91aa

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from ca546c8 to a3a91aa Compare November 12, 2017 18:01

gkalpak suggested changes Nov 13, 2017

View reviewed changes

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch 2 times, most recently from 4ecd741 to adc359f Compare November 14, 2017 10:55

gkalpak approved these changes Nov 14, 2017

View reviewed changes

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch 3 times, most recently from 22c40fb to fc0811f Compare November 15, 2017 08:26

fix($compile): review remarks

b8807dc

curlydevil force-pushed the compile-service-should-omit-special-chars-from-directive-name branch from fc0811f to b8807dc Compare November 15, 2017 08:48

Narretz merged commit 12cf994 into angular:master Nov 17, 2017

curlydevil deleted the compile-service-should-omit-special-chars-from-directive-name branch November 22, 2017 14:08

dependencies bot mentioned this pull request Dec 5, 2017

Update 26 dependencies from npm JetBrains/ring-ui#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($compile): sanitize special chars in directive name #16314

fix($compile): sanitize special chars in directive name #16314

curlydevil commented Nov 1, 2017

googlebot commented Nov 1, 2017

googlebot commented Nov 1, 2017

googlebot commented Nov 1, 2017

googlebot commented Nov 1, 2017

Narretz commented Nov 1, 2017

curlydevil commented Nov 1, 2017

Narretz commented Nov 1, 2017

curlydevil commented Nov 1, 2017 •

edited

Loading

petebacondarwin commented Nov 1, 2017

curlydevil commented Nov 2, 2017 •

edited

Loading

curlydevil commented Nov 2, 2017

Narretz commented Nov 2, 2017

gkalpak Nov 2, 2017

gkalpak Nov 2, 2017 •

edited

Loading

curlydevil commented Nov 6, 2017

gkalpak Nov 6, 2017

gkalpak Nov 6, 2017

gkalpak commented Nov 6, 2017

googlebot commented Nov 8, 2017

googlebot commented Nov 8, 2017

Narretz Nov 9, 2017

curlydevil commented Nov 13, 2017

gkalpak Nov 13, 2017

gkalpak Nov 13, 2017

fix($compile): sanitize special chars in directive name #16314

fix($compile): sanitize special chars in directive name #16314

Conversation

curlydevil commented Nov 1, 2017

googlebot commented Nov 1, 2017

googlebot commented Nov 1, 2017

googlebot commented Nov 1, 2017

googlebot commented Nov 1, 2017

Narretz commented Nov 1, 2017

curlydevil commented Nov 1, 2017

Narretz commented Nov 1, 2017

curlydevil commented Nov 1, 2017 • edited Loading

petebacondarwin commented Nov 1, 2017

curlydevil commented Nov 2, 2017 • edited Loading

curlydevil commented Nov 2, 2017

Narretz commented Nov 2, 2017

gkalpak Nov 2, 2017

Choose a reason for hiding this comment

gkalpak Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

curlydevil commented Nov 6, 2017

gkalpak Nov 6, 2017

Choose a reason for hiding this comment

gkalpak Nov 6, 2017

Choose a reason for hiding this comment

gkalpak commented Nov 6, 2017

googlebot commented Nov 8, 2017

googlebot commented Nov 8, 2017

Narretz Nov 9, 2017

Choose a reason for hiding this comment

curlydevil commented Nov 13, 2017

gkalpak Nov 13, 2017

Choose a reason for hiding this comment

gkalpak Nov 13, 2017

Choose a reason for hiding this comment

curlydevil commented Nov 1, 2017 •

edited

Loading

curlydevil commented Nov 2, 2017 •

edited

Loading

gkalpak Nov 2, 2017 •

edited

Loading