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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from Mar 7, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2e073a1
Initial
sergeygorbaty Mar 1, 2017
34b7072
Script EL support added
sergeygorbaty Mar 1, 2017
a987c77
Added support for quoted context
sergeygorbaty Mar 1, 2017
2106e99
Revert quoted context
sergeygorbaty Mar 1, 2017
c4497d5
Logic bug fix
sergeygorbaty Mar 1, 2017
b7946ba
fix for special tags
sergeygorbaty Mar 2, 2017
32762c4
fix for unbalanced quotes
sergeygorbaty Mar 2, 2017
6137baf
More test coverage
sergeygorbaty Mar 2, 2017
1942e94
Bug fix
sergeygorbaty Mar 2, 2017
81c67a5
Fallback for JS arrays and defs
sergeygorbaty Mar 3, 2017
b38642a
Merged
sergeygorbaty Mar 3, 2017
caf27ad
deleted unused file
sergeygorbaty Mar 3, 2017
d12c1f7
Iterative DotExpression evaluation instead of checking the first one
sergeygorbaty Mar 3, 2017
309d2d1
Style fix
sergeygorbaty Mar 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 34 additions & 19 deletions pmd-visualforce/etc/grammar/VfParser.jjt
View file
Open in desktop
Expand Up @@ -90,7 +90,7 @@ PARSER_END(VfParser)
< (<WHITESPACE>)+ >
}

<AfterTagState, InTagState, HtmlScriptContentState, HtmlStyleContentState, ElTagState, ElAttribTagStateSQ, ElAttribTagStateDQ> SPECIAL_TOKEN:
<AfterTagState, InTagState, HtmlScriptContentState, HtmlStyleContentState, ElTagState, ElAttribTagStateSQ, ElAttribTagStateDQ, ElInScriptState > SPECIAL_TOKEN:
{
< (<WHITESPACE>)+ >
}
Expand All @@ -114,7 +114,7 @@ PARSER_END(VfParser)
| <UNPARSED_TEXT: ( <NO_LT_OR_OPENBRACE>|<OPENBRACE><NO_BANG>)+ >
}

<ElTagState, ElAttribTagStateSQ, ElAttribTagStateDQ, ElAttribTagStateNQ> TOKEN :
<ElTagState, ElAttribTagStateSQ, ElAttribTagStateDQ, ElAttribTagStateNQ, ElInScriptState> TOKEN :
{
<NULL: "null" >
| <TRUE: "true" >
Expand Down Expand Up @@ -175,6 +175,11 @@ PARSER_END(VfParser)
<END_OF_EL_ATTRIB_NQ: (<WHITESPACES>)? <CLOSEBRACE> > : AttrValueNoQuotesState
}

<ElInScriptState> TOKEN :
{
<END_OF_EL_SCRIPT: (<WHITESPACES>)? <CLOSEBRACE> > : HtmlScriptContentState
}

<DocTypeState, DocTypeExternalIdState> TOKEN :
{
<WHITESPACES: (<WHITESPACE>)+ >
Expand Down Expand Up @@ -255,8 +260,9 @@ PARSER_END(VfParser)

<HtmlScriptContentState> TOKEN :
{
<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 •
edited

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.

| <HTML_SCRIPT_CONTENT: ( (~["{"]) | (["{"] ~["!"]) ) >
}

<HtmlStyleContentState> TOKEN :
Expand Down Expand Up @@ -342,54 +348,50 @@ void ElOrText() #void :
( ElExpression() | Text() )+
}

String Text() :
void Text() :
{ Token t; }
{
t = <UNPARSED_TEXT>
{
jjtThis.setImage(t.image);
return t.image;
}
}


String UnparsedTextNoWhitespace() #Text :
void UnparsedTextNoWhitespace() #Text :
{ Token t;}
{
(
t = <UNPARSED_TEXT_NO_WHITESPACE>
)
{
jjtThis.setImage(t.image);
return t.image;
}
}

/**
* Text that contains no single quotes, and that does not contain the start
* of a EL expression.
*/
String UnparsedTextNoSingleQuotes() #Text :
void UnparsedTextNoSingleQuotes() #Text :
{ Token t; }
{
t = <UNPARSED_TEXT_NO_SINGLE_QUOTES>
{
jjtThis.setImage(t.image);
return t.image;
}
}

/**
* Text that contains no double quotes, and that does not contain the start
* of a EL expression.
*/
String UnparsedTextNoDoubleQuotes() #Text :
void UnparsedTextNoDoubleQuotes() #Text :
{ Token t; }
{
t = <UNPARSED_TEXT_NO_DOUBLE_QUOTES>
{
jjtThis.setImage(t.image);
return t.image;
}
}

