From d9254599f9bb47632313ecb90c5f281ceca5da3a Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Wed, 19 Feb 2020 22:36:26 -0500 Subject: [PATCH] manifest/tests: get them passing under Windows We also need to check more things in the manifest/project handlers, and use platform_utils in a few places to address Windows behavior. Drop Python 2.7 from Windows testing as it definitely doesn't work and we won't be fixing it. Change-Id: I83d00ee9f1612312bb3f7147cb9535fc61268245 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/256113 Tested-by: Mike Frysinger Reviewed-by: Jonathan Nieder Reviewed-by: David Pursehouse --- .github/workflows/test-ci.yml | 3 +++ manifest_xml.py | 17 ++++++++++++++--- project.py | 7 ++++++- tests/test_manifest_xml.py | 6 ++++++ tests/test_project.py | 28 ++++++++++++++++------------ 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/.github/workflows/test-ci.yml b/.github/workflows/test-ci.yml index 93061814..c33002dc 100644 --- a/.github/workflows/test-ci.yml +++ b/.github/workflows/test-ci.yml @@ -15,6 +15,9 @@ jobs: matrix: os: [ubuntu-latest, macos-latest, windows-latest] python-version: [2.7, 3.6, 3.7, 3.8] + exclude: + - os: windows-latest + python-version: 2.7 runs-on: ${{ matrix.os }} steps: diff --git a/manifest_xml.py b/manifest_xml.py index 41628003..fe0735a8 100644 --- a/manifest_xml.py +++ b/manifest_xml.py @@ -1010,19 +1010,30 @@ class XmlManifest(object): # Assume paths might be used on case-insensitive filesystems. path = path.lower() + # Split up the path by its components. We can't use os.path.sep exclusively + # as some platforms (like Windows) will convert / to \ and that bypasses all + # our constructed logic here. Especially since manifest authors only use + # / in their paths. + resep = re.compile(r'[/%s]' % re.escape(os.path.sep)) + parts = resep.split(path) + # Some people use src="." to create stable links to projects. Lets allow # that but reject all other uses of "." to keep things simple. - parts = path.split(os.path.sep) if parts != ['.']: for part in set(parts): if part in {'.', '..', '.git'} or part.startswith('.repo'): return 'bad component: %s' % (part,) - if not symlink and path.endswith(os.path.sep): + if not symlink and resep.match(path[-1]): return 'dirs not allowed' + # NB: The two abspath checks here are to handle platforms with multiple + # filesystem path styles (e.g. Windows). norm = os.path.normpath(path) - if norm == '..' or norm.startswith('../') or norm.startswith(os.path.sep): + if (norm == '..' or + (len(norm) >= 3 and norm.startswith('..') and resep.match(norm[0])) or + os.path.isabs(norm) or + norm.startswith('/')): return 'path cannot be outside' @classmethod diff --git a/project.py b/project.py index 2112ee32..99ef238f 100644 --- a/project.py +++ b/project.py @@ -275,7 +275,12 @@ def _SafeExpandPath(base, subpath, skipfinal=False): NB: We rely on a number of paths already being filtered out while parsing the manifest. See the validation logic in manifest_xml.py for more details. """ - components = subpath.split(os.path.sep) + # Split up the path by its components. We can't use os.path.sep exclusively + # as some platforms (like Windows) will convert / to \ and that bypasses all + # our constructed logic here. Especially since manifest authors only use + # / in their paths. + resep = re.compile(r'[/%s]' % re.escape(os.path.sep)) + components = resep.split(subpath) if skipfinal: # Whether the caller handles the final component itself. finalpart = components.pop() diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py index b6ec5b86..1cb72971 100644 --- a/tests/test_manifest_xml.py +++ b/tests/test_manifest_xml.py @@ -18,6 +18,7 @@ from __future__ import print_function +import os import unittest import error @@ -78,6 +79,11 @@ class ManifestValidateFilePaths(unittest.TestCase): # Block Unicode characters that get normalized out by filesystems. u'foo\u200Cbar', ) + # Make sure platforms that use path separators (e.g. Windows) are also + # rejected properly. + if os.path.sep != '/': + PATHS += tuple(x.replace('/', os.path.sep) for x in PATHS) + for path in PATHS: self.assertRaises( error.ManifestInvalidPathError, self.check_both, path, 'a') diff --git a/tests/test_project.py b/tests/test_project.py index b416386e..67574cb8 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -27,6 +27,7 @@ import unittest import error import git_config +import platform_utils import project @@ -40,7 +41,7 @@ def TempGitTree(): subprocess.check_call(['git', 'init'], cwd=tempdir) yield tempdir finally: - shutil.rmtree(tempdir) + platform_utils.rmtree(tempdir) class RepoHookShebang(unittest.TestCase): @@ -243,17 +244,19 @@ class CopyFile(CopyLinkTestCase): src = os.path.join(self.worktree, 'foo.txt') sym = os.path.join(self.worktree, 'sym') self.touch(src) - os.symlink('foo.txt', sym) + platform_utils.symlink('foo.txt', sym) self.assertExists(sym) cf = self.CopyFile('sym', 'foo') self.assertRaises(error.ManifestInvalidPathError, cf._Copy) def test_src_block_symlink_traversal(self): """Do not allow reading through a symlink dir.""" - src = os.path.join(self.worktree, 'bar', 'passwd') - os.symlink('/etc', os.path.join(self.worktree, 'bar')) + realfile = os.path.join(self.tempdir, 'file.txt') + self.touch(realfile) + src = os.path.join(self.worktree, 'bar', 'file.txt') + platform_utils.symlink(self.tempdir, os.path.join(self.worktree, 'bar')) self.assertExists(src) - cf = self.CopyFile('bar/foo.txt', 'foo') + cf = self.CopyFile('bar/file.txt', 'foo') self.assertRaises(error.ManifestInvalidPathError, cf._Copy) def test_src_block_copy_from_dir(self): @@ -267,7 +270,7 @@ class CopyFile(CopyLinkTestCase): """Do not allow writing to a symlink.""" src = os.path.join(self.worktree, 'foo.txt') self.touch(src) - os.symlink('dest', os.path.join(self.topdir, 'sym')) + platform_utils.symlink('dest', os.path.join(self.topdir, 'sym')) cf = self.CopyFile('foo.txt', 'sym') self.assertRaises(error.ManifestInvalidPathError, cf._Copy) @@ -275,7 +278,8 @@ class CopyFile(CopyLinkTestCase): """Do not allow writing through a symlink dir.""" src = os.path.join(self.worktree, 'foo.txt') self.touch(src) - os.symlink('/tmp', os.path.join(self.topdir, 'sym')) + platform_utils.symlink(tempfile.gettempdir(), + os.path.join(self.topdir, 'sym')) cf = self.CopyFile('foo.txt', 'sym/foo.txt') self.assertRaises(error.ManifestInvalidPathError, cf._Copy) @@ -303,7 +307,7 @@ class LinkFile(CopyLinkTestCase): dest = os.path.join(self.topdir, 'foo') self.assertExists(dest) self.assertTrue(os.path.islink(dest)) - self.assertEqual('git-project/foo.txt', os.readlink(dest)) + self.assertEqual(os.path.join('git-project', 'foo.txt'), os.readlink(dest)) def test_src_subdir(self): """Link to a file in a subdir of a project.""" @@ -320,7 +324,7 @@ class LinkFile(CopyLinkTestCase): lf = self.LinkFile('.', 'foo/bar') lf._Link() self.assertExists(dest) - self.assertEqual('../git-project', os.readlink(dest)) + self.assertEqual(os.path.join('..', 'git-project'), os.readlink(dest)) def test_dest_subdir(self): """Link a file to a subdir of a checkout.""" @@ -354,10 +358,10 @@ class LinkFile(CopyLinkTestCase): self.touch(src) lf = self.LinkFile('foo.txt', 'sym') lf._Link() - self.assertEqual('git-project/foo.txt', os.readlink(dest)) + self.assertEqual(os.path.join('git-project', 'foo.txt'), os.readlink(dest)) # Point the symlink somewhere else. os.unlink(dest) - os.symlink('/', dest) + platform_utils.symlink(self.tempdir, dest) lf._Link() - self.assertEqual('git-project/foo.txt', os.readlink(dest)) + self.assertEqual(os.path.join('git-project', 'foo.txt'), os.readlink(dest))