Merge branch 'no-git-exec-in-update' into 'master'

update: never execute VCS e.g. git

See merge request fdroid/fdroidserver!1630

fdroidserver/build.py

@@ -721,8 +721,7 @@ def build_local(app, build, vcs, build_dir, output_dir, log_dir, srclib_dir, ext
         bindir = os.path.join(root_dir, 'bin')
 
     if os.path.isdir(os.path.join(build_dir, '.git')):
-        import git
+        commit_id = str(common.get_head_commit_id(build_dir))
-        commit_id = common.get_head_commit_id(git.repo.Repo(build_dir))
     else:
         commit_id = build.commit
 

fdroidserver/common.py

@@ -1286,12 +1286,29 @@ def write_status_json(output, pretty=False, name=None):
         rsync_status_file_to_repo(path, repo_subdir='status')
 
 
-def get_head_commit_id(git_repo):
+def get_head_commit_id(git_repo_dir):
-    """Get git commit ID for HEAD as a str."""
+    """Get git commit ID for HEAD as a str.
+
+    This only reads files, so it should be safe to use on untrusted
+    repos.  It was created to avoid running the git executable, no
+    matter what. It uses a tiny subset of the git.Repo class to avoid
+    setting up the git executable.
+
+    """
     try:
-        return git_repo.head.commit.hexsha
+        if type(git_repo_dir) is git.Repo:
-    except ValueError:
+            d = git_repo_dir.git_dir
-        return "None"
+        else:
+            d = os.path.join(git_repo_dir, '.git')
+        repo = type(
+            'Repo',
+            (object,),
+            {'common_dir': d, 'git_dir': d, 're_hexsha_only': git.Repo.re_hexsha_only},
+        )()
+        return git.refs.symbolic.SymbolicReference.dereference_recursive(repo, 'HEAD')
+    except (FileNotFoundError, ValueError) as e:
+        msg = _("Cannot read {path}: {error}").format(path=os.getcwd(), error=str(e))
+        logging.debug(msg)
 
 
 def setup_vcs(app):

fdroidserver/import_subcommand.py

@@ -342,7 +342,7 @@ def main():
 
     app.AutoUpdateMode = 'Version'
     app.UpdateCheckMode = 'Tags'
-    build.commit = common.get_head_commit_id(git_repo)
+    build.commit = common.get_head_commit_id(tmp_importer_dir)
 
     # Extract some information...
     paths = get_all_gradle_and_manifests(tmp_importer_dir)

fdroidserver/metadata.py

@@ -18,7 +18,6 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import git
 from pathlib import Path
 import math
 import platform
@@ -658,14 +657,12 @@ def parse_metadata(metadatapath):
         build_dir = common.get_build_dir(app)
         metadata_in_repo = build_dir / '.fdroid.yml'
         if metadata_in_repo.is_file():
-            try:
+            commit_id = common.get_head_commit_id(build_dir)
-                commit_id = common.get_head_commit_id(git.Repo(build_dir))
+            if commit_id is not None:
                 logging.debug(
                     _('Including metadata from %s@%s') % (metadata_in_repo, commit_id)
                 )
-            # See https://github.com/PyCQA/pylint/issues/2856 .
+            else:
-            # pylint: disable-next=no-member
-            except git.exc.InvalidGitRepositoryError:
                 logging.debug(
                     _('Including metadata from {path}').format(path=metadata_in_repo)
                 )

tests/test_common.py

@@ -3470,3 +3470,53 @@ class UnsafePermissionsTest(SetUpTearDownMixin, unittest.TestCase):
             fdroidserver.common.read_config()
             self.assertTrue('unsafe' not in lw.output[0])
             self.assertEqual(1, len(lw.output))
+
+
+class GetHeadCommitIdTest(unittest.TestCase):
+    """Test and compare two methods of getting the commit ID."""
+
+    def setUp(self):
+        self._td = mkdtemp()
+        self.testdir = self._td.name
+        os.chdir(self.testdir)
+        logging.getLogger('git.cmd').setLevel(logging.INFO)
+
+    def tearDown(self):
+        os.chdir(basedir)
+        self._td.cleanup()
+
+    @unittest.skipUnless((basedir.parent / '.git').exists(), 'Needs a working git repo')
+    def test_get_head_commit_id_compare(self):
+        """Run on this git repo to get some real world noise in there."""
+        git_dir = basedir.parent
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_dir))
+
+    def test_get_head_commit_id_error_bare_repo(self):
+        """Error because it is an empty, bare git repo."""
+        git_repo = git.Repo.init(self.testdir)
+        self.assertIsNone(fdroidserver.common.get_head_commit_id(git_repo))
+
+    def test_get_head_commit_id_error_no_repo(self):
+        """Error because there is no .git/ dir."""
+        with self.assertLogs('root', level=logging.DEBUG):
+            self.assertIsNone(fdroidserver.common.get_head_commit_id(self.testdir))
+
+    def test_get_head_commit_id_detached_and_branch(self):
+        """Fetching commit ID must work from detached HEADs and branches."""
+        git_repo = git.Repo.init(self.testdir)
+        Path('README').write_text('this is just a test')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("add README")
+        Path('LICENSE').write_text('free!')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("add LICENSE")
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_repo))
+        # detached HEAD
+        git_repo.git.checkout('HEAD^')
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_repo))
+        # on a branch with a new commits
+        git_repo.git.checkout('test', b=True)
+        Path('foo.py').write_text('print("code!")')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("add code")
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_repo))