Bazaar

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

  1. 264275-fix
  2. Merge into bzr.dev
Proposed by Declan McGrath
Status: Work in progress
Proposed branch: lp://staging/~declanmg/bzr/264275-fix
Merge into: lp://staging/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://staging/~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.staging.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
1=== modified file 'bzrlib/builtins.py'
2--- bzrlib/builtins.py 2009-11-23 00:57:38 +0000
3+++ bzrlib/builtins.py 2009-11-23 23:05:42 +0000
4@@ -67,6 +67,19 @@
5 )
6 from bzrlib.trace import mutter, note, warning, is_quiet, get_verbosity_level
7
8+def is_realpath_outside_tree(tree_path, filepath):
9+ """True if filepath is outside tree once symlinks are considered.
10+
11+ :param tree_path Path to the tree
12+ :param filepath Path to test if outside tree
13+ """
14+
15+ if not osutils.is_inside(
16+ osutils.realpath(tree_path),
17+ osutils.realpath(filepath)):
18+ return True
19+
20+ return False
21
22 def tree_files(file_list, default_branch=u'.', canonicalize=True,
23 apply_view=True):
24@@ -107,6 +120,18 @@
25 file_list = view_files
26 view_str = views.view_display_str(view_files)
27 note("Ignoring files outside view. View is %s" % view_str)
28+
29+ # Check if any parent dirs are sym-links whose dereferenced
30+ # path is outside the working copy tree
31+ if file_list:
32+ tree_path = tree.basedir
33+ for file in file_list:
34+ parent_directory = os.path.split(file)[0]
35+ if osutils.islink(parent_directory) and is_realpath_outside_tree(
36+ tree_path,
37+ parent_directory):
38+ raise errors.SymLinkFolderContentsOutsideTree(file)
39+
40 return tree, file_list
41
42
43
44=== modified file 'bzrlib/errors.py'
45--- bzrlib/errors.py 2009-08-30 21:34:42 +0000
46+++ bzrlib/errors.py 2009-11-23 23:05:42 +0000
47@@ -548,6 +548,11 @@
48
49 _fmt = 'Hard-linking "%(path)s" is not supported'
50
51+class SymLinkFolderContentsOutsideTree(PathError):
52+
53+ _fmt = ('Adding contents "%(path)s" under a sym-link which points to a '
54+ 'directory outside the working copy is not supported')
55+
56
57 class ReadingCompleted(InternalBzrError):
58
59
60=== modified file 'bzrlib/tests/blackbox/test_add.py'
61--- bzrlib/tests/blackbox/test_add.py 2009-11-08 00:05:26 +0000
62+++ bzrlib/tests/blackbox/test_add.py 2009-11-23 23:05:42 +0000
63@@ -19,6 +19,7 @@
64
65 import os
66
67+import bzrlib.builtins
68 from bzrlib import osutils
69 from bzrlib.tests import (
70 condition_isinstance,
71@@ -224,3 +225,57 @@
72 os.symlink(osutils.abspath('target'), 'tree/link')
73 out = self.run_bzr(['add', 'tree/link'])[0]
74 self.assertEquals(out, 'adding link\n')
75+
76+ def test_add_file_under_symlinked_directory_inside_working_copy(self):
77+ self.requireFeature(SymlinkFeature)
78+ self.make_branch_and_tree('tree')
79+ self.build_tree(['tree/inside_working_copy/',
80+ 'tree/inside_working_copy/a_file.txt'])
81+ os.symlink(osutils.abspath('tree/inside_working_copy'), 'tree/link')
82+ out = self.run_bzr(['add', 'tree/link/a_file.txt'])[0]
83+ self.assertEquals(out, 'adding link\nadding link/a_file.txt\n')
84+
85+ def test_add_file_under_symlinked_directory_outside_working_copy(self):
86+ self.requireFeature(SymlinkFeature)
87+ self.make_branch_and_tree('tree')
88+ self.build_tree(['outside_working_copy/',
89+ 'outside_working_copy/a_file.txt'])
90+ os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
91+ err = self.run_bzr(['add', 'tree/link/a_file.txt'], retcode = 3)[1]
92+ expected = ('bzr: ERROR: Adding contents '
93+ '"' + osutils.getcwd() + '/tree/link/a_file.txt" under a sym-link '
94+ 'which points to a directory outside the working copy is not '
95+ 'supported\n')
96+ self.assertEqual(err, expected)
97+
98+ def test_add_a_symlink_under_symlinked_directory_outside_working_copy(self):
99+ self.requireFeature(SymlinkFeature)
100+ self.make_branch_and_tree('tree')
101+ self.build_tree(['outside_working_copy/'])
102+ os.symlink(osutils.abspath('outside_working_copy/does_not_exit'),
103+ osutils.abspath('outside_working_copy/broken_link'))
104+ os.symlink(osutils.abspath('outside_working_copy'), 'tree/link')
105+ err = self.run_bzr(['add', 'tree/link/broken_link'], retcode = 3)[1]
106+ expected = ('bzr: ERROR: Adding contents '
107+ '"' + osutils.getcwd() + '/tree/link/broken_link" under a sym-link '
108+ 'which points to a directory outside the working copy is not '
109+ 'supported\n')
110+ self.assertEqual(err, expected)
111+
112+ def test_realpath_is_outside_tree(self):
113+ self.make_branch_and_tree('tree')
114+ self.build_tree(['containing_folder/',
115+ 'a_file.txt'])
116+ out = bzrlib.builtins.is_realpath_outside_tree(
117+ osutils.abspath('containing_folder/'),
118+ osutils.abspath('a_file.txt'))
119+ self.assertEqual(out, True)
120+
121+ def test_realpath_is_under_tree(self):
122+ self.make_branch_and_tree('tree')
123+ self.build_tree(['tree/containing_folder/',
124+ 'tree/containing_folder/a_file.txt'])
125+ out = bzrlib.builtins.is_realpath_outside_tree(
126+ osutils.abspath('containing_folder/'),
127+ osutils.abspath('containing_folder/a_file.txt'))
128+ self.assertEqual(out, False)
129
130=== modified file 'bzrlib/tests/test_errors.py'
131--- bzrlib/tests/test_errors.py 2009-08-21 02:37:18 +0000
132+++ bzrlib/tests/test_errors.py 2009-11-23 23:05:42 +0000
133@@ -528,6 +528,12 @@
134 "user encoding " + osutils.get_user_encoding(),
135 str(err))
136
137+ def test_unable_to_add_contents_under_symlinked_folder_outside_tree(self):
138+ err = errors.SymLinkFolderContentsOutsideTree('foo')
139+ self.assertEquals(("Adding contents \"foo\" under a sym-link which "
140+ "points to a directory outside the working copy is not supported"),
141+ str(err))
142+
143 def test_unknown_format(self):
144 err = errors.UnknownFormatError('bar', kind='foo')
145 self.assertEquals("Unknown foo format: 'bar'", str(err))