From 8ad8271db17aa6a249513cdb2220322832ed6477 Mon Sep 17 00:00:00 2001
From: Max Erenberg <merenber@csclub.uwaterloo.ca>
Date: Sun, 4 Sep 2022 23:19:09 -0400
Subject: [PATCH] Fix some bugs in ClubWebHostingService

* Don't read the value of an Apache directive unless we are sure
  it can only accept one argument
* Handle the case where a club's www directory is not readable
---
 .drone/common.sh                    |  7 +++++-
 README.md                           |  2 +-
 ceod/model/ClubWebHostingService.py | 34 +++++++++++++++++++----------
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/.drone/common.sh b/.drone/common.sh
index aaf452a..185d701 100644
--- a/.drone/common.sh
+++ b/.drone/common.sh
@@ -2,7 +2,12 @@
 chmod 1777 /tmp
 
 # don't resolve container names to *real* CSC machines
-sed -E '/^(domain|search)[[:space:]]+csclub.uwaterloo.ca/d' /etc/resolv.conf > /tmp/resolv.conf
+sed -E 's/([[:alnum:]-]+\.)*uwaterloo\.ca//g' /etc/resolv.conf > /tmp/resolv.conf
+# remove empty 'search' lines, if we created them
+sed -E -i '/^search[[:space:]]*$/d' /tmp/resolv.conf
+# also remove the 'rotate' option, since this can cause the Docker DNS server
+# to be circumvented
+sed -E -i '/^options.*\brotate/d' /tmp/resolv.conf
 cp /tmp/resolv.conf /etc/resolv.conf
 rm /tmp/resolv.conf
 
diff --git a/README.md b/README.md
index 2c5d828..c28372f 100644
--- a/README.md
+++ b/README.md
@@ -16,7 +16,7 @@ Docker containers instead, which are much easier to work with than the VM.
 
 First, make sure you create the virtualenv:
 ```sh
-docker run --rm -v "$PWD:$PWD" -w "$PWD" python:3.7-buster sh -c 'apt update && apt install -y libaugeas0 && python -m venv venv && . venv/bin/activate && pip install -r requirements.txt -r dev-requirements.txt'
+docker run --rm -v "$PWD:$PWD:z" -w "$PWD" python:3.7-buster sh -c 'apt update && apt install -y libaugeas0 && python -m venv venv && . venv/bin/activate && pip install -r requirements.txt -r dev-requirements.txt'
 ```
 Then bring up the containers:
 ```sh
diff --git a/ceod/model/ClubWebHostingService.py b/ceod/model/ClubWebHostingService.py
index c57b4c0..2e472fb 100644
--- a/ceod/model/ClubWebHostingService.py
+++ b/ceod/model/ClubWebHostingService.py
@@ -84,16 +84,19 @@ class ClubWebHostingService:
         logger.debug('Reloading Apache')
         self._run(['systemctl', 'reload', 'apache2'])
 
-    # This requires the APACHE_CONFIG_CRON environment variable to be
-    # set to 1 (e.g. in a systemd drop-in)
-    # See /etc/apache2/.git/hooks/pre-commit on caffeine
     def _git_commit(self):
         if not os.path.isdir(os.path.join(self.apache_dir, '.git')):
             logger.debug('No git folder found in Apache directory')
             return
         logger.debug('Committing changes to git repository')
-        self._run(['git', 'add', APACHE_DISABLED_CLUBS_FILE], cwd=self.apache_dir)
-        self._run(['git', 'commit', '-m', '[ceo] disable club websites'], cwd=self.apache_dir)
+        self._run(
+            ['git', 'add', APACHE_DISABLED_CLUBS_FILE],
+            cwd=self.apache_dir)
+        # See /etc/apache2/.git/hooks/pre-commit on caffeine
+        self._run(
+            ['git', 'commit', '-m', '[ceo] disable club websites'],
+            cwd=self.apache_dir,
+            env={**os.environ, 'APACHE_CONFIG_CRON': '1'})
 
     def commit(self):
         if not self.made_at_least_one_change:
@@ -112,12 +115,13 @@ class ClubWebHostingService:
             directive_paths = self.aug.match(f'/files/etc/apache2/sites-available/{filename}/VirtualHost/directive')
             for directive_path in directive_paths:
                 directive = self.aug.get(directive_path)
-                directive_value = self.aug.get(directive_path + '/arg')
                 if directive == 'DocumentRoot':
+                    directive_value = self.aug.get(directive_path + '/arg')
                     match = APACHE_USERDIR_RE.match(directive_value)
                     if match is not None:
                         club_name = match.group('club_name')
                 elif directive == 'ServerAdmin':
+                    directive_value = self.aug.get(directive_path + '/arg')
                     club_email = directive_value
             if club_name is not None:
                 self.clubs[club_name]['email'] = club_email
@@ -157,12 +161,20 @@ class ClubWebHostingService:
 
     def _site_uses_php(self, club_name: str) -> bool:
         www = f'{self.clubs_home}/{club_name}/www'
-        if os.path.isdir(www):
+        if not os.path.isdir(www):
+            return False
+        try:
             # We're just going to look one level deep; that should be good enough.
-            for filename in os.listdir(www):
-                filepath = os.path.join(www, filename)
-                if os.path.isfile(filepath) and filename.endswith('.php'):
-                    return True
+            filenames = os.listdir(www)
+        except os.error:
+            # If we're unable to read the directory (e.g. permissions error),
+            # then this means that the Apache user (www-data) can't read it either.
+            # So we can just return False here.
+            return False
+        for filename in filenames:
+            filepath = os.path.join(www, filename)
+            if os.path.isfile(filepath) and filename.endswith('.php'):
+                return True
         return False
 
     # This method needs to be called from within a transaction (uses self.clubs)