From 858068c64b279ef17113ea2b59ad852e38e7d624 Mon Sep 17 00:00:00 2001
From: Hans-Christoph Steiner <hans@eds.org>
Date: Wed, 5 Mar 2025 12:22:22 +0100
Subject: [PATCH] only show "unsafe perms on config.yml" when secrets are
 present

This should make for fewer false positives.
---
 fdroidserver/common.py | 21 +++++++++--------
 tests/test_common.py   | 51 +++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/fdroidserver/common.py b/fdroidserver/common.py
index b2386396..2a63803f 100644
--- a/fdroidserver/common.py
+++ b/fdroidserver/common.py
@@ -598,15 +598,6 @@ def read_config():
                                       'sun.security.pkcs11.SunPKCS11',
                                       '-providerArg', 'opensc-fdroid.cfg']
 
-    if any(k in config for k in ["keystore", "keystorepass", "keypass"]):
-        st = os.stat(CONFIG_FILE)
-        if st.st_mode & stat.S_IRWXG or st.st_mode & stat.S_IRWXO:
-            logging.warning(
-                _("unsafe permissions on '{config_file}' (should be 0600)!").format(
-                    config_file=CONFIG_FILE
-                )
-            )
-
     fill_config_defaults(config)
 
     if 'serverwebroot' in config:
@@ -666,6 +657,18 @@ def read_config():
     for configname in confignames_to_delete:
         del config[configname]
 
+    if any(
+        k in config and config.get(k)
+        for k in ["awssecretkey", "keystorepass", "keypass"]
+    ):
+        st = os.stat(CONFIG_FILE)
+        if st.st_mode & stat.S_IRWXG or st.st_mode & stat.S_IRWXO:
+            logging.warning(
+                _("unsafe permissions on '{config_file}' (should be 0600)!").format(
+                    config_file=CONFIG_FILE
+                )
+            )
+
     return config
 
 
diff --git a/tests/test_common.py b/tests/test_common.py
index cbbba221..293d69ec 100755
--- a/tests/test_common.py
+++ b/tests/test_common.py
@@ -1964,16 +1964,6 @@ class CommonTest(SetUpTearDownMixin, unittest.TestCase):
         with self.assertRaises(ruamel.yaml.scanner.ScannerError):
             fdroidserver.common.read_config()
 
-    def test_config_perm_warning(self):
-        """Exercise the code path that issues a warning about unsafe permissions."""
-        os.chdir(self.tmpdir)
-        fdroidserver.common.write_config_file('keystore: foo.jks')
-        self.assertTrue(os.path.exists(fdroidserver.common.CONFIG_FILE))
-        os.chmod(fdroidserver.common.CONFIG_FILE, 0o666)  # nosec B103
-        fdroidserver.common.read_config()
-        os.remove(fdroidserver.common.CONFIG_FILE)
-        fdroidserver.common.config = None
-
     def test_config_repo_url(self):
         """repo_url ends in /repo, archive_url ends in /archive."""
         os.chdir(self.tmpdir)
@@ -3444,3 +3434,44 @@ class ConfigOptionsScopeTest(unittest.TestCase):
             'config' not in vars() and 'config' not in globals(),
             "The config should not be set in the global context, only module-level.",
         )
+
+
+class UnsafePermissionsTest(SetUpTearDownMixin, unittest.TestCase):
+    def setUp(self):
+        config = dict()
+        fdroidserver.common.find_apksigner(config)
+        if not config.get('apksigner'):
+            self.skipTest('SKIPPING, apksigner not installed!')
+
+        super().setUp()
+        os.chdir(self.testdir)
+        fdroidserver.common.write_config_file('keypass: {env: keypass}')
+        os.chmod(fdroidserver.common.CONFIG_FILE, 0o666)  # nosec B103
+
+    def test_config_perm_no_warning(self):
+        fdroidserver.common.write_config_file('keystore: foo.jks')
+        with self.assertNoLogs(level=logging.WARNING):
+            fdroidserver.common.read_config()
+
+    def test_config_perm_keypass_warning(self):
+        fdroidserver.common.write_config_file('keypass: supersecret')
+        with self.assertLogs(level=logging.WARNING) as lw:
+            fdroidserver.common.read_config()
+            self.assertTrue('unsafe' in lw.output[0])
+
+    @mock.patch.dict(os.environ, {'PATH': os.getenv('PATH')}, clear=True)
+    def test_config_perm_env_warning(self):
+        os.environ['keypass'] = 'supersecret'
+        fdroidserver.common.write_config_file('keypass: {env: keypass}')
+        with self.assertLogs(level=logging.WARNING) as lw:
+            fdroidserver.common.read_config()
+            self.assertTrue('unsafe' in lw.output[0])
+            self.assertEqual(1, len(lw.output))
+
+    @mock.patch.dict(os.environ, {'PATH': os.getenv('PATH')}, clear=True)
+    def test_config_perm_unset_env_no_warning(self):
+        fdroidserver.common.write_config_file('keypass: {env: keypass}')
+        with self.assertLogs(level=logging.WARNING) as lw:
+            fdroidserver.common.read_config()
+            self.assertTrue('unsafe' not in lw.output[0])
+            self.assertEqual(1, len(lw.output))