Merge lp:~jelmer/launchpad/820453-safe-open-dup into lp:launchpad

Proposed by Jelmer Vernooij
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merged at revision: 13652
Proposed branch: lp:~jelmer/launchpad/820453-safe-open-dup
Merge into: lp:launchpad
Diff against target: 466 lines (+90/-179)
9 files modified
lib/launchpad_loggerhead/app.py (+1/-1)
lib/lp/code/interfaces/branch.py (+1/-1)
lib/lp/code/model/branch.py (+1/-1)
lib/lp/code/model/tests/test_branch.py (+3/-3)
lib/lp/codehosting/bzrutils.py (+0/-74)
lib/lp/codehosting/puller/tests/test_errors.py (+5/-2)
lib/lp/codehosting/safe_open.py (+30/-0)
lib/lp/codehosting/tests/test_bzrutils.py (+0/-92)
lib/lp/codehosting/tests/test_safe_open.py (+49/-5)
To merge this branch: bzr merge lp:~jelmer/launchpad/820453-safe-open-dup
Reviewer Review Type Date Requested Status
Steve Kowalik (community) code Approve
Review via email: mp+70901@code.launchpad.net

This proposal supersedes a proposal from 2011-08-09.

Commit message

[r=stevenk][bug=820453] Make lp.codehosting.bzrutils.safe_open a trivial wrapper around SafeBranchOpener rather than duplicating the code for safely opening a branch in two places.

Description of the change

Make lp.codehosting.bzrutils.safe_open a trivial wrapper around SafeBranchOpener rather than duplicating the code for safely opening a branch in two places.

They both do basically the same thing, SafeBranchOpener is a bit more advanced and was recently extracted from BranchMirrorer.

