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

[Declan McGrath]

[Declan McGrath (declanmg)]

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).

[John A Meinel (jameinel)]

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

[Declan McGrath (declanmg)]

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?

[Declan McGrath (declanmg)]

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).

[Declan McGrath (declanmg)]

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

[Robert Collins (lifeless)]

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

[Declan McGrath (declanmg)]

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

Thanks,
Declan

[Robert Collins (lifeless)]

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.

[Robert Collins (lifeless)]

Opps, forgot to vote.

review: Needs Fixing

[Declan McGrath (declanmg)]

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.

Unmerged revisions

4743. By jack <jack@pixie> on 2009-11-22

Added tests for is_realpath_outside_tree().

4742. By jack <jack@pixie> on 2009-11-22

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> on 2009-11-08

Tiny refactor. Send for review now.

4740. By jack <jack@pixie> on 2009-11-08

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

4739. By jack <jack@pixie> on 2009-11-08

Fixed typos in tests.

4738. By jack <jack@pixie> on 2009-11-08

Finished writing test logic.

4737. By jack <jack@pixie> on 2009-11-08

Added an error test for bug 264275 fix.

4736. By jack <jack@pixie> on 2009-11-08

Refactored code to reveal intent and tidied tests.

4735. By jack <jack@pixie> on 2009-10-31

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

4734. By Declan McGrath <jack@jack> on 2009-10-18

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

=== 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 @@
     )
 from bzrlib.trace import mutter, note, warning, is_quiet, get_verbosity_level
 
+def is_realpath_outside_tree(tree_path, filepath):
+    """True if filepath is outside tree once symlinks are considered.
+
+    :param tree_path Path to the tree
+    :param filepath Path to test if outside tree
+    """
+
+    if not osutils.is_inside(
+            osutils.realpath(tree_path),
+            osutils.realpath(filepath)):
+        return True
+
+    return False
 
 def tree_files(file_list, default_branch=u'.', canonicalize=True,
     apply_view=True):
@@ -107,6 +120,18 @@
                 file_list = view_files
                 view_str = views.view_display_str(view_files)
                 note("Ignoring files outside view. View is %s" % view_str)
+
+    # Check if any parent dirs are sym-links whose dereferenced
+    # path is outside the working copy tree
+    if file_list:
+        tree_path = tree.basedir
+        for file in file_list:
+            parent_directory = os.path.split(file)[0]
+            if osutils.islink(parent_directory) and is_realpath_outside_tree(
+                tree_path,
+                parent_directory):
+                raise errors.SymLinkFolderContentsOutsideTree(file)
+            
     return tree, file_list
 
 

=== 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 @@
 
     _fmt = 'Hard-linking "%(path)s" is not supported'
 
+class SymLinkFolderContentsOutsideTree(PathError):
+
+    _fmt = ('Adding contents "%(path)s" under a sym-link which points to a '
+        'directory outside the working copy is not supported')
+
 
 class ReadingCompleted(InternalBzrError):
 

=== 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 @@
 
 import os
 
+import bzrlib.builtins
 from bzrlib import osutils
 from bzrlib.tests import (
     condition_isinstance,
@@ -224,3 +225,57 @@
         os.symlink(osutils.abspath('target'), 'tree/link')
         out = self.run_bzr(['add', 'tree/link'])[0]
         self.assertEquals(out, 'adding link\n')
+
+    def test_add_file_under_symlinked_directory_inside_working_copy(self):
+        self.requireFeature(SymlinkFeature)
+        self.make_branch_and_tree('tree')
+        self.build_tree(['tree/inside_working_copy/',
+            'tree/inside_working_copy/a_file.txt'])
+        os.symlink(osutils.abspath('tree/inside_working_copy'), 'tree/link')
+        out = self.run_bzr(['add', 'tree/link/a_file.txt'])[0]
+        self.assertEquals(out, 'adding link\nadding link/a_file.txt\n')
+
+    def test_add_file_under_symlinked_directory_outside_working_copy(self):
+        self.requireFeature(SymlinkFeature)
+        self.make_branch_and_tree('tree')
+        self.build_tree(['outside_working_copy/',
+            'outside_working_copy/a_file.txt'])
+        os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
+        err = self.run_bzr(['add', 'tree/link/a_file.txt'], retcode = 3)[1]
+        expected = ('bzr: ERROR: Adding contents '
+            '"' + osutils.getcwd() + '/tree/link/a_file.txt" under a sym-link '
+            'which points to a directory outside the working copy is not '
+            'supported\n')
+        self.assertEqual(err, expected)
+
+    def test_add_a_symlink_under_symlinked_directory_outside_working_copy(self):
+        self.requireFeature(SymlinkFeature)
+        self.make_branch_and_tree('tree')
+        self.build_tree(['outside_working_copy/'])
+        os.symlink(osutils.abspath('outside_working_copy/does_not_exit'),
+            osutils.abspath('outside_working_copy/broken_link'))
+        os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
+        err = self.run_bzr(['add', 'tree/link/broken_link'], retcode = 3)[1]
+        expected = ('bzr: ERROR: Adding contents '
+            '"' + osutils.getcwd() + '/tree/link/broken_link" under a sym-link '
+            'which points to a directory outside the working copy is not '
+            'supported\n')
+        self.assertEqual(err, expected)
+
+    def test_realpath_is_outside_tree(self):
+        self.make_branch_and_tree('tree')
+        self.build_tree(['containing_folder/',
+            'a_file.txt'])
+        out = bzrlib.builtins.is_realpath_outside_tree(
+            osutils.abspath('containing_folder/'),
+            osutils.abspath('a_file.txt'))
+        self.assertEqual(out, True)
+
+    def test_realpath_is_under_tree(self):
+        self.make_branch_and_tree('tree')
+        self.build_tree(['tree/containing_folder/',
+            'tree/containing_folder/a_file.txt'])
+        out = bzrlib.builtins.is_realpath_outside_tree(
+            osutils.abspath('containing_folder/'),
+            osutils.abspath('containing_folder/a_file.txt'))
+        self.assertEqual(out, False)

=== 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 @@
             "user encoding " + osutils.get_user_encoding(),
             str(err))
 
+    def test_unable_to_add_contents_under_symlinked_folder_outside_tree(self):
+        err = errors.SymLinkFolderContentsOutsideTree('foo')
+        self.assertEquals(("Adding contents \"foo\" under a sym-link which "
+            "points to a directory outside the working copy is not supported"),
+            str(err))
+
     def test_unknown_format(self):
         err = errors.UnknownFormatError('bar', kind='foo')
         self.assertEquals("Unknown foo format: 'bar'", str(err))