Bazaar

Merge lp:~declanmg/bzr/264275-fix into lp:bzr

  1. 264275-fix
  2. Merge into bzr.dev
Proposed by Declan McGrath
Status: Work in progress
Proposed branch: lp:~declanmg/bzr/264275-fix
Merge into: lp:bzr
Diff against target: 145 lines (+91/-0)
4 files modified
bzrlib/builtins.py (+25/-0)
bzrlib/errors.py (+5/-0)
bzrlib/tests/blackbox/test_add.py (+55/-0)
bzrlib/tests/test_errors.py (+6/-0)
To merge this branch: bzr merge lp:~declanmg/bzr/264275-fix
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Fixing
John A Meinel Pending
Review via email: mp+15175@code.launchpad.net

This proposal supersedes a proposal from 2009-11-10.

To post a comment you must log in.
Revision history for this message
Declan McGrath (declanmg) wrote : Posted in a previous version of this proposal

Please read this comment https://bugs.launchpad.net/ubuntu/+source/bzr/+bug/264275/comments/7 for a high level overview of this changeset (you can skip the 'Update' section).

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

Your error class seems a bit... verbose
AddingContentsUnderSymLinkedDirectoryOutsideTreeNotSupported

Also, you have some lines that are longer than 80 characters. You can usually split them with:

foo = ("this is some text"
       " and a bit more text\n")

6 from bzrlib.trace import mutter, note, warning, is_quiet, get_verbosity_level
7
8 +def is_realpath_outside_tree(tree_path, filepath):
9 +
10 + if not osutils.is_inside(

^- You should have two blank spaces before the def, and is_realpath... should have a docstring.

..

66 + os.mkdir('tree/inside_working_copy')
67 + self.build_tree(['tree/inside_working_copy/a_file.txt'])

^- you can use:

self.build_tree(['tree/inside_working_copy/', 'tree/inside_working_copy/a_file.txt'])
instead.

I think it would be good to have a couple of tests directly for "is_realpath_outside_tree", rather than only indirectly via 'run_bzr(['add', ...])'. Especially since that doesn't actually require symlinks to do the testing, which means it can be done on all platforms.

Otherwise I think I'm happy with the conceptual change.

review: Needs Fixing
Revision history for this message
Declan McGrath (declanmg) wrote : Posted in a previous version of this proposal

Thanks for the review. I have made the following changes
* Renamed overly verbose error class
* Made all text lines less than 80 chars long
* Added docstring to is_realpath_outside_tree()
* Removed use of os.mkdir where appropriate in tests

I have the following work left to do
* Add a couple of tests directly for is_realpath_outside_tree()

I have the following questions
* When you talk of adding tests directly for is_realpath_outside_tree() I take it that you mean adding more blackbox tests to test_add.py. Is this correct?
* "You should have two blank spaces before the def" Why? Surely there should be no spaces before the def - in the same way as the function that follows it (def tree_files) has no preceding spaces
* Would is_realpath_outside_tree() be better off in osutils.py?

Revision history for this message
Declan McGrath (declanmg) wrote : Posted in a previous version of this proposal

More work committed and pushed to branch. Here are some questions

* I have added 2 tests for is_realpath_outside_tree(). To exercise the method I had to 'import bzrlib.builtins' at the top of the tests/blackbox/test_add.py
  - Please advise if this is ok. It may violate the spirit of a blackbox test.

* Alternatively, I can move the method to osutils.py. I was thinking I could reverse the method's logic and rename it to is_realfilepath_in_realfolderpath() to make it more generic (as it doesn't need to be tree specific).

Revision history for this message
Declan McGrath (declanmg) wrote :

Hi,

I'm resubmitting this for review as I've made as many of the changes as possible. Please advise if resubmitting was the wrong thing to do.

The last two comments contain some questions and points I wanted to raise.

Kind regards,
Declan

Reply
Revision history for this message
Robert Collins (lifeless) wrote :

