Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harmless bug in validator.js cdata blacklisted regexp #7246

Closed
cgorbit opened this issue Jan 29, 2017 · 4 comments
Closed

Harmless bug in validator.js cdata blacklisted regexp #7246

cgorbit opened this issue Jan 29, 2017 · 4 comments
Assignees

Comments

@cgorbit
Copy link

cgorbit commented Jan 29, 2017

https://github.com/ampproject/amphtml/blob/master/validator/engine/validator.js#L1643

var blacklistedCdataRegexStr = '';
if (tagSpec.cdata !== null) {
    for (const blacklist of tagSpec.cdata.blacklistedCdataRegex) {
        blacklistedCdataRegexStr += blacklist + '|';
    }
}

blacklistedCdataRegexStr will contain something like this:
"[object Object]|[object Object]|[object Object]|"

Consider last vertical bar!

@Gregable
Copy link
Member

Gregable commented Feb 1, 2017

Fixed as commited (https://github.com/ampproject/amphtml/blob/master/validator/engine/validator.js#L1619). Will be deployed later.

@Gregable Gregable closed this as completed Feb 1, 2017
@cgorbit
Copy link
Author

cgorbit commented Feb 1, 2017

blacklistedCdataRegexStr = tagSpec.cdata.blacklistedCdataRegex
    .map(function(b) { return b.regex })
    .join('|');

@jridgewell jridgewell reopened this Feb 1, 2017
@Gregable
Copy link
Member

Gregable commented Feb 1, 2017

Hm, I thought I had tests that confirmed this was working. Investigating now.

@Gregable
Copy link
Member

Gregable commented Feb 1, 2017

OK, so we had a test that should have been catching this: incorrect-custom-style, but we were not outputting any error for the violating line containing !important. This was why the issue hadn't been noticed before. No error was being generated because we were hiding them if the cdata already had an error, which we should no longer do.

I'll fix all of this up in an upcoming PR, and include your join changes.

Thanks for noticing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants