[PATCH] FTP transport append()
Alexandre Saint
stalst at gmail.com
Sat Apr 22 21:37:19 BST 2006
First, thanks for your review John.
On Fri, Apr 21, 2006 at 02:41:42PM -0500, John Arbash Meinel wrote:
> I'll try to give it a decent review:
> >
> > === modified file 'a/bzrlib/transport/ftp.py'
> > --- a/bzrlib/transport/ftp.py
> > +++ b/bzrlib/transport/ftp.py
> > @@ -134,8 +134,8 @@
> > relpath_parts = relpath[:]
> > if len(relpath_parts) > 1:
> > if relpath_parts[0] == '':
> > - raise ValueError("path %r within branch %r seems to be absolute"
> > - % (relpath, self._path))
> > + # the path seems to be absolute
> > + return relpath
> > basepath = self._path.split('/')
> > if len(basepath) > 0 and basepath[-1] == '':
> > basepath = basepath[:-1]
>
>
> I don't think we should allow arbitrary absolute paths at this point.
> Because that would allow people to access files outside of this branch,
> which seems like a security risk.
> We probably need to check that the path starts with our expected base path.
>
> I also don't think the interface for _abspath() allows absolute paths to
> be passed in. But I wasn't able to find _abspath in other transports, so
> I don't have much to go off of. (In other words, passing in a
> non-relative path really is an error)
>
I didn't consider this case, but that makes sense. Reverted in this patch.
(Actually, I did that due to an error I made in append(): I put
"self.has(abspath)" instead of "self.has(relpath)")
I'll be more careful next time.
> > @@ -303,11 +303,39 @@
> > raise TransportError(msg="Cannot remove directory at %s" % \
> > self._abspath(rel_path), extra=str(e))
> >
> > - def append(self, relpath, f):
> > + def append(self, relpath, fp, retries=0):
> > """Append the text in the file-like object into the final
> > location.
> > """
> > - raise TransportNotPossible('ftp does not support append()')
> > + if isinstance(fp, basestring):
> > + fp = StringIO(fp)
> > + fp.seek(0)
>
> I'm pretty sure that we decided to get rid of magic interfaces which
> have us check isinstance(fp, basestring)
>
So, I'll assume it's a file-like object.
> Also, 'fp' is not guaranteed not to be in the middle of some other file.
> So doing "fp.seek(0)" is not safe. LocalTransport uses fp.seek(0,2) to
> make sure that we have a location (because fp.tell() fails on win32).
>
The problem is, if the function fails during "conn.sendall(f.read())", 'f'
will probably be read, and thus the position in the file will change, so we'll
lose some data at the next try. Unless we keep the position somewhere.
So 'fp.seek(0)' is definitely not a solution.
"fp.tell()" seems to me that it'll resolve the problem. But, now, I'm a little
confused on how to do that since you seem to say that "fp.tell()" fails on
Windows.
For the moment I keep "f.tell()". What do you propose?
> Also, changing the parameter name from 'f' to 'fp' is an interface
> change, so any code that does transport.append(relpath='xxx', f=myfile)
> will fail.
>
> I would leave the parameter named 'f'.
>
>
> > + try:
> > + abspath = self._abspath(relpath)
> > + mutter("FTP appe to %s" % abspath)
> > + ftp = self._get_FTP()
> > + if self.has(abspath):
> > + length = ftp.size(abspath)
> > + else:
> > + length = 0
> > + ftp.voidcmd("TYPE I")
> > + cmd = "APPE %s" % abspath
> > + conn = ftp.transfercmd(cmd)
> > + conn.sendall(fp.read())
> > + conn.close()
> > + ftp.getresp()
> > + return length
> > + except ftplib.error_perm, e:
> > + FtpTransportError("Error appending data to %s" % abspath,
> > + orig_error=e)
> > + except ftplib.error_temp, e:
> > + if retries > _number_of_retries:
> > + raise TransportError("FTP temporary error during APPEND %s." \
> > + "Aborting." % self.abspath(relpath), orig_error=e)
> > + else:
> > + warning("FTP temporary error: %s. Retrying." % str(e))
> > + self._FTP_instance = None
> > + self.append(relpath, fp, retries+1)
>
> Otherwise this function looks good. (As long as APPE really is a
> standard ftp command)
>
Yes, it is a standard FTP command as of RFC 959.
(http://www.faqs.org/rfcs/rfc959.html)
> >
> > def copy(self, rel_from, rel_to):
> > """Copy the item at rel_from to the location at rel_to"""
> > @@ -347,11 +375,7 @@
> > mutter("FTP nlst: %s" % self._abspath(relpath))
> > f = self._get_FTP()
> > basepath = self._abspath(relpath)
> > - # FTP.nlst returns paths prefixed by relpath, strip 'em
> > - the_list = f.nlst(basepath)
> > - stripped = [path[len(basepath)+1:] for path in the_list]
> > - # Remove . and .. if present, and return
> > - return [path for path in stripped if path not in (".", "..")]
> > + return f.nlst(basepath)
> > except ftplib.error_perm, e:
> > raise TransportError(orig_error=e)
>
> Are you sure this is the case for all ftp servers? I have the feeling
> that some of them return one form, and some return another. So we need
> to have some sort of detection of what type of files we are getting
> back, and properly handle it. I'm guessing someone was testing the code,
> and that is what they found.
>
You'd be right. It now tests to detect if we are getting absolute or relative
paths.
> That said, I can say that with the only ftp server I have access to, it
> does indeed only return the base name, with no prefix on it. Because
> doing "t.listdir('.')" returns files with their first letter chopped off.
>
> John
> =:->
>
>
Also, I added support for changing permissions on files and the 'mode'
parameter is handled in append().
Thanks for your review.
BTW, I noticed that the 'mode' parameter is now used in the append() method of
several transports. But it is not present in the definition of append() in the
base Transport class (bzrlib/transport/__init__.py). Shouldn't it be there?
Cheers.
--
alex
-------------- next part --------------
=== modified file 'a/bzrlib/transport/ftp.py'
--- a/bzrlib/transport/ftp.py
+++ b/bzrlib/transport/ftp.py
@@ -303,11 +303,56 @@
raise TransportError(msg="Cannot remove directory at %s" % \
self._abspath(rel_path), extra=str(e))
- def append(self, relpath, f):
+ def append(self, relpath, f, mode=None, retries=0):
"""Append the text in the file-like object into the final
location.
"""
- raise TransportNotPossible('ftp does not support append()')
+ offset = f.tell()
+ try:
+ abspath = self._abspath(relpath)
+ mutter("FTP appe to %s" % abspath)
+ ftp = self._get_FTP()
+ if self.has(relpath):
+ result = ftp.size(abspath)
+ else:
+ result = 0
+ ftp.voidcmd("TYPE I")
+ cmd = "APPE %s" % abspath
+ conn = ftp.transfercmd(cmd)
+ conn.sendall(f.read())
+ conn.close()
+ if mode is not None:
+ self._setmode(relpath, mode)
+ ftp.getresp()
+ return result
+ except ftplib.error_perm, e:
+ FtpTransportError("Error appending data to %s" % abspath,
+ orig_error=e)
+ except ftplib.error_temp, e:
+ if retries > _number_of_retries:
+ raise TransportError("FTP temporary error during APPEND %s." \
+ "Aborting." % self.abspath(relpath), orig_error=e)
+ else:
+ warning("FTP temporary error: %s. Retrying." % str(e))
+ self._FTP_instance = None
+ # Make sure the file is at the same position it was given.
+ f.seek(offset)
+ self.append(relpath, f, retries+1)
+
+ def _setmode(self, relpath, mode):
+ """Set permissions on a path.
+
+ Only set permissions if the FTP server support the 'SITE CHMOD'
+ extension.
+ """
+ try:
+ ftp = self._get_FTP()
+ cmd = "SITE CHMOD %s %s" % (self._abspath(relpath), str(mode))
+ ftp.sendcmd(cmd)
+ except ftplib.error_perm, e:
+ # Command probably not available on this server
+ pass
+
def copy(self, rel_from, rel_to):
"""Copy the item at rel_from to the location at rel_to"""
@@ -347,11 +392,12 @@
mutter("FTP nlst: %s" % self._abspath(relpath))
f = self._get_FTP()
basepath = self._abspath(relpath)
- # FTP.nlst returns paths prefixed by relpath, strip 'em
- the_list = f.nlst(basepath)
- stripped = [path[len(basepath)+1:] for path in the_list]
+ paths = f.nlst(basepath)
+ # If FTP.nlst returns paths prefixed by relpath, strip 'em
+ if paths[0].startswith(basepath):
+ paths = [path[len(basepath)+1:] for path in paths]
# Remove . and .. if present, and return
- return [path for path in stripped if path not in (".", "..")]
+ return [path for path in paths if path not in (".", "..")]
except ftplib.error_perm, e:
raise TransportError(orig_error=e)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060422/2bfb7700/attachment.pgp
More information about the bazaar
mailing list