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 <[email protected]> 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:[email protected] 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:[email protected]/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)
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
