Merge lp:~vila/bzr/1509156-stricter-readonly into lp:bzr

Proposed by Vincent Ladeuil on 2016-01-21
Status: Merged
Approved by: Vincent Ladeuil on 2016-01-21
Approved revision: 6609
Merged at revision: 6612
Proposed branch: lp:~vila/bzr/1509156-stricter-readonly
Merge into: lp:bzr
Diff against target: 137 lines (+40/-7)
4 files modified
bzrlib/tests/per_transport.py (+10/-3)
bzrlib/tests/test_smart_transport.py (+16/-1)
bzrlib/transport/readonly.py (+11/-3)
doc/en/release-notes/bzr-2.7.txt (+3/-0)
To merge this branch: bzr merge lp:~vila/bzr/1509156-stricter-readonly
Reviewer Review Type Date Requested Status
Richard Wilbur 2016-01-21 Approve on 2016-01-21
Review via email: mp+283512@code.launchpad.net

This proposal supersedes a proposal from 2016-01-21.

Commit message

Forbid more operations on ReadonlyTransportDecorator

Description of the change

This fixes bug #1509156 by forbidding rename() and open_write_stream() to be called on a transport decorated with ReadonlyTransportDecorator.

This ensures such transports can't be misused but so far they only allowed renaming existing files.

So this fix mostly closes the remaining holes but doesn't change the intent of the decorator.

To post a comment you must log in.
Richard Wilbur (richard-wilbur) wrote : Posted in a previous version of this proposal

I think this is a great idea, Vincent. 9 hours after you made the merge proposal, the diff still doesn't show! ("Updating diff...
An updated diff will be available in a few minutes. Reload to see the changes.")

Sounds like something is stuck. I don't know that approving it will be sufficient to merge it at this point when it is confused about the changes.

Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

The bug was embargoed for security reasons but really, after diagnosing, there is no security issue there.

6609. By Vincent Ladeuil on 2016-01-21

Complete the explanation.

Richard Wilbur (richard-wilbur) wrote :

Looks like this is good for security! I approve.
+2

I am curious about the change in exception type raised by
bzrlib/transport/readonly.py: get_smart_client()
- NoSmartServer
+ NoSmartMedium

review: Approve
Vincent Ladeuil (vila) :
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Richard Wilbur (richard-wilbur) wrote :

