Launchpad itself

Merge lp:~mwhudson/launchpad/no-hosted-area-safe-getBzrBranch into lp:launchpad

  1. no-hosted-area-safe-getBzrBranch
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: 10828
Proposed branch: lp:~mwhudson/launchpad/no-hosted-area-safe-getBzrBranch
Merge into: lp:launchpad
Prerequisite: lp:~mwhudson/launchpad/no-hosted-area-fix-reclaim-branch-space
Diff against target: 381 lines (+228/-10)
7 files modified
lib/lp/code/configure.zcml (+1/-0)
lib/lp/code/interfaces/branch.py (+10/-0)
lib/lp/code/model/branch.py (+3/-6)
lib/lp/code/model/tests/test_branch.py (+40/-0)
lib/lp/codehosting/bzrutils.py (+73/-0)
lib/lp/codehosting/tests/test_bzrutils.py (+93/-4)
lib/lp_sitecustomize.py (+8/-0)
To merge this branch: bzr merge lp:~mwhudson/launchpad/no-hosted-area-safe-getBzrBranch
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+23972@code.launchpad.net

Description of the change

This branch adds a way to open a branch 'safely', i.e. without following stacked on pointers or branch references to URLs that are not of a given URL scheme.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/configure.zcml'
2--- lib/lp/code/configure.zcml 2010-04-27 02:19:39 +0000
3+++ lib/lp/code/configure.zcml 2010-04-27 02:19:55 +0000
4@@ -519,6 +519,7 @@
5 getNotificationRecipients
6 getScannerData
7 getPullURL
8+ getBzrBranch
9 requestMirror
10 startMirroring
11 mirrorFailed
12
13=== modified file 'lib/lp/code/interfaces/branch.py'
14--- lib/lp/code/interfaces/branch.py 2010-04-27 02:19:39 +0000
15+++ lib/lp/code/interfaces/branch.py 2010-04-27 02:19:55 +0000
16@@ -1076,6 +1076,16 @@
17 the corresponding BranchRevision rows for this branch.
18 """
19
20+ def getBzrBranch(self):
21+ """Return the BzrBranch for this database Branch.
22+
23+ You can only call this if a server returned by `get_ro_server` or
24+ `get_rw_server` is running.
25+
26+ :raise lp.codehosting.bzrutils.UnsafeUrlSeen: If the branch is stacked
27+ on or a reference to an unacceptable URL.
28+ """
29+
30 def getPullURL():
31 """Return the URL used to pull the branch into the mirror area."""
32
33
34=== modified file 'lib/lp/code/model/branch.py'
35--- lib/lp/code/model/branch.py 2010-04-27 02:19:39 +0000
36+++ lib/lp/code/model/branch.py 2010-04-27 02:19:55 +0000
37@@ -13,7 +13,6 @@
38 from datetime import datetime
39 import os.path
40
41-from bzrlib.branch import Branch as BzrBranch
42 from bzrlib.revision import NULL_REVISION
43 from bzrlib import urlutils
44 import pytz
45@@ -76,6 +75,7 @@
46 from lp.code.interfaces.branchtarget import IBranchTarget
47 from lp.code.interfaces.seriessourcepackagebranch import (
48 IFindOfficialBranchLinks)
49+from lp.codehosting.bzrutils import safe_open
50 from lp.registry.interfaces.person import (
51 validate_person_not_private_membership, validate_public_person)
52 from lp.services.job.interfaces.job import JobStatus
53@@ -466,11 +466,8 @@
54 return 'lp-mirrored:///%s' % self.unique_name
55
56 def getBzrBranch(self):
57- """Return the BzrBranch for this database Branch.
58-
59- This provides the mirrored copy of the branch.
60- """
61- return BzrBranch.open(self.warehouse_url)
62+ """See `IBranch`."""
63+ return safe_open('lp-internal', 'lp-internal:///' + self.unique_name)
64
65 @property
66 def displayname(self):
67
68=== modified file 'lib/lp/code/model/tests/test_branch.py'
69--- lib/lp/code/model/tests/test_branch.py 2010-04-27 02:19:39 +0000
70+++ lib/lp/code/model/tests/test_branch.py 2010-04-27 02:19:55 +0000
71@@ -13,6 +13,8 @@
72 from datetime import datetime, timedelta
73 from unittest import TestLoader
74
75+from bzrlib.bzrdir import BzrDir
76+
77 from pytz import UTC
78
79 from storm.locals import Store
80@@ -67,6 +69,7 @@
81 from lp.code.model.codeimport import CodeImport, CodeImportSet
82 from lp.code.model.codereviewcomment import CodeReviewComment
83 from lp.code.tests.helpers import add_revision_to_branch
84+from lp.codehosting.bzrutils import UnsafeUrlSeen
85 from lp.registry.interfaces.person import IPersonSet
86 from lp.registry.model.product import ProductSet
87 from lp.registry.model.sourcepackage import SourcePackage
88@@ -2360,5 +2363,42 @@
89 self.assertEqual([new, old], branch_revisions)
90
91
92+class TestGetBzrBranch(TestCaseWithFactory):
93+ """Tests for `IBranch.safe_open`."""
94+
95+ layer = DatabaseFunctionalLayer
96+
97+ def setUp(self):
98+ TestCaseWithFactory.setUp(self)
99+ self.useBzrBranches(real_server=True, direct_database=True)
100+
101+ def test_simple(self):
102+ # safe_open returns the underlying bzr branch of a database branch in
103+ # the simple, unstacked, case.
104+ db_branch, tree = self.create_branch_and_tree()
105+ revid = tree.commit('')
106+ bzr_branch = db_branch.getBzrBranch()
107+ self.assertEqual(revid, bzr_branch.last_revision())
108+
109+ def test_acceptable_stacking(self):
110+ # If the underlying bzr branch of a database branch is stacked on
111+ # another launchpad branch safe_open returns it.
112+ db_stacked_on, stacked_on_tree = self.create_branch_and_tree()
113+ db_stacked, stacked_tree = self.create_branch_and_tree()
114+ stacked_tree.branch.set_stacked_on_url(
115+ '/' + db_stacked_on.unique_name)
116+ bzr_branch = db_stacked.getBzrBranch()
117+ self.assertEqual(
118+ '/' + db_stacked_on.unique_name, bzr_branch.get_stacked_on_url())
119+
120+ def test_unacceptable_stacking(self):
121+ # If the underlying bzr branch of a database branch is stacked on
122+ # a non-Launchpad url, it cannot be opened.
123+ branch = BzrDir.create_branch_convenience('local')
124+ db_stacked, stacked_tree = self.create_branch_and_tree()
125+ stacked_tree.branch.set_stacked_on_url(branch.base)
126+ self.assertRaises(UnsafeUrlSeen, db_stacked.getBzrBranch)
127+
128+
129 def test_suite():
130 return TestLoader().loadTestsFromName(__name__)
131
132=== modified file 'lib/lp/codehosting/bzrutils.py'
133--- lib/lp/codehosting/bzrutils.py 2010-01-20 23:10:44 +0000
134+++ lib/lp/codehosting/bzrutils.py 2010-04-27 02:19:55 +0000
135@@ -18,12 +18,17 @@
136 'install_oops_handler',
137 'is_branch_stackable',
138 'remove_exception_logging_hook',
139+ 'safe_open',
140+ 'UnsafeUrlSeen',
141 ]
142
143 import os
144 import sys
145+import threading
146
147 from bzrlib import config, trace
148+from bzrlib.branch import Branch
149+from bzrlib.bzrdir import BzrDir
150 from bzrlib.errors import (
151 NotStacked, UnstackableBranchFormat, UnstackableRepositoryFormat)
152 from bzrlib.remote import RemoteBranch, RemoteBzrDir, RemoteRepository
153@@ -266,3 +271,71 @@
154 """
155 return (get_vfs_format_classes(branch_one) ==
156 get_vfs_format_classes(branch_two))
157+
158+
159+checked_open_data = threading.local()
160+
161+
162+def _install_checked_open_hook():
163+ """Install `_checked_open_pre_open_hook` as a ``pre_open`` hook.
164+
165+ This is done at module import time, but _checked_open_pre_open_hook
166+ doesn't do anything unless the `checked_open_data` threading.Local object
167+ has a 'checked_opener' attribute in this thread.
168+
169+ This is in a module-level function rather than performed at module level
170+ so that it can be called in setUp for testing `checked_open` as
171+ bzrlib.tests.TestCase.setUp clears hooks.
172+ """
173+ BzrDir.hooks.install_named_hook(
174+ 'pre_open', _checked_open_pre_open_hook, 'safe open')
175+
176+
177+def _checked_open_pre_open_hook(transport):
178+ """If a checked_open validate function is present in this thread, call it.
179+ """
180+ if not getattr(checked_open_data, 'validate', False):
181+ return
182+ checked_open_data.validate(transport.base)
183+
184+
185+_install_checked_open_hook()
186+
187+
188+def checked_open(validation_function, url):
189+ """Open a branch, calling `validation_function` with any URL thus found.
190+
191+ This is intended to be used to open a branch ensuring that it's not
192+ stacked or a reference to something unexpected.
193+ """
194+ if hasattr(checked_open_data, 'validate'):
195+ raise AssertionError("checked_open called recursively")
196+ checked_open_data.validate = validation_function
197+ try:
198+ return Branch.open(url)
199+ finally:
200+ del checked_open_data.validate
201+
202+
203+class UnsafeUrlSeen(Exception):
204+ """`safe_open` found a URL that was not on the configured scheme."""
205+
206+
207+def makeURLChecker(allowed_scheme):
208+ """Make a callable that rejects URLs not on the given scheme."""
209+ def checkURL(url):
210+ """Check that `url` is safe to open."""
211+ if URI(url).scheme != allowed_scheme:
212+ raise UnsafeUrlSeen(
213+ "Attempt to open %r which is not a %s URL" % (
214+ url, allowed_scheme))
215+ return checkURL
216+
217+
218+def safe_open(allowed_scheme, url):
219+ """Open the branch at `url`, only accessing URLs on `allowed_scheme`.
220+
221+ :raises UnsafeUrlSeen: An attempt was made to open a URL that was not on
222+ `allowed_scheme`.
223+ """
224+ return checked_open(makeURLChecker(allowed_scheme), url)
225
226=== modified file 'lib/lp/codehosting/tests/test_bzrutils.py'
227--- lib/lp/codehosting/tests/test_bzrutils.py 2010-01-21 22:44:31 +0000
228+++ lib/lp/codehosting/tests/test_bzrutils.py 2010-04-27 02:19:55 +0000
229@@ -8,20 +8,24 @@
230 import gc
231 import sys
232
233+from lazr.uri import URI
234+
235 from bzrlib import errors, trace
236-from bzrlib.branch import Branch
237-from bzrlib.bzrdir import format_registry
238+from bzrlib.branch import Branch, BranchReferenceFormat
239+from bzrlib.bzrdir import BzrDir, format_registry
240 from bzrlib.remote import RemoteBranch
241 from bzrlib.smart import server
242 from bzrlib.tests import (
243 multiply_tests, TestCase, TestCaseWithTransport, TestLoader,
244 TestNotApplicable)
245 from bzrlib.tests.per_branch import TestCaseWithBzrDir, branch_scenarios
246+from bzrlib.transport import chroot
247
248 from lp.codehosting.bzrutils import (
249- add_exception_logging_hook, DenyingServer, get_branch_stacked_on_url,
250+ DenyingServer, UnsafeUrlSeen, _install_checked_open_hook,
251+ add_exception_logging_hook, checked_open, get_branch_stacked_on_url,
252 get_vfs_format_classes, is_branch_stackable,
253- remove_exception_logging_hook)
254+ remove_exception_logging_hook, safe_open)
255 from lp.codehosting.tests.helpers import TestResultWrapper
256
257
258@@ -202,6 +206,89 @@
259 get_vfs_format_classes(remote_branch))
260
261
262+
263+class TestCheckedOpen(TestCaseWithTransport):
264+ """Tests for `checked_open`."""
265+
266+ def setUp(self):
267+ TestCaseWithTransport.setUp(self)
268+ _install_checked_open_hook()
269+
270+ def test_simple(self):
271+ # Opening a branch with checked_open checks the branches url.
272+ url = self.make_branch('branch').base
273+ seen_urls = []
274+ checked_open(seen_urls.append, url)
275+ self.assertEqual([url], seen_urls)
276+
277+ def test_stacked(self):
278+ # Opening a stacked branch with checked_open checks the branches url
279+ # and then the stacked-on url.
280+ stacked = self.make_branch('stacked')
281+ stacked_on = self.make_branch('stacked_on')
282+ stacked.set_stacked_on_url(stacked_on.base)
283+ seen_urls = []
284+ checked_open(seen_urls.append, stacked.base)
285+ self.assertEqual([stacked.base, stacked_on.base], seen_urls)
286+
287+ def test_reference(self):
288+ # Opening a branch reference with checked_open checks the branch
289+ # references url and then the target of the reference.
290+ target = self.make_branch('target')
291+ reference_url = self.get_url('reference/')
292+ BranchReferenceFormat().initialize(
293+ BzrDir.create(reference_url), target_branch=target)
294+ seen_urls = []
295+ checked_open(seen_urls.append, reference_url)
296+ self.assertEqual([reference_url, target.base], seen_urls)
297+
298+
299+class TestSafeOpen(TestCaseWithTransport):
300+ """Tests for `safe_open`."""
301+
302+ def setUp(self):
303+ TestCaseWithTransport.setUp(self)
304+ _install_checked_open_hook()
305+
306+ def get_chrooted_scheme(self, relpath):
307+ """Create a server that is chrooted to `relpath`.
308+
309+ :return: ``(scheme, get_url)`` where ``scheme`` is the scheme of the
310+ chroot server and ``get_url`` returns URLs on said server.
311+ """
312+ transport = self.get_transport(relpath)
313+ chroot_server = chroot.ChrootServer(transport)
314+ chroot_server.start_server()
315+ self.addCleanup(chroot_server.stop_server)
316+ def get_url(relpath):
317+ return chroot_server.get_url() + relpath
318+ return URI(chroot_server.get_url()).scheme, get_url
319+
320+ def test_stacked_within_scheme(self):
321+ # A branch that is stacked on a URL of the same scheme is safe to
322+ # open.
323+ self.get_transport().mkdir('inside')
324+ self.make_branch('inside/stacked')
325+ self.make_branch('inside/stacked-on')
326+ scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
327+ Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
328+ get_chrooted_url('stacked-on'))
329+ safe_open(scheme, get_chrooted_url('stacked'))
330+
331+ def test_stacked_outside_scheme(self):
332+ # A branch that is stacked on a URL that is not of the same scheme is
333+ # not safe to open.
334+ self.get_transport().mkdir('inside')
335+ self.get_transport().mkdir('outside')
336+ self.make_branch('inside/stacked')
337+ self.make_branch('outside/stacked-on')
338+ scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
339+ Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
340+ self.get_url('outside/stacked-on'))
341+ self.assertRaises(
342+ UnsafeUrlSeen, safe_open, scheme, get_chrooted_url('stacked'))
343+
344+
345 def load_tests(basic_tests, module, loader):
346 """Parametrize the tests of get_branch_stacked_on_url by branch format."""
347 result = loader.suiteClass()
348@@ -216,6 +303,8 @@
349 result.addTests(loader.loadTestsFromTestCase(TestDenyingServer))
350 result.addTests(loader.loadTestsFromTestCase(TestExceptionLoggingHooks))
351 result.addTests(loader.loadTestsFromTestCase(TestGetVfsFormatClasses))
352+ result.addTests(loader.loadTestsFromTestCase(TestSafeOpen))
353+ result.addTests(loader.loadTestsFromTestCase(TestCheckedOpen))
354 return result
355
356
357
358=== modified file 'lib/lp_sitecustomize.py'
359--- lib/lp_sitecustomize.py 2010-02-24 16:54:03 +0000
360+++ lib/lp_sitecustomize.py 2010-04-27 02:19:55 +0000
361@@ -6,6 +6,13 @@
362
363 import os
364 from lp.services.mime import customizeMimetypes
365+from zope.security import checker
366+from bzrlib.branch import Branch
367+
368+def dont_wrap_class_and_subclasses(cls):
369+ checker.BasicTypes.update({cls: checker.NoProxy})
370+ for subcls in cls.__subclasses__():
371+ dont_wrap_class_and_subclasses(subcls)
372
373 def main():
374 # Note that we configure the LPCONFIG environmental variable in the
375@@ -18,5 +25,6 @@
376 # initialization as possible here, in a more visible place.
377 os.environ['STORM_CEXTENSIONS'] = '1'
378 customizeMimetypes()
379+ dont_wrap_class_and_subclasses(Branch)
380
381 main()