manifest: validate project name & path and include name attributes

These attribute values are used to construct local filesystem paths,
so apply the existing filesystem checks to them.

Bug: https://crbug.com/gerrit/14156
Change-Id: Ibcceecd60fa74f0eb97cd9ed1a9792e139534ed4
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/298443
Reviewed-by: Michael Mortensen <mmortensen@google.com>
Tested-by: Mike Frysinger <vapier@google.com>

2 files changed, 173 insertions, 75 deletions

diff --git a/manifest_xml.py b/manifest_xml.py
index d05f4d0a..cd5954df 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -670,6 +670,10 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     for node in manifest.childNodes:
       if node.nodeName == 'include':
         name = self._reqatt(node, 'name')
+        msg = self._CheckLocalPath(name)
+        if msg:
+          raise ManifestInvalidPathError(
+              '<include> invalid "name": %s: %s' % (name, msg))
         include_groups = ''
         if parent_groups:
           include_groups = parent_groups
@@ -979,6 +983,10 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     reads a <project> element from the manifest file
     """
     name = self._reqatt(node, 'name')
+    msg = self._CheckLocalPath(name, dir_ok=True)
+    if msg:
+      raise ManifestInvalidPathError(
+          '<project> invalid "name": %s: %s' % (name, msg))
     if parent:
       name = self._JoinName(parent.name, name)
 
@@ -999,9 +1007,11 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     path = node.getAttribute('path')
     if not path:
       path = name
-    if path.startswith('/'):
-      raise ManifestParseError("project %s path cannot be absolute in %s" %
-                               (name, self.manifestFile))
+    else:
+      msg = self._CheckLocalPath(path, dir_ok=True)
+      if msg:
+        raise ManifestInvalidPathError(
+            '<project> invalid "path": %s: %s' % (path, msg))
 
     rebase = XmlBool(node, 'rebase', True)
     sync_c = XmlBool(node, 'sync-c', False)
@@ -1124,7 +1134,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
   def _CheckLocalPath(path, dir_ok=False, cwd_dot_ok=False):
     """Verify |path| is reasonable for use in filesystem paths.
 
-    Used with <copyfile> & <linkfile> elements.
+    Used with <copyfile> & <linkfile> & <project> elements.
 
     This only validates the |path| in isolation: it does not check against the
     current filesystem state.  Thus it is suitable as a first-past in a parser.
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index 4b545140..f69e9cf8 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -24,6 +24,40 @@ import error
 import manifest_xml
 
 