Thank you for cleaning up a bit of code rot!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/per_transport.py'
2--- bzrlib/tests/per_transport.py 2011-12-23 19:38:22 +0000
3+++ bzrlib/tests/per_transport.py 2016-01-21 17:36:38 +0000
4@@ -1,4 +1,4 @@
5-# Copyright (C) 2005-2011 Canonical Ltd
6+# Copyright (C) 2005-2011, 2015 Canonical Ltd
7 #
8 # This program is free software; you can redistribute it and/or modify
9 # it under the terms of the GNU General Public License as published by
10@@ -630,10 +630,13 @@
11 def test_opening_a_file_stream_can_set_mode(self):
12 t = self.get_transport()
13 if t.is_readonly():
14+ self.assertRaises((TransportNotPossible, NotImplementedError),
15+ t.open_write_stream, 'foo')
16 return
17 if not t._can_roundtrip_unix_modebits():
18 # Can't roundtrip, so no need to run this test
19 return
20+
21 def check_mode(name, mode, expected):
22 handle = t.open_write_stream(name, mode=mode)
23 handle.close()
24@@ -921,7 +924,9 @@
25 def test_rename_dir_succeeds(self):
26 t = self.get_transport()
27 if t.is_readonly():
28- raise TestSkipped("transport is readonly")
29+ self.assertRaises((TransportNotPossible, NotImplementedError),
30+ t.rename, 'foo', 'bar')
31+ return
32 t.mkdir('adir')
33 t.mkdir('adir/asubdir')
34 t.rename('adir', 'bdir')
35@@ -932,7 +937,9 @@
36 """Attempting to replace a nonemtpy directory should fail"""
37 t = self.get_transport()
38 if t.is_readonly():
39- raise TestSkipped("transport is readonly")
40+ self.assertRaises((TransportNotPossible, NotImplementedError),
41+ t.rename, 'foo', 'bar')
42+ return
43 t.mkdir('adir')
44 t.mkdir('adir/asubdir')
45 t.mkdir('bdir')
46
47=== modified file 'bzrlib/tests/test_smart_transport.py'
48--- bzrlib/tests/test_smart_transport.py 2014-02-07 10:20:38 +0000
49+++ bzrlib/tests/test_smart_transport.py 2016-01-21 17:36:38 +0000
50@@ -1,4 +1,4 @@
51-# Copyright (C) 2006-2011 Canonical Ltd
52+# Copyright (C) 2006-2015 Canonical Ltd
53 #
54 # This program is free software; you can redistribute it and/or modify
55 # it under the terms of the GNU General Public License as published by
56@@ -1640,6 +1640,21 @@
57 self.assertRaises(errors.TransportNotPossible, self.transport.mkdir,
58 'foo')
59
60+ def test_rename_error_readonly(self):
61+ """TransportNotPossible should be preserved from the backing transport."""
62+ self.overrideEnv('BZR_NO_SMART_VFS', None)
63+ self.start_server(readonly=True)
64+ self.assertRaises(errors.TransportNotPossible, self.transport.rename,
65+ 'foo', 'bar')
66+
67+ def test_open_write_stream_error_readonly(self):
68+ """TransportNotPossible should be preserved from the backing transport."""
69+ self.overrideEnv('BZR_NO_SMART_VFS', None)
70+ self.start_server(readonly=True)
71+ self.assertRaises(
72+ errors.TransportNotPossible, self.transport.open_write_stream,
73+ 'foo')
74+
75
76 class TestServerHooks(SmartTCPTests):
77
78
79=== modified file 'bzrlib/transport/readonly.py'
80--- bzrlib/transport/readonly.py 2011-12-19 13:23:58 +0000
81+++ bzrlib/transport/readonly.py 2016-01-21 17:36:38 +0000
82@@ -1,4 +1,4 @@
83-# Copyright (C) 2006, 2009 Canonical Ltd
84+# Copyright (C) 2006, 2007, 2009, 2010, 2011, 2015 Canonical Ltd
85 #
86 # This program is free software; you can redistribute it and/or modify
87 # it under the terms of the GNU General Public License as published by
88@@ -41,6 +41,10 @@
89 """Readonly transport decorators are invoked via 'readonly+'"""
90 return 'readonly+'
91
92+ def rename(self, rel_from, rel_to):
93+ """See Transport.rename."""
94+ raise TransportNotPossible('readonly transport')
95+
96 def delete(self, relpath):
97 """See Transport.delete()."""
98 raise TransportNotPossible('readonly transport')
99@@ -61,6 +65,10 @@
100 """See Transport.mkdir()."""
101 raise TransportNotPossible('readonly transport')
102
103+ def open_write_stream(self, relpath, mode=None):
104+ """See Transport.open_write_stream()."""
105+ raise TransportNotPossible('readonly transport')
106+
107 def is_readonly(self):
108 """See Transport.is_readonly."""
109 return True
110@@ -74,7 +82,7 @@
111 raise TransportNotPossible('readonly transport')
112
113 def get_smart_client(self):
114- raise NoSmartServer(self.base)
115+ raise NoSmartMedium(self)
116
117 def get_smart_medium(self):
118 raise NoSmartMedium(self)
119@@ -83,4 +91,4 @@
120 def get_test_permutations():
121 """Return the permutations to be used in testing."""
122 from bzrlib.tests import test_server
123- return [(ReadonlyTransportDecorator, test_server.ReadonlyServer),]
124+ return [(ReadonlyTransportDecorator, test_server.ReadonlyServer)]
125
126=== modified file 'doc/en/release-notes/bzr-2.7.txt'
127--- doc/en/release-notes/bzr-2.7.txt 2016-01-21 11:42:23 +0000
128+++ doc/en/release-notes/bzr-2.7.txt 2016-01-21 17:36:38 +0000
129@@ -46,6 +46,9 @@
130 * Fix pyrex version checking to be more robust.
131 (Andrew Starr-Bochicchio, #1030521 )
132
133+* Forbid more operations for ReadonlyTransportDecorator so no more write
134+ methods can be used my mistake. (Vincent Ladeuil, #150196)
135+
136 Documentation
137 *************
138