Do you have the previous review link? resubmit breaks the discussion and I have no idea what changes you are referring to.

review: Needs Information
Reply
Revision history for this message
Declan McGrath (declanmg) wrote :

Previous review link is at - https://code.launchpad.net/~declanmg/bzr/264275-fix/+merge/14688

Thanks,
Declan

Reply
Revision history for this message
Robert Collins (lifeless) wrote :

Sadly I think this is basically not avoiding the problem of dereferencing symlinks. We actually have to dereference them properly, and not add via the link.

What it is doing according to the tests is adding the thing the link points at as a child of the link, but that is an invalid directory structure and will almost certainly blow up on commmit.

Secondly, doing the check in the command isn't the best place - better to check in WorkingTree.smart_add.

So, to merge this I'd like to see:
 - the entry point code checks moved into MutableTree.smart_add
 - tests of smart_add that check when link/thing is added that the inventory
   *does not* contain children under the link entry.
 - the tests for is_realpath_outside_tree move to test_osutils.py (as that is
   where the function is)

A minor tweak to the tests would be to test with both a relative symlink (tree/link pointing to 'target' rather than abspath('target')).

And I think (but don't insist) that the helper function would be better named as 'realpath_is_outside_path', as the function works in paths, not trees.

Reply
Revision history for this message
Robert Collins (lifeless) wrote :

Opps, forgot to vote.

review: Needs Fixing
Reply
Revision history for this message
Declan McGrath (declanmg) wrote :

Thanks for the feedback. I'll need some more information before continuing, see below...

> Sadly I think this is basically not avoiding the problem of dereferencing
> symlinks. We actually have to dereference them properly, and not add via the
> link.

* What do you mean by "dereference them properly"? ie. What should the system do in the following cases
1) The symlink points to a location that is under the source controlled tree
2) The symlink points to a location that is not under the source controlled tree

My understanding is that in case (1), from what you are now saying, we need to dereference the link and then add via the dereferenced path. In case (2) the add should fail.

Functionally, my patch as it stands fulfills case (2). It also fulfills case (1) except that the scope of the fix should be expanded to dereference symlinks when they are added to the tree. Is it then fair to say that dereferencing of symlinks is currently not carried out by Bazaar when the add command adds a symlink? If so is it then ok to open a separate bug to carry out this work first? Then I can complete the work for the original bug afterwards?

Architecturally, the fix should be moved to MutableTree.smart_add to be correct.

>
> What it is doing according to the tests is adding the thing the link points at
> as a child of the link, but that is an invalid directory structure and will
> almost certainly blow up on commmit.

This statement may not be correct, depending on interpretation. If you try to add my_bzr_repo/symlink_to_dir_outside_tree/thing I believe that it will be denied - as the thing truly lies outside the source controlled tree.

If you try to add my_bzr_repo/symlink_to_dir_inside_tree/thing I believe that it will be carried out. But (just repeating what I said earlier) this invalid addition may be better handled first as a separate bug.

>
> Secondly, doing the check in the command isn't the best place - better to
> check in WorkingTree.smart_add.
>
> So, to merge this I'd like to see:
> - the entry point code checks moved into MutableTree.smart_add
> - tests of smart_add that check when link/thing is added that the inventory
> *does not* contain children under the link entry.
> - the tests for is_realpath_outside_tree move to test_osutils.py (as that is
> where the function is)

Will do.

>
>
> A minor tweak to the tests would be to test with both a relative symlink
> (tree/link pointing to 'target' rather than abspath('target')).
>

Will look into this.

