diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index d1c2b7165..b08337440 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -821,7 +821,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): else: return non_ssl_vh_fp + self.conf("le_vhost_ext") - def _sift_line(self, line): + def _sift_rewrite_rule(self, line): """Decides whether a line should be copied to a SSL vhost. A canonical example of when sifting a line is required: @@ -872,18 +872,62 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): with open(avail_fp, "r") as orig_file: with open(ssl_fp, "w") as new_file: new_file.write("\n") + + comment = ("# Some rewrite rules in this file were " + "disabled on your HTTPS site,\n" + "# because they have the potential to create " + "redirection loops.\n") + for line in orig_file: - if self._sift_line(line): + A = line.lstrip().startswith("RewriteCond") + B = line.lstrip().startswith("RewriteRule") + + if not (A or B): + new_file.write(line) + continue + + # A RewriteRule that doesn't need filtering + if B and not self._sift_rewrite_rule(line): + new_file.write(line) + continue + + # A RewriteRule that does need filtering + if B and self._sift_rewrite_rule(line): if not sift: - new_file.write( - "# Some rewrite rules in this file were " - "were disabled on your HTTPS site,\n" - "# because they have the potential to " - "create redirection loops.\n") + new_file.write(comment) sift = True new_file.write("# " + line) - else: - new_file.write(line) + continue + + # We save RewriteCond(s) and their corresponding + # RewriteRule in 'chunk'. + # We then decide whether we comment out the entire + # chunk based on its RewriteRule. + chunk = [] + if A: + chunk.append(line) + line = next(orig_file) + + # RewriteCond(s) must be followed by one RewriteRule + while not line.lstrip().startswith("RewriteRule"): + chunk.append(line) + line = next(orig_file) + + # Now, current line must start with a RewriteRule + chunk.append(line) + + if self._sift_rewrite_rule(line): + if not sift: + new_file.write(comment) + sift = True + + new_file.write(''.join( + ['# ' + l for l in chunk])) + continue + else: + new_file.write(''.join(chunk)) + continue + new_file.write("\n") except IOError: logger.fatal("Error writing/reading to file in make_vhost_ssl") diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 9a034c3e0..57c6a8009 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -1110,16 +1110,19 @@ class MultipleVhostsTest(util.ApacheTest): self.config._enable_redirect(self.vh_truth[1], "") self.assertEqual(len(self.config.vhosts), 9) - def test_sift_line(self): + def test_sift_rewrite_rule(self): # pylint: disable=protected-access small_quoted_target = "RewriteRule ^ \"http://\"" - self.assertFalse(self.config._sift_line(small_quoted_target)) + self.assertFalse(self.config._sift_rewrite_rule(small_quoted_target)) https_target = "RewriteRule ^ https://satoshi" - self.assertTrue(self.config._sift_line(https_target)) + self.assertTrue(self.config._sift_rewrite_rule(https_target)) normal_target = "RewriteRule ^/(.*) http://www.a.com:1234/$1 [L,R]" - self.assertFalse(self.config._sift_line(normal_target)) + self.assertFalse(self.config._sift_rewrite_rule(normal_target)) + + not_rewriterule = "NotRewriteRule ^ ..." + self.assertFalse(self.config._sift_rewrite_rule(not_rewriterule)) @mock.patch("certbot_apache.configurator.zope.component.getUtility") def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility): @@ -1148,7 +1151,61 @@ class MultipleVhostsTest(util.ApacheTest): "[L,QSA,R=permanent]") self.assertTrue(commented_rewrite_rule in conf_text) mock_get_utility().add_message.assert_called_once_with(mock.ANY, + mock.ANY) + @mock.patch("certbot_apache.configurator.zope.component.getUtility") + def test_make_vhost_ssl_with_existing_rewrite_conds(self, mock_get_utility): + self.config.parser.modules.add("rewrite_module") + + http_vhost = self.vh_truth[0] + + self.config.parser.add_dir( + http_vhost.path, "RewriteEngine", "on") + + # Add a chunk that should not be commented out. + self.config.parser.add_dir(http_vhost.path, + "RewriteCond", ["%{DOCUMENT_ROOT}/%{REQUEST_FILENAME}", "!-f"]) + self.config.parser.add_dir( + http_vhost.path, "RewriteRule", + ["^(.*)$", "b://u%{REQUEST_URI}", "[P,QSA,L]"]) + + # Add a chunk that should be commented out. + self.config.parser.add_dir(http_vhost.path, + "RewriteCond", ["%{HTTPS}", "!=on"]) + self.config.parser.add_dir(http_vhost.path, + "RewriteCond", ["%{HTTPS}", "!^$"]) + self.config.parser.add_dir( + http_vhost.path, "RewriteRule", + ["^", + "https://%{SERVER_NAME}%{REQUEST_URI}", + "[L,QSA,R=permanent]"]) + + self.config.save() + + ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0]) + + conf_line_set = set(open(ssl_vhost.filep).read().splitlines()) + + not_commented_cond1 = ("RewriteCond " + "%{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f") + not_commented_rewrite_rule = ("RewriteRule " + "^(.*)$ b://u%{REQUEST_URI} [P,QSA,L]") + + commented_cond1 = "# RewriteCond %{HTTPS} !=on" + commented_cond2 = "# RewriteCond %{HTTPS} !^$" + commented_rewrite_rule = ("# RewriteRule ^ " + "https://%{SERVER_NAME}%{REQUEST_URI} " + "[L,QSA,R=permanent]") + + self.assertTrue(not_commented_cond1 in conf_line_set) + self.assertTrue(not_commented_rewrite_rule in conf_line_set) + + self.assertTrue(commented_cond1 in conf_line_set) + self.assertTrue(commented_cond2 in conf_line_set) + self.assertTrue(commented_rewrite_rule in conf_line_set) + mock_get_utility().add_message.assert_called_once_with(mock.ANY, + mock.ANY) + def get_achalls(self): """Return testing achallenges."""