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

[vf] Adding support for parsing EL in script tags #284

Merged
merged 14 commits into from Mar 7, 2017

Conversation

sgorbaty
Copy link
Contributor

@sgorbaty sgorbaty commented Mar 3, 2017

Added support to the parser to understand EL in script tags.
Also improved XSS detection logic.

@jsotuyod jsotuyod added this to the 5.6.0 milestone Mar 3, 2017
@jsotuyod jsotuyod added the an:enhancement An improvement on existing features / rules label Mar 3, 2017
<HTML_SCRIPT_CONTENT: (~[]) >
| <HTML_SCRIPT_END_TAG : "</script>" > : AfterTagState
<HTML_SCRIPT_END_TAG : "</script>" > : AfterTagState
| <EL_EXPRESSION_IN_SCRIPT: "{!" (<WHITESPACES>)? > : ElInScriptState
Copy link
Member

@jsotuyod jsotuyod Mar 3, 2017

Choose a reason for hiding this comment

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

@sgorbaty I am curious, how does visualforce handle constructs such as {!function(){console.log("test");}();} which, even though ugly, are completely valid JS (prints "test", and evaluates to true).

I don't think we will ever be able to tell for sure if what we are parsing is actually EL or just more JS, but I wonder on the impact of not being able to tell them apart...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. The only way to solve this is to have a dedicated JS parser (Acorn/Aspree) added. I could do that but figured we should invest in PMD being able to switch parsers on the fly.

Copy link
Member

Choose a reason for hiding this comment

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

@sgorbaty how would such parser solve this issue? {!myVar} could be either valid JS or valid EL... Even more with JS not needing to declare variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsotuyod this is a parser for VF, not Javascript. :)
VF is a very opinionated platform, which treats {! } construct anywhere in code 100% of the time as EL; therefore, any javascript construct that looks like {!varName} would result in compilation error because there would be a missing property with that name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const myVar = "this is my JS var";
{!myVar} // always resolves to an Apex class property

Copy link
Member

Choose a reason for hiding this comment

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

@sgorbaty awesome, that is what I was after! Being that way, then this grammar is correct 100% of the time identifying those as EL. This is the best scenario possible.

@jsotuyod jsotuyod self-assigned this Mar 4, 2017
jsotuyod added a commit to Monits/pmd that referenced this pull request Mar 7, 2017
@jsotuyod jsotuyod merged commit 309d2d1 into pmd:master Mar 7, 2017
@jsotuyod
Copy link
Member

jsotuyod commented Mar 7, 2017

Thanks! This we included in PMD 5.6.0 and later.

@sgorbaty sgorbaty deleted the ScriptStyleSupportForEl branch April 13, 2017 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
an:enhancement An improvement on existing features / rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants