Merge pull request certbot#3234 from sagi/rewrite

Comment out corresponding RewriteConds for filtered RewriteRule

Author: SwartzCr

certbot-apache/certbot_apache/configurator.py

@@ -821,7 +821,7 @@ def _get_ssl_vhost_path(self, non_ssl_vh_fp):
         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 @@ def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp):
             with open(avail_fp, "r") as orig_file:
                 with open(ssl_fp, "w") as new_file:
                     new_file.write("<IfModule mod_ssl.c>\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("</IfModule>\n")
         except IOError:
             logger.fatal("Error writing/reading to file in make_vhost_ssl")

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -1110,16 +1110,19 @@ def test_create_own_redirect_for_old_apache_version(self):
         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 @@ def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility):
                                   "[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."""