Expand Down Expand Up @@ -566,6 +568,12 @@ void ElExpressionInAttribute() #ElExpression :
| <EL_EXPRESSION_IN_ATTRIBUTE_NQ> [Expression()] <END_OF_EL_ATTRIB_NQ>
}

void ElExpressionInScript() #ElExpression :
{}
{
<EL_EXPRESSION_IN_SCRIPT> [Expression()] <END_OF_EL_SCRIPT>
}

void CData() :
{
StringBuffer content = new StringBuffer();
Expand Down Expand Up @@ -709,19 +717,15 @@ void DoctypeExternalId() :
}

void HtmlScript() :
{
StringBuffer content = new StringBuffer();
String tagName;
Token t;
}
{}
{
<HTML_SCRIPT_START> {}
(Attribute() )* {}
(
(
<TAG_END> {token_source.SwitchTo(HtmlScriptContentState);}
(t = <HTML_SCRIPT_CONTENT> { content.append(t.image); })*
<HTML_SCRIPT_END_TAG> { jjtThis.setImage(content.toString().trim());}
( ( HtmlScriptContent() | ElExpressionInScript() ) )*
<HTML_SCRIPT_END_TAG>

)
|
Expand All @@ -731,6 +735,17 @@ void HtmlScript() :
)
}

void HtmlScriptContent() #Text :
{
StringBuffer content = new StringBuffer();
Token t;
}
{
( t = <HTML_SCRIPT_CONTENT> { content.append(t.image); } )+
{ jjtThis.setImage(content.toString()); }
}


