From d727531e6d96eb1b89679469e6baaca4b26b1d47 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Sun, 22 Oct 2017 11:32:22 +0800 Subject: [PATCH 1/2] send: use normpath() on argument to remove trailing slashes This ought to help with #251, where bash-on-windows makes it easy to add a forward-slash, and os.path.normpath() knows how to remove them, but os.sep is a backslash. --- src/wormhole/cli/cmd_send.py | 26 ++++++++++++++-- src/wormhole/test/test_cli.py | 57 +++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/wormhole/cli/cmd_send.py b/src/wormhole/cli/cmd_send.py index 773dd45..9274de6 100644 --- a/src/wormhole/cli/cmd_send.py +++ b/src/wormhole/cli/cmd_send.py @@ -243,11 +243,33 @@ class Sender: # be unicode or bytes. We need it to be something that can be # os.path.joined with the unicode args.what . what = os.path.join(args.cwd, args.what) - what = what.rstrip(os.sep) + + # We always tell the receiver to create a file (or directory) with the + # same basename as what the local user typed, even if the local object + # is a symlink to something with a different name. The normpath() is + # there to remove trailing slashes. + basename = os.path.basename(os.path.normpath(what)) + assert basename != "", what # normpath shouldn't allow this + + # We use realpath() instead of normpath() to locate the actual + # file/directory, because the path might contain symlinks, and + # normpath() would collapse those before resolving them. + + # Unfortunately on windows, realpath() is built out of normpath() + # because of a no-longer-true belief that windows does not have a + # working os.path.islink(): see https://bugs.python.org/issue9949 + # The consequence is that "wormhole send PATH" might send the wrong + # file, if: + # * we're running on windows + # * PATH goes down through a symlink and then up with parent-directory + # navigation (".."), then back down again + # * the back-down-again portion of the path also exists under the + # original directory (an error is thrown if not) + + what = os.path.realpath(what) if not os.path.exists(what): raise TransferError("Cannot send: no file/directory named '%s'" % args.what) - basename = os.path.basename(what) if os.path.isfile(what): # we're sending a file diff --git a/src/wormhole/test/test_cli.py b/src/wormhole/test/test_cli.py index ffc995a..9409838 100644 --- a/src/wormhole/test/test_cli.py +++ b/src/wormhole/test/test_cli.py @@ -183,6 +183,63 @@ class OfferData(unittest.TestCase): self.assertEqual(str(e), "'%s' is neither file nor directory" % filename) + def test_symlink(self): + if not hasattr(os, 'symlink'): + raise unittest.SkipTest("host OS does not support symlinks") + # build A/B1 -> B2 (==A/B2), and A/B2/C.txt + parent_dir = self.mktemp() + os.mkdir(parent_dir) + os.mkdir(os.path.join(parent_dir, "B2")) + with open(os.path.join(parent_dir, "B2", "C.txt"), "wb") as f: + f.write(b"success") + os.symlink("B2", os.path.join(parent_dir, "B1")) + # now send "B1/C.txt" from A, and it should get the right file + self.cfg.cwd = parent_dir + self.cfg.what = os.path.join("B1", "C.txt") + d, fd_to_send = build_offer(self.cfg) + self.assertEqual(d["file"]["filename"], "C.txt") + self.assertEqual(fd_to_send.read(), b"success") + + def test_symlink_collapse(self): + if not hasattr(os, 'symlink'): + raise unittest.SkipTest("host OS does not support symlinks") + if os.name == "nt": + # ntpath.py's realpath() is built out of normpath(), and does not + # follow symlinks properly, so this test always fails. "wormhole + # send PATH" on windows will do the wrong thing. See + # https://bugs.python.org/issue9949" for details. + raise unittest.SkipTest("host OS has broken os.path.realpath(), see https://bugs.python.org/issue9949") + # build A/B1, A/B1/D.txt + # A/B2/C2, A/B2/D.txt + # symlink A/B1/C1 -> A/B2/C2 + parent_dir = self.mktemp() + os.mkdir(parent_dir) + os.mkdir(os.path.join(parent_dir, "B1")) + with open(os.path.join(parent_dir, "B1", "D.txt"), "wb") as f: + f.write(b"fail") + os.mkdir(os.path.join(parent_dir, "B2")) + os.mkdir(os.path.join(parent_dir, "B2", "C2")) + with open(os.path.join(parent_dir, "B2", "D.txt"), "wb") as f: + f.write(b"success") + os.symlink(os.path.abspath(os.path.join(parent_dir, "B2", "C2")), + os.path.join(parent_dir, "B1", "C1")) + # Now send "B1/C1/../D.txt" from A. The correct traversal will be: + # * start: A + # * B1: A/B1 + # * C1: follow symlink to A/B2/C2 + # * ..: climb to A/B2 + # * D.txt: open A/B2/D.txt, which contains "success" + # If the code mistakenly uses normpath(), it would do: + # * normpath turns B1/C1/../D.txt into B1/D.txt + # * start: A + # * B1: A/B1 + # * D.txt: open A/B1/D.txt , which contains "fail" + self.cfg.cwd = parent_dir + self.cfg.what = os.path.join("B1", "C1", os.pardir, "D.txt") + d, fd_to_send = build_offer(self.cfg) + self.assertEqual(d["file"]["filename"], "D.txt") + self.assertEqual(fd_to_send.read(), b"success") + class LocaleFinder: def __init__(self): self._run_once = False From 1c5f29337ee0a2103ce25ef5ec099e59eef70938 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Wed, 1 Nov 2017 17:42:51 -0700 Subject: [PATCH 2/2] add notes, make test TODO instead of SKIP --- src/wormhole/cli/cmd_send.py | 20 +++++++++++++++----- src/wormhole/test/test_cli.py | 15 +++++++++------ 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/wormhole/cli/cmd_send.py b/src/wormhole/cli/cmd_send.py index 9274de6..bbba2ce 100644 --- a/src/wormhole/cli/cmd_send.py +++ b/src/wormhole/cli/cmd_send.py @@ -254,18 +254,28 @@ class Sender: # We use realpath() instead of normpath() to locate the actual # file/directory, because the path might contain symlinks, and # normpath() would collapse those before resolving them. + # test_cli.OfferData.test_symlink_collapse tests this. - # Unfortunately on windows, realpath() is built out of normpath() - # because of a no-longer-true belief that windows does not have a - # working os.path.islink(): see https://bugs.python.org/issue9949 - # The consequence is that "wormhole send PATH" might send the wrong - # file, if: + # Unfortunately on windows, realpath() (on py3) is built out of + # normpath() because of a py2-era belief that windows lacks a working + # os.path.islink(): see https://bugs.python.org/issue9949 . The + # consequence is that "wormhole send PATH" might send the wrong file, + # if: # * we're running on windows # * PATH goes down through a symlink and then up with parent-directory # navigation (".."), then back down again # * the back-down-again portion of the path also exists under the # original directory (an error is thrown if not) + # I'd like to fix this. The core issue is sending directories with a + # trailing slash: we need to 1: open the right directory, and 2: strip + # the right parent path out of the filenames we get from os.walk(). We + # used to use what.rstrip() for this, but bug #251 reported this + # failing on windows-with-bash. realpath() works in both those cases, + # but fails with the up-down symlinks situation. I think we'll need to + # find a third way to strip the trailing slash reliably in all + # environments. + what = os.path.realpath(what) if not os.path.exists(what): raise TransferError("Cannot send: no file/directory named '%s'" % diff --git a/src/wormhole/test/test_cli.py b/src/wormhole/test/test_cli.py index 9409838..6b0265c 100644 --- a/src/wormhole/test/test_cli.py +++ b/src/wormhole/test/test_cli.py @@ -203,12 +203,6 @@ class OfferData(unittest.TestCase): def test_symlink_collapse(self): if not hasattr(os, 'symlink'): raise unittest.SkipTest("host OS does not support symlinks") - if os.name == "nt": - # ntpath.py's realpath() is built out of normpath(), and does not - # follow symlinks properly, so this test always fails. "wormhole - # send PATH" on windows will do the wrong thing. See - # https://bugs.python.org/issue9949" for details. - raise unittest.SkipTest("host OS has broken os.path.realpath(), see https://bugs.python.org/issue9949") # build A/B1, A/B1/D.txt # A/B2/C2, A/B2/D.txt # symlink A/B1/C1 -> A/B2/C2 @@ -239,6 +233,15 @@ class OfferData(unittest.TestCase): d, fd_to_send = build_offer(self.cfg) self.assertEqual(d["file"]["filename"], "D.txt") self.assertEqual(fd_to_send.read(), b"success") + if os.name == "nt": + test_symlink_collapse.todo = "host OS has broken os.path.realpath()" + # ntpath.py's realpath() is built out of normpath(), and does not + # follow symlinks properly, so this test always fails. "wormhole send + # PATH" on windows will do the wrong thing. See + # https://bugs.python.org/issue9949" for details. I'm making this a + # TODO instead of a SKIP because 1: this causes an observable + # misbehavior (albeit in rare circumstances), 2: it probably used to + # work (sometimes, but not in #251). See cmd_send.py for more notes. class LocaleFinder: def __init__(self):