launchpad-reviewers team mailing list archive

[Colin Watson <mp+427515@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad-buildd:tighten-url-sanitization into launchpad-buildd:master.

Commit message:
Make URL sanitization a little less greedy

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/427515

It could previously be confused by multiple URLs on the same line, resulting in very confusing output in build logs.  This change is safe because RFC 1738 says:

  Within the user and password field, any ":", "@", or "/" must be encoded.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:tighten-url-sanitization into launchpad-buildd:master.

diff --git a/debian/changelog b/debian/changelog
index 7824911..c3e6e11 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,7 @@
 launchpad-buildd (217) UNRELEASED; urgency=medium
 
   * Improve deployment documentation.
+  * Make URL sanitization a little less greedy.
 
  -- Colin Watson <cjwatson@xxxxxxxxxx>  Mon, 18 Jul 2022 15:07:23 +0100
 
diff --git a/lpbuildd/builder.py b/lpbuildd/builder.py
index 61357cf..336594c 100644
--- a/lpbuildd/builder.py
+++ b/lpbuildd/builder.py
@@ -47,7 +47,7 @@ def _sanitizeURLs(bytes_seq):
     """
     # This regular expression will be used to remove authentication
     # credentials from URLs.
-    password_re = re.compile(br'://([^:]*:[^@]+@)(\S+)')
+    password_re = re.compile(br'://([^:@/]*:[^:@/]+@)(\S+)')
     # Builder proxy passwords are UUIDs.
     proxy_auth_re = re.compile(br',proxyauth=[^:]+:[A-Za-z0-9-]+')
 
diff --git a/lpbuildd/tests/test_builder.py b/lpbuildd/tests/test_builder.py
index db13dd8..1f08126 100644
--- a/lpbuildd/tests/test_builder.py
+++ b/lpbuildd/tests/test_builder.py
@@ -23,10 +23,56 @@ from twisted.logger import (
 from lpbuildd.builder import (
     Builder,
     BuildManager,
+    _sanitizeURLs,
     )
 from lpbuildd.tests.fakebuilder import FakeConfig
 
 
+class TestSanitizeURLs(TestCase):
+    """Unit-test URL sanitization.
+
+    `lpbuildd.tests.test_buildd.LaunchpadBuilddTests` also covers some of
+    this, but at a higher level.
+    """
+
+    def test_non_urls(self):
+        lines = [b"not a URL", b"still not a URL"]
+        self.assertEqual(lines, list(_sanitizeURLs(lines)))
+
+    def test_url_without_credentials(self):
+        lines = [b"Get:1 http://ftpmaster.internal focal InRelease"]
+        self.assertEqual(lines, list(_sanitizeURLs(lines)))
+
+    def test_url_with_credentials(self):
+        lines = [
+            b"Get:1 http://buildd:secret@ftpmaster.internal focal InRelease",
+        ]
+        expected_lines = [b"Get:1 http://ftpmaster.internal focal InRelease"]
+        self.assertEqual(expected_lines, list(_sanitizeURLs(lines)))
+
+    def test_multiple_urls(self):
+        lines = [
+            b"http_proxy=http://squid.internal:3128/ "
+            b"GOPROXY=http://user:password@xxxxxxxxxxx/goproxy";,
+        ]
+        expected_lines = [
+            b"http_proxy=http://squid.internal:3128/ "
+            b"GOPROXY=http://example.com/goproxy";,
+        ]
+        self.assertEqual(expected_lines, list(_sanitizeURLs(lines)))
+
+    def test_proxyauth(self):
+        lines = [
+            b"socat STDIO PROXY:builder-proxy.launchpad.dev:github.com:443,"
+            b"proxyport=3128,proxyauth=user:blah",
+        ]
+        expected_lines = [
+            b"socat STDIO PROXY:builder-proxy.launchpad.dev:github.com:443,"
+            b"proxyport=3128",
+        ]
+        self.assertEqual(expected_lines, list(_sanitizeURLs(lines)))
+
+
 class TestBuildManager(TestCase):
 
     run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)