Add tests against non-match groups being used in replacements (resolves EFForg#4327)

Hainish · Hainish · commit 950c21217204 · 2016-03-21T18:06:20.000-07:00
diff --git a/test/rules/coverage.checker.config b/test/rules/coverage.checker.config
@@ -4,6 +4,7 @@
 [rulesets]
 rulesdir = src/chrome/content/rules
 check_coverage = true
+check_nonmatch_groups = true
 include_default_off = false
 skiplist = utils/ruleset-coverage-whitelist.txt
 
diff --git a/test/rules/src/https_everywhere_checker/check_rules.py b/test/rules/src/https_everywhere_checker/check_rules.py
@@ -279,6 +279,9 @@ def cli():
 	checkCoverage = False
 	if config.has_option("rulesets", "check_coverage"):
 		checkCoverage = config.getboolean("rulesets", "check_coverage")
+	checkNonmatchGroups = False
+	if config.has_option("rulesets", "check_nonmatch_groups"):
+		checkNonmatchGroups = config.getboolean("rulesets", "check_nonmatch_groups")
 	certdir = config.get("certificates", "basedir")
 	if config.has_option("rulesets", "check_coverage"):
 		checkCoverage = config.getboolean("rulesets", "check_coverage")
@@ -322,6 +325,7 @@ def cli():
 	
 	rulesets = []
 	coverageProblemsExist = False
+	nonmatchGroupProblemsExist = False
 	for xmlFname in xmlFnames:
 		logging.debug("Parsing %s", xmlFname)
 		if skipFile(xmlFname):
@@ -342,6 +346,12 @@ def cli():
 			for problem in problems:
 				coverageProblemsExist = True
 				logging.error(problem)
+		if checkNonmatchGroups:
+			logging.debug("Checking non-match groups for '%s'." % ruleset.name)
+			problems = ruleset.getNonmatchGroupProblems()
+			for problem in problems:
+				nonmatchGroupProblemsExist = True
+				logging.error(problem)
 		trie.addRuleset(ruleset)
 		rulesets.append(ruleset)
 	
@@ -411,8 +421,10 @@ def cli():
 	if checkCoverage:
 		if coverageProblemsExist:
 			return 1 # exit with error code
-		else:
-			return 0 # exit with success
+	if checkNonmatchGroups:
+		if nonmatchGroupProblemsExist:
+			return 1 # exit with error code
+	return 0 # exit with success
 
 if __name__ == '__main__':
 	sys.exit(cli())
diff --git a/test/rules/src/https_everywhere_checker/rules.py b/test/rules/src/https_everywhere_checker/rules.py
@@ -122,6 +122,7 @@ def __init__(self, xmlTree, filename):
 		self.exclusions = []
 		self.filename = filename
 		self.tests = []
+		self.determine_test_application_run = False
 		
 		for (attrName, xpath, conversion) in self._attrConvert:
 			elems = root.xpath(xpath)
@@ -161,6 +162,21 @@ def _addTests(self):
 				continue
 			self.tests.append(Test("http://%s/" % target))
 
+	def _determineTestApplication(self):
+		"""Match each test against a rule or exclusion if possible, and hang them
+			 off that rule or exclusion.  Return any coverage problems."""
+		if not self.determine_test_application_run:
+			self.test_application_problems = []
+			for test in self.tests:
+				applies = self.whatApplies(test.url)
+				if applies:
+					applies.tests.append(test)
+				else:
+					self.test_application_problems.append("%s: No rule or exclusion applies to test URL %s" % (
+						self.filename, test.url))
+				self.determine_test_application_run = True
+		return self.test_application_problems
+
 	def getCoverageProblems(self):
 		"""Verify that each rule and each exclusion has the right number of tests
 			 that applies to it. TODO: Also check that each target has the right
@@ -170,16 +186,7 @@ def getCoverageProblems(self):
 			 Returns an array of strings reporting any coverage problems if they exist,
 			 or empty list if coverage is sufficient.
 			 """
-		# First, match each test against a rule or exclusion if possible, and hang
-		# them off that rule or exclusion.
-		problems = []
-		for test in self.tests:
-			applies = self.whatApplies(test.url)
-			if applies:
-				applies.tests.append(test)
-			else:
-				problems.append("%s: No rule or exclusion applies to test URL %s" % (
-					self.filename, test.url))
+		problems = self._determineTestApplication()
 		# Next, make sure each rule or exclusion has sufficient tests.
 		for rule in self.rules:
 			needed_count = 1 + len(regex.findall("[+*?|]", rule.fromPattern))
@@ -206,6 +213,22 @@ def getCoverageProblems(self):
 								self.filename, actual_count, needed_count, exclusion))
 		return problems
 
+	def getNonmatchGroupProblems(self):
+		"""Verify that when rules are actually applied, no non-match groups (e.g.
+			 '$1', '$2' etc.) will exist in the rewritten url"""
+		self._determineTestApplication()
+		problems = []
+		for rule in self.rules:
+			test_urls = map(lambda test: test.url, rule.tests)
+			for test in rule.tests:
+				try:
+					replacement_url = rule.apply(test.url)
+				except Exception, e:
+					if ~e.message.index("invalid group reference"):
+						problems.append("%s: Rules include non-matched groups in replacement for url: %s" % (
+							self.filename, test.url))
+		return problems
+
 	def whatApplies(self, url):
 		for exclusion in self.exclusions:
 			if exclusion.matches(url):
