Merge ~cjwatson/launchpad-buildd:tighten-url-sanitization into launchpad-buildd:master

[Colin Watson]

Make URL sanitization a little less greedy

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.

[Andrey Fedoseev (andrey-fedoseev)]

Could you add a test for the case when username/password include the special characters (":", "@", or "/")?

review: Approve

[Colin Watson (cjwatson)]

Can you elaborate on why? I'm not sure it makes sense to test that as you describe, because such cases would be out of spec. I'm sure the regex parsing will do something with them, but I'm not sure that what it does will be very interesting.

[Andrey Fedoseev (andrey-fedoseev)]

I might have confused "encoded" with "escaped". I imagined they would become something like "\@" which may break the regex potentially, but it looks like this won't be the case, so never mind.

[Colin Watson (cjwatson)]

Ah yeah, it means URL-encoding. Cool then.

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 <[e-mail]>  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:[e-mail] 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:[e-mail]/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)