+# Invalid paths that we don't want in the filesystem.
+INVALID_FS_PATHS = (
+    '',
+    '.',
+    '..',
+    '../',
+    './',
+    'foo/',
+    './foo',
+    '../foo',
+    'foo/./bar',
+    'foo/../../bar',
+    '/foo',
+    './../foo',
+    '.git/foo',
+    # Check case folding.
+    '.GIT/foo',
+    'blah/.git/foo',
+    '.repo/foo',
+    '.repoconfig',
+    # Block ~ due to 8.3 filenames on Windows filesystems.
+    '~',
+    'foo~',
+    'blah/foo~',
+    # 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 != '/':
+  INVALID_FS_PATHS += tuple(x.replace('/', os.path.sep) for x in INVALID_FS_PATHS)
+
+
 class ManifestParseTestCase(unittest.TestCase):
   """TestCase for parsing manifests."""
 
@@ -86,38 +120,7 @@ class ManifestValidateFilePaths(unittest.TestCase):
 
   def test_bad_paths(self):
     """Make sure bad paths (src & dest) are rejected."""
-    PATHS = (
-        '',
-        '.',
-        '..',
-        '../',
-        './',
-        'foo/',
-        './foo',
-        '../foo',
-        'foo/./bar',
-        'foo/../../bar',
-        '/foo',
-        './../foo',
-        '.git/foo',
-        # Check case folding.
-        '.GIT/foo',
-        'blah/.git/foo',
-        '.repo/foo',
-        '.repoconfig',
-        # Block ~ due to 8.3 filenames on Windows filesystems.
-        '~',
-        'foo~',
-        'blah/foo~',
-        # 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:
+    for path in INVALID_FS_PATHS:
       self.assertRaises(
           error.ManifestInvalidPathError, self.check_both, path, 'a')
       self.assertRaises(
@@ -248,7 +251,82 @@ class XmlManifestTests(ManifestParseTestCase):
         '<superproject name="superproject"/>' +
         '</manifest>')
 
-  def test_project_group(self):
+
+class IncludeElementTests(ManifestParseTestCase):
+  """Tests for <include>."""
+
+  def test_group_levels(self):
+    root_m = os.path.join(self.manifest_dir, 'root.xml')
+    with open(root_m, 'w') as fp:
+      fp.write("""
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <include name="level1.xml" groups="level1-group" />
+  <project name="root-name1" path="root-path1" />
+  <project name="root-name2" path="root-path2" groups="r2g1,r2g2" />
+</manifest>
+""")
+    with open(os.path.join(self.manifest_dir, 'level1.xml'), 'w') as fp:
+      fp.write("""
+<manifest>
+  <include name="level2.xml" groups="level2-group" />
+  <project name="level1-name1" path="level1-path1" />
+</manifest>
+""")
+    with open(os.path.join(self.manifest_dir, 'level2.xml'), 'w') as fp:
+      fp.write("""
+<manifest>
+  <project name="level2-name1" path="level2-path1" groups="l2g1,l2g2" />
+</manifest>
+""")
+    include_m = manifest_xml.XmlManifest(self.repodir, root_m)
+    for proj in include_m.projects:
+      if proj.name == 'root-name1':
+        # Check include group not set on root level proj.
+        self.assertNotIn('level1-group', proj.groups)
+      if proj.name == 'root-name2':
+        # Check root proj group not removed.
+        self.assertIn('r2g1', proj.groups)
+      if proj.name == 'level1-name1':
+        # Check level1 proj has inherited group level 1.
+        self.assertIn('level1-group', proj.groups)
+      if proj.name == 'level2-name1':
+        # Check level2 proj has inherited group levels 1 and 2.
+        self.assertIn('level1-group', proj.groups)
+        self.assertIn('level2-group', proj.groups)
+        # Check level2 proj group not removed.
+        self.assertIn('l2g1', proj.groups)
+
+  def test_bad_name_checks(self):
+    """Check handling of bad name attribute."""
+    def parse(name):
+      manifest = self.getXmlManifest(f"""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <include name="{name}" />
+</manifest>
+""")
+      # Force the manifest to be parsed.
+      manifest.ToXml()
+
+    # Handle empty name explicitly because a different codepath rejects it.
+    with self.assertRaises(error.ManifestParseError):
+      parse('')
+
+    for path in INVALID_FS_PATHS:
+      if not path:
+        continue
+
+      with self.assertRaises(error.ManifestInvalidPathError):
+        parse(path)
+
+
+class ProjectElementTests(ManifestParseTestCase):
+  """Tests for <project>."""
+
+  def test_group(self):
     """Check project group settings."""
     manifest = self.getXmlManifest("""
 <manifest>
@@ -272,7 +350,7 @@ class XmlManifestTests(ManifestParseTestCase):
         result['extras'],
         ['g1', 'g2', 'g1', 'name:extras', 'all', 'path:path'])
 
-  def test_project_set_revision_id(self):
+  def test_set_revision_id(self):
     """Check setting of project's revisionId."""
     manifest = self.getXmlManifest("""
 <manifest>
@@ -292,51 +370,61 @@ class XmlManifestTests(ManifestParseTestCase):
         '<project name="test-name" revision="ABCDEF"/>' +
         '</manifest>')
 
-  def test_include_levels(self):
-    root_m = os.path.join(self.manifest_dir, 'root.xml')
-    with open(root_m, 'w') as fp:
-      fp.write("""
-<manifest>
-  <remote name="test-remote" fetch="http://localhost" />
-  <default remote="test-remote" revision="refs/heads/main" />
-  <include name="level1.xml" groups="level1-group" />
-  <project name="root-name1" path="root-path1" />
-  <project name="root-name2" path="root-path2" groups="r2g1,r2g2" />
-</manifest>
-""")
-    with open(os.path.join(self.manifest_dir, 'level1.xml'), 'w') as fp:
-      fp.write("""
+  def test_trailing_slash(self):
+    """Check handling of trailing slashes in attributes."""
+    def parse(name, path):
+      return self.getXmlManifest(f"""
 <manifest>
-  <include name="level2.xml" groups="level2-group" />
-  <project name="level1-name1" path="level1-path1" />
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="{name}" path="{path}" />
 </manifest>
 """)
-    with open(os.path.join(self.manifest_dir, 'level2.xml'), 'w') as fp:
-      fp.write("""
+
+    manifest = parse('a/path/', 'foo')
+    self.assertEqual(manifest.projects[0].gitdir,
+                     os.path.join(self.tempdir, '.repo/projects/foo.git'))
+    self.assertEqual(manifest.projects[0].objdir,
+                     os.path.join(self.tempdir, '.repo/project-objects/a/path.git'))
+
+    manifest = parse('a/path', 'foo/')
+    self.assertEqual(manifest.projects[0].gitdir,
+                     os.path.join(self.tempdir, '.repo/projects/foo.git'))
+    self.assertEqual(manifest.projects[0].objdir,
+                     os.path.join(self.tempdir, '.repo/project-objects/a/path.git'))
+
+  def test_bad_path_name_checks(self):
+    """Check handling of bad path & name attributes."""
+    def parse(name, path):
+      manifest = self.getXmlManifest(f"""
 <manifest>
-  <project name="level2-name1" path="level2-path1" groups="l2g1,l2g2" />
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="{name}" path="{path}" />
 </manifest>
 """)
-    include_m = manifest_xml.XmlManifest(self.repodir, root_m)
-    for proj in include_m.projects:
-      if proj.name == 'root-name1':
-        # Check include group not set on root level proj.
-        self.assertNotIn('level1-group', proj.groups)
-      if proj.name == 'root-name2':
-        # Check root proj group not removed.
-        self.assertIn('r2g1', proj.groups)
-      if proj.name == 'level1-name1':
-        # Check level1 proj has inherited group level 1.
-        self.assertIn('level1-group', proj.groups)
-      if proj.name == 'level2-name1':
-        # Check level2 proj has inherited group levels 1 and 2.
-        self.assertIn('level1-group', proj.groups)
-        self.assertIn('level2-group', proj.groups)
-        # Check level2 proj group not removed.
-        self.assertIn('l2g1', proj.groups)
+      # Force the manifest to be parsed.
+      manifest.ToXml()
+
+    # Verify the parser is valid by default to avoid buggy tests below.
+    parse('ok', 'ok')
+
+    # Handle empty name explicitly because a different codepath rejects it.
+    # Empty path is OK because it defaults to the name field.
+    with self.assertRaises(error.ManifestParseError):
+      parse('', 'ok')
+
+    for path in INVALID_FS_PATHS:
+      if not path or path.endswith('/'):
+        continue
+
+      with self.assertRaises(error.ManifestInvalidPathError):
+        parse(path, 'ok')
+      with self.assertRaises(error.ManifestInvalidPathError):
+        parse('ok', path)
 
 
-class SuperProjectTests(ManifestParseTestCase):
+class SuperProjectElementTests(ManifestParseTestCase):
   """Tests for <superproject>."""
 
   def test_superproject(self):