launchpad-reviewers team mailing list archive

[Ioana Lasc <ioana.lasc@xxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/turnip:bug-1870080 into turnip:master.

Commit message:
Add version to the set of safe params

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilasc/turnip/+git/turnip/+merge/381720

After parsing the extra params after 2 NUL bytes for git clients with
version 2 we need to allow the version param to pass through the 
Git pack protocol conversion proxy (PackFrontendServerProtocol).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/turnip:bug-1870080 into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index 0aa46c3..28bc2da 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -36,7 +36,7 @@ from turnip.pack.helpers import (
 ERROR_PREFIX = b'ERR '
 VIRT_ERROR_PREFIX = b'turnip virt error: '
 
-SAFE_PARAMS = frozenset(['host'])
+SAFE_PARAMS = frozenset(['host', 'version'])
 
 
 class RequestIDLogger(Logger):
diff --git a/turnip/pack/tests/test_git.py b/turnip/pack/tests/test_git.py
index 802f031..98ece7c 100644
--- a/turnip/pack/tests/test_git.py
+++ b/turnip/pack/tests/test_git.py
@@ -118,6 +118,53 @@ class DummyPackBackendProtocol(git.PackBackendProtocol):
         self.test_process = (cmd, args, env)
 
 
+class TestPackFrontendServerProtocol(TestCase):
+
+    def setUp(self):
+        super(TestPackFrontendServerProtocol, self).setUp()
+        self.factory = git.PackFrontendFactory('example.com', 12345)
+        self.proto = git.PackFrontendServerProtocol()
+        self.proto.factory = self.factory
+        self.transport = testing.StringTransportWithDisconnection()
+        self.transport.protocol = self.proto
+        self.proto.makeConnection(self.transport)
+
+    def assertKilledWith(self, message):
+        self.assertFalse(self.transport.connected)
+        self.assertEqual(
+            (b'ERR ' + message + b'\n', b''),
+            helpers.decode_packet(self.transport.value()))
+
+    def test_git_receive(self):
+        self.proto.pauseProducing()
+        self.proto.got_request = True
+        yield self.proto.requestReceived(
+            b'git-upload-pack', b'/foo.git', {b'host': b'example.com'})
+        self.assertEqual(b'git-upload-pack', self.proto.command)
+        self.transport.loseConnection()
+
+    def test_git_receive_with_version_param(self):
+        self.proto.pauseProducing()
+        self.proto.got_request = True
+        yield self.proto.requestReceived(
+            b'git-upload-pack', b'/test_repo',
+            {b'host': 'example.com', 'version': '2'}),
+        self.assertIn(b'host', self.proto.params)
+        self.assertIn('version', self.proto.params)
+        self.transport.loseConnection()
+
+    def test_git_receive_with_extra_undefined_params(self):
+        self.proto.pauseProducing()
+        self.proto.got_request = True
+        yield self.proto.requestReceived(
+            b'git-upload-pack', b'/test_repo',
+            {b'host': 'example.com', 'version': '2',
+             'undefined_param': 'value'}),
+        self.assertIsNone(self.proto.params)
+        self.assertKilledWith(b'Illegal request parameters')
+        self.transport.loseConnection()
+
+
 class TestPackBackendProtocol(TestCase):
     """Test the Git pack backend protocol."""