To post a comment you must log in.
Revision history for this message
Steve Kowalik (stevenk) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/launchpad_loggerhead/app.py'
2--- lib/launchpad_loggerhead/app.py 2011-03-10 18:27:08 +0000
3+++ lib/launchpad_loggerhead/app.py 2011-08-09 15:14:56 +0000
4@@ -32,7 +32,7 @@
5 from lp.code.interfaces.codehosting import (
6 BRANCH_TRANSPORT, LAUNCHPAD_ANONYMOUS)
7 from lp.codehosting.vfs import get_lp_server
8-from lp.codehosting.bzrutils import safe_open
9+from lp.codehosting.safe_open import safe_open
10
11 robots_txt = '''\
12 User-agent: *
13
14=== modified file 'lib/lp/code/interfaces/branch.py'
15--- lib/lp/code/interfaces/branch.py 2011-07-11 03:58:41 +0000
16+++ lib/lp/code/interfaces/branch.py 2011-08-09 15:14:56 +0000
17@@ -911,7 +911,7 @@
18 You can only call this if a server returned by `get_ro_server` or
19 `get_rw_server` is running.
20
21- :raise lp.codehosting.bzrutils.UnsafeUrlSeen: If the branch is stacked
22+ :raise lp.codehosting.safe_open.BadUrl: If the branch is stacked
23 on or a reference to an unacceptable URL.
24 """
25
26
27=== modified file 'lib/lp/code/model/branch.py'
28--- lib/lp/code/model/branch.py 2011-06-24 18:08:18 +0000
29+++ lib/lp/code/model/branch.py 2011-08-09 15:14:56 +0000
30@@ -136,7 +136,7 @@
31 RevisionAuthor,
32 )
33 from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
34-from lp.codehosting.bzrutils import safe_open
35+from lp.codehosting.safe_open import safe_open
36 from lp.registry.interfaces.person import (
37 validate_person,
38 validate_public_person,
39
40=== modified file 'lib/lp/code/model/tests/test_branch.py'
41--- lib/lp/code/model/tests/test_branch.py 2011-07-11 03:58:41 +0000
42+++ lib/lp/code/model/tests/test_branch.py 2011-08-09 15:14:56 +0000
43@@ -108,7 +108,7 @@
44 from lp.code.model.codereviewcomment import CodeReviewComment
45 from lp.code.model.revision import Revision
46 from lp.code.tests.helpers import add_revision_to_branch
47-from lp.codehosting.bzrutils import UnsafeUrlSeen
48+from lp.codehosting.safe_open import BadUrl
49 from lp.registry.interfaces.pocket import PackagePublishingPocket
50 from lp.registry.model.sourcepackage import SourcePackage
51 from lp.services.osutils import override_environ
52@@ -2686,7 +2686,7 @@
53
54
55 class TestGetBzrBranch(TestCaseWithFactory):
56- """Tests for `IBranch.safe_open`."""
57+ """Tests for `IBranch.getBzrBranch`."""
58
59 layer = DatabaseFunctionalLayer
60
61@@ -2722,7 +2722,7 @@
62 branch = BzrDir.create_branch_convenience('local')
63 db_stacked, stacked_tree = self.create_branch_and_tree()
64 stacked_tree.branch.set_stacked_on_url(branch.base)
65- self.assertRaises(UnsafeUrlSeen, db_stacked.getBzrBranch)
66+ self.assertRaises(BadUrl, db_stacked.getBzrBranch)
67
68
69 class TestMergeQueue(TestCaseWithFactory):
70
71=== modified file 'lib/lp/codehosting/bzrutils.py'
72--- lib/lp/codehosting/bzrutils.py 2011-05-25 19:57:07 +0000
73+++ lib/lp/codehosting/bzrutils.py 2011-08-09 15:14:56 +0000
74@@ -22,20 +22,16 @@
75 'read_locked',
76 'remove_exception_logging_hook',
77 'safe_open',
78- 'UnsafeUrlSeen',
79 ]
80
81 from contextlib import contextmanager
82 import os
83 import sys
84-import threading
85
86 from bzrlib import (
87 config,
88 trace,
89 )
90-from bzrlib.branch import Branch
91-from bzrlib.bzrdir import BzrDir
92 from bzrlib.errors import (
93 NotStacked,
94 UnstackableBranchFormat,
95@@ -292,76 +288,6 @@
96 get_vfs_format_classes(branch_two))
97
98
99-checked_open_data = threading.local()
100-
101-
102-def _install_checked_open_hook():
103- """Install `_checked_open_pre_open_hook` as a ``pre_open`` hook.
104-
105- This is done at module import time, but _checked_open_pre_open_hook
106- doesn't do anything unless the `checked_open_data` threading.Local object
107- has a 'checked_opener' attribute in this thread.
108-
109- This is in a module-level function rather than performed at module level
110- so that it can be called in setUp for testing `checked_open` as
111- bzrlib.tests.TestCase.setUp clears hooks.
112- """
113- BzrDir.hooks.install_named_hook(
114- 'pre_open', _checked_open_pre_open_hook, 'safe open')
115-
116-
117-def _checked_open_pre_open_hook(transport):
118- """If a checked_open validate function is present in this thread, call it.
119- """
120- if not getattr(checked_open_data, 'validate', False):
121- return
122- checked_open_data.validate(transport.base)
123-
124-
125-_install_checked_open_hook()
126-
127-
128-def checked_open(validation_function, url, possible_transports=None):
129- """Open a branch, calling `validation_function` with any URL thus found.
130-
131- This is intended to be used to open a branch ensuring that it's not
132- stacked or a reference to something unexpected.
133- """
134- if hasattr(checked_open_data, 'validate'):
135- raise AssertionError("checked_open called recursively")
136- checked_open_data.validate = validation_function
137- try:
138- return Branch.open(url, possible_transports=possible_transports)
139- finally:
140- del checked_open_data.validate
141-
142-
143-class UnsafeUrlSeen(Exception):
144- """`safe_open` found a URL that was not on the configured scheme."""
145-
146-
147-def makeURLChecker(allowed_scheme):
148- """Make a callable that rejects URLs not on the given scheme."""
149-
150- def checkURL(url):
151- """Check that `url` is safe to open."""
152- if URI(url).scheme != allowed_scheme:
153- raise UnsafeUrlSeen(
154- "Attempt to open %r which is not a %s URL" % (
155- url, allowed_scheme))
156- return checkURL
157-
158-
159-def safe_open(allowed_scheme, url, possible_transports=None):
160- """Open the branch at `url`, only accessing URLs on `allowed_scheme`.
161-
162- :raises UnsafeUrlSeen: An attempt was made to open a URL that was not on
163- `allowed_scheme`.
164- """
165- return checked_open(
166- makeURLChecker(allowed_scheme), url, possible_transports)
167-
168-
169 def get_stacked_on_url(branch):
170 """Get the stacked-on URL for 'branch', or `None` if not stacked."""
171 try:
172
173=== modified file 'lib/lp/codehosting/puller/tests/test_errors.py'
174--- lib/lp/codehosting/puller/tests/test_errors.py 2011-08-06 17:37:47 +0000
175+++ lib/lp/codehosting/puller/tests/test_errors.py 2011-08-09 15:14:56 +0000
176@@ -26,13 +26,16 @@
177 BadUrlLaunchpad,
178 BadUrlScheme,
179 BadUrlSsh,
180- BranchLoopError,
181 BranchMirrorer,
182- BranchReferenceForbidden,
183 PullerWorker,
184 PullerWorkerProtocol,
185 )
186
187+from lp.codehosting.safe_open import (
188+ BranchLoopError,
189+ BranchReferenceForbidden,
190+ )
191+
192
193 class StubbedPullerWorkerProtocol(PullerWorkerProtocol):
194 """A `PullerWorkerProtocol` that logs events without acting on them."""
195
196=== modified file 'lib/lp/codehosting/safe_open.py'
197--- lib/lp/codehosting/safe_open.py 2011-08-06 18:45:40 +0000
198+++ lib/lp/codehosting/safe_open.py 2011-08-09 15:14:56 +0000
199@@ -9,6 +9,8 @@
200 from bzrlib.branch import Branch
201 from bzrlib.bzrdir import BzrDir
202
203+from lazr.uri import URI
204+
205 __all__ = [
206 'AcceptAnythingPolicy',
207 'BadUrl',
208@@ -18,6 +20,7 @@
209 'BranchReferenceForbidden',
210 'SafeBranchOpener',
211 'WhitelistPolicy',
212+ 'safe_open',
213 ]
214
215
216@@ -231,3 +234,30 @@
217 url = self.checkAndFollowBranchReference(url)
218 return self.runWithTransformFallbackLocationHookInstalled(
219 Branch.open, url)
220+
221+
222+class URLChecker(BranchOpenPolicy):
223+ """Branch open policy that rejects URLs not on the given scheme."""
224+
225+ def __init__(self, allowed_scheme):
226+ self.allowed_scheme = allowed_scheme
227+
228+ def shouldFollowReferences(self):
229+ return True
230+
231+ def transformFallbackLocation(self, branch, url):
232+ return urlutils.join(branch.base, url), True
233+
234+ def checkOneURL(self, url):
235+ """Check that `url` is safe to open."""
236+ if URI(url).scheme != self.allowed_scheme:
237+ raise BadUrl(url)
238+
239+
240+def safe_open(allowed_scheme, url):
241+ """Open the branch at `url`, only accessing URLs on `allowed_scheme`.
242+
243+ :raises BadUrl: An attempt was made to open a URL that was not on
244+ `allowed_scheme`.
245+ """
246+ return SafeBranchOpener(URLChecker(allowed_scheme)).open(url)
247
248=== modified file 'lib/lp/codehosting/tests/test_bzrutils.py'
249--- lib/lp/codehosting/tests/test_bzrutils.py 2011-05-19 13:31:09 +0000
250+++ lib/lp/codehosting/tests/test_bzrutils.py 2011-08-09 15:14:56 +0000
251@@ -14,10 +14,8 @@
252 )
253 from bzrlib.branch import (
254 Branch,
255- BranchReferenceFormat,
256 )
257 from bzrlib.bzrdir import (
258- BzrDir,
259 format_registry,
260 )
261 from bzrlib.remote import RemoteBranch
262@@ -33,20 +31,14 @@
263 branch_scenarios,
264 TestCaseWithControlDir,
265 )
266-from bzrlib.transport import chroot
267-from lazr.uri import URI
268
269 from lp.codehosting.bzrutils import (
270- _install_checked_open_hook,
271 add_exception_logging_hook,
272- checked_open,
273 DenyingServer,
274 get_branch_stacked_on_url,
275 get_vfs_format_classes,
276 is_branch_stackable,
277 remove_exception_logging_hook,
278- safe_open,
279- UnsafeUrlSeen,
280 )
281 from lp.codehosting.tests.helpers import TestResultWrapper
282
283@@ -229,88 +221,6 @@
284
285
286
287-class TestCheckedOpen(TestCaseWithTransport):
288- """Tests for `checked_open`."""
289-
290- def setUp(self):
291- TestCaseWithTransport.setUp(self)
292- _install_checked_open_hook()
293-
294- def test_simple(self):
295- # Opening a branch with checked_open checks the branches url.
296- url = self.make_branch('branch').base
297- seen_urls = []
298- checked_open(seen_urls.append, url)
299- self.assertEqual([url], seen_urls)
300-
301- def test_stacked(self):
302- # Opening a stacked branch with checked_open checks the branches url
303- # and then the stacked-on url.
304- stacked = self.make_branch('stacked')
305- stacked_on = self.make_branch('stacked_on')
306- stacked.set_stacked_on_url(stacked_on.base)
307- seen_urls = []
308- checked_open(seen_urls.append, stacked.base)
309- self.assertEqual([stacked.base, stacked_on.base], seen_urls)
310-
311- def test_reference(self):
312- # Opening a branch reference with checked_open checks the branch
313- # references url and then the target of the reference.
314- target = self.make_branch('target')
315- reference_url = self.get_url('reference/')
316- BranchReferenceFormat().initialize(
317- BzrDir.create(reference_url), target_branch=target)
318- seen_urls = []
319- checked_open(seen_urls.append, reference_url)
320- self.assertEqual([reference_url, target.base], seen_urls)
321-
322-
323-class TestSafeOpen(TestCaseWithTransport):
324- """Tests for `safe_open`."""
325-
326- def setUp(self):
327- TestCaseWithTransport.setUp(self)
328- _install_checked_open_hook()
329-
330- def get_chrooted_scheme(self, relpath):
331- """Create a server that is chrooted to `relpath`.
332-
333- :return: ``(scheme, get_url)`` where ``scheme`` is the scheme of the
334- chroot server and ``get_url`` returns URLs on said server.
335- """
336- transport = self.get_transport(relpath)
337- chroot_server = chroot.ChrootServer(transport)
338- chroot_server.start_server()
339- self.addCleanup(chroot_server.stop_server)
340- def get_url(relpath):
341- return chroot_server.get_url() + relpath
342- return URI(chroot_server.get_url()).scheme, get_url
343-
344- def test_stacked_within_scheme(self):
345- # A branch that is stacked on a URL of the same scheme is safe to
346- # open.
347- self.get_transport().mkdir('inside')
348- self.make_branch('inside/stacked')
349- self.make_branch('inside/stacked-on')
350- scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
351- Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
352- get_chrooted_url('stacked-on'))
353- safe_open(scheme, get_chrooted_url('stacked'))
354-
355- def test_stacked_outside_scheme(self):
356- # A branch that is stacked on a URL that is not of the same scheme is
357- # not safe to open.
358- self.get_transport().mkdir('inside')
359- self.get_transport().mkdir('outside')
360- self.make_branch('inside/stacked')
361- self.make_branch('outside/stacked-on')
362- scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
363- Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
364- self.get_url('outside/stacked-on'))
365- self.assertRaises(
366- UnsafeUrlSeen, safe_open, scheme, get_chrooted_url('stacked'))
367-
368-
369 def load_tests(basic_tests, module, loader):
370 """Parametrize the tests of get_branch_stacked_on_url by branch format."""
371 result = loader.suiteClass()
372@@ -325,8 +235,6 @@
373 result.addTests(loader.loadTestsFromTestCase(TestDenyingServer))
374 result.addTests(loader.loadTestsFromTestCase(TestExceptionLoggingHooks))
375 result.addTests(loader.loadTestsFromTestCase(TestGetVfsFormatClasses))
376- result.addTests(loader.loadTestsFromTestCase(TestSafeOpen))
377- result.addTests(loader.loadTestsFromTestCase(TestCheckedOpen))
378 return result
379
380
381
382=== modified file 'lib/lp/codehosting/tests/test_safe_open.py'
383--- lib/lp/codehosting/tests/test_safe_open.py 2011-08-06 16:32:08 +0000
384+++ lib/lp/codehosting/tests/test_safe_open.py 2011-08-09 15:14:56 +0000
385@@ -6,21 +6,22 @@
386
387 __metaclass__ = type
388
389+from lazr.uri import URI
390
391-from lp.codehosting.puller.worker import (
392+from lp.codehosting.safe_open import (
393+ BadUrl,
394+ BlacklistPolicy,
395 BranchLoopError,
396 BranchReferenceForbidden,
397 SafeBranchOpener,
398- )
399-from lp.codehosting.safe_open import (
400- BadUrl,
401- BlacklistPolicy,
402 WhitelistPolicy,
403+ safe_open,
404 )
405
406 from lp.testing import TestCase
407
408 from bzrlib.branch import (
409+ Branch,
410 BzrBranchFormat7,
411 )
412 from bzrlib.bzrdir import (
413@@ -30,6 +31,7 @@
414 from bzrlib.tests import (
415 TestCaseWithTransport,
416 )
417+from bzrlib.transport import chroot
418
419
420 class TestSafeBranchOpenerCheckAndFollowBranchReference(TestCase):
421@@ -221,3 +223,45 @@
422 opener = self.makeBranchOpener([a.base, b.base])
423 self.assertRaises(BranchLoopError, opener.open, a.base)
424 self.assertRaises(BranchLoopError, opener.open, b.base)
425+
426+
427+class TestSafeOpen(TestCaseWithTransport):
428+ """Tests for `safe_open`."""
429+
430+ def get_chrooted_scheme(self, relpath):
431+ """Create a server that is chrooted to `relpath`.
432+
433+ :return: ``(scheme, get_url)`` where ``scheme`` is the scheme of the
434+ chroot server and ``get_url`` returns URLs on said server.
435+ """
436+ transport = self.get_transport(relpath)
437+ chroot_server = chroot.ChrootServer(transport)
438+ chroot_server.start_server()
439+ self.addCleanup(chroot_server.stop_server)
440+ def get_url(relpath):
441+ return chroot_server.get_url() + relpath
442+ return URI(chroot_server.get_url()).scheme, get_url
443+
444+ def test_stacked_within_scheme(self):
445+ # A branch that is stacked on a URL of the same scheme is safe to
446+ # open.
447+ self.get_transport().mkdir('inside')
448+ self.make_branch('inside/stacked')
449+ self.make_branch('inside/stacked-on')
450+ scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
451+ Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
452+ get_chrooted_url('stacked-on'))
453+ safe_open(scheme, get_chrooted_url('stacked'))
454+
455+ def test_stacked_outside_scheme(self):
456+ # A branch that is stacked on a URL that is not of the same scheme is
457+ # not safe to open.
458+ self.get_transport().mkdir('inside')
459+ self.get_transport().mkdir('outside')
460+ self.make_branch('inside/stacked')
461+ self.make_branch('outside/stacked-on')
462+ scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
463+ Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
464+ self.get_url('outside/stacked-on'))
465+ self.assertRaises(
466+ BadUrl, safe_open, scheme, get_chrooted_url('stacked'))