void HtmlStyle() :
{
StringBuffer content = new StringBuffer();
Expand Down
1 change: 1 addition & 0 deletions pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/ast/VfParserVisitorAdapter.java
View file
Open in desktop
Expand Up @@ -93,4 +93,5 @@ public Object visit(ASTDotExpression node, Object data) {
public Object visit(ASTContent node, Object data) {
return visit((VfNode) node, data);
}

}
148 changes: 125 additions & 23 deletions ...visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
import net.sourceforge.pmd.lang.vf.ast.ASTElExpression;
import net.sourceforge.pmd.lang.vf.ast.ASTElement;
import net.sourceforge.pmd.lang.vf.ast.ASTExpression;
import net.sourceforge.pmd.lang.vf.ast.ASTHtmlScript;
import net.sourceforge.pmd.lang.vf.ast.ASTIdentifier;
import net.sourceforge.pmd.lang.vf.ast.ASTLiteral;
import net.sourceforge.pmd.lang.vf.ast.ASTText;
Expand All @@ -43,18 +44,94 @@ public class VfUnescapeElRule extends AbstractVfRule {
private static final Pattern ON_EVENT = Pattern.compile("^on(\\w)+$");
private static final Pattern PLACEHOLDERS = Pattern.compile("\\{(\\w|,|\\.|'|:|\\s)*\\}");

@Override
public Object visit(ASTHtmlScript node, Object data) {
checkIfCorrectlyEscaped(node, data);

return super.visit(node, data);
}

private void checkIfCorrectlyEscaped(ASTHtmlScript node, Object data) {
ASTText prevText = null;

// churn thru every child just once instead of twice
for (int i = 0; i < node.jjtGetNumChildren(); i++) {
Node n = node.jjtGetChild(i);

if (n instanceof ASTText) {
prevText = (ASTText) n;
continue;
}

if (n instanceof ASTElExpression) {
processElInScriptContext((ASTElExpression) n, prevText, data);
}
}
}

private void processElInScriptContext(ASTElExpression elExpression, ASTText prevText, Object data) {
boolean quoted = false;

if (prevText != null) {
if (isUnbalanced(prevText.getImage(), "'") || isUnbalanced(prevText.getImage(), "\"")) {
quoted = true;
}
}
if (quoted) {
// check escaping too
if (!(startsWithSafeResource(elExpression) || containsSafeFields(elExpression))) {
if (doesElContainAnyUnescapedIdentifiers(elExpression, Escaping.JSENCODE)) {
addViolation(data, elExpression);
}
}
} else {
if (!(startsWithSafeResource(elExpression) || containsSafeFields(elExpression))) {
addViolation(data, elExpression);
}
}
}

private boolean isUnbalanced(String image, String pattern) {
int occurance = 0;
int index = image.indexOf("=");
if (index < 0) {
index = image.indexOf(":");
}

index = image.indexOf(pattern, index + 1);
while (index >= 0) {
occurance++;
index = image.indexOf(pattern, index + 1);
}

if ((occurance % 2) != 0) {
return true;
}

return false;
}

@Override
public Object visit(ASTElement node, Object data) {
if (doesTagSupportEscaping(node)) {
checkTagsThatSupportEscaping(node, data);
checkApexTagsThatSupportEscaping(node, data);
} else {
checkAllOtherTags(node, data);
checkLimitedFlags(node, data);
checkAllOnEventTags(node, data);
}

return super.visit(node, data);
}

private void checkAllOtherTags(ASTElement node, Object data) {
private void checkLimitedFlags(ASTElement node, Object data) {
switch (node.getName().toLowerCase()) {
case "iframe":
case "a":
break;
default:
return;
}

final List<ASTAttribute> attributes = node.findChildrenOfType(ASTAttribute.class);
boolean isEL = false;
final Set<ASTElExpression> toReport = new HashSet<>();
Expand All @@ -63,29 +140,14 @@ private void checkAllOtherTags(ASTElement node, Object data) {
String name = attr.getName().toLowerCase();
// look for onevents

if (ON_EVENT.matcher(name).matches()) {
final List<ASTElExpression> elsInVal = attr.findDescendantsOfType(ASTElExpression.class);
for (ASTElExpression el : elsInVal) {
if (startsWithSafeResource(el)) {
continue;
}

if (doesElContainAnyUnescapedIdentifiers(el,
EnumSet.of(Escaping.JSINHTMLENCODE, Escaping.JSENCODE))) {
isEL = true;
toReport.add(el);
}
}
}

if (HREF.equalsIgnoreCase(name) || SRC.equalsIgnoreCase(name)) {
boolean startingWithSlashText = false;

final ASTText attrText = attr.getFirstDescendantOfType(ASTText.class);
if (attrText != null) {
if (0 == attrText.jjtGetChildIndex()) {
if (attrText.getImage().startsWith("/")
|| attrText.getImage().toLowerCase().startsWith("http")) {
if (attrText.getImage().startsWith("/") || attrText.getImage().toLowerCase().startsWith("http")
|| attrText.getImage().toLowerCase().startsWith("mailto")) {
startingWithSlashText = true;
}
}
Expand All @@ -110,6 +172,41 @@ private void checkAllOtherTags(ASTElement node, Object data) {
}

}

}

if (isEL) {
for (ASTElExpression expr : toReport) {
addViolation(data, expr);
}
}

}

private void checkAllOnEventTags(ASTElement node, Object data) {
final List<ASTAttribute> attributes = node.findChildrenOfType(ASTAttribute.class);
boolean isEL = false;
final Set<ASTElExpression> toReport = new HashSet<>();

for (ASTAttribute attr : attributes) {
String name = attr.getName().toLowerCase();
// look for onevents

if (ON_EVENT.matcher(name).matches()) {
final List<ASTElExpression> elsInVal = attr.findDescendantsOfType(ASTElExpression.class);
for (ASTElExpression el : elsInVal) {
if (startsWithSafeResource(el)) {
continue;
}

if (doesElContainAnyUnescapedIdentifiers(el,
EnumSet.of(Escaping.JSINHTMLENCODE, Escaping.JSENCODE))) {
isEL = true;
toReport.add(el);
}
}
}

}

if (isEL) {
Expand All @@ -133,6 +230,9 @@ private boolean startsWithSafeResource(final ASTElExpression el) {
case "urlfor":
case "$site":
case "$page":
case "$action":
case "casesafeid":
case "$remoteaction":
return true;

default:
Expand Down Expand Up @@ -161,7 +261,7 @@ private boolean startsWithSlashLiteral(final ASTElExpression elExpression) {
return false;
}

private void checkTagsThatSupportEscaping(ASTElement node, Object data) {
private void checkApexTagsThatSupportEscaping(ASTElement node, Object data) {
final List<ASTAttribute> attributes = node.findChildrenOfType(ASTAttribute.class);
final Set<ASTElExpression> toReport = new HashSet<>();
boolean isUnescaped = false;
Expand Down Expand Up @@ -286,13 +386,15 @@ private boolean containsSafeFields(final AbstractVFNode expression) {
case "id":
case "size":
case "caseNumber":
return true;
return true;
default:
}
}

if (child instanceof ASTDotExpression) {
return containsSafeFields((ASTDotExpression) child);
if (containsSafeFields((ASTDotExpression) child)) {
return true;
}
}

}
Expand Down