Merge ~cjwatson/launchpad:py3-restricted-codehosting-session into launchpad:master

Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: 6eea481b643548f77da39cf87be4d0e39b7d9c3a
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:py3-restricted-codehosting-session
Merge into: launchpad:master
Diff against target: 189 lines (+30/-28)
2 files modified
lib/lp/codehosting/sshserver/session.py (+4/-3)
lib/lp/codehosting/sshserver/tests/test_session.py (+26/-25)
Reviewer Review Type Date Requested Status
Cristian Gonzalez (community) Approve
Review via email: mp+397325@code.launchpad.net

Commit message

Fix restricted codehosting session for Python 3

Description of the change

The command to execute is passed to us as bytes, not text.

To post a comment you must log in.
Revision history for this message
Cristian Gonzalez (cristiangsp) wrote :

Looks good!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/codehosting/sshserver/session.py b/lib/lp/codehosting/sshserver/session.py
2index 482dd4f..28a0a69 100644
3--- a/lib/lp/codehosting/sshserver/session.py
4+++ b/lib/lp/codehosting/sshserver/session.py
5@@ -14,6 +14,7 @@ import os
6
7 from lazr.sshserver.events import AvatarEvent
8 from lazr.sshserver.session import DoNothingSession
9+import six
10 from six.moves.urllib.parse import urlparse
11 from twisted.internet import process
12 from twisted.python import log
13@@ -107,7 +108,7 @@ class ExecOnlySession(DoNothingSession):
14 executable and arguments is a sequence of command-line arguments
15 with the name of the executable as the first value.
16 """
17- args = command.split()
18+ args = six.ensure_binary(command).split()
19 return args[0], args
20
21
22@@ -161,8 +162,8 @@ def lookup_command_template(command):
23 command_template = python_command + args
24
25 if command in (
26- 'bzr serve --inet --directory=/ --allow-writes',
27- 'brz serve --inet --directory=/ --allow-writes'):
28+ b'bzr serve --inet --directory=/ --allow-writes',
29+ b'brz serve --inet --directory=/ --allow-writes'):
30 return command_template
31 # At the moment, only bzr/brz branch serving is allowed.
32 raise ForbiddenCommand("Not allowed to execute %r." % (command,))
33diff --git a/lib/lp/codehosting/sshserver/tests/test_session.py b/lib/lp/codehosting/sshserver/tests/test_session.py
34index fb07cea..305c65b 100644
35--- a/lib/lp/codehosting/sshserver/tests/test_session.py
36+++ b/lib/lp/codehosting/sshserver/tests/test_session.py
37@@ -77,9 +77,9 @@ class MockProcessTransport:
38 self.log.append(('childConnectionLost', childFD, reason))
39
40 def signalProcess(self, signal):
41- if self._executable == 'raise-os-error':
42+ if self._executable == b'raise-os-error':
43 raise OSError()
44- if self._executable == 'already-terminated':
45+ if self._executable == b'already-terminated':
46 raise ProcessExitedAlready()
47 self.log.append(('signalProcess', signal))
48
49@@ -116,7 +116,7 @@ class TestExecOnlySession(AvatarTestCase):
50
51 def test_openShellNotImplemented(self):
52 # openShell closes the connection.
53- protocol = MockProcessTransport('bash')
54+ protocol = MockProcessTransport(b'bash')
55 self.session.openShell(protocol)
56 self.assertEqual(
57 [('writeExtended', connection.EXTENDED_DATA_STDERR,
58@@ -148,7 +148,7 @@ class TestExecOnlySession(AvatarTestCase):
59 # inside, it tells the process transport to end the connection between
60 # the SSH server and the child process.
61 protocol = ProcessProtocol()
62- self.session.execCommand(protocol, 'cat /etc/hostname')
63+ self.session.execCommand(protocol, b'cat /etc/hostname')
64 self.session.closed()
65 self.assertEqual(
66 [('signalProcess', 'HUP'), ('loseConnection',)],
67@@ -160,7 +160,7 @@ class TestExecOnlySession(AvatarTestCase):
68 protocol = ProcessProtocol()
69 # MockTransport will raise an OSError on signalProcess if the executed
70 # command is 'raise-os-error'.
71- self.session.execCommand(protocol, 'raise-os-error')
72+ self.session.execCommand(protocol, b'raise-os-error')
73 self.session.closed()
74 self.assertEqual(
75 [('loseConnection',)],
76@@ -172,22 +172,22 @@ class TestExecOnlySession(AvatarTestCase):
77 protocol = ProcessProtocol()
78 # MockTransport will raise a ProcessExitedAlready on signalProcess if
79 # the executed command is 'already-terminated'.
80- self.session.execCommand(protocol, 'already-terminated')
81+ self.session.execCommand(protocol, b'already-terminated')
82 self.session.closed()
83 self.assertEqual([('loseConnection',)], self.session._transport.log)
84
85 def test_getCommandToRunSplitsCommandLine(self):
86 # getCommandToRun takes a command line and splits it into the name of
87 # an executable to run and a sequence of arguments.
88- command = 'cat foo bar'
89+ command = b'cat foo bar'
90 executable, arguments = self.session.getCommandToRun(command)
91- self.assertEqual('cat', executable)
92- self.assertEqual(['cat', 'foo', 'bar'], list(arguments))
93+ self.assertEqual(b'cat', executable)
94+ self.assertEqual([b'cat', b'foo', b'bar'], list(arguments))
95
96 def test_execCommandSpawnsProcess(self):
97 # ExecOnlySession.execCommand spawns the appropriate process.
98 protocol = ProcessProtocol()
99- command = 'cat /etc/hostname'
100+ command = b'cat /etc/hostname'
101 self.session.execCommand(protocol, command)
102 executable, arguments = self.session.getCommandToRun(command)
103 self.assertEqual([(protocol, executable, arguments, None, None,
104@@ -204,7 +204,7 @@ class TestExecOnlySession(AvatarTestCase):
105 # 'eofReceived' closes standard input when called while a command is
106 # running.
107 protocol = ProcessProtocol()
108- self.session.execCommand(protocol, 'cat /etc/hostname')
109+ self.session.execCommand(protocol, b'cat /etc/hostname')
110 self.session.eofReceived()
111 self.assertEqual([('closeStdin',)], self.session._transport.log)
112
113@@ -227,10 +227,10 @@ class TestExecOnlySession(AvatarTestCase):
114 session = ExecOnlySession(
115 self.avatar, self.reactor, environment={'FOO': 'BAR'})
116 protocol = ProcessProtocol()
117- session.execCommand(protocol, 'yes')
118+ session.execCommand(protocol, b'yes')
119 self.assertEqual({'FOO': 'BAR'}, session.environment)
120 self.assertEqual(
121- [(protocol, 'yes', ['yes'], {'FOO': 'BAR'}, None, None, None, 0,
122+ [(protocol, b'yes', [b'yes'], {'FOO': 'BAR'}, None, None, None, 0,
123 None)],
124 self.reactor.log)
125
126@@ -289,12 +289,12 @@ class TestRestrictedExecOnlySession(AvatarTestCase):
127 # Note that Conch doesn't have a well-defined way of rejecting
128 # commands. Disconnecting in execCommand will do. We don't raise
129 # an exception to avoid logging an OOPS.
130- protocol = MockProcessTransport('cat')
131+ protocol = MockProcessTransport(b'cat')
132 self.assertEqual(
133- None, self.session.execCommand(protocol, 'cat'))
134+ None, self.session.execCommand(protocol, b'cat'))
135 self.assertEqual(
136 [('writeExtended', connection.EXTENDED_DATA_STDERR,
137- "Not allowed to execute 'cat'.\r\n"),
138+ "Not allowed to execute %r.\r\n" % b'cat'),
139 ('loseConnection',)],
140 protocol.log)
141
142@@ -303,9 +303,10 @@ class TestRestrictedExecOnlySession(AvatarTestCase):
143 # executable and arguments corresponding to the provided executed
144 # command template.
145 executable, arguments = self.session.getCommandToRun('foo')
146- self.assertEqual('bar', executable)
147+ self.assertEqual(b'bar', executable)
148 self.assertEqual(
149- ['bar', 'baz', str(self.avatar.user_id)], list(arguments))
150+ [b'bar', b'baz', str(self.avatar.user_id).encode('UTF-8')],
151+ list(arguments))
152
153 def test_getAvatarAdapter(self):
154 # getAvatarAdapter is a convenience classmethod so that
155@@ -357,15 +358,15 @@ class TestSessionIntegration(AvatarTestCase):
156 session.environment['BRZ_EMAIL'])
157
158 executable, arguments = session.getCommandToRun(
159- 'bzr serve --inet --directory=/ --allow-writes')
160- interpreter = '%s/bin/py' % config.root
161+ b'bzr serve --inet --directory=/ --allow-writes')
162+ interpreter = ('%s/bin/py' % config.root).encode('UTF-8')
163 self.assertEqual(interpreter, executable)
164 self.assertEqual(
165- [interpreter, get_brz_path(), 'lp-serve',
166- '--inet', str(self.avatar.user_id)],
167+ [interpreter, get_brz_path().encode('UTF-8'), b'lp-serve',
168+ b'--inet', str(self.avatar.user_id).encode('UTF-8')],
169 list(arguments))
170 self.assertRaises(
171- ForbiddenCommand, session.getCommandToRun, 'rm -rf /')
172+ ForbiddenCommand, session.getCommandToRun, b'rm -rf /')
173
174
175 class TestLookupCommand(TestCase):
176@@ -378,11 +379,11 @@ class TestLookupCommand(TestCase):
177 config.root + '/bin/py ' + get_brz_path() +
178 ' lp-serve --inet %(user_id)s',
179 lookup_command_template(
180- 'bzr serve --inet --directory=/ --allow-writes'))
181+ b'bzr serve --inet --directory=/ --allow-writes'))
182
183 def test_brz(self):
184 self.assertEqual(
185 config.root + '/bin/py ' + get_brz_path() +
186 ' lp-serve --inet %(user_id)s',
187 lookup_command_template(
188- 'brz serve --inet --directory=/ --allow-writes'))
189+ b'brz serve --inet --directory=/ --allow-writes'))

Subscribers

People subscribed via source and target branches

to status/vote changes: