launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+490604@xxxxxxxxxxxxxxxxxx>]

Simone Pelosi has proposed merging ~pelpsi/launchpad-buildd:remove-assert-usage into launchpad-buildd:master.

Commit message:
Remove assert usage and log try-catch

This is to improve the security posture of the launchpad-buildd
project. We fixed all the warnings from ruff security checks.

The ones suppressed are intentional.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pelpsi/launchpad-buildd/+git/launchpad-buildd/+merge/490604
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad-buildd:remove-assert-usage into launchpad-buildd:master.

diff --git a/debian/changelog b/debian/changelog
index dd16d81..358617a 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,8 @@
 launchpad-buildd (257) UNRELEASED; urgency=medium
 
-  * Use requests Basic Auth for security. 
+  * Use requests Basic Auth for security.
+  * Remove `assert` usage.
+  * Log `try-catch`.
 
  -- Simone Pelosi <simone.pelosi@xxxxxxxxxxxxx>  Wed, 23 Jul 2025 15:42:34 +0200
 
diff --git a/lpbuildd/target/lxd.py b/lpbuildd/target/lxd.py
index ab35590..8bdbb39 100644
--- a/lpbuildd/target/lxd.py
+++ b/lpbuildd/target/lxd.py
@@ -3,6 +3,7 @@
 
 import io
 import json
+import logging
 import os
 import platform
 import re
@@ -389,8 +390,8 @@ class LXD(Backend):
             with open(self.dnsmasq_pid_file) as f:
                 try:
                     dnsmasq_pid = int(f.read())
-                except Exception:
-                    pass
+                except Exception as e:
+                    logging.warning("Failed to read dnsmasq PID file: %s", e)
                 else:
                     # dnsmasq is supposed to drop privileges, but kill it as
                     # root just in case it fails to do so for some reason.
diff --git a/lpbuildd/target/vcs.py b/lpbuildd/target/vcs.py
index 2f79225..e287c87 100644
--- a/lpbuildd/target/vcs.py
+++ b/lpbuildd/target/vcs.py
@@ -45,7 +45,10 @@ class VCSOperationMixin(StatusOperationMixin):
         if self.args.branch is not None:
             return self.args.branch
         else:
-            assert self.args.git_repository is not None
+            if self.args.git_repository is None:
+                raise ValueError(
+                    "git_repository must be provided when branch is None"
+                )
             description = self.args.git_repository
             if self.args.git_path is not None:
                 description += " " + self.args.git_path
@@ -85,7 +88,10 @@ class VCSOperationMixin(StatusOperationMixin):
                 cmd.insert(1, "-Ossl.cert_reqs=none")
         else:
             # this handles the git case
-            assert self.args.git_repository is not None
+            if self.args.git_repository is None:
+                raise ValueError(
+                    "git_repository must be provided for git operations"
+                )
             cmd = ["git", "clone", "-n"]
             if quiet:
                 cmd.append("-q")
diff --git a/lpbuildd/util.py b/lpbuildd/util.py
index 9231206..ce483f4 100644
--- a/lpbuildd/util.py
+++ b/lpbuildd/util.py
@@ -1,7 +1,6 @@
 # Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-import base64
 import os
 import subprocess
 import sys
@@ -48,7 +47,8 @@ def get_arch_bits(arch):
 
 def set_personality(args, arch, series=None):
     bits = get_arch_bits(arch)
-    assert bits in (32, 64)
+    if bits not in (32, 64):
+        raise ValueError(f"Unsupported architecture bit size: {bits}")
     if bits == 32:
         setarch_cmd = ["linux32"]
     else:
diff --git a/pyproject.toml b/pyproject.toml
index 3168b1e..2bfbbab 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,19 @@
 [tool.black]
 line-length = 79
-target-version = ['py38']
+target-version = ["py38"]
 
 [tool.isort]
 line_length = 79
+
+[tool.ruff.lint]
+# Enable all security rules (SXXX = Bandit checks)
+select = ["S"]
+
+# Ignore specific security checks that are intentionally allowed
+ignore = [
+    "S113", # request timeout - intention
+    "S324", # SHA1 usage - legacy
+    "S603", # subprocess call - intentional
+    "S606",  # os.execv without shell - intentional
+    "S607", # partial executable paths - intentional
+]