> And I think (but don't insist) that the helper function would be better named
> as 'realpath_is_outside_path', as the function works in paths, not trees.

Will do.

Reply

Unmerged revisions

4743. By jack <jack@pixie>

Added tests for is_realpath_outside_tree().

4742. By jack <jack@pixie>

Made some post-review changes as requested (more to do) as follows...

* Renamed overly verbose error class
* Made all text lines less than 80 chars long
* Added docstring to is_realpath_outside_tree()
* Removed use of os.mkdir where appropriate in tests

4741. By jack <jack@pixie>

Tiny refactor. Send for review now.

4740. By jack <jack@pixie>

Moved tests from wip test to test_add.py. Bug 264275 ready for comment.

4739. By jack <jack@pixie>

Fixed typos in tests.

4738. By jack <jack@pixie>

Finished writing test logic.

4737. By jack <jack@pixie>

Added an error test for bug 264275 fix.

4736. By jack <jack@pixie>

Refactored code to reveal intent and tidied tests.

4735. By jack <jack@pixie>

Added a test concerning adding a file under a symlinked dir outside working copy

4734. By Declan McGrath <jack@jack>

Trying to target the specific test case more accurately. Still WIP.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2009-11-23 00:57:38 +0000
+++ bzrlib/builtins.py 2009-11-23 23:05:42 +0000
@@ -67,6 +67,19 @@
67 )67 )
68from bzrlib.trace import mutter, note, warning, is_quiet, get_verbosity_level68from bzrlib.trace import mutter, note, warning, is_quiet, get_verbosity_level
6969
70def is_realpath_outside_tree(tree_path, filepath):
71 """True if filepath is outside tree once symlinks are considered.
72
73 :param tree_path Path to the tree
74 :param filepath Path to test if outside tree
75 """
76
77 if not osutils.is_inside(
78 osutils.realpath(tree_path),
79 osutils.realpath(filepath)):
80 return True
81
82 return False
7083
71def tree_files(file_list, default_branch=u'.', canonicalize=True,84def tree_files(file_list, default_branch=u'.', canonicalize=True,
72 apply_view=True):85 apply_view=True):
@@ -107,6 +120,18 @@
107 file_list = view_files120 file_list = view_files
108 view_str = views.view_display_str(view_files)121 view_str = views.view_display_str(view_files)
109 note("Ignoring files outside view. View is %s" % view_str)122 note("Ignoring files outside view. View is %s" % view_str)
123
124 # Check if any parent dirs are sym-links whose dereferenced
125 # path is outside the working copy tree
126 if file_list:
127 tree_path = tree.basedir
128 for file in file_list:
129 parent_directory = os.path.split(file)[0]
130 if osutils.islink(parent_directory) and is_realpath_outside_tree(
131 tree_path,
132 parent_directory):
133 raise errors.SymLinkFolderContentsOutsideTree(file)
134
110 return tree, file_list135 return tree, file_list
111136
112137
113138
=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py 2009-08-30 21:34:42 +0000
+++ bzrlib/errors.py 2009-11-23 23:05:42 +0000
@@ -548,6 +548,11 @@
548548
549 _fmt = 'Hard-linking "%(path)s" is not supported'549 _fmt = 'Hard-linking "%(path)s" is not supported'
550550
551class SymLinkFolderContentsOutsideTree(PathError):
552
553 _fmt = ('Adding contents "%(path)s" under a sym-link which points to a '
554 'directory outside the working copy is not supported')
555
551556
552class ReadingCompleted(InternalBzrError):557class ReadingCompleted(InternalBzrError):
553558
554559
=== modified file 'bzrlib/tests/blackbox/test_add.py'
--- bzrlib/tests/blackbox/test_add.py 2009-11-08 00:05:26 +0000
+++ bzrlib/tests/blackbox/test_add.py 2009-11-23 23:05:42 +0000
@@ -19,6 +19,7 @@
1919
20import os20import os
2121
22import bzrlib.builtins
22from bzrlib import osutils23from bzrlib import osutils
23from bzrlib.tests import (24from bzrlib.tests import (
24 condition_isinstance,25 condition_isinstance,
@@ -224,3 +225,57 @@
224 os.symlink(osutils.abspath('target'), 'tree/link')225 os.symlink(osutils.abspath('target'), 'tree/link')
225 out = self.run_bzr(['add', 'tree/link'])[0]226 out = self.run_bzr(['add', 'tree/link'])[0]
226 self.assertEquals(out, 'adding link\n')227 self.assertEquals(out, 'adding link\n')
228
229 def test_add_file_under_symlinked_directory_inside_working_copy(self):
230 self.requireFeature(SymlinkFeature)
231 self.make_branch_and_tree('tree')
232 self.build_tree(['tree/inside_working_copy/',
233 'tree/inside_working_copy/a_file.txt'])
234 os.symlink(osutils.abspath('tree/inside_working_copy'), 'tree/link')
235 out = self.run_bzr(['add', 'tree/link/a_file.txt'])[0]
236 self.assertEquals(out, 'adding link\nadding link/a_file.txt\n')
237
238 def test_add_file_under_symlinked_directory_outside_working_copy(self):
239 self.requireFeature(SymlinkFeature)
240 self.make_branch_and_tree('tree')
241 self.build_tree(['outside_working_copy/',
242 'outside_working_copy/a_file.txt'])
243 os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
244 err = self.run_bzr(['add', 'tree/link/a_file.txt'], retcode = 3)[1]
245 expected = ('bzr: ERROR: Adding contents '
246 '"' + osutils.getcwd() + '/tree/link/a_file.txt" under a sym-link '
247 'which points to a directory outside the working copy is not '
248 'supported\n')
249 self.assertEqual(err, expected)
250
251 def test_add_a_symlink_under_symlinked_directory_outside_working_copy(self):
252 self.requireFeature(SymlinkFeature)
253 self.make_branch_and_tree('tree')
254 self.build_tree(['outside_working_copy/'])
255 os.symlink(osutils.abspath('outside_working_copy/does_not_exit'),
256 osutils.abspath('outside_working_copy/broken_link'))
257 os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
258 err = self.run_bzr(['add', 'tree/link/broken_link'], retcode = 3)[1]
259 expected = ('bzr: ERROR: Adding contents '
260 '"' + osutils.getcwd() + '/tree/link/broken_link" under a sym-link '
261 'which points to a directory outside the working copy is not '
262 'supported\n')
263 self.assertEqual(err, expected)
264
265 def test_realpath_is_outside_tree(self):
266 self.make_branch_and_tree('tree')
267 self.build_tree(['containing_folder/',
268 'a_file.txt'])
269 out = bzrlib.builtins.is_realpath_outside_tree(
270 osutils.abspath('containing_folder/'),
271 osutils.abspath('a_file.txt'))
272 self.assertEqual(out, True)
273
274 def test_realpath_is_under_tree(self):
275 self.make_branch_and_tree('tree')
276 self.build_tree(['tree/containing_folder/',
277 'tree/containing_folder/a_file.txt'])
278 out = bzrlib.builtins.is_realpath_outside_tree(
279 osutils.abspath('containing_folder/'),
280 osutils.abspath('containing_folder/a_file.txt'))
281 self.assertEqual(out, False)
227282
=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py 2009-08-21 02:37:18 +0000
+++ bzrlib/tests/test_errors.py 2009-11-23 23:05:42 +0000
@@ -528,6 +528,12 @@
528 "user encoding " + osutils.get_user_encoding(),528 "user encoding " + osutils.get_user_encoding(),
529 str(err))529 str(err))
530530
531 def test_unable_to_add_contents_under_symlinked_folder_outside_tree(self):
532 err = errors.SymLinkFolderContentsOutsideTree('foo')
533 self.assertEquals(("Adding contents \"foo\" under a sym-link which "
534 "points to a directory outside the working copy is not supported"),
535 str(err))
536
531 def test_unknown_format(self):537 def test_unknown_format(self):
532 err = errors.UnknownFormatError('bar', kind='foo')538 err = errors.UnknownFormatError('bar', kind='foo')
533 self.assertEquals("Unknown foo format: 'bar'", str(err))539 self.assertEquals("Unknown foo format: 'bar'", str(err))