Corrupted logic of JsSanitizer#checkBraces

[Taggert]

If we'll look at the methods logic we'll see strange thing:

for (final Pattern pattern : LACK_EXPECTED_BRACES) {
final Matcher matcher = pattern.matcher(RemoveComments.perform(beautifiedJs));
if (matcher.find()) {
String line = "";
int index = matcher.start();
while (index >= 0 && beautifiedJs.charAt(index) != '\n' ) {
line = beautifiedJs.charAt(index)+line;
index--;
}
int singleParaCount = line.length() - line.replace("'", "").length();
int doubleParaCount = line.length() - line.replace(""", "").length();
if (singleParaCount % 2 != 0 || doubleParaCount % 2 != 0) {
// for in string
} else {
throw new BracesException("No block braces after function|for|while|do. Found ["+matcher.group()+"]");
}
}
}

Matcher finds all the blocks according to the pattern from JS that was already cleande from comments with RemoveComments. If matcher finds any block it gives to the for loot start index of the block.
Then for loop starts to build a line from js. But start index was taken for js cleaned from comments and now line is fom incoming real js.
It means that in will be a wrong line.
Also line is built from the end to the beginning. And start index is the beginning of the block.
Also method throws exception when singleParaCount/2 == 0 and doubleParacount/2 == 0.
It means that whe there is odd amount of single or double quetes it throws exception.

This method is used in another library works with PAC scripts (Proxy authomatic configuration java scripts). And this method breaks the flow of evaluation of the script.

Don't you think that checkBraces method should be refactored?
Or please can I receive better comments on how this method should work

[mxro]

Thank you for raising this issue, I was just looking at this for #102.

I agree that this method seems very brittle. It can easily lead to false positives. However, it also seems to be able to detect a number of straightforward cases for erroneous scripts.

I think since using setMaxCPUTime is a possible fallback for this, the best option for now is to disable this check by default and update the documentation which I have done now. I cannot really think of a fool-proof way to improve this method. If anyone can think of that, please be welcome to let me know.

Hope this resolves this for now. Changes released in 